diff --git a/packages/browser-utils/src/instrumentation/dom.ts b/packages/browser-utils/src/instrumentation/dom.ts index d326fd281ef2..521038334ba6 100644 --- a/packages/browser-utils/src/instrumentation/dom.ts +++ b/packages/browser-utils/src/instrumentation/dom.ts @@ -18,9 +18,9 @@ type RemoveEventListener = ( type InstrumentedElement = Element & { __sentry_instrumentation_handlers__?: { [key in 'click' | 'keypress']?: { - handler?: unknown; - /** The number of custom listeners attached to this element */ - refCount: number; + handler?: EventListenerOrEventListenerObject; + capture?: boolean; + listeners: Map>; }; }; }; @@ -31,6 +31,48 @@ let debounceTimerID: number | undefined; let lastCapturedEventType: string | undefined; let lastCapturedEventTargetId: string | undefined; +function getCapture(options: boolean | EventListenerOptions | AddEventListenerOptions | undefined): boolean { + return typeof options === 'boolean' ? options : !!options?.capture; +} + +function hasListener( + handler: { listeners: Map> }, + listener: EventListenerOrEventListenerObject, + capture: boolean, +): boolean { + return handler.listeners.get(listener)?.has(capture) ?? false; +} + +function trackListener( + handler: { listeners: Map> }, + listener: EventListenerOrEventListenerObject, + capture: boolean, +): void { + const captures = handler.listeners.get(listener); + if (captures) { + captures.add(capture); + } else { + handler.listeners.set(listener, new Set([capture])); + } +} + +function untrackListener( + handler: { listeners: Map> }, + listener: EventListenerOrEventListenerObject, + capture: boolean, +): boolean { + const captures = handler.listeners.get(listener); + if (!captures?.delete(capture)) { + return false; + } + + if (captures.size === 0) { + handler.listeners.delete(listener); + } + + return true; +} + /** * Add an instrumentation handler for when a click or a keypress happens. * @@ -77,15 +119,21 @@ 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] || { + listeners: new Map>(), + }); + const capture = getCapture(options); + + if (!hasListener(handlerForType, listener, capture)) { + if (!handlerForType.handler) { + const handler = makeDOMEventHandler(triggerDOMHandler); + originalAddEventListener.call(this, type, handler, capture); + handlerForType.handler = handler; + handlerForType.capture = capture; + } - if (!handlerForType.handler) { - const handler = makeDOMEventHandler(triggerDOMHandler); - handlerForType.handler = handler; - originalAddEventListener.call(this, type, handler, options); + trackListener(handlerForType, listener, capture); } - - handlerForType.refCount++; } catch { // Accessing dom properties is always fragile. // Also allows us to skip `addEventListeners` calls with no proper `this` context. @@ -106,11 +154,10 @@ export function instrumentDOM(): void { const handlers = this.__sentry_instrumentation_handlers__ || {}; const handlerForType = handlers[type]; - if (handlerForType) { - handlerForType.refCount--; + if (handlerForType && untrackListener(handlerForType, listener, getCapture(options))) { // 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.listeners.size === 0) { + 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..29596384f0fc 100644 --- a/packages/browser-utils/test/instrumentation/dom.test.ts +++ b/packages/browser-utils/test/instrumentation/dom.test.ts @@ -1,12 +1,47 @@ -import { describe, expect, it } from 'vitest'; +/** @vitest-environment jsdom */ + +import { describe, expect, it, vi } 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', () => { - expect(instrumentDOM).not.toThrow(); +describe('instrumentDOM', () => { + it('does not leak its handler when removing unregistered listeners', () => { + const addEventListener = vi.spyOn(EventTarget.prototype, 'addEventListener'); + const removeEventListener = vi.spyOn(EventTarget.prototype, 'removeEventListener'); + + try { + expect(instrumentDOM).not.toThrow(); + + const target = new EventTarget(); + const onCapture = (): void => {}; + const onBubble = (): void => {}; + const neverRegistered = (): void => {}; + const addCallsBeforeTest = addEventListener.mock.calls.length; + + for (let i = 0; i < 20; i++) { + target.addEventListener('click', onCapture, true); + target.addEventListener('click', onBubble); + target.removeEventListener('click', neverRegistered); + target.removeEventListener('click', neverRegistered); + target.removeEventListener('click', onCapture, true); + target.removeEventListener('click', onBubble); + } + + const instrumentedHandlers = addEventListener.mock.calls + .slice(addCallsBeforeTest) + .map(([, listener]) => listener) + .filter(listener => listener !== onCapture && listener !== onBubble); + + expect(instrumentedHandlers).toHaveLength(20); + for (const handler of instrumentedHandlers) { + expect(removeEventListener).toHaveBeenCalledWith('click', handler, true); + } + } finally { + addEventListener.mockRestore(); + removeEventListener.mockRestore(); + } }); });