Skip to content

Commit 7116874

Browse files
Lms24Lms24
authored
feat(browser): Add unregisterOriginalCallbacks option to browserApiErrorsIntegration (#16412)
This patch adds an option to fix a very unfortunate, effective double invocation of callbacks on objects inheriting from our `EventTarget.addEventListener` instrumentation (to be found in `browserApiErrorsIntegration`. Affected users can switch on the new option and doing so will make our instrumentation of `addEventListener` also call `removeEventListener` on for the original callback. Applied to the example above, it will essentially restore the idempotency of `addEventListener`. At the same time, it feels very risky to do this so for the time being, we'll keep this (and document it as) opt-in. Usage: ```js Sentry.init({ // ... integrations: [ Sentry.browserApiErrorsIntegration({ unregisterOriginalCallbacks: true, }), ], }); ``` Docs PR: getsentry/sentry-docs#13859 fixes #16398
1 parent b5c8344 commit 7116874

File tree

6 files changed

+132
-2
lines changed

6 files changed

+132
-2
lines changed

‎dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/init.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
5+
const btn = document.getElementById('btn');
6+
7+
const myClickListener = () => {
8+
// eslint-disable-next-line no-console
9+
console.log('clicked');
10+
};
11+
12+
btn.addEventListener('click', myClickListener);
13+
14+
Sentry.init({
15+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
16+
});
17+
18+
btn.addEventListener('click', myClickListener);

‎dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/template.html‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<!doctype html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
</head>
6+
<body>
7+
<button id="btn">Click me</button>
8+
</body>
9+
</html>

‎dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/test.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
import { expect } from '@playwright/test';
2+
import { sentryTest } from '../../../utils/fixtures';
3+
4+
/**
5+
* This test demonstrates an unfortunate edge case with our EventTarget.addEventListener instrumentation.
6+
* If a listener is registered before Sentry.init() and then again, the same listener is added
7+
* after Sentry.init(), our `browserApiErrorsIntegration`'s instrumentation causes the listener to be
8+
* added twice, while without the integration it would only be added and invoked once.
9+
*
10+
* Real-life example of such an issue:
11+
* https://github.com/getsentry/sentry-javascript/issues/16398
12+
*/
13+
sentryTest(
14+
'causes listeners to be invoked twice if registered before and after Sentry initialization',
15+
async ({ getLocalTestUrl, page }) => {
16+
const consoleLogs: string[] = [];
17+
page.on('console', msg => {
18+
consoleLogs.push(msg.text());
19+
});
20+
21+
await page.goto(await getLocalTestUrl({ testDir: __dirname }));
22+
23+
await page.waitForFunction('window.Sentry');
24+
25+
await page.locator('#btn').click();
26+
27+
expect(consoleLogs).toHaveLength(2);
28+
expect(consoleLogs).toEqual(['clicked', 'clicked']);
29+
},
30+
);

‎dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/unregisterOriginalCallbacks/init.js‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
5+
const btn = document.getElementById('btn');
6+
7+
const myClickListener = () => {
8+
// eslint-disable-next-line no-console
9+
console.log('clicked');
10+
};
11+
12+
btn.addEventListener('click', myClickListener);
13+
14+
Sentry.init({
15+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
16+
integrations: [
17+
Sentry.browserApiErrorsIntegration({
18+
unregisterOriginalCallbacks: true,
19+
}),
20+
],
21+
});
22+
23+
btn.addEventListener('click', myClickListener);

‎dev-packages/browser-integration-tests/suites/integrations/browserApiErrors/unregisterOriginalCallbacks/test.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
import { expect } from '@playwright/test';
2+
import { sentryTest } from '../../../../utils/fixtures';
3+
4+
/**
5+
* By setting `unregisterOriginalCallbacks` to `true`, we can avoid the issue of double-invocations
6+
* (see other test for more details).
7+
*/
8+
sentryTest(
9+
'causes listeners to be invoked twice if registered before and after Sentry initialization',
10+
async ({ getLocalTestUrl, page }) => {
11+
const consoleLogs: string[] = [];
12+
page.on('console', msg => {
13+
consoleLogs.push(msg.text());
14+
});
15+
16+
await page.goto(await getLocalTestUrl({ testDir: __dirname }));
17+
18+
await page.waitForFunction('window.Sentry');
19+
20+
await page.locator('#btn').click();
21+
22+
expect(consoleLogs).toHaveLength(1);
23+
expect(consoleLogs).toEqual(['clicked']);
24+
},
25+
);

‎packages/browser/src/integrations/browserapierrors.ts‎

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,15 @@ interface BrowserApiErrorsOptions {
4646
requestAnimationFrame: boolean;
4747
XMLHttpRequest: boolean;
4848
eventTarget: boolean | string[];
49+
50+
/**
51+
* If you experience issues with this integration causing double-invocations of event listeners,
52+
* try setting this option to `true`. It will unregister the original callbacks from the event targets
53+
* before adding the instrumented callback.
54+
*
55+
* @default false
56+
*/
57+
unregisterOriginalCallbacks: boolean;
4958
}
5059

5160
const _browserApiErrorsIntegration = ((options: Partial<BrowserApiErrorsOptions> = {}) => {
@@ -55,6 +64,7 @@ const _browserApiErrorsIntegration = ((options: Partial<BrowserApiErrorsOptions>
5564
requestAnimationFrame: true,
5665
setInterval: true,
5766
setTimeout: true,
67+
unregisterOriginalCallbacks: false,
5868
...options,
5969
};
6070

@@ -82,7 +92,7 @@ const _browserApiErrorsIntegration = ((options: Partial<BrowserApiErrorsOptions>
8292
const eventTargetOption = _options.eventTarget;
8393
if (eventTargetOption) {
8494
const eventTarget = Array.isArray(eventTargetOption) ? eventTargetOption : DEFAULT_EVENT_TARGET;
85-
eventTarget.forEach(_wrapEventTarget);
95+
eventTarget.forEach(target => _wrapEventTarget(target, _options));
8696
}
8797
},
8898
};
@@ -160,7 +170,7 @@ function _wrapXHR(originalSend: () => void): () => void {
160170
};
161171
}
162172

163-
function _wrapEventTarget(target: string): void {
173+
function _wrapEventTarget(target: string, integrationOptions: BrowserApiErrorsOptions): void {
164174
const globalObject = WINDOW as unknown as Record<string, { prototype?: object }>;
165175
const proto = globalObject[target]?.prototype;
166176

@@ -197,6 +207,10 @@ function _wrapEventTarget(target: string): void {
197207
// can sometimes get 'Permission denied to access property "handle Event'
198208
}
199209

210+
if (integrationOptions.unregisterOriginalCallbacks) {
211+
unregisterOriginalCallback(this, eventName, fn);
212+
}
213+
200214
return original.apply(this, [
201215
eventName,
202216
wrap(fn, {
@@ -253,3 +267,14 @@ function _wrapEventTarget(target: string): void {
253267
function isEventListenerObject(obj: unknown): obj is EventListenerObject {
254268
return typeof (obj as EventListenerObject).handleEvent === 'function';
255269
}
270+
271+
function unregisterOriginalCallback(target: unknown, eventName: string, fn: EventListenerOrEventListenerObject): void {
272+
if (
273+
target &&
274+
typeof target === 'object' &&
275+
'removeEventListener' in target &&
276+
typeof target.removeEventListener === 'function'
277+
) {
278+
target.removeEventListener(eventName, fn);
279+
}
280+
}

0 commit comments

Comments
 (0)