From c44ac4be1296c1d22a8969e9c89b7dc4c92bd1a7 Mon Sep 17 00:00:00 2001 From: arturovt Date: Fri, 5 Jul 2024 17:19:22 +0300 Subject: [PATCH 1/2] fix(angular): remove `afterSendEvent` listener once root injector is destroyed In this commit, we added cleanup logic to handle the removal of `afterSendEvent`, which is set up within the Angular error handler. This fixes memory leaks that occur when the event is still being handled after the root view is removed. --- packages/angular/src/errorhandler.ts | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/packages/angular/src/errorhandler.ts b/packages/angular/src/errorhandler.ts index 28c06e1e6bfd..b23b352db170 100644 --- a/packages/angular/src/errorhandler.ts +++ b/packages/angular/src/errorhandler.ts @@ -1,5 +1,5 @@ import { HttpErrorResponse } from '@angular/common/http'; -import type { ErrorHandler as AngularErrorHandler } from '@angular/core'; +import type { ErrorHandler as AngularErrorHandler, OnDestroy } from '@angular/core'; import { Inject, Injectable } from '@angular/core'; import * as Sentry from '@sentry/browser'; import type { ReportDialogOptions } from '@sentry/browser'; @@ -81,21 +81,28 @@ function isErrorOrErrorLikeObject(value: unknown): value is Error { * Implementation of Angular's ErrorHandler provider that can be used as a drop-in replacement for the stock one. */ @Injectable({ providedIn: 'root' }) -class SentryErrorHandler implements AngularErrorHandler { +class SentryErrorHandler implements AngularErrorHandler, OnDestroy { protected readonly _options: ErrorHandlerOptions; - /* indicates if we already registered our the afterSendEvent handler */ - private _registeredAfterSendEventHandler; + /** The cleanup function is executed when the injector is destroyed. */ + private _removeAfterSendEventListener?: VoidFunction; public constructor(@Inject('errorHandlerOptions') options?: ErrorHandlerOptions) { - this._registeredAfterSendEventHandler = false; - this._options = { logErrors: true, ...options, }; } + /** + * Method executed when the injector is destroyed. + */ + public ngOnDestroy(): void { + if (this._removeAfterSendEventListener) { + this._removeAfterSendEventListener(); + } + } + /** * Method called for every value captured through the ErrorHandler */ @@ -119,17 +126,14 @@ class SentryErrorHandler implements AngularErrorHandler { if (this._options.showDialog) { const client = Sentry.getClient(); - if (client && !this._registeredAfterSendEventHandler) { - client.on('afterSendEvent', (event: Event) => { + if (client && !this._removeAfterSendEventListener) { + this._removeAfterSendEventListener = client.on('afterSendEvent', (event: Event) => { if (!event.type && event.event_id) { runOutsideAngular(() => { Sentry.showReportDialog({ ...this._options.dialogOptions, eventId: event.event_id! }); }); } }); - - // We only want to register this hook once in the lifetime of the error handler - this._registeredAfterSendEventHandler = true; } else if (!client) { runOutsideAngular(() => { Sentry.showReportDialog({ ...this._options.dialogOptions, eventId }); From ed22329444a3a5a21d32cddc3807a6b87d25772d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 18 Jul 2024 13:47:38 +0200 Subject: [PATCH 2/2] add tests --- packages/angular/src/errorhandler.ts | 4 +- packages/angular/test/errorhandler.test.ts | 44 ++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/packages/angular/src/errorhandler.ts b/packages/angular/src/errorhandler.ts index b23b352db170..14ca380ea3ea 100644 --- a/packages/angular/src/errorhandler.ts +++ b/packages/angular/src/errorhandler.ts @@ -85,7 +85,7 @@ class SentryErrorHandler implements AngularErrorHandler, OnDestroy { protected readonly _options: ErrorHandlerOptions; /** The cleanup function is executed when the injector is destroyed. */ - private _removeAfterSendEventListener?: VoidFunction; + private _removeAfterSendEventListener?: () => void; public constructor(@Inject('errorHandlerOptions') options?: ErrorHandlerOptions) { this._options = { @@ -130,7 +130,7 @@ class SentryErrorHandler implements AngularErrorHandler, OnDestroy { this._removeAfterSendEventListener = client.on('afterSendEvent', (event: Event) => { if (!event.type && event.event_id) { runOutsideAngular(() => { - Sentry.showReportDialog({ ...this._options.dialogOptions, eventId: event.event_id! }); + Sentry.showReportDialog({ ...this._options.dialogOptions, eventId: event.event_id }); }); } }); diff --git a/packages/angular/test/errorhandler.test.ts b/packages/angular/test/errorhandler.test.ts index c30a7a87efc9..1ae415c5706d 100644 --- a/packages/angular/test/errorhandler.test.ts +++ b/packages/angular/test/errorhandler.test.ts @@ -546,5 +546,49 @@ describe('SentryErrorHandler', () => { expect(showReportDialogSpy).toBeCalledTimes(1); }); }); + + it('only registers the client "afterSendEvent" listener to open the dialog once', () => { + const unsubScribeSpy = vi.fn(); + const client = { + cbs: [] as ((event: Event) => void)[], + on: vi.fn((_, cb) => { + client.cbs.push(cb); + return unsubScribeSpy; + }), + }; + + vi.spyOn(SentryBrowser, 'getClient').mockImplementation(() => client as unknown as Client); + + const errorhandler = createErrorHandler({ showDialog: true }); + expect(client.cbs).toHaveLength(0); + + errorhandler.handleError(new Error('error 1')); + expect(client.cbs).toHaveLength(1); + + errorhandler.handleError(new Error('error 2')); + errorhandler.handleError(new Error('error 3')); + expect(client.cbs).toHaveLength(1); + }); + + it('cleans up the "afterSendEvent" listener once the ErrorHandler is destroyed', () => { + const unsubScribeSpy = vi.fn(); + const client = { + cbs: [] as ((event: Event) => void)[], + on: vi.fn((_, cb) => { + client.cbs.push(cb); + return unsubScribeSpy; + }), + }; + + vi.spyOn(SentryBrowser, 'getClient').mockImplementation(() => client as unknown as Client); + + const errorhandler = createErrorHandler({ showDialog: true }); + + errorhandler.handleError(new Error('error 1')); + expect(client.cbs).toHaveLength(1); + + errorhandler.ngOnDestroy(); + expect(unsubScribeSpy).toHaveBeenCalledTimes(1); + }); }); });