From 67836196e0919f38c6538bfd562473868481145a Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Wed, 9 Sep 2026 23:27:06 -0400 Subject: [PATCH 1/2] fix(browser): Release the XHR `virtualError` once the request completed `instrumentXHR` captures a `new Error()` per `open()` call so the HttpClient integration can report where a failed request came from. Nothing ever formats that stack in the common case, so V8 keeps the raw frames, and a raw frame keeps its receiver alive. When a request is opened from the `readystatechange` callback of the previous one, the receiver of one of those frames is the previous XMLHttpRequest, so every completed request in the chain stays reachable from the one currently in flight. The error is only needed while the completion handlers run, so drop it right after. Fixes #24249 --- .../browser-utils/src/instrumentation/xhr.ts | 16 +++- .../test/instrumentation/xhrRetention.test.ts | 95 +++++++++++++++++++ 2 files changed, 106 insertions(+), 5 deletions(-) create mode 100644 packages/browser-utils/test/instrumentation/xhrRetention.test.ts diff --git a/packages/browser-utils/src/instrumentation/xhr.ts b/packages/browser-utils/src/instrumentation/xhr.ts index 6887723ae1d3..6325b31bfb03 100644 --- a/packages/browser-utils/src/instrumentation/xhr.ts +++ b/packages/browser-utils/src/instrumentation/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,18 @@ export function instrumentXHR(): void { }; triggerHandlers('xhr', handlerData); + // The handlers above are the last ones that can read `virtualError.stack`. While that stack stays + // unformatted, V8 holds on to the raw frames and every raw frame keeps its receiver alive. For a + // request opened from the `readystatechange` callback of the previous one, that receiver is the + // previous XMLHttpRequest, so the whole chain of requests would stay reachable from the one currently + // in flight. + virtualError = undefined; + // 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. + // done to avoid pinning the XMLHttpRequest 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); } }; diff --git a/packages/browser-utils/test/instrumentation/xhrRetention.test.ts b/packages/browser-utils/test/instrumentation/xhrRetention.test.ts new file mode 100644 index 000000000000..a6596ed4a84b --- /dev/null +++ b/packages/browser-utils/test/instrumentation/xhrRetention.test.ts @@ -0,0 +1,95 @@ +import { setFlagsFromString } from 'node:v8'; +import { runInNewContext } from 'node:vm'; +import { afterEach, describe, expect, it } from 'vitest'; +import { instrumentXHR } from '../../src/instrumentation/xhr'; +import { WINDOW } from '../../src/types'; + +// This lives in its own file on purpose: instrumentation handlers are registered in a module-level +// registry that is never torn down, and a handler from another test would hold on to 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 invokes readystatechange 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 one in flight, which the browser keeps + // alive as a pending activity and which therefore roots the chain + 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(); + }); +}); From 1633639b934f96fdafdab846553e0189d0ca3459 Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Thu, 10 Sep 2026 01:16:52 -0400 Subject: [PATCH 2/2] ref: Trim comments --- packages/browser-utils/src/instrumentation/xhr.ts | 15 +++++---------- .../test/instrumentation/xhrRetention.test.ts | 13 ++++++------- 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/packages/browser-utils/src/instrumentation/xhr.ts b/packages/browser-utils/src/instrumentation/xhr.ts index 6325b31bfb03..41dbe93a42ae 100644 --- a/packages/browser-utils/src/instrumentation/xhr.ts +++ b/packages/browser-utils/src/instrumentation/xhr.ts @@ -91,18 +91,13 @@ export function instrumentXHR(): void { }; triggerHandlers('xhr', handlerData); - // The handlers above are the last ones that can read `virtualError.stack`. While that stack stays - // unformatted, V8 holds on to the raw frames and every raw frame keeps its receiver alive. For a - // request opened from the `readystatechange` callback of the previous one, that receiver is the - // previous XMLHttpRequest, so the whole chain of requests would stay reachable from the one currently - // in flight. + // 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 handler is the only - // `readystatechange` listener we add, so detach it once the request is - // done to avoid pinning the XMLHttpRequest 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. + // 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/instrumentation/xhrRetention.test.ts b/packages/browser-utils/test/instrumentation/xhrRetention.test.ts index a6596ed4a84b..a2c722eb9c7e 100644 --- a/packages/browser-utils/test/instrumentation/xhrRetention.test.ts +++ b/packages/browser-utils/test/instrumentation/xhrRetention.test.ts @@ -4,9 +4,8 @@ import { afterEach, describe, expect, it } from 'vitest'; import { instrumentXHR } from '../../src/instrumentation/xhr'; import { WINDOW } from '../../src/types'; -// This lives in its own file on purpose: instrumentation handlers are registered in a module-level -// registry that is never torn down, and a handler from another test would hold on to the requests -// this one needs to see collected. +// 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; @@ -35,8 +34,8 @@ class MockXMLHttpRequest { 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 invokes readystatechange listeners with the request as `this`, which is what - // puts the request into the stack frames of anything the listener calls + // 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); } } @@ -56,8 +55,8 @@ describe('instrumentXHR memory retention', () => { instrumentXHR(); let firstRequest: WeakRef | undefined; - // the request that never finishes stands in for the one in flight, which the browser keeps - // alive as a pending activity and which therefore roots the chain + // 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 => {