fix(core): clean up afterRender after it is executed (#58119)

crisbeto · AndrewKushnir · commit 46bafb0b0a95 · 2024-10-08T13:27:07.000-07:00
We stop tracking `afterRender` hooks as soon as they execute, but their on destroy callbacks stay registered until either the injector is destroyed or the user calls `destroy` manually. This was leading to memory leaks in the `@defer` triggers based on top of `afterRender` when placed inside long-lived views, because the callback would execute, but its destroy logic was waiting for the view to be destroyed. These changes resolve the issue by destroying the `AfterRenderRef` once it is executed. PR Close #58119
diff --git a/packages/core/src/render3/after_render/manager.ts b/packages/core/src/render3/after_render/manager.ts
@@ -82,6 +82,9 @@ export class AfterRenderImpl {
       sequence.afterRun();
       if (sequence.once) {
         this.sequences.delete(sequence);
+        // Destroy the sequence so its on destroy callbacks can be cleaned up
+        // immediately, instead of waiting until the injector is destroyed.
+        sequence.destroy();
       }
     }
 
diff --git a/packages/core/test/acceptance/after_render_hook_spec.ts b/packages/core/test/acceptance/after_render_hook_spec.ts
@@ -1394,6 +1394,40 @@ describe('after render hooks', () => {
         appRef.tick();
       }).toThrowError(/NG0103.*(Infinite change detection while refreshing application views)/);
     });
+
+    it('should destroy after the hook has run', () => {
+      let hookRef: AfterRenderRef | null = null;
+      let afterRenderCount = 0;
+
+      @Component({selector: 'comp'})
+      class Comp {
+        constructor() {
+          hookRef = afterNextRender(() => {
+            afterRenderCount++;
+          });
+        }
+      }
+
+      TestBed.configureTestingModule({
+        declarations: [Comp],
+        ...COMMON_CONFIGURATION,
+      });
+      createAndAttachComponent(Comp);
+      const appRef = TestBed.inject(ApplicationRef);
+      const destroySpy = spyOn(hookRef!, 'destroy').and.callThrough();
+      expect(afterRenderCount).toBe(0);
+      expect(destroySpy).not.toHaveBeenCalled();
+
+      // Run once and ensure that it was called and then cleaned up.
+      appRef.tick();
+      expect(afterRenderCount).toBe(1);
+      expect(destroySpy).toHaveBeenCalledTimes(1);
+
+      // Make sure we're not retaining it.
+      appRef.tick();
+      expect(afterRenderCount).toBe(1);
+      expect(destroySpy).toHaveBeenCalledTimes(1);
+    });
   });
 
   describe('server', () => {

Original file line number	Diff line number	Diff line change
`@@ -82,6 +82,9 @@ export class AfterRenderImpl {`
`82`	`82`	`sequence.afterRun();`
`83`	`83`	`if (sequence.once) {`
`84`	`84`	`this.sequences.delete(sequence);`
	`85`	`+ // Destroy the sequence so its on destroy callbacks can be cleaned up`
	`86`	`+ // immediately, instead of waiting until the injector is destroyed.`
	`87`	`+ sequence.destroy();`
`85`	`88`	`}`
`86`	`89`	`}`
`87`	`90`