From 974d5884cfbf8715b421ef9a01f1fb8806b05049 Mon Sep 17 00:00:00 2001 From: arturovt Date: Thu, 27 Nov 2025 21:24:01 +0200 Subject: [PATCH] fix(core): stop running further effects once one destroys the view mid-flush MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a view has more than one effect scheduled to run, and one of them destroys the view (e.g. by calling `componentRef.destroy()`), the remaining effects in that same flush could still run afterward, against a view that no longer exists. In some cases this crashed outright with `TypeError: view[EFFECTS] is not iterable`. Here's why: `runEffectsInView` walks a view's effects in a `for...of` loop, wrapped in an outer `while` loop that re-checks for any effects that became dirty as a side effect of ones that already ran. When an effect destroys its view, `view[EFFECTS]` gets set to `null` as part of tearing the view down. First attempt checked for that inside the `for...of` loop, before each effect runs. That covers a sibling effect later in the *same* pass, but misses a second case: if the effect that destroys the view *also* dirties another effect on that same view in the process (e.g. by writing a signal the sibling depends on), the outer `while` loop sees `HasChildViewsToRefresh` set and tries to restart — and immediately crashes re-entering `for (const effect of view[EFFECTS])` on a now-null value, before the in-loop check ever gets a chance to run. Reproduced that exact crash with a test first: two effects on one view, the second one writes a signal the first depends on and then destroys the view in the same call — confirmed it throws before touching the fix. Fixed by checking right after `effect.run()` instead of before it, covering both cases in one place: the remaining effects in the current pass, and the loop trying to restart afterward. As soon as one effect destroys the view, nothing else runs against it again. This is intentionally narrow in scope. An earlier version of this fix also tried to guarantee that `onCleanup()` callbacks still ran even when registered after an effect destroyed its own view. That's been dropped — destroying your own view and then continuing to register more work for it isn't something the framework should have to paper over. If you need to do both, register `onCleanup` first, then destroy. --- .../render3/reactivity/view_effect_runner.ts | 5 ++ packages/core/test/render3/reactivity_spec.ts | 83 ++++++++++++++++++- 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/packages/core/src/render3/reactivity/view_effect_runner.ts b/packages/core/src/render3/reactivity/view_effect_runner.ts index e81e75ca22bc..15f18333b680 100644 --- a/packages/core/src/render3/reactivity/view_effect_runner.ts +++ b/packages/core/src/render3/reactivity/view_effect_runner.ts @@ -32,6 +32,11 @@ export function runEffectsInView(view: LView): void { } else { effect.zone.run(() => effect.run()); } + + // Stop immediately if the view was destroyed during effect execution. + if (view[EFFECTS] === null) { + return; + } } // Check if we need to continue flushing. If we didn't find any dirty effects, then there's diff --git a/packages/core/test/render3/reactivity_spec.ts b/packages/core/test/render3/reactivity_spec.ts index 560a01dcfcef..e6196903371c 100644 --- a/packages/core/test/render3/reactivity_spec.ts +++ b/packages/core/test/render3/reactivity_spec.ts @@ -44,7 +44,7 @@ import { ViewContainerRef, } from '../../src/core'; import {EffectNode} from '../../src/render3/reactivity/effect'; -import {TestBed} from '../../testing'; +import {type ComponentFixture, TestBed} from '../../testing'; describe('reactivity', () => { describe('effects', () => { @@ -487,6 +487,87 @@ describe('reactivity', () => { expect(destroyed).toBeTrue(); }); + it("should stop running a view's remaining effects once an earlier one destroys the view", async () => { + const recorder: string[] = []; + let fixture: ComponentFixture; + + @Component({}) + class TestCmp { + readonly counter = signal(0); + + constructor() { + // Added first, so it's visited first when the view's effects are flushed. + effect(() => { + recorder.push(`a: ${this.counter()}`); + if (this.counter() === 1) { + fixture.destroy(); + } + }); + + // Added second. Also dirty in the same flush pass as "a" above, so it must not + // run once "a" has destroyed the view partway through that pass. + effect(() => { + recorder.push(`b: ${this.counter()}`); + }); + } + } + + fixture = TestBed.createComponent(TestCmp); + fixture.detectChanges(); + await fixture.whenStable(); + expect(recorder).toEqual(['a: 0', 'b: 0']); + + fixture.componentInstance.counter.set(1); + fixture.detectChanges(); + await fixture.whenStable(); + + expect(recorder).toEqual(['a: 0', 'b: 0', 'a: 1']); + }); + + it('should not restart the flush loop once an effect dirties a sibling and destroys the view in the same run', async () => { + const recorder: string[] = []; + const trigger = signal(0); + let fixture: ComponentFixture; + + @Component({}) + class TestCmp { + readonly counter = signal(0); + + constructor() { + // Added first, so it's already had its turn (and was not dirty) earlier in the + // same flush pass by the time "a" below dirties it. + effect(() => { + trigger(); + recorder.push('b'); + }); + + // Added second. On its second run, dirties "b" above (which sets + // HasChildViewsToRefresh on this view) and destroys the view in the same call. + // That combination makes the outer while loop want to restart even though the + // view is already gone. + effect(() => { + const value = this.counter(); + recorder.push(`a: ${value}`); + if (value === 1) { + trigger.update((v) => v + 1); + fixture.destroy(); + } + }); + } + } + + fixture = TestBed.createComponent(TestCmp); + fixture.detectChanges(); + await fixture.whenStable(); + expect(recorder).toEqual(['b', 'a: 0']); + + fixture.componentInstance.counter.set(1); + fixture.detectChanges(); + await fixture.whenStable(); + + expect(recorder).toEqual(['b', 'a: 0', 'a: 1']); + }); + it('should destroy effects when their DestroyRef is separately destroyed', () => { let destroyed = false; @Component({})