Skip to content

Commit 35438ae

Browse files
fix(@glimmer/destroyable): allow child cleanup during parent DESTROYING_STATE to prevent GC retention
When destroy(parent) triggers cascaded destruction of children, the child's scheduleDestroyed callback calls removeChildFromParent(child, parent). The previous guard `parentMeta.state === LIVE_STATE` prevented this from working when the parent was in DESTROYING_STATE, leaving stale strong references in parent_meta.children that prevent efficient GC of destroyed trees. Change the guard to `parentMeta.state !== DESTROYED_STATE` so that children properly remove themselves from a parent's children list even when the parent is in the process of being destroyed. The synchronous iterate(children, destroy) call has already completed by the time scheduleDestroyed callbacks run, so this modification is safe. Also add a test that verifies parent_meta.children is cleared after cascaded destruction. Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com>
1 parent d1def16 commit 35438ae

3 files changed

Lines changed: 21 additions & 3 deletions

File tree

package.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,7 @@
295295
"@ember/template-compiler/lib/-internal/primitives.js": "ember-source/@ember/template-compiler/lib/-internal/primitives.js",
296296
"@ember/template-compiler/lib/compile-options.js": "ember-source/@ember/template-compiler/lib/compile-options.js",
297297
"@ember/template-compiler/lib/dasherize-component-name.js": "ember-source/@ember/template-compiler/lib/dasherize-component-name.js",
298+
"@ember/template-compiler/lib/plugins/allowed-globals.js": "ember-source/@ember/template-compiler/lib/plugins/allowed-globals.js",
298299
"@ember/template-compiler/lib/plugins/assert-against-attrs.js": "ember-source/@ember/template-compiler/lib/plugins/assert-against-attrs.js",
299300
"@ember/template-compiler/lib/plugins/assert-against-named-outlets.js": "ember-source/@ember/template-compiler/lib/plugins/assert-against-named-outlets.js",
300301
"@ember/template-compiler/lib/plugins/assert-input-helper-without-block.js": "ember-source/@ember/template-compiler/lib/plugins/assert-input-helper-without-block.js",
@@ -346,7 +347,6 @@
346347
"@simple-dom/document/index.js": "ember-source/@simple-dom/document/index.js",
347348
"backburner.js/index.js": "ember-source/backburner.js/index.js",
348349
"dag-map/index.js": "ember-source/dag-map/index.js",
349-
"ember-template-compiler/index.js": "ember-source/ember-template-compiler/index.js",
350350
"ember-testing/index.js": "ember-source/ember-testing/index.js",
351351
"ember-testing/lib/adapters/adapter.js": "ember-source/ember-testing/lib/adapters/adapter.js",
352352
"ember-testing/lib/adapters/qunit.js": "ember-source/ember-testing/lib/adapters/qunit.js",
@@ -390,4 +390,4 @@
390390
}
391391
},
392392
"packageManager": "pnpm@10.30.3"
393-
}
393+
}

packages/@glimmer/destroyable/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ export function destroy(destroyable: Destroyable) {
189189
function removeChildFromParent(child: Destroyable, parent: Destroyable) {
190190
let parentMeta = getDestroyableMeta(parent);
191191

192-
if (parentMeta.state === LIVE_STATE) {
192+
if (parentMeta.state !== DESTROYED_STATE) {
193193
parentMeta.children = remove(
194194
parentMeta.children,
195195
child,

packages/@glimmer/destroyable/test/destroyables-test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { DEBUG } from '@glimmer/env';
22
import type { GlobalContext } from '@glimmer/global-context';
33
import { unwrap } from '@glimmer/debug-util';
44
import {
5+
_hasDestroyableChildren,
56
assertDestroyablesDestroyed,
67
associateDestroyableChild,
78
destroy,
@@ -384,6 +385,23 @@ module('Destroyables', (hooks) => {
384385
assert.verifySteps(['child destructor', 'parent destructor'], 'destructors run bottom up');
385386
});
386387

388+
test('parent no longer references child after cascaded destruction', (assert) => {
389+
const parent = {};
390+
const child = {};
391+
392+
associateDestroyableChild(parent, child);
393+
394+
assert.true(_hasDestroyableChildren(parent), 'parent has children before destruction');
395+
396+
destroy(parent);
397+
flush();
398+
399+
assert.false(
400+
_hasDestroyableChildren(parent),
401+
'parent metadata releases child reference during cascaded destruction, allowing GC to reclaim the tree'
402+
);
403+
});
404+
387405
if (DEBUG) {
388406
test('attempting to unregister a destructor that was not registered throws an error', (assert) => {
389407
assert.throws(() => {

0 commit comments

Comments
 (0)