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

This PR fixes two (related) problems in our `instrumentDOM` event
listener instrumentation:

TIL about event listener
[capture](https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#usecapture)
modes.

1. We didn't differentiate between `addEventListener(fn, {capture:
true})` and `addEventListener(fn, {capture: false})` calls, causing
leakage of our event listeners when event listeners were removed with
different options.

=> Fixed by checking the `capture` option, registering our own listeners
in the same capture config and keeping the capture option on the meta
object of the event target so that we can then remove it in the correct
capture config

2. More generally fixes an issue with `refCount` where e.g. calling
`removeEventListener` with a callback that was never added via
`addEventListener`: Browsers just ignore this call but our refCount was
decremented anyway.

=> Fixed by replacing the general ref count with two sets of callbacks
(for both capture modes) and only removing our listener if all user-set
listeners were removed

Fixes #24702

supersedes #24725
supersedes #24723

Author: Lms24

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