From 7301bf6d5929a87d161af89d9a1e03e637502d65 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 28 May 2025 14:45:09 +0200 Subject: [PATCH 1/4] test(browser): Add test demonstrating EventTarget double instrumentation --- .../integrations/browserApiErrors/init.js | 17 +++++++++++ .../browserApiErrors/template.html | 9 ++++++ .../integrations/browserApiErrors/test.ts | 30 +++++++++++++++++++ 3 files changed, 56 insertions(+) create mode 100644 dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/init.js create mode 100644 dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/template.html create mode 100644 dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/test.ts diff --git a/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/init.js b/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/init.js new file mode 100644 index 000000000000..6b8d52a57eac --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/init.js @@ -0,0 +1,17 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; + +const btn = document.getElementById('btn'); + +const myClickListener = () => { + console.log('clicked'); +}; + +btn.addEventListener('click', myClickListener); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', +}); + +btn.addEventListener('click', myClickListener); diff --git a/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/template.html b/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/template.html new file mode 100644 index 000000000000..1e8bf7954ed3 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/template.html @@ -0,0 +1,9 @@ + + + + + + + + + diff --git a/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/test.ts b/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/test.ts new file mode 100644 index 000000000000..4ad3880f638a --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/test.ts @@ -0,0 +1,30 @@ +import { expect } from '@playwright/test'; +import { sentryTest } from '../../../utils/fixtures'; + +/** + * This test demonstrates an unfortunate edge case with our EventTarget.addEventListener instrumentation. + * If a listener is registered before Sentry.init() and then again, the same listener is added + * after Sentry.init(), our `browserApiErrorsIntegration`'s instrumentation causes the listener to be + * added twice, while without the integration it would only be added and invoked once. + * + * Real-life example of such an issue: + * https://github.com/getsentry/sentry-javascript/issues/16398 + */ +sentryTest( + 'causes listeners to be invoked twice if registered before and after Sentry initialization', + async ({ getLocalTestUrl, page }) => { + const consoleLogs: string[] = []; + page.on('console', msg => { + consoleLogs.push(msg.text()); + }); + + await page.goto(await getLocalTestUrl({ testDir: __dirname })); + + await page.waitForFunction('window.Sentry'); + + await page.locator('#btn').click(); + + expect(consoleLogs).toHaveLength(2); + expect(consoleLogs).toEqual(['clicked', 'clicked']); + }, +); From 589d813860eab7a13aab98144050268fdf9df58f Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 28 May 2025 14:54:25 +0200 Subject: [PATCH 2/4] lint --- .../suites/integrations/browserApiErrors/init.js | 1 + 1 file changed, 1 insertion(+) diff --git a/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/init.js b/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/init.js index 6b8d52a57eac..eac01ad8ff64 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/init.js +++ b/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/init.js @@ -5,6 +5,7 @@ window.Sentry = Sentry; const btn = document.getElementById('btn'); const myClickListener = () => { + // eslint-disable-next-line no-console console.log('clicked'); }; From 596732e9e96a77c7ef604facaef0ca9fc4ea5984 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 28 May 2025 15:56:01 +0200 Subject: [PATCH 3/4] feat(browser): Add `unregisterOriginalCallbacks` option to `browserApiErrorsIntegration` --- .../unregisterOriginalCallbacks/init.js | 23 ++++++++++++++ .../unregisterOriginalCallbacks/test.ts | 25 ++++++++++++++++ .../src/integrations/browserapierrors.ts | 30 +++++++++++++++++-- 3 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/unregisterOriginalCallbacks/init.js create mode 100644 dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/unregisterOriginalCallbacks/test.ts diff --git a/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/unregisterOriginalCallbacks/init.js b/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/unregisterOriginalCallbacks/init.js new file mode 100644 index 000000000000..ecf0d4331b0d --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/unregisterOriginalCallbacks/init.js @@ -0,0 +1,23 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; + +const btn = document.getElementById('btn'); + +const myClickListener = () => { + // eslint-disable-next-line no-console + console.log('clicked'); +}; + +btn.addEventListener('click', myClickListener); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + integrations: [ + Sentry.browserApiErrorsIntegration({ + unregisterOriginalCallbacks: true, + }), + ], +}); + +btn.addEventListener('click', myClickListener); diff --git a/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/unregisterOriginalCallbacks/test.ts b/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/unregisterOriginalCallbacks/test.ts new file mode 100644 index 000000000000..f579793be336 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/unregisterOriginalCallbacks/test.ts @@ -0,0 +1,25 @@ +import { expect } from '@playwright/test'; +import { sentryTest } from '../../../../utils/fixtures'; + +/** + * By setting `unregisterOriginalCallbacks` to `true`, we can avoid the issue of double-invocations + * (see other test for more details). + */ +sentryTest( + 'causes listeners to be invoked twice if registered before and after Sentry initialization', + async ({ getLocalTestUrl, page }) => { + const consoleLogs: string[] = []; + page.on('console', msg => { + consoleLogs.push(msg.text()); + }); + + await page.goto(await getLocalTestUrl({ testDir: __dirname })); + + await page.waitForFunction('window.Sentry'); + + await page.locator('#btn').click(); + + expect(consoleLogs).toHaveLength(1); + expect(consoleLogs).toEqual(['clicked']); + }, +); diff --git a/packages/browser/src/integrations/browserapierrors.ts b/packages/browser/src/integrations/browserapierrors.ts index 55e716fd8627..c0560ee6f184 100644 --- a/packages/browser/src/integrations/browserapierrors.ts +++ b/packages/browser/src/integrations/browserapierrors.ts @@ -1,6 +1,7 @@ import type { IntegrationFn, WrappedFunction } from '@sentry/core'; import { defineIntegration, fill, getFunctionName, getOriginalFunction } from '@sentry/core'; import { WINDOW, wrap } from '../helpers'; +import { __propKey } from 'tslib'; const DEFAULT_EVENT_TARGET = [ 'EventTarget', @@ -46,6 +47,15 @@ interface BrowserApiErrorsOptions { requestAnimationFrame: boolean; XMLHttpRequest: boolean; eventTarget: boolean | string[]; + + /** + * If you experience issues with this integration causing double-invocations of event listeners, + * try setting this option to `true`. It will unregister the original callbacks from the event targets + * before adding the instrumented callback. + * + * @default false + */ + unregisterOriginalCallbacks: boolean; } const _browserApiErrorsIntegration = ((options: Partial = {}) => { @@ -55,6 +65,7 @@ const _browserApiErrorsIntegration = ((options: Partial requestAnimationFrame: true, setInterval: true, setTimeout: true, + unregisterOriginalCallbacks: false, ...options, }; @@ -82,7 +93,7 @@ const _browserApiErrorsIntegration = ((options: Partial const eventTargetOption = _options.eventTarget; if (eventTargetOption) { const eventTarget = Array.isArray(eventTargetOption) ? eventTargetOption : DEFAULT_EVENT_TARGET; - eventTarget.forEach(_wrapEventTarget); + eventTarget.forEach(target => _wrapEventTarget(target, _options)); } }, }; @@ -160,7 +171,7 @@ function _wrapXHR(originalSend: () => void): () => void { }; } -function _wrapEventTarget(target: string): void { +function _wrapEventTarget(target: string, integrationOptions: BrowserApiErrorsOptions): void { const globalObject = WINDOW as unknown as Record; const proto = globalObject[target]?.prototype; @@ -197,6 +208,10 @@ function _wrapEventTarget(target: string): void { // can sometimes get 'Permission denied to access property "handle Event' } + if (integrationOptions.unregisterOriginalCallbacks) { + unregisterOriginalCallback(this, eventName, fn); + } + return original.apply(this, [ eventName, wrap(fn, { @@ -253,3 +268,14 @@ function _wrapEventTarget(target: string): void { function isEventListenerObject(obj: unknown): obj is EventListenerObject { return typeof (obj as EventListenerObject).handleEvent === 'function'; } + +function unregisterOriginalCallback(target: unknown, eventName: string, fn: EventListenerOrEventListenerObject): void { + if ( + target && + typeof target === 'object' && + 'removeEventListener' in target && + typeof target.removeEventListener === 'function' + ) { + target.removeEventListener(eventName, fn); + } +} From 92ef750e6be4649bf7a05d8b076f1b67b86cf563 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 28 May 2025 16:03:33 +0200 Subject: [PATCH 4/4] cleanup import --- packages/browser/src/integrations/browserapierrors.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/browser/src/integrations/browserapierrors.ts b/packages/browser/src/integrations/browserapierrors.ts index c0560ee6f184..113c969b9ebc 100644 --- a/packages/browser/src/integrations/browserapierrors.ts +++ b/packages/browser/src/integrations/browserapierrors.ts @@ -1,7 +1,6 @@ import type { IntegrationFn, WrappedFunction } from '@sentry/core'; import { defineIntegration, fill, getFunctionName, getOriginalFunction } from '@sentry/core'; import { WINDOW, wrap } from '../helpers'; -import { __propKey } from 'tslib'; const DEFAULT_EVENT_TARGET = [ 'EventTarget',