fix(browser-utils): Stop leaking DOM instrumentation listeners on mismatched removals

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<EventListenerOrEventListenerObject>;
+      // 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<EventListenerOrEventListenerObject>;
+      // 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
                 }

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<string, any> };
+
+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<unknown>(), bubble: new Set<unknown>() };
+
+    const phase = (options?: boolean | EventListenerOptions): Set<unknown> =>
+      (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);
+  });
 });