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); + }); });