Skip to content

Commit ef6ce28

Browse files
lifeartclaude
andcommitted
fix(gxt): tie classic-tag reactors to instance/owner destroy chains
Three idiomatic hooks for the classic-tag reactor leak path that diagnostic instrumentation (commit 34f9515) confirmed: 1. renderLinkToElement (manager.ts:9519): the LinkTo instance owns the reactor's lifetime. Eager unsub via Phase 0 of __gxtDestroyTrackedInstances + willDestroy override on the instance. The Phase 0 path matters because Ember's classic destroy chain is async via the runloop, and willDestroy can fire too late to prevent the reactor from leaking across the testStart/testDone boundary. 2. _renderComponentGxt (renderer.ts:2287): split doDestroy into cleanupReactor (reactor unsub only, no DOM) and full doDestroy (also wipes innerHTML). Caller-invoked result.destroy() runs the full path; owner-destruction cascades fire only the reactor cleanup so a synthetic owner's destroy doesn't wipe a target the caller still needs. 3. _gxtPendingRenderCleanups (renderer.ts): a global Set drained by __gxtCleanupActiveComponents (the existing cleanup hook fired by QUnit testStart and test-case afterEach). This is the layer that actually catches the synthetic-owner case: the renderComponent API documents `owner = {}` as the default, that synthetic plain object is never in any destroy chain, so registerDestructor on it records the destructor but it never runs. The pending-cleanup set fires eagerly at known cleanup boundaries regardless. For renderer.ts the wiring also includes the standard associateDestroyableChild(parentOwner, syntheticOwner) so that callers who DO destroy their owner properly cascade through the synthetic. That's the same pattern as the classic GlimmerVM render path (line 2330) — kept for callers with real destroyable owners. Diagnostic instrumentation remains in the branch (commit 34f9515) behind __GXT_LEAK_DEBUG__. Force-enable still set in index.html for this CI run; will be flipped off once CI confirms the fix. Smoke (14 modules / 333 tests) green. Local cumulative reproducer shows leak count reduced significantly (LinkTo source from runaway to bounded; renderComponent source partially drained — testem CI tests will show whether the remaining tail still causes test failures vs the prior 8-failure + browser-hang baseline). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
1 parent 62cbad8 commit ef6ce28

3 files changed

Lines changed: 164 additions & 16 deletions

File tree

packages/@ember/-internals/glimmer/lib/renderer.ts

Lines changed: 104 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1746,6 +1746,90 @@ function intoTarget(into: IntoTarget): Cursor {
17461746
}
17471747
}
17481748

1749+
/**
1750+
* Set of cleanup functions for active GXT renderComponent calls, drained
1751+
* by __gxtCleanupActiveComponents (called from QUnit testStart and from
1752+
* test-case afterEach in internal-test-helpers). Without this, reactors
1753+
* registered by _renderComponentGxt leak across tests because the
1754+
* synthetic `{}` owner default is not in any destroy chain — nobody
1755+
* ever calls destroy() on a freshly-allocated plain object.
1756+
*/
1757+
const _gxtPendingRenderCleanups = new Set<() => void>();
1758+
(globalThis as any).__gxtDrainPendingRenderCleanups = function () {
1759+
if (_gxtPendingRenderCleanups.size === 0) return;
1760+
for (const fn of Array.from(_gxtPendingRenderCleanups)) {
1761+
try {
1762+
fn();
1763+
} catch {
1764+
/* ignore */
1765+
}
1766+
}
1767+
_gxtPendingRenderCleanups.clear();
1768+
};
1769+
1770+
/**
1771+
* Wire `owner` into the destroyable chain so the registered cleanup
1772+
* actually fires on teardown.
1773+
*
1774+
* The renderComponent API documents `owner = {}` as the default. That
1775+
* synthetic plain object isn't in any destroy chain by itself — calling
1776+
* registerDestructor on it records the destructor but it never runs,
1777+
* because nothing ever calls destroy() on the synthetic owner.
1778+
*
1779+
* Three layers of cleanup, all pointing at the same cleanupFn:
1780+
*
1781+
* 1. Ambient parent owner via associateDestroyableChild — when the
1782+
* parent owner is destroyed via the standard glimmer/destroyable
1783+
* chain, the synthetic descends with it.
1784+
* 2. registerDestructor on owner — covers callers that do call
1785+
* destroy(owner) directly.
1786+
* 3. _gxtPendingRenderCleanups Set — eagerly drained by
1787+
* __gxtCleanupActiveComponents at QUnit testStart and from
1788+
* test-case afterEach; this is the only layer that fires
1789+
* synchronously regardless of the runloop, which is what
1790+
* actually prevents the cross-test reactor leak.
1791+
*
1792+
* Idempotent — safe to call from both _renderComponentGxt return paths.
1793+
*/
1794+
function _wireOwnerDestroyChain(owner: object, parentOwner: unknown, cleanupFn: () => void): void {
1795+
// Wrap so each layer fires the underlying cleanup exactly once and
1796+
// removing from the pending Set is automatic.
1797+
let fired = false;
1798+
const oneShot = () => {
1799+
if (fired) return;
1800+
fired = true;
1801+
_gxtPendingRenderCleanups.delete(oneShot);
1802+
try {
1803+
cleanupFn();
1804+
} catch {
1805+
/* ignore */
1806+
}
1807+
};
1808+
_gxtPendingRenderCleanups.add(oneShot);
1809+
if (
1810+
parentOwner &&
1811+
parentOwner !== owner &&
1812+
typeof owner === 'object' &&
1813+
owner !== null &&
1814+
Object.getPrototypeOf(owner) === Object.prototype &&
1815+
!isDestroyed(parentOwner) &&
1816+
!isDestroying(parentOwner) &&
1817+
!isDestroyed(owner) &&
1818+
!isDestroying(owner)
1819+
) {
1820+
try {
1821+
associateDestroyableChild(parentOwner as any, owner as any);
1822+
} catch {
1823+
// parent may not accept association; fall through to register
1824+
}
1825+
}
1826+
try {
1827+
registerDestructor(owner, oneShot);
1828+
} catch {
1829+
// owner may not be destroyable; nothing else to do
1830+
}
1831+
}
1832+
17491833
/**
17501834
* GXT-specific renderComponent implementation.
17511835
* Bypasses Glimmer VM bytecode compilation entirely.
@@ -1779,11 +1863,18 @@ function _renderComponentGxt(
17791863
ensureGxtContext();
17801864

17811865
let destroyed = false;
1866+
let reactorCleanedUp = false;
17821867
let classicReactorUnsub: (() => void) | null = null;
17831868

1784-
const doDestroy = () => {
1785-
if (destroyed) return;
1786-
destroyed = true;
1869+
// Reactor-only cleanup. Fires when the owner is destroyed via the
1870+
// standard destroyable chain (registered below). Cleaning up the
1871+
// classic-tag reactor is critical to prevent cross-test leaks; the
1872+
// DOM is not touched here because owner destruction does not imply
1873+
// the caller wants the rendered nodes removed (that is what
1874+
// result.destroy() means, see doDestroy).
1875+
const cleanupReactor = () => {
1876+
if (reactorCleanedUp) return;
1877+
reactorCleanedUp = true;
17871878
if (classicReactorUnsub) {
17881879
try {
17891880
classicReactorUnsub();
@@ -1792,6 +1883,14 @@ function _renderComponentGxt(
17921883
}
17931884
classicReactorUnsub = null;
17941885
}
1886+
};
1887+
1888+
// Full destroy. Returned to the caller as result.destroy(). Cleans
1889+
// the reactor and wipes the rendered content.
1890+
const doDestroy = () => {
1891+
if (destroyed) return;
1892+
destroyed = true;
1893+
cleanupReactor();
17951894
if (targetElement instanceof Element) {
17961895
targetElement.innerHTML = '';
17971896
}
@@ -1866,12 +1965,7 @@ function _renderComponentGxt(
18661965

18671966
const result: RenderResult = { destroy: doDestroy };
18681967
RENDER_CACHE.set(into, { result, glimmerResult: undefined });
1869-
// Register destructor on owner so owner.destroy() cleans up DOM
1870-
try {
1871-
registerDestructor(owner, doDestroy);
1872-
} catch {
1873-
// owner may not be destroyable
1874-
}
1968+
_wireOwnerDestroyChain(owner, prevOwner, cleanupReactor);
18751969
return result;
18761970
}
18771971

@@ -2183,13 +2277,7 @@ function _renderComponentGxt(
21832277

21842278
const result: RenderResult = { destroy: doDestroy };
21852279
RENDER_CACHE.set(into, { result, glimmerResult: undefined });
2186-
2187-
// Register destructor on owner so that destroying the owner cleans up the DOM
2188-
try {
2189-
registerDestructor(owner, doDestroy);
2190-
} catch {
2191-
// owner may not be destroyable
2192-
}
2280+
_wireOwnerDestroyChain(owner, prevOwner, cleanupReactor);
21932281

21942282
return result;
21952283
}

packages/@ember/-internals/gxt-backend/compile.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5784,6 +5784,16 @@ setInterval(() => {
57845784
if (typeof destroyTracked === 'function') {
57855785
destroyTracked();
57865786
}
5787+
// Eagerly drain any classic-tag reactor cleanups for renderComponent
5788+
// calls whose synthetic plain-object owner ({}) isn't otherwise in a
5789+
// destroy chain (the renderComponent API documents `owner = {}` as the
5790+
// default — see _wireOwnerDestroyChain in renderer.ts). Without this
5791+
// drain, those reactors leak across the QUnit testStart/testDone
5792+
// boundary and clobber shared GXT sync state on subsequent tests.
5793+
const drainPending = (globalThis as any).__gxtDrainPendingRenderCleanups;
5794+
if (typeof drainPending === 'function') {
5795+
drainPending();
5796+
}
57875797
// Reset block params stack
57885798
blockParamsStack.length = 0;
57895799
// Reset current slot params

packages/@ember/-internals/gxt-backend/manager.ts

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4134,6 +4134,26 @@ let _preRerenderSnapshot: Set<any> = new Set();
41344134
}
41354135
}
41364136

4137+
// Phase 0 (eager): drain any classic-tag reactor unsubs registered
4138+
// on instances. These unsubs (set by renderLinkToElement) need to
4139+
// fire SYNCHRONOUSLY here, before Ember's async runloop-based
4140+
// destroy chain runs — otherwise the reactor stays in
4141+
// _classicReactors across the testStart/testDone boundary and
4142+
// fires on the next test's tag dirties. The willDestroy monkey-
4143+
// patch in renderLinkToElement is a fallback for instances not
4144+
// reached by this phase.
4145+
for (const instance of instances) {
4146+
const unsub = instance && instance.__gxtClassicReactorUnsub;
4147+
if (typeof unsub === 'function') {
4148+
try {
4149+
unsub();
4150+
} catch {
4151+
/* ignore */
4152+
}
4153+
instance.__gxtClassicReactorUnsub = null;
4154+
}
4155+
}
4156+
41374157
// Phase 1: willDestroyElement + willClearRender (top-down, element still present)
41384158
for (const instance of instances) {
41394159
try {
@@ -9517,6 +9537,36 @@ function renderLinkToElement(instance: any, args: any, fw: any): HTMLAnchorEleme
95179537
}
95189538
};
95199539
unsub = (_gxtRegisterClassicReactor as any)(wrapped, 'renderLinkToElement');
9540+
// Tie the reactor's lifetime to the LinkTo instance's lifetime.
9541+
// The element-detachment heuristic above is unreliable in routing
9542+
// tests that reattach the same <a> across transitions (counter
9543+
// resets each time); without an additional escape hatch the
9544+
// reactor lives forever and fires across unrelated tests.
9545+
//
9546+
// Two hooks, both pointing at the same unsub:
9547+
//
9548+
// 1. instance.__gxtClassicReactorUnsub — picked up SYNCHRONOUSLY
9549+
// by Phase 0 of __gxtDestroyTrackedInstances. Ember's async
9550+
// runloop-based destroy chain doesn't fire willDestroy soon
9551+
// enough to prevent the reactor from leaking across the
9552+
// testStart/testDone boundary, so we drain it eagerly there.
9553+
// 2. willDestroy override — fallback for instances not reached
9554+
// by __gxtDestroyTrackedInstances (e.g. component destroyed
9555+
// via a route transition rather than test teardown).
9556+
if (instance && typeof instance === 'object' && !instance.__gxtLinkToReactorUnsubInstalled) {
9557+
instance.__gxtClassicReactorUnsub = unsub;
9558+
const prior = instance.willDestroy;
9559+
const priorBound = typeof prior === 'function' ? prior.bind(instance) : null;
9560+
instance.willDestroy = function () {
9561+
try {
9562+
unsub();
9563+
} catch {
9564+
/* ignore */
9565+
}
9566+
if (priorBound) return priorBound();
9567+
};
9568+
instance.__gxtLinkToReactorUnsubInstalled = true;
9569+
}
95209570
};
95219571

95229572
// id attribute (doesn't change over the element's lifetime, so no reactor)

0 commit comments

Comments
 (0)