From 171dec61d4f25b33b589200ab3ad2ea1c1ef3e95 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 25 Sep 2026 10:55:12 +0200 Subject: [PATCH] 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); + }); });