From 171dec61d4f25b33b589200ab3ad2ea1c1ef3e95 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 25 Sep 2026 10:55:12 +0200 Subject: [PATCH 1/5] fix(browser-utils): Stop leaking DOM instrumentation listeners on mismatched removals instrumentDOM refcounted add/removeEventListener calls without regard to listener identity or capture phase, so no-op removals (e.g. Radix DismissableLayer removing a bubble-phase listener that was added in capture phase) decremented the count and our handler was removed with the wrong capture flag, leaking it on every cycle. Track listeners per capture phase in sets so only removals that the browser would actually honor count, and always detach our handler with the capture flag it was attached with. Fixes #24702 Co-Authored-By: Claude Opus 5.5 (1M context) --- .../browser-utils/src/instrumentation/dom.ts | 38 ++++++-- .../test/instrumentation/dom.test.ts | 88 ++++++++++++++++++- 2 files changed, 114 insertions(+), 12 deletions(-) diff --git a/packages/browser-utils/src/instrumentation/dom.ts b/packages/browser-utils/src/instrumentation/dom.ts index d326fd281ef2..c4e97dcc86a7 100644 --- a/packages/browser-utils/src/instrumentation/dom.ts +++ b/packages/browser-utils/src/instrumentation/dom.ts @@ -19,8 +19,13 @@ type InstrumentedElement = Element & { __sentry_instrumentation_handlers__?: { [key in 'click' | 'keypress']?: { handler?: unknown; - /** The number of custom listeners attached to this element */ - refCount: number; + capture?: boolean; + // listeners added with `capture: true`, meaning the listener is invoked before other + // listeners inside the element's hierarchy. + captureListeners: Set; + // listeners added with `capture: false` (default), meaning the listener is invoked + // after other listeners inside the element's hierarchy (i.e. the event bubbles up) + bubbleListeners: Set; }; }; }; @@ -31,6 +36,10 @@ let debounceTimerID: number | undefined; let lastCapturedEventType: string | undefined; let lastCapturedEventTargetId: string | undefined; +function getCapture(options: boolean | EventListenerOptions | undefined): boolean { + return typeof options === 'boolean' ? options : !!options?.capture; +} + /** * Add an instrumentation handler for when a click or a keypress happens. * @@ -77,15 +86,26 @@ export function instrumentDOM(): void { try { const handlers = (this.__sentry_instrumentation_handlers__ = this.__sentry_instrumentation_handlers__ || {}); - const handlerForType = (handlers[type] = handlers[type] || { refCount: 0 }); + + const handlerForType = (handlers[type] = handlers[type] || { + captureListeners: new Set(), + bubbleListeners: new Set(), + }); + + const capture = getCapture(options); if (!handlerForType.handler) { const handler = makeDOMEventHandler(triggerDOMHandler); handlerForType.handler = handler; - originalAddEventListener.call(this, type, handler, options); + // 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 = capture; + originalAddEventListener.call(this, type, handler, handlerForType.capture); } - handlerForType.refCount++; + handlerForType[capture ? 'captureListeners' : 'bubbleListeners'].add(listener); } catch { // Accessing dom properties is always fragile. // Also allows us to skip `addEventListeners` calls with no proper `this` context. @@ -106,11 +126,11 @@ export function instrumentDOM(): void { const handlers = this.__sentry_instrumentation_handlers__ || {}; const handlerForType = handlers[type]; - if (handlerForType) { - handlerForType.refCount--; + // Removing a listener that was never added is a no-op in the browser, so it mustn't count for ours either. + if (handlerForType?.[getCapture(options) ? 'captureListeners' : 'bubbleListeners'].delete(listener)) { // 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); + if (!handlerForType.captureListeners.size && !handlerForType.bubbleListeners.size) { + 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..54d143fc8950 100644 --- a/packages/browser-utils/test/instrumentation/dom.test.ts +++ b/packages/browser-utils/test/instrumentation/dom.test.ts @@ -1,12 +1,94 @@ -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', () => { + const { addEventListener: nativeAdd, removeEventListener: nativeRemove } = EventTarget.prototype; + + // `instrumentDOM` patches `EventTarget.prototype` and isn't idempotent, so restore the native methods after every test. + afterEach(() => { + EventTarget.prototype.addEventListener = nativeAdd; + EventTarget.prototype.removeEventListener = nativeRemove; + }); + + /** Runs `instrumentDOM` and returns a function counting the click listeners actually attached to `document`. */ + function instrumentAndTrackDocumentClickListeners(): () => number { + const documentClickListeners = { capture: new Set(), bubble: new Set() }; + + const phase = (options?: boolean | EventListenerOptions): Set => + (typeof options === 'boolean' ? options : !!options?.capture) + ? documentClickListeners.capture + : documentClickListeners.bubble; + + // 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') { + phase(options).add(listener); + } + return nativeAdd.call(this, type, listener, options); + }; + + EventTarget.prototype.removeEventListener = function (type, listener, options) { + if (this === document && type === 'click') { + phase(options).delete(listener); + } + return nativeRemove.call(this, type, listener, options); + }; + + instrumentDOM(); + + return () => documentClickListeners.capture.size + documentClickListeners.bubble.size; + } + + 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', () => { + const countDocumentClickListeners = instrumentAndTrackDocumentClickListeners(); + + // baseline listenercount is 1 which comes from the SDK's global click handler registered + // in instrumentDOM(). + const baseline = countDocumentClickListeners(); + + const never = (): void => {}; + const onCapture = (): void => {}; + const onBubble = (): void => {}; + + 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(countDocumentClickListeners() - baseline).toBe(0); + }); + + it('keeps its handler attached while listeners remain, even after removing listeners that were never added', () => { + const countDocumentClickListeners = instrumentAndTrackDocumentClickListeners(); + const baseline = countDocumentClickListeners(); + + const never = (): void => {}; + const onClick = (): void => {}; + + document.addEventListener('click', onClick); + document.removeEventListener('click', never); + document.removeEventListener('click', onClick, true); + + // `onClick` plus the SDK's handler for it + expect(countDocumentClickListeners() - baseline).toBe(2); + + document.removeEventListener('click', onClick); + + expect(countDocumentClickListeners() - baseline).toBe(0); + }); }); From 9e2792579197ccda3c1f91c4e194a624309ab95d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 28 Sep 2026 14:49:45 +0200 Subject: [PATCH 2/5] fix once or signal handlers being added to the set --- .../browser-utils/src/instrumentation/dom.ts | 16 +++++++-- .../test/instrumentation/dom.test.ts | 35 +++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/packages/browser-utils/src/instrumentation/dom.ts b/packages/browser-utils/src/instrumentation/dom.ts index c4e97dcc86a7..f937f6c7158a 100644 --- a/packages/browser-utils/src/instrumentation/dom.ts +++ b/packages/browser-utils/src/instrumentation/dom.ts @@ -26,6 +26,9 @@ type InstrumentedElement = Element & { // listeners added with `capture: false` (default), meaning the listener is invoked // after other listeners inside the element's hierarchy (i.e. the event bubbles up) bubbleListeners: Set; + // Set once a `once` or `signal` listener was added. The browser removes those without going + // through `removeEventListener`, so we can't tell when they're gone and must keep our handler attached. + sticky?: boolean; }; }; }; @@ -105,7 +108,12 @@ export function instrumentDOM(): void { originalAddEventListener.call(this, type, handler, handlerForType.capture); } - handlerForType[capture ? 'captureListeners' : 'bubbleListeners'].add(listener); + if (typeof options === 'object' && (options.once || options.signal)) { + // Not tracked to avoid retaining listeners the browser auto-removes. + handlerForType.sticky = true; + } else { + handlerForType[capture ? 'captureListeners' : 'bubbleListeners'].add(listener); + } } catch { // Accessing dom properties is always fragile. // Also allows us to skip `addEventListeners` calls with no proper `this` context. @@ -129,7 +137,11 @@ export function instrumentDOM(): void { // Removing a listener that was never added is a no-op in the browser, so it mustn't count for ours either. if (handlerForType?.[getCapture(options) ? 'captureListeners' : 'bubbleListeners'].delete(listener)) { // If there are no longer any custom handlers of the current type on this element, we can remove ours, too. - if (!handlerForType.captureListeners.size && !handlerForType.bubbleListeners.size) { + if ( + !handlerForType.sticky && + !handlerForType.captureListeners.size && + !handlerForType.bubbleListeners.size + ) { 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 54d143fc8950..5e3344c81865 100644 --- a/packages/browser-utils/test/instrumentation/dom.test.ts +++ b/packages/browser-utils/test/instrumentation/dom.test.ts @@ -8,6 +8,8 @@ import { WINDOW } from '../../src/types'; // @ts-expect-error - idk WINDOW.XMLHttpRequest = undefined; +type InstrumentedDocument = Document & { __sentry_instrumentation_handlers__?: Record }; + describe('instrumentDOM', () => { const { addEventListener: nativeAdd, removeEventListener: nativeRemove } = EventTarget.prototype; @@ -15,6 +17,7 @@ describe('instrumentDOM', () => { afterEach(() => { EventTarget.prototype.addEventListener = nativeAdd; EventTarget.prototype.removeEventListener = nativeRemove; + delete (document as InstrumentedDocument).__sentry_instrumentation_handlers__; }); /** Runs `instrumentDOM` and returns a function counting the click listeners actually attached to `document`. */ @@ -91,4 +94,36 @@ describe('instrumentDOM', () => { expect(countDocumentClickListeners() - baseline).toBe(0); }); + + it('does not retain listeners added with `once` or `signal`, which the browser removes on its own', () => { + instrumentDOM(); + + for (let i = 0; i < 20; i++) { + const controller = new AbortController(); + document.addEventListener('click', () => {}, { signal: controller.signal }); + document.addEventListener('click', () => {}, { once: true, capture: true }); + controller.abort(); + } + document.dispatchEvent(new MouseEvent('click')); + + const clickHandlers = (document as InstrumentedDocument).__sentry_instrumentation_handlers__?.click; + expect(clickHandlers.bubbleListeners.size).toBe(0); + expect(clickHandlers.captureListeners.size).toBe(0); + expect(clickHandlers.handler).toBeDefined(); + }); + + it('keeps its handler attached after removing tracked listeners if a `once` or `signal` listener was added', () => { + const countDocumentClickListeners = instrumentAndTrackDocumentClickListeners(); + const baseline = countDocumentClickListeners(); + + const onClick = (): void => {}; + const onceClick = (): void => {}; + + document.addEventListener('click', onClick); + document.addEventListener('click', onceClick, { once: true }); + document.removeEventListener('click', onClick); + + // `onceClick` plus the SDK's handler, which must still see the pending `once` listener's event + expect(countDocumentClickListeners() - baseline).toBe(2); + }); }); From 9ed8e5719057034b9bdb943ce11a06b998abcad5 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 30 Sep 2026 10:20:52 +0200 Subject: [PATCH 3/5] fix null option issue --- packages/browser-utils/src/instrumentation/dom.ts | 2 +- .../browser-utils/test/instrumentation/dom.test.ts | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/packages/browser-utils/src/instrumentation/dom.ts b/packages/browser-utils/src/instrumentation/dom.ts index f937f6c7158a..91d96aa48dee 100644 --- a/packages/browser-utils/src/instrumentation/dom.ts +++ b/packages/browser-utils/src/instrumentation/dom.ts @@ -108,7 +108,7 @@ export function instrumentDOM(): void { originalAddEventListener.call(this, type, handler, handlerForType.capture); } - if (typeof options === 'object' && (options.once || options.signal)) { + if (typeof options === 'object' && (options?.once || options?.signal)) { // Not tracked to avoid retaining listeners the browser auto-removes. handlerForType.sticky = true; } else { diff --git a/packages/browser-utils/test/instrumentation/dom.test.ts b/packages/browser-utils/test/instrumentation/dom.test.ts index 5e3344c81865..452dc0f71aa7 100644 --- a/packages/browser-utils/test/instrumentation/dom.test.ts +++ b/packages/browser-utils/test/instrumentation/dom.test.ts @@ -95,6 +95,20 @@ describe('instrumentDOM', () => { expect(countDocumentClickListeners() - baseline).toBe(0); }); + it('removes its handler when listeners are added and removed with `null` options', () => { + const countDocumentClickListeners = instrumentAndTrackDocumentClickListeners(); + const baseline = countDocumentClickListeners(); + + const onClick = (): void => {}; + + // @ts-expect-error - `null` is valid at runtime and treated like default options + document.addEventListener('click', onClick, null); + // @ts-expect-error - see above + document.removeEventListener('click', onClick, null); + + expect(countDocumentClickListeners() - baseline).toBe(0); + }); + it('does not retain listeners added with `once` or `signal`, which the browser removes on its own', () => { instrumentDOM(); From f1b673549cd1e1e804fedddac065f96b61d3cd49 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 30 Sep 2026 14:47:14 +0200 Subject: [PATCH 4/5] switch to weakset --- .../browser-utils/src/instrumentation/dom.ts | 30 +++++++++++-------- .../test/instrumentation/dom.test.ts | 3 +- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/packages/browser-utils/src/instrumentation/dom.ts b/packages/browser-utils/src/instrumentation/dom.ts index 91d96aa48dee..134ac9acf621 100644 --- a/packages/browser-utils/src/instrumentation/dom.ts +++ b/packages/browser-utils/src/instrumentation/dom.ts @@ -22,10 +22,13 @@ type InstrumentedElement = Element & { capture?: boolean; // listeners added with `capture: true`, meaning the listener is invoked before other // listeners inside the element's hierarchy. - captureListeners: Set; + captureListeners: WeakSet; // listeners added with `capture: false` (default), meaning the listener is invoked // after other listeners inside the element's hierarchy (i.e. the event bubbles up) - bubbleListeners: Set; + bubbleListeners: WeakSet; + // Total number of listeners in `captureListeners` and `bubbleListeners`. WeakSets have no `size`, but we use them + // so listeners removed without going through our `removeEventListener` patch aren't retained by us. + listenerCount: number; // Set once a `once` or `signal` listener was added. The browser removes those without going // through `removeEventListener`, so we can't tell when they're gone and must keep our handler attached. sticky?: boolean; @@ -85,14 +88,16 @@ export function instrumentDOM(): void { fill(proto, 'addEventListener', function (originalAddEventListener: AddEventListener): AddEventListener { return function (this: InstrumentedElement, type, listener, options): AddEventListener { - if (type === 'click' || type == 'keypress') { + // The browser ignores `null` listeners, so there's nothing for our handler to accompany. + if ((type === 'click' || type == 'keypress') && listener) { try { const handlers = (this.__sentry_instrumentation_handlers__ = this.__sentry_instrumentation_handlers__ || {}); const handlerForType = (handlers[type] = handlers[type] || { - captureListeners: new Set(), - bubbleListeners: new Set(), + captureListeners: new WeakSet(), + bubbleListeners: new WeakSet(), + listenerCount: 0, }); const capture = getCapture(options); @@ -112,7 +117,12 @@ export function instrumentDOM(): void { // Not tracked to avoid retaining listeners the browser auto-removes. handlerForType.sticky = true; } else { - handlerForType[capture ? 'captureListeners' : 'bubbleListeners'].add(listener); + const listeners = handlerForType[capture ? 'captureListeners' : 'bubbleListeners']; + // Adding the same listener twice in the same phase is a no-op in the browser. + if (!listeners.has(listener)) { + listeners.add(listener); + handlerForType.listenerCount++; + } } } catch { // Accessing dom properties is always fragile. @@ -129,7 +139,7 @@ export function instrumentDOM(): void { 'removeEventListener', function (originalRemoveEventListener: RemoveEventListener): RemoveEventListener { return function (this: InstrumentedElement, type, listener, options): () => void { - if (type === 'click' || type == 'keypress') { + if ((type === 'click' || type == 'keypress') && listener) { try { const handlers = this.__sentry_instrumentation_handlers__ || {}; const handlerForType = handlers[type]; @@ -137,11 +147,7 @@ export function instrumentDOM(): void { // Removing a listener that was never added is a no-op in the browser, so it mustn't count for ours either. if (handlerForType?.[getCapture(options) ? 'captureListeners' : 'bubbleListeners'].delete(listener)) { // If there are no longer any custom handlers of the current type on this element, we can remove ours, too. - if ( - !handlerForType.sticky && - !handlerForType.captureListeners.size && - !handlerForType.bubbleListeners.size - ) { + if (!--handlerForType.listenerCount && !handlerForType.sticky) { 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 452dc0f71aa7..ee67b729bab9 100644 --- a/packages/browser-utils/test/instrumentation/dom.test.ts +++ b/packages/browser-utils/test/instrumentation/dom.test.ts @@ -121,8 +121,7 @@ describe('instrumentDOM', () => { document.dispatchEvent(new MouseEvent('click')); const clickHandlers = (document as InstrumentedDocument).__sentry_instrumentation_handlers__?.click; - expect(clickHandlers.bubbleListeners.size).toBe(0); - expect(clickHandlers.captureListeners.size).toBe(0); + expect(clickHandlers.listenerCount).toBe(0); expect(clickHandlers.handler).toBeDefined(); }); From d55f18f55fdaa0de1621792cd99129c730085c9d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 30 Sep 2026 14:57:26 +0200 Subject: [PATCH 5/5] fix more edge cases and add tests, fix typo --- .../browser-utils/src/instrumentation/dom.ts | 16 +++++------ .../test/instrumentation/dom.test.ts | 27 +++++++++++++++++++ 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/packages/browser-utils/src/instrumentation/dom.ts b/packages/browser-utils/src/instrumentation/dom.ts index 134ac9acf621..9a9ca9ad265a 100644 --- a/packages/browser-utils/src/instrumentation/dom.ts +++ b/packages/browser-utils/src/instrumentation/dom.ts @@ -107,19 +107,19 @@ export function instrumentDOM(): void { 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 + // Our listener needs to have the same capture setting, so that subsequent calls or removeEventListener // calls correspond to the correct handler function. handlerForType.capture = capture; originalAddEventListener.call(this, type, handler, handlerForType.capture); } - if (typeof options === 'object' && (options?.once || options?.signal)) { - // Not tracked to avoid retaining listeners the browser auto-removes. - handlerForType.sticky = true; - } else { - const listeners = handlerForType[capture ? 'captureListeners' : 'bubbleListeners']; - // Adding the same listener twice in the same phase is a no-op in the browser. - if (!listeners.has(listener)) { + const listeners = handlerForType[capture ? 'captureListeners' : 'bubbleListeners']; + // Adding the same listener twice in the same phase is a no-op in the browser. + if (!listeners.has(listener)) { + if (typeof options === 'object' && (options?.once || options?.signal)) { + // Not tracked to avoid retaining listeners the browser auto-removes. + handlerForType.sticky = true; + } else { listeners.add(listener); handlerForType.listenerCount++; } diff --git a/packages/browser-utils/test/instrumentation/dom.test.ts b/packages/browser-utils/test/instrumentation/dom.test.ts index ee67b729bab9..3fc235776158 100644 --- a/packages/browser-utils/test/instrumentation/dom.test.ts +++ b/packages/browser-utils/test/instrumentation/dom.test.ts @@ -139,4 +139,31 @@ describe('instrumentDOM', () => { // `onceClick` plus the SDK's handler, which must still see the pending `once` listener's event expect(countDocumentClickListeners() - baseline).toBe(2); }); + + it('counts a listener added twice in the same phase only once', () => { + const countDocumentClickListeners = instrumentAndTrackDocumentClickListeners(); + const baseline = countDocumentClickListeners(); + + const onClick = (): void => {}; + + document.addEventListener('click', onClick); + document.addEventListener('click', onClick); + document.removeEventListener('click', onClick); + + expect(countDocumentClickListeners() - baseline).toBe(0); + }); + + it('removes its handler if a listener is re-added with `once` while already registered', () => { + const countDocumentClickListeners = instrumentAndTrackDocumentClickListeners(); + const baseline = countDocumentClickListeners(); + + const onClick = (): void => {}; + + document.addEventListener('click', onClick); + // no-op in the browser, since `onClick` is already registered for the bubble phase + document.addEventListener('click', onClick, { once: true }); + document.removeEventListener('click', onClick); + + expect(countDocumentClickListeners() - baseline).toBe(0); + }); });