From 292b77ee93520697a65b7b7c4a28aa24a042044c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 25 Sep 2026 05:52:15 +0000 Subject: [PATCH 1/2] fix(browser-utils): Remove DOM instrumentation click listeners with stable capture flag Fixes #24702 Co-Authored-By: Cursor Agent Co-authored-by: lamberto --- .../browser-utils/src/instrumentation/dom.ts | 6 +- .../test/instrumentation/dom.test.ts | 67 ++++++++++++++++++- 2 files changed, 68 insertions(+), 5 deletions(-) diff --git a/packages/browser-utils/src/instrumentation/dom.ts b/packages/browser-utils/src/instrumentation/dom.ts index d326fd281ef2..d465db838369 100644 --- a/packages/browser-utils/src/instrumentation/dom.ts +++ b/packages/browser-utils/src/instrumentation/dom.ts @@ -19,6 +19,7 @@ type InstrumentedElement = Element & { __sentry_instrumentation_handlers__?: { [key in 'click' | 'keypress']?: { handler?: unknown; + capture?: boolean; /** The number of custom listeners attached to this element */ refCount: number; }; @@ -82,7 +83,8 @@ export function instrumentDOM(): void { if (!handlerForType.handler) { const handler = makeDOMEventHandler(triggerDOMHandler); handlerForType.handler = handler; - originalAddEventListener.call(this, type, handler, options); + handlerForType.capture = typeof options === 'boolean' ? options : !!options?.capture; + originalAddEventListener.call(this, type, handler, handlerForType.capture); } handlerForType.refCount++; @@ -110,7 +112,7 @@ export function instrumentDOM(): void { handlerForType.refCount--; // If there are no longer any custom handlers of the current type on this element, we can remove ours, too. if (handlerForType.refCount <= 0) { - originalRemoveEventListener.call(this, type, handlerForType.handler, options); + originalRemoveEventListener.call(this, type, handlerForType.handler, handlerForType.capture); handlerForType.handler = undefined; delete handlers[type]; // eslint-disable-line @typescript-eslint/no-dynamic-delete } diff --git a/packages/browser-utils/test/instrumentation/dom.test.ts b/packages/browser-utils/test/instrumentation/dom.test.ts index 23681014150b..9dd62cc90f3a 100644 --- a/packages/browser-utils/test/instrumentation/dom.test.ts +++ b/packages/browser-utils/test/instrumentation/dom.test.ts @@ -1,12 +1,73 @@ -import { describe, expect, it } from 'vitest'; +/** + * @vitest-environment jsdom + */ +import { afterEach, describe, expect, it } from 'vitest'; import { instrumentDOM } from '../../src/instrumentation/dom'; import { WINDOW } from '../../src/types'; // @ts-expect-error - idk WINDOW.XMLHttpRequest = undefined; -describe('instrumentXHR', () => { - it('it does not throw if XMLHttpRequest is a key on window but not defined', () => { +describe('instrumentDOM', () => { + afterEach(() => { + // @ts-expect-error - idk + WINDOW.XMLHttpRequest = undefined; + }); + + it('does not throw if XMLHttpRequest is a key on window but not defined', () => { expect(instrumentDOM).not.toThrow(); }); + + it('does not leak document click listeners when removeEventListener uses mismatched capture options', () => { + instrumentDOM(); + + const live = { + capture: new Set(), + bubble: new Set(), + }; + const patchedAdd = EventTarget.prototype.addEventListener; + const patchedRemove = EventTarget.prototype.removeEventListener; + + const captureFlag = (options?: boolean | AddEventListenerOptions | EventListenerOptions): boolean => + typeof options === 'boolean' ? options : !!options?.capture; + + EventTarget.prototype.addEventListener = function ( + type: string, + listener: EventListenerOrEventListenerObject, + options?: boolean | AddEventListenerOptions, + ) { + if (this === document && type === 'click') { + (captureFlag(options) ? live.capture : live.bubble).add(listener); + } + return patchedAdd.call(this, type, listener, options); + }; + + EventTarget.prototype.removeEventListener = function ( + type: string, + listener: EventListenerOrEventListenerObject, + options?: boolean | EventListenerOptions, + ) { + if (this === document && type === 'click') { + (captureFlag(options) ? live.capture : live.bubble).delete(listener); + } + return patchedRemove.call(this, type, listener, options); + }; + + const baseline = live.capture.size + live.bubble.size; + + const never = () => {}; + const onCapture = () => {}; + const onBubble = () => {}; + + for (let i = 0; i < 20; i++) { + document.addEventListener('click', onCapture, true); + document.addEventListener('click', onBubble); + document.removeEventListener('click', never); + document.removeEventListener('click', never); + document.removeEventListener('click', onCapture, true); + document.removeEventListener('click', onBubble); + } + + expect(live.capture.size + live.bubble.size - baseline).toBe(0); + }); }); From ca9ded4721d90d05f4c2dda75f1a674c9aa4907a Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 25 Sep 2026 10:45:29 +0200 Subject: [PATCH 2/2] adjust tests and explain code --- .../browser-utils/src/instrumentation/dom.ts | 4 ++ .../test/instrumentation/dom.test.ts | 57 +++++++++---------- 2 files changed, 30 insertions(+), 31 deletions(-) diff --git a/packages/browser-utils/src/instrumentation/dom.ts b/packages/browser-utils/src/instrumentation/dom.ts index d465db838369..b2f8326f35eb 100644 --- a/packages/browser-utils/src/instrumentation/dom.ts +++ b/packages/browser-utils/src/instrumentation/dom.ts @@ -83,6 +83,10 @@ export function instrumentDOM(): void { if (!handlerForType.handler) { const handler = makeDOMEventHandler(triggerDOMHandler); handlerForType.handler = handler; + // Track the user-set `capture` option because it changes the identity of the registration of the + // event listener callback function (addEL(fn, true) vs addEL(fn, false) are two different registrations). + // Our listener needs to have the same capture setting, so that subsequent calls or removaleEventListener + // calls correspond to the correct handler function. handlerForType.capture = typeof options === 'boolean' ? options : !!options?.capture; originalAddEventListener.call(this, type, handler, handlerForType.capture); } diff --git a/packages/browser-utils/test/instrumentation/dom.test.ts b/packages/browser-utils/test/instrumentation/dom.test.ts index 9dd62cc90f3a..fff88dd49278 100644 --- a/packages/browser-utils/test/instrumentation/dom.test.ts +++ b/packages/browser-utils/test/instrumentation/dom.test.ts @@ -9,9 +9,12 @@ import { WINDOW } from '../../src/types'; WINDOW.XMLHttpRequest = undefined; describe('instrumentDOM', () => { + const { addEventListener: nativeAdd, removeEventListener: nativeRemove } = EventTarget.prototype; + + // `instrumentDOM` patches `EventTarget.prototype` and isn't idempotent, so restore the native methods after every test. afterEach(() => { - // @ts-expect-error - idk - WINDOW.XMLHttpRequest = undefined; + EventTarget.prototype.addEventListener = nativeAdd; + EventTarget.prototype.removeEventListener = nativeRemove; }); it('does not throw if XMLHttpRequest is a key on window but not defined', () => { @@ -19,45 +22,37 @@ describe('instrumentDOM', () => { }); it('does not leak document click listeners when removeEventListener uses mismatched capture options', () => { - instrumentDOM(); - - const live = { - capture: new Set(), - bubble: new Set(), - }; - const patchedAdd = EventTarget.prototype.addEventListener; - const patchedRemove = EventTarget.prototype.removeEventListener; + const documentClickListeners = { capture: new Set(), bubble: new Set() }; - const captureFlag = (options?: boolean | AddEventListenerOptions | EventListenerOptions): boolean => - typeof options === 'boolean' ? options : !!options?.capture; + const phase = (options?: boolean | EventListenerOptions): Set => + (typeof options === 'boolean' ? options : !!options?.capture) + ? documentClickListeners.capture + : documentClickListeners.bubble; - EventTarget.prototype.addEventListener = function ( - type: string, - listener: EventListenerOrEventListenerObject, - options?: boolean | AddEventListenerOptions, - ) { + // Installed before `instrumentDOM` so these sit underneath the SDK and also see the listeners it attaches itself. + EventTarget.prototype.addEventListener = function (type, listener, options) { if (this === document && type === 'click') { - (captureFlag(options) ? live.capture : live.bubble).add(listener); + phase(options).add(listener); } - return patchedAdd.call(this, type, listener, options); + return nativeAdd.call(this, type, listener, options); }; - EventTarget.prototype.removeEventListener = function ( - type: string, - listener: EventListenerOrEventListenerObject, - options?: boolean | EventListenerOptions, - ) { + EventTarget.prototype.removeEventListener = function (type, listener, options) { if (this === document && type === 'click') { - (captureFlag(options) ? live.capture : live.bubble).delete(listener); + phase(options).delete(listener); } - return patchedRemove.call(this, type, listener, options); + return nativeRemove.call(this, type, listener, options); }; - const baseline = live.capture.size + live.bubble.size; + instrumentDOM(); + + // baseline listenercount is 1 which comes from the SDK's global click handler registered + // in instrumentDOM(). + const baseline = documentClickListeners.capture.size + documentClickListeners.bubble.size; - const never = () => {}; - const onCapture = () => {}; - const onBubble = () => {}; + const never = (): void => {}; + const onCapture = (): void => {}; + const onBubble = (): void => {}; for (let i = 0; i < 20; i++) { document.addEventListener('click', onCapture, true); @@ -68,6 +63,6 @@ describe('instrumentDOM', () => { document.removeEventListener('click', onBubble); } - expect(live.capture.size + live.bubble.size - baseline).toBe(0); + expect(documentClickListeners.capture.size + documentClickListeners.bubble.size - baseline).toBe(0); }); });