fix(browser): Fix memory leak in addEventListener instrumentation (#5147)

Author: lforst

packages/browser/src/helpers.ts

@@ -32,7 +32,8 @@ export function ignoreNextOnError(): void {
  * Instruments the given function and sends an event to Sentry every time the
  * function throws an exception.
  *
- * @param fn A function to wrap.
+ * @param fn A function to wrap. It is generally safe to pass an unbound function, because the returned wrapper always
+ * has a correct `this` context.
  * @returns The wrapped function.
  * @hidden
  */
@@ -75,8 +76,8 @@ export function wrap(
   }
 
   /* eslint-disable prefer-rest-params */
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  const sentryWrapped: WrappedFunction = function (this: any): void {
+  // It is important that `sentryWrapped` is not an arrow function to preserve the context of `this`
+  const sentryWrapped: WrappedFunction = function (this: unknown): void {
     const args = Array.prototype.slice.call(arguments);
 
     try {

packages/browser/src/integrations/trycatch.ts

@@ -208,7 +208,13 @@ function _wrapEventTarget(target: string): void {
     ): (eventName: string, fn: EventListenerObject, capture?: boolean, secure?: boolean) => void {
       try {
         if (typeof fn.handleEvent === 'function') {
-          fn.handleEvent = wrap(fn.handleEvent.bind(fn), {
+          // ESlint disable explanation:
+          //  First, it is generally safe to call `wrap` with an unbound function. Furthermore, using `.bind()` would
+          //  introduce a bug here, because bind returns a new function that doesn't have our
+          //  flags(like __sentry_original__) attached. `wrap` checks for those flags to avoid unnecessary wrapping.
+          //  Without those flags, every call to addEventListener wraps the function again, causing a memory leak.
+          // eslint-disable-next-line @typescript-eslint/unbound-method
+          fn.handleEvent = wrap(fn.handleEvent, {
             mechanism: {
               data: {
                 function: 'handleEvent',

packages/integration-tests/suites/public-api/instrumentation/eventListener-instrumentation-behaviour/subject.js

@@ -0,0 +1,21 @@
+// Simple function event listener
+const functionListener = () => {
+  functionListenerCallback();
+};
+
+// Attach event listener twice
+window.addEventListener('click', functionListener);
+window.addEventListener('click', functionListener);
+
+// Event listener that has handleEvent() method: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#listener
+class EventHandlerClass {
+  handleEvent() {
+    objectListenerCallback();
+  }
+}
+
+const objectListener = new EventHandlerClass();
+
+// Attach event listener twice
+window.addEventListener('click', objectListener);
+window.addEventListener('click', objectListener);

packages/integration-tests/suites/public-api/instrumentation/eventListener-instrumentation-behaviour/test.ts

@@ -0,0 +1,28 @@
+import { expect } from '@playwright/test';
+
+import { sentryTest } from '../../../../utils/fixtures';
+
+sentryTest(
+  'Event listener instrumentation should attach the same event listener only once',
+  async ({ getLocalTestPath, page }) => {
+    const url = await getLocalTestPath({ testDir: __dirname });
+    await page.goto(url);
+
+    let functionListenerCalls = 0;
+    await page.exposeFunction('functionListenerCallback', () => {
+      functionListenerCalls = functionListenerCalls + 1;
+    });
+
+    let objectListenerCalls = 0;
+    await page.exposeFunction('objectListenerCallback', () => {
+      objectListenerCalls = objectListenerCalls + 1;
+    });
+
+    // Trigger event listeners twice
+    await page.evaluate('document.body.click()');
+    await page.evaluate('document.body.click()');
+
+    expect(functionListenerCalls).toBe(2);
+    expect(objectListenerCalls).toBe(2);
+  },
+);

packages/integration-tests/suites/public-api/instrumentation/eventListener-this-preservation/subject.js

@@ -0,0 +1,24 @@
+const btn = document.createElement('button');
+btn.id = 'btn';
+document.body.appendChild(btn);
+
+const functionListener = function () {
+  functionCallback(this.constructor.name);
+};
+
+class EventHandlerClass {
+  handleEvent() {
+    classInstanceCallback(this.constructor.name);
+  }
+}
+const objectListener = new EventHandlerClass();
+
+// Attach event listeners a few times for good measure
+
+btn.addEventListener('click', functionListener);
+btn.addEventListener('click', functionListener);
+btn.addEventListener('click', functionListener);
+
+btn.addEventListener('click', objectListener);
+btn.addEventListener('click', objectListener);
+btn.addEventListener('click', objectListener);

packages/integration-tests/suites/public-api/instrumentation/eventListener-this-preservation/test.ts

@@ -0,0 +1,24 @@
+import { expect } from '@playwright/test';
+
+import { sentryTest } from '../../../../utils/fixtures';
+
+sentryTest('Event listener instrumentation preserves "this" context', async ({ getLocalTestPath, page }) => {
+  const url = await getLocalTestPath({ testDir: __dirname });
+  await page.goto(url);
+
+  let assertions = 0;
+
+  await page.exposeFunction('functionCallback', (thisInstanceName: unknown) => {
+    expect(thisInstanceName).toBe('HTMLButtonElement');
+    assertions = assertions + 1;
+  });
+
+  await page.exposeFunction('classInstanceCallback', (thisInstanceName: unknown) => {
+    expect(thisInstanceName).toBe('EventHandlerClass');
+    assertions = assertions + 1;
+  });
+
+  await page.evaluate('document.getElementById("btn").click()');
+
+  expect(assertions).toBe(2);
+});

packages/integration-tests/suites/public-api/instrumentation/eventListener-wrapping/subject.js

@@ -0,0 +1,18 @@
+// Simple function event listener
+const functionListener = () => {
+  reportFunctionListenerStackHeight(new Error().stack.split('\n').length);
+};
+
+// Event listener that has handleEvent() method: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#listener
+class EventHandlerClass {
+  handleEvent() {
+    reportObjectListenerStackHeight(new Error().stack.split('\n').length);
+  }
+}
+
+const objectListener = new EventHandlerClass();
+
+window.attachListeners = function () {
+  window.addEventListener('click', functionListener);
+  window.addEventListener('click', objectListener);
+};

packages/integration-tests/suites/public-api/instrumentation/eventListener-wrapping/test.ts

@@ -0,0 +1,38 @@
+import { expect } from '@playwright/test';
+
+import { sentryTest } from '../../../../utils/fixtures';
+
+sentryTest(
+  'Event listener instrumentation should not wrap event listeners multiple times',
+  async ({ getLocalTestPath, page }) => {
+    const url = await getLocalTestPath({ testDir: __dirname });
+    await page.goto(url);
+
+    const functionListenerStackHeights: number[] = [];
+    const objectListenerStackHeights: number[] = [];
+
+    await page.exposeFunction('reportFunctionListenerStackHeight', (height: number) => {
+      functionListenerStackHeights.push(height);
+    });
+
+    await page.exposeFunction('reportObjectListenerStackHeight', (height: number) => {
+      objectListenerStackHeights.push(height);
+    });
+
+    // Attach initial listeners
+    await page.evaluate('window.attachListeners()');
+    await page.evaluate('document.body.click()');
+
+    await page.evaluate('window.attachListeners()');
+    await page.evaluate('window.attachListeners()');
+    await page.evaluate('window.attachListeners()');
+    await page.evaluate('document.body.click()');
+
+    expect(functionListenerStackHeights).toHaveLength(2);
+    expect(objectListenerStackHeights).toHaveLength(2);
+
+    // check if all error stack traces are the same height
+    expect(functionListenerStackHeights.every((val, _i, arr) => val === arr[0])).toBeTruthy();
+    expect(objectListenerStackHeights.every((val, _i, arr) => val === arr[0])).toBeTruthy();
+  },
+);

packages/integration-tests/suites/public-api/instrumentation/eventListener/subject.js

@@ -0,0 +1,5 @@
+window.addEventListener('click', () => {
+  throw new Error('event_listener_error');
+});
+
+document.body.click();

packages/integration-tests/suites/public-api/instrumentation/eventListener/test.ts

@@ -0,0 +1,27 @@
+import { expect } from '@playwright/test';
+import { Event } from '@sentry/types';
+
+import { sentryTest } from '../../../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers';
+
+sentryTest(
+  'Event listener instrumentation should capture an error thrown in an event handler',
+  async ({ getLocalTestPath, page }) => {
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
+
+    expect(eventData.exception?.values).toHaveLength(1);
+    expect(eventData.exception?.values?.[0]).toMatchObject({
+      type: 'Error',
+      value: 'event_listener_error',
+      mechanism: {
+        type: 'instrument',
+        handled: true,
+      },
+      stacktrace: {
+        frames: expect.any(Array),
+      },
+    });
+  },
+);

packages/integration-tests/suites/public-api/instrumentation/init.js

@@ -0,0 +1,7 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+});