diff --git a/packages/browser-utils/src/instrumentation/dom.ts b/packages/browser-utils/src/instrumentation/dom.ts index d326fd281ef2..9a9ca9ad265a 100644 --- a/packages/browser-utils/src/instrumentation/dom.ts +++ b/packages/browser-utils/src/instrumentation/dom.ts @@ -19,8 +19,19 @@ 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: 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: 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; }; }; }; @@ -31,6 +42,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. * @@ -73,19 +88,42 @@ 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] || { refCount: 0 }); + + const handlerForType = (handlers[type] = handlers[type] || { + captureListeners: new WeakSet(), + bubbleListeners: new WeakSet(), + listenerCount: 0, + }); + + 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 removeEventListener + // calls correspond to the correct handler function. + handlerForType.capture = capture; + originalAddEventListener.call(this, type, handler, handlerForType.capture); } - handlerForType.refCount++; + 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++; + } + } } catch { // Accessing dom properties is always fragile. // Also allows us to skip `addEventListeners` calls with no proper `this` context. @@ -101,16 +139,16 @@ 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]; - 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.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 23681014150b..3fc235776158 100644 --- a/packages/browser-utils/test/instrumentation/dom.test.ts +++ b/packages/browser-utils/test/instrumentation/dom.test.ts @@ -1,12 +1,169 @@ -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', () => { +type InstrumentedDocument = Document & { __sentry_instrumentation_handlers__?: Record }; + +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; + delete (document as InstrumentedDocument).__sentry_instrumentation_handlers__; + }); + + /** 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); + }); + + 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(); + + 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.listenerCount).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); + }); + + 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); + }); });