From ea1cfb9fe2e4517b0dabd6832c4c76ceb84c59e7 Mon Sep 17 00:00:00 2001
From: JoostK <joost.koehoorn@gmail.com>
Date: Mon, 4 Dec 2023 19:01:36 +0100
Subject: [PATCH] fix(core): cleanup signal consumers for all views (#53351)

This commit fixes a memory leak where signal consumers would not be cleaned up for
descendant views when a view is destroyed, because the cleanup logic was only invoked
for the view that is itself being destroyed.

PR Close #53351
---
 .../core/src/render3/node_manipulation.ts     |  4 +-
 packages/core/test/acceptance/BUILD.bazel     |  1 +
 .../change_detection_signals_in_zones_spec.ts | 56 +++++++++++++++++--
 3 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts
index 0b94493d6fa96..afa0a1e69417b 100644
--- a/packages/core/src/render3/node_manipulation.ts
+++ b/packages/core/src/render3/node_manipulation.ts
@@ -374,8 +374,6 @@ export function destroyLView(tView: TView, lView: LView) {
   if (!(lView[FLAGS] & LViewFlags.Destroyed)) {
     const renderer = lView[RENDERER];
 
-    lView[REACTIVE_TEMPLATE_CONSUMER] && consumerDestroy(lView[REACTIVE_TEMPLATE_CONSUMER]);
-
     if (renderer.destroyNode) {
       applyView(tView, lView, renderer, WalkTNodeTreeAction.Destroy, null, null);
     }
@@ -405,6 +403,8 @@ function cleanUpView(tView: TView, lView: LView): void {
     // really more of an "afterDestroy" hook if you think about it.
     lView[FLAGS] |= LViewFlags.Destroyed;
 
+    lView[REACTIVE_TEMPLATE_CONSUMER] && consumerDestroy(lView[REACTIVE_TEMPLATE_CONSUMER]);
+
     executeOnDestroys(tView, lView);
     processCleanups(tView, lView);
     // For component views only, the local renderer is destroyed at clean up time.
diff --git a/packages/core/test/acceptance/BUILD.bazel b/packages/core/test/acceptance/BUILD.bazel
index 109fc1a760e8b..9874784a0ef4c 100644
--- a/packages/core/test/acceptance/BUILD.bazel
+++ b/packages/core/test/acceptance/BUILD.bazel
@@ -23,6 +23,7 @@ ts_library(
         "//packages/common/locales",
         "//packages/compiler",
         "//packages/core",
+        "//packages/core/primitives/signals",
         "//packages/core/src/util",
         "//packages/core/test/render3:matchers",
         "//packages/core/testing",
diff --git a/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts b/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts
index dd961f98f4384..597ef2f9f2553 100644
--- a/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts
+++ b/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts
@@ -9,6 +9,7 @@
 import {NgFor, NgIf} from '@angular/common';
 import {PLATFORM_BROWSER_ID} from '@angular/common/src/platform_id';
 import {afterNextRender, ChangeDetectionStrategy, ChangeDetectorRef, Component, computed, Directive, inject, Input, PLATFORM_ID, signal, TemplateRef, ViewChild, ViewContainerRef} from '@angular/core';
+import {ReactiveNode, SIGNAL} from '@angular/core/primitives/signals';
 import {TestBed} from '@angular/core/testing';
 
 describe('CheckAlways components', () => {
@@ -753,10 +754,10 @@ describe('OnPush components with signals', () => {
         changeDetection: ChangeDetectionStrategy.OnPush,
         imports: [NgIf],
         template: `
-          <div *ngIf="true"> 
-            <div *ngIf="true"> 
-              <div *ngIf="true"> 
-                {{value()}} 
+          <div *ngIf="true">
+            <div *ngIf="true">
+              <div *ngIf="true">
+                {{value()}}
               </div>
             </div>
           </div>
@@ -910,6 +911,53 @@ describe('OnPush components with signals', () => {
     const fixture = TestBed.createComponent(TestCmp);
     expect(() => fixture.detectChanges()).toThrowError(/ExpressionChanged/);
   });
+
+  it('destroys all signal consumers when destroying the view tree', () => {
+    const val = signal(1);
+    const double = computed(() => val() * 2);
+
+    @Component({
+      template: '{{double()}}',
+      selector: 'child',
+      standalone: true,
+    })
+    class Child {
+      double = double;
+    }
+
+    @Component({
+      template: '|{{double()}}|<child />|',
+      imports: [Child],
+      standalone: true,
+    })
+    class SignalComponent {
+      double = double;
+    }
+
+    const fixture = TestBed.createComponent(SignalComponent);
+    fixture.detectChanges();
+    expect(fixture.nativeElement.innerText).toEqual('|2|2|');
+
+    const node = double[SIGNAL] as ReactiveNode;
+    expect(node.dirty).toBe(false);
+
+    // Change the signal to verify that the computed is dirtied while being read from the template.
+    val.set(2);
+    expect(node.dirty).toBe(true);
+    fixture.detectChanges();
+    expect(node.dirty).toBe(false);
+    expect(fixture.nativeElement.innerText).toEqual('|4|4|');
+
+    // Destroy the view tree to verify that the computed is unconnected from the graph for all
+    // views.
+    fixture.destroy();
+    expect(node.dirty).toBe(false);
+
+    // Writing further updates to the signal should not cause the computed to become dirty, since it
+    // is no longer being observed.
+    val.set(3);
+    expect(node.dirty).toBe(false);
+  });
 });