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..eac01ad8ff64 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/init.js @@ -0,0 +1,18 @@ +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', +}); + +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']); + }, +); 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..113c969b9ebc 100644 --- a/packages/browser/src/integrations/browserapierrors.ts +++ b/packages/browser/src/integrations/browserapierrors.ts @@ -46,6 +46,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 +64,7 @@ const _browserApiErrorsIntegration = ((options: Partial requestAnimationFrame: true, setInterval: true, setTimeout: true, + unregisterOriginalCallbacks: false, ...options, }; @@ -82,7 +92,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 +170,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 +207,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 +267,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); + } +}