fix(browser-utils): Remove readystatechange listener to prevent memory leaks (#22216)

Remove the `readystatechange` event listener once an XHR request reaches
`readyState === 4` (done).
Previously, the listener added in the instrumented `open()` was never
removed, so every XHR kept its listener

This is problematic on low-end TV devices, during video playback the
player continuously triggers network requests (often several per
second), so on long-lived, memory-constrained devices these leaked
listeners and pinned objects accumulate and steadily grow memory usage

---------

Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>

Author: dobladov

packages/browser-utils/src/instrument/xhr.ts

@@ -90,6 +90,14 @@ export function instrumentXHR(): void {
             virtualError,
           };
           triggerHandlers('xhr', handlerData);
+
+          // In the `addEventListener` branch below, this handler is the only
+          // `readystatechange` listener we add, so detach it once the request is
+          // done to avoid pinning the XMLHttpRequest and its captured
+          // `virtualError` per HTTP call on long-lived pages. In the
+          // `onreadystatechange` proxy branch the handler isn't registered via
+          // `addEventListener`, so this is a harmless no-op there.
+          xhrOpenThisArg.removeEventListener('readystatechange', onreadystatechangeHandler);
         }
       };
 

packages/browser-utils/test/instrument/xhr.test.ts

@@ -1,12 +1,133 @@
-import { describe, expect, it } from 'vitest';
-import { instrumentXHR } from '../../src/instrument/xhr';
+import type { HandlerDataXhr } from '@sentry/core';
+import { afterEach, describe, expect, it, vi } from 'vitest';
+import { addXhrInstrumentationHandler, instrumentXHR } from '../../src/instrument/xhr';
 import { WINDOW } from '../../src/types';
 
-// @ts-expect-error - idk
-WINDOW.XMLHttpRequest = undefined;
+const win = WINDOW as typeof WINDOW & { XMLHttpRequest?: typeof XMLHttpRequest };
+const originalXMLHttpRequest = win.XMLHttpRequest;
 
 describe('instrumentXHR', () => {
-  it('it does not throw if XMLHttpRequest is a key on window but not defined', () => {
+  afterEach(() => {
+    win.XMLHttpRequest = originalXMLHttpRequest;
+  });
+
+  it('does not throw if XMLHttpRequest is a key on window but not defined', () => {
+    win.XMLHttpRequest = undefined;
     expect(instrumentXHR).not.toThrow();
   });
+
+  it('removes readystatechange event listener on readyState 4 when registered with addEventListener', () => {
+    const addEventListenerSpy = vi.fn();
+    const removeEventListenerSpy = vi.fn();
+
+    class MockXMLHttpRequest {
+      public readyState: number = 0;
+      public status: number = 200;
+      public addEventListener = addEventListenerSpy;
+      public removeEventListener = removeEventListenerSpy;
+      public open(_method: string, _url: string): void {}
+      public send(): void {}
+      public setRequestHeader(_header: string, _value: string): void {}
+    }
+
+    win.XMLHttpRequest = MockXMLHttpRequest as unknown as typeof XMLHttpRequest;
+
+    instrumentXHR();
+
+    const xhr = new MockXMLHttpRequest();
+    xhr.open('GET', 'http://example.com');
+
+    expect(addEventListenerSpy).toHaveBeenCalledWith('readystatechange', expect.any(Function));
+    const handler = addEventListenerSpy.mock.calls[0]?.[1] as (this: MockXMLHttpRequest) => void;
+
+    xhr.readyState = 2;
+    handler.call(xhr);
+    expect(removeEventListenerSpy).not.toHaveBeenCalled();
+
+    xhr.readyState = 4;
+    handler.call(xhr);
+    expect(removeEventListenerSpy).toHaveBeenCalledWith('readystatechange', handler);
+  });
+
+  it('still reports the xhr completion (with endTimestamp) after removing the listener', () => {
+    const addEventListenerSpy = vi.fn();
+    const removeEventListenerSpy = vi.fn();
+
+    class MockXMLHttpRequest {
+      public readyState: number = 0;
+      public status: number = 200;
+      public addEventListener = addEventListenerSpy;
+      public removeEventListener = removeEventListenerSpy;
+      public open(_method: string, _url: string): void {}
+      public send(): void {}
+      public setRequestHeader(_header: string, _value: string): void {}
+    }
+
+    win.XMLHttpRequest = MockXMLHttpRequest as unknown as typeof XMLHttpRequest;
+
+    const instrumentationHandler = vi.fn();
+    addXhrInstrumentationHandler(instrumentationHandler);
+    instrumentXHR();
+
+    const xhr = new MockXMLHttpRequest();
+    xhr.open('GET', 'http://example.com');
+
+    const handler = addEventListenerSpy.mock.calls[0]?.[1] as (this: MockXMLHttpRequest) => void;
+
+    xhr.readyState = 4;
+    handler.call(xhr);
+
+    const completionCall = instrumentationHandler.mock.calls.find(
+      ([data]: [HandlerDataXhr]) => data.endTimestamp !== undefined,
+    );
+    expect(completionCall).toBeDefined();
+    expect(completionCall?.[0].startTimestamp).toBeDefined();
+
+    // completion must be reported before we detach the listener, so a throwing
+    // `removeEventListener` could never silently drop the completion event
+    const completionOrder = Math.min(...instrumentationHandler.mock.invocationCallOrder);
+    expect(removeEventListenerSpy.mock.invocationCallOrder[0]).toBeGreaterThan(completionOrder);
+  });
+
+  it('does not remove a listener via addEventListener when onreadystatechange is used', () => {
+    const addEventListenerSpy = vi.fn();
+    const removeEventListenerSpy = vi.fn();
+
+    class MockXMLHttpRequest {
+      public readyState: number = 0;
+      public status: number = 200;
+      public onreadystatechange: (() => void) | null = null;
+      public addEventListener = addEventListenerSpy;
+      public removeEventListener = removeEventListenerSpy;
+      public open(_method: string, _url: string): void {}
+      public send(): void {}
+      public setRequestHeader(_header: string, _value: string): void {}
+    }
+
+    win.XMLHttpRequest = MockXMLHttpRequest as unknown as typeof XMLHttpRequest;
+
+    const handlerData: HandlerDataXhr[] = [];
+    addXhrInstrumentationHandler(data => {
+      handlerData.push(data);
+    });
+    instrumentXHR();
+
+    const xhr = new MockXMLHttpRequest();
+    const originalOnReadyStateChange = vi.fn();
+    xhr.onreadystatechange = originalOnReadyStateChange;
+    xhr.open('GET', 'http://example.com');
+
+    // onreadystatechange path must not register a separate readystatechange listener
+    expect(addEventListenerSpy).not.toHaveBeenCalled();
+
+    // the SDK wraps the user's handler rather than replacing it
+    expect(xhr.onreadystatechange).not.toBe(originalOnReadyStateChange);
+
+    xhr.readyState = 4;
+    xhr.onreadystatechange?.();
+
+    // the user's original handler still runs and completion is still reported
+    expect(originalOnReadyStateChange).toHaveBeenCalledTimes(1);
+    expect(handlerData.some(data => data.endTimestamp !== undefined)).toBe(true);
+  });
 });