diff --git a/packages/browser-utils/src/instrumentation/dom.ts b/packages/browser-utils/src/instrumentation/dom.ts index d326fd281ef2..b2f8326f35eb 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,12 @@ export function instrumentDOM(): void { 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 = typeof options === 'boolean' ? options : !!options?.capture; + originalAddEventListener.call(this, type, handler, handlerForType.capture); } handlerForType.refCount++; @@ -110,7 +116,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..fff88dd49278 100644 --- a/packages/browser-utils/test/instrumentation/dom.test.ts +++ b/packages/browser-utils/test/instrumentation/dom.test.ts @@ -1,12 +1,68 @@ -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; + }); + + 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 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(); + + // 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 = (): 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(documentClickListeners.capture.size + documentClickListeners.bubble.size - baseline).toBe(0); + }); });