From f6d2a0a57a3737aabfd5193f417e37f9bbb27b77 Mon Sep 17 00:00:00 2001 From: Yash Suresh Chandra Date: Wed, 5 Aug 2026 15:51:37 +0530 Subject: [PATCH 1/2] fix(ember): Stop retaining component render payloads in beforeEntries Co-Authored-By: Claude Fable 5 --- .../addon/utils/instrumentEmberGlobals.ts | 28 +++++----- .../unit/instrument-ember-globals-test.ts | 51 +++++++++++++++++++ 2 files changed, 66 insertions(+), 13 deletions(-) create mode 100644 packages/ember/tests/unit/instrument-ember-globals-test.ts diff --git a/packages/ember/addon/utils/instrumentEmberGlobals.ts b/packages/ember/addon/utils/instrumentEmberGlobals.ts index 291c18faf56b..dee0e0b406d4 100644 --- a/packages/ember/addon/utils/instrumentEmberGlobals.ts +++ b/packages/ember/addon/utils/instrumentEmberGlobals.ts @@ -19,9 +19,7 @@ type RenderEntry = { now: number; }; -interface RenderEntries { - [name: string]: RenderEntry; -} +export type RenderEntries = Map; /** This is global, so should only be run once in tests! */ export function instrumentGlobalsForPerformance(config: { @@ -127,26 +125,30 @@ function _instrumentEmberRunloop(config: { minimumRunloopQueueDuration?: number }); } -function processComponentRenderBefore(payload: Payload, beforeEntries: RenderEntries): void { +export function _processComponentRenderBefore(payload: Payload, beforeEntries: RenderEntries): void { const info = { payload, now: timestampInSeconds(), }; - beforeEntries[payload.object] = info; + beforeEntries.set(payload.object, info); } -function processComponentRenderAfter( +export function _processComponentRenderAfter( payload: Payload, beforeEntries: RenderEntries, op: string, minComponentDuration: number, ): void { - const begin = beforeEntries[payload.object]; + const begin = beforeEntries.get(payload.object); if (!begin) { return; } + // Remove the entry so the render payload (which references the component + // instance) is not retained forever in this module-scope map. + beforeEntries.delete(payload.object); + const now = timestampInSeconds(); const componentRenderDuration = now - begin.now; @@ -174,27 +176,27 @@ function _instrumentComponents(config: { const minComponentDuration = minimumComponentRenderDuration ?? 2; - const beforeEntries = {} as RenderEntries; - const beforeComponentDefinitionEntries = {} as RenderEntries; + const beforeEntries: RenderEntries = new Map(); + const beforeComponentDefinitionEntries: RenderEntries = new Map(); function _subscribeToRenderEvents(): void { subscribe('render.component', { before(_name: string, _timestamp: number, payload: Payload) { - processComponentRenderBefore(payload, beforeEntries); + _processComponentRenderBefore(payload, beforeEntries); }, after(_name: string, _timestamp: number, payload: Payload, _beganIndex: number) { - processComponentRenderAfter(payload, beforeEntries, BROWSER_UI_RENDER_SPAN_OP, minComponentDuration); + _processComponentRenderAfter(payload, beforeEntries, BROWSER_UI_RENDER_SPAN_OP, minComponentDuration); }, }); if (enableComponentDefinitions) { subscribe('render.getComponentDefinition', { before(_name: string, _timestamp: number, payload: Payload) { - processComponentRenderBefore(payload, beforeComponentDefinitionEntries); + _processComponentRenderBefore(payload, beforeComponentDefinitionEntries); }, after(_name: string, _timestamp: number, payload: Payload, _beganIndex: number) { - processComponentRenderAfter(payload, beforeComponentDefinitionEntries, GENERAL_FUNCTION_SPAN_OP, 0); + _processComponentRenderAfter(payload, beforeComponentDefinitionEntries, GENERAL_FUNCTION_SPAN_OP, 0); }, }); } diff --git a/packages/ember/tests/unit/instrument-ember-globals-test.ts b/packages/ember/tests/unit/instrument-ember-globals-test.ts new file mode 100644 index 000000000000..24910709a95a --- /dev/null +++ b/packages/ember/tests/unit/instrument-ember-globals-test.ts @@ -0,0 +1,51 @@ +import type { RenderEntries } from '@sentry/ember/utils/instrumentEmberGlobals'; +import { + _processComponentRenderAfter, + _processComponentRenderBefore, +} from '@sentry/ember/utils/instrumentEmberGlobals'; +import { setupTest } from 'ember-qunit'; +import { module, test } from 'qunit'; +import type { SentryTestContext } from '../helpers/setup-sentry'; +import { setupSentryTest } from '../helpers/setup-sentry'; + +module('Unit | Utility | instrument-ember-globals', function (hooks) { + setupTest(hooks); + setupSentryTest(hooks); + + test('_processComponentRenderAfter removes the entry recorded for the render', function (this: SentryTestContext, assert) { + const beforeEntries: RenderEntries = new Map(); + const payload = { containerKey: 'component:test-component', initialRender: true as const, object: '' }; + + _processComponentRenderBefore(payload, beforeEntries); + assert.strictEqual(beforeEntries.size, 1, 'Entry is recorded when the render starts'); + + _processComponentRenderAfter(payload, beforeEntries, 'ui.ember.component.render', 1_000); + assert.strictEqual( + beforeEntries.size, + 0, + 'Entry is removed when the render finishes, so the payload (and the component instance it references) is not retained', + ); + }); + + test('_processComponentRenderAfter removes the entry even when the render is long enough to create a span', function (this: SentryTestContext, assert) { + const beforeEntries: RenderEntries = new Map(); + const payload = { containerKey: 'component:test-component', initialRender: true as const, object: '' }; + + _processComponentRenderBefore(payload, beforeEntries); + _processComponentRenderAfter(payload, beforeEntries, 'ui.ember.component.render', 0); + + assert.strictEqual(beforeEntries.size, 0, 'Entry is removed after the span is created'); + }); + + test('_processComponentRenderAfter without a matching before-entry leaves other entries alone', function (this: SentryTestContext, assert) { + const beforeEntries: RenderEntries = new Map(); + const trackedPayload = { containerKey: 'component:tracked', initialRender: true as const, object: '' }; + const unknownPayload = { containerKey: 'component:unknown', initialRender: true as const, object: '' }; + + _processComponentRenderBefore(trackedPayload, beforeEntries); + _processComponentRenderAfter(unknownPayload, beforeEntries, 'ui.ember.component.render', 1_000); + + assert.strictEqual(beforeEntries.size, 1, 'Unrelated in-flight entries are kept'); + assert.true(beforeEntries.has(trackedPayload.object), 'The in-flight entry is still tracked'); + }); +}); From 03bb5b5579e5f1d9122780c8ec495f1c677b20a4 Mon Sep 17 00:00:00 2001 From: Yash Suresh Chandra Date: Mon, 10 Aug 2026 20:28:28 +0530 Subject: [PATCH 2/2] fix(ember): Key render before-entries weakly on the payload object Per review on #23052, switch RenderEntries from Map to WeakMap. Keying weakly on the payload object guarantees nothing lingers even if a render's `after` hook is never called; the prompt delete is kept so a long-lived payload doesn't hold a stale entry between renders. Ember passes the same payload object to a subscriber's before/after hooks, so identity-based lookup is safe. Co-Authored-By: Claude Fable 5 --- .../addon/utils/instrumentEmberGlobals.ts | 21 +++++++------------ .../unit/instrument-ember-globals-test.ts | 19 ++++++++--------- 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/packages/ember/addon/utils/instrumentEmberGlobals.ts b/packages/ember/addon/utils/instrumentEmberGlobals.ts index dee0e0b406d4..41916e0c4a22 100644 --- a/packages/ember/addon/utils/instrumentEmberGlobals.ts +++ b/packages/ember/addon/utils/instrumentEmberGlobals.ts @@ -15,11 +15,10 @@ type Payload = { }; type RenderEntry = { - payload: Payload; now: number; }; -export type RenderEntries = Map; +export type RenderEntries = WeakMap; /** This is global, so should only be run once in tests! */ export function instrumentGlobalsForPerformance(config: { @@ -126,11 +125,7 @@ function _instrumentEmberRunloop(config: { minimumRunloopQueueDuration?: number } export function _processComponentRenderBefore(payload: Payload, beforeEntries: RenderEntries): void { - const info = { - payload, - now: timestampInSeconds(), - }; - beforeEntries.set(payload.object, info); + beforeEntries.set(payload, { now: timestampInSeconds() }); } export function _processComponentRenderAfter( @@ -139,15 +134,15 @@ export function _processComponentRenderAfter( op: string, minComponentDuration: number, ): void { - const begin = beforeEntries.get(payload.object); + const begin = beforeEntries.get(payload); if (!begin) { return; } - // Remove the entry so the render payload (which references the component - // instance) is not retained forever in this module-scope map. - beforeEntries.delete(payload.object); + // A WeakMap entry cannot outlive its payload, but delete promptly anyway + // so a long-lived payload doesn't keep the entry around between renders. + beforeEntries.delete(payload); const now = timestampInSeconds(); const componentRenderDuration = now - begin.now; @@ -176,8 +171,8 @@ function _instrumentComponents(config: { const minComponentDuration = minimumComponentRenderDuration ?? 2; - const beforeEntries: RenderEntries = new Map(); - const beforeComponentDefinitionEntries: RenderEntries = new Map(); + const beforeEntries: RenderEntries = new WeakMap(); + const beforeComponentDefinitionEntries: RenderEntries = new WeakMap(); function _subscribeToRenderEvents(): void { subscribe('render.component', { diff --git a/packages/ember/tests/unit/instrument-ember-globals-test.ts b/packages/ember/tests/unit/instrument-ember-globals-test.ts index 24910709a95a..a11b7e299297 100644 --- a/packages/ember/tests/unit/instrument-ember-globals-test.ts +++ b/packages/ember/tests/unit/instrument-ember-globals-test.ts @@ -13,39 +13,38 @@ module('Unit | Utility | instrument-ember-globals', function (hooks) { setupSentryTest(hooks); test('_processComponentRenderAfter removes the entry recorded for the render', function (this: SentryTestContext, assert) { - const beforeEntries: RenderEntries = new Map(); + const beforeEntries: RenderEntries = new WeakMap(); const payload = { containerKey: 'component:test-component', initialRender: true as const, object: '' }; _processComponentRenderBefore(payload, beforeEntries); - assert.strictEqual(beforeEntries.size, 1, 'Entry is recorded when the render starts'); + assert.true(beforeEntries.has(payload), 'Entry is recorded when the render starts'); _processComponentRenderAfter(payload, beforeEntries, 'ui.ember.component.render', 1_000); - assert.strictEqual( - beforeEntries.size, - 0, + assert.false( + beforeEntries.has(payload), 'Entry is removed when the render finishes, so the payload (and the component instance it references) is not retained', ); }); test('_processComponentRenderAfter removes the entry even when the render is long enough to create a span', function (this: SentryTestContext, assert) { - const beforeEntries: RenderEntries = new Map(); + const beforeEntries: RenderEntries = new WeakMap(); const payload = { containerKey: 'component:test-component', initialRender: true as const, object: '' }; _processComponentRenderBefore(payload, beforeEntries); _processComponentRenderAfter(payload, beforeEntries, 'ui.ember.component.render', 0); - assert.strictEqual(beforeEntries.size, 0, 'Entry is removed after the span is created'); + assert.false(beforeEntries.has(payload), 'Entry is removed after the span is created'); }); test('_processComponentRenderAfter without a matching before-entry leaves other entries alone', function (this: SentryTestContext, assert) { - const beforeEntries: RenderEntries = new Map(); + const beforeEntries: RenderEntries = new WeakMap(); const trackedPayload = { containerKey: 'component:tracked', initialRender: true as const, object: '' }; const unknownPayload = { containerKey: 'component:unknown', initialRender: true as const, object: '' }; _processComponentRenderBefore(trackedPayload, beforeEntries); _processComponentRenderAfter(unknownPayload, beforeEntries, 'ui.ember.component.render', 1_000); - assert.strictEqual(beforeEntries.size, 1, 'Unrelated in-flight entries are kept'); - assert.true(beforeEntries.has(trackedPayload.object), 'The in-flight entry is still tracked'); + assert.true(beforeEntries.has(trackedPayload), 'The in-flight entry is still tracked'); + assert.false(beforeEntries.has(unknownPayload), 'The unknown payload was not added'); }); });