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

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

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