diff --git a/packages/browser-utils/src/instrument/xhr.ts b/packages/browser-utils/src/instrument/xhr.ts index 9775a5441da7..3ad7ec8d0d4a 100644 --- a/packages/browser-utils/src/instrument/xhr.ts +++ b/packages/browser-utils/src/instrument/xhr.ts @@ -42,7 +42,7 @@ export function instrumentXHR(): void { // have a stack trace. If you are using HttpClient integration, // this is the expected behavior, as we are using this virtual error to capture // the location of your XHR call, and group your HttpClient events accordingly. - const virtualError = new Error(); + let virtualError: Error | undefined = new Error(); const startTimestamp = timestampInSeconds() * 1000; @@ -91,12 +91,13 @@ export function instrumentXHR(): void { }; 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. + // An unformatted stack keeps its raw frames, and each frame keeps its receiver alive. For a request + // opened from the previous one's `readystatechange` callback that receiver is the previous + // XMLHttpRequest, so holding on would chain every completed request to the one still in flight. + virtualError = undefined; + + // In the `addEventListener` branch below this is the only `readystatechange` listener we add, so + // detach it once the request is done. It's a no-op in the `onreadystatechange` proxy branch. xhrOpenThisArg.removeEventListener('readystatechange', onreadystatechangeHandler); } }; diff --git a/packages/browser-utils/test/instrument/xhrRetention.test.ts b/packages/browser-utils/test/instrument/xhrRetention.test.ts new file mode 100644 index 000000000000..9321aed18be5 --- /dev/null +++ b/packages/browser-utils/test/instrument/xhrRetention.test.ts @@ -0,0 +1,94 @@ +import { setFlagsFromString } from 'node:v8'; +import { runInNewContext } from 'node:vm'; +import { afterEach, describe, expect, it } from 'vitest'; +import { instrumentXHR } from '../../src/instrument/xhr'; +import { WINDOW } from '../../src/types'; + +// Own file on purpose: instrumentation handlers live in a module-level registry that is never torn +// down, so a handler from another test would retain the requests this one needs to see collected. + +const win = WINDOW as typeof WINDOW & { XMLHttpRequest?: typeof XMLHttpRequest }; +const originalXMLHttpRequest = win.XMLHttpRequest; + +function collectGarbage(): void { + setFlagsFromString('--expose-gc'); + const gc = runInNewContext('gc') as () => void; + setFlagsFromString('--no-expose-gc'); + gc(); + gc(); +} + +class MockXMLHttpRequest { + public readyState: number = 0; + public status: number = 200; + private _listeners: Array<() => void> = []; + + public addEventListener(_type: string, listener: () => void): void { + this._listeners.push(listener); + } + + public removeEventListener(_type: string, listener: () => void): void { + this._listeners = this._listeners.filter(registered => registered !== listener); + } + + public dispatch(): void { + // the SDK detaches its own listener while it runs, so iterate over a copy + for (const listener of this._listeners.slice()) { + // the browser calls listeners with the request as `this`, which is what puts the request into + // the stack frames of anything the listener calls + listener.call(this); + } + } + + public open(_method: string, _url: string): void {} + public send(): void {} + public setRequestHeader(_header: string, _value: string): void {} +} + +describe('instrumentXHR memory retention', () => { + afterEach(() => { + win.XMLHttpRequest = originalXMLHttpRequest; + }); + + it('does not retain completed requests that were chained from readystatechange', async () => { + win.XMLHttpRequest = MockXMLHttpRequest as unknown as typeof XMLHttpRequest; + instrumentXHR(); + + let firstRequest: WeakRef | undefined; + // the request that never finishes stands in for the in-flight one, which the browser roots as a + // pending activity + let inFlightRequest: MockXMLHttpRequest | undefined; + + const openRequest = (remaining: number): void => { + const xhr = new MockXMLHttpRequest(); + xhr.open('GET', 'http://example.com'); + + if (!firstRequest) { + firstRequest = new WeakRef(xhr); + } + + if (remaining === 0) { + inFlightRequest = xhr; + return; + } + + xhr.addEventListener('readystatechange', function (this: MockXMLHttpRequest) { + if (this.readyState === 4) { + openRequest(remaining - 1); + } + }); + + xhr.readyState = 4; + xhr.dispatch(); + }; + + openRequest(4); + + // a WeakRef created in the current job is never cleared during that job + await new Promise(resolve => setTimeout(resolve, 0)); + collectGarbage(); + + expect(inFlightRequest).toBeDefined(); + expect(firstRequest?.deref()).toBeUndefined(); + }); +});