fix(core): onDestroy should be registered only on valid DestroyRef (#49804)

pkozlowski-opensource · thePunderWoman · commit 2c22e6fb5f13 · 2023-04-13T21:14:44.000Z
It might happen that the lifecycle scope represented by DestroyRef becomes invalid before an onDestroy hook is registered (ex. injector or component instance got destroyed). In such cases registration of the onDestroy hooks should no longer be possible. Fixes #49658 PR Close #49804
diff --git a/goldens/public-api/core/errors.md b/goldens/public-api/core/errors.md
@@ -123,7 +123,9 @@ export const enum RuntimeErrorCode {
     // (undocumented)
     UNSUPPORTED_PROJECTION_DOM_NODES = -503,
     // (undocumented)
-    VIEW_ALREADY_ATTACHED = 902
+    VIEW_ALREADY_ATTACHED = 902,
+    // (undocumented)
+    VIEW_ALREADY_DESTROYED = 911
 }
 
 // (No @packageDocumentation comment for this package)
diff --git a/packages/core/src/di/r3_injector.ts b/packages/core/src/di/r3_injector.ts
@@ -212,6 +212,7 @@ export class R3Injector extends EnvironmentInjector {
   }
 
   override onDestroy(callback: () => void): () => void {
+    this.assertNotDestroyed();
     this._onDestroyHooks.push(callback);
     return () => this.removeOnDestroy(callback);
   }
diff --git a/packages/core/src/errors.ts b/packages/core/src/errors.ts
@@ -95,6 +95,7 @@ export const enum RuntimeErrorCode {
   MISSING_ZONEJS = 908,
   UNEXPECTED_ZONE_STATE = 909,
   UNSAFE_IFRAME_ATTRS = -910,
+  VIEW_ALREADY_DESTROYED = 911,
 }
 
 
diff --git a/packages/core/src/render3/assert.ts b/packages/core/src/render3/assert.ts
@@ -15,7 +15,7 @@ import {TIcu} from './interfaces/i18n';
 import {NodeInjectorOffset} from './interfaces/injector';
 import {TNode} from './interfaces/node';
 import {isLContainer, isLView} from './interfaces/type_checks';
-import {DECLARATION_COMPONENT_VIEW, HEADER_OFFSET, LView, T_HOST, TVIEW, TView} from './interfaces/view';
+import {DECLARATION_COMPONENT_VIEW, FLAGS, HEADER_OFFSET, LView, LViewFlags, T_HOST, TVIEW, TView} from './interfaces/view';
 
 // [Assert functions do not constraint type when they are guarded by a truthy
 // expression.](https://github.com/microsoft/TypeScript/issues/37295)
diff --git a/packages/core/src/render3/util/view_utils.ts b/packages/core/src/render3/util/view_utils.ts
@@ -6,6 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {RuntimeError, RuntimeErrorCode} from '../../errors';
 import {assertGreaterThan, assertGreaterThanOrEqual, assertIndexInRange, assertLessThan} from '../../util/assert';
 import {assertTNode, assertTNodeForLView} from '../assert';
 import {LContainer, TYPE} from '../interfaces/container';
@@ -187,6 +188,10 @@ export function updateTransplantedViewCount(lContainer: LContainer, amount: 1|-
  * Stores a LView-specific destroy callback.
  */
 export function storeLViewOnDestroy(lView: LView, onDestroyCallback: () => void) {
+  if ((lView[FLAGS] & LViewFlags.Destroyed) === LViewFlags.Destroyed) {
+    throw new RuntimeError(
+        RuntimeErrorCode.VIEW_ALREADY_DESTROYED, ngDevMode && 'View has already been destroyed.');
+  }
   if (lView[ON_DESTROY_HOOKS] === null) {
     lView[ON_DESTROY_HOOKS] = [];
   }
diff --git a/packages/core/test/acceptance/destroy_ref_spec.ts b/packages/core/test/acceptance/destroy_ref_spec.ts
@@ -56,6 +56,17 @@ describe('DestroyRef', () => {
       // (since we've unregistered one of the callbacks)
       expect(onDestroyCalls).toBe(2);
     });
+
+    it('should throw when trying to register destroy callback on destroyed injector', () => {
+      const envInjector = createEnvironmentInjector([], TestBed.inject(EnvironmentInjector));
+      const destroyRef = envInjector.get(DestroyRef);
+
+      envInjector.destroy();
+
+      expect(() => {
+        destroyRef.onDestroy(() => {});
+      }).toThrowError('NG0205: Injector has already been destroyed.');
+    });
   });
 
   describe('for node injector', () => {
@@ -229,4 +240,24 @@ describe('DestroyRef', () => {
     // (since we've unregistered one of the callbacks)
     expect(onDestroyCalls).toBe(2);
   });
+
+  it('should throw when trying to register destroy callback on destroyed LView', () => {
+    @Component({
+      selector: 'test',
+      standalone: true,
+      template: ``,
+    })
+    class TestCmp {
+      constructor(public destroyRef: DestroyRef) {}
+    }
+
+
+    const fixture = TestBed.createComponent(TestCmp);
+    const destroyRef = fixture.componentRef.instance.destroyRef;
+    fixture.componentRef.destroy();
+
+    expect(() => {
+      destroyRef.onDestroy(() => {});
+    }).toThrowError('NG0911: View has already been destroyed.');
+  });
 });

Original file line number	Diff line number	Diff line change
`@@ -212,6 +212,7 @@ export class R3Injector extends EnvironmentInjector {`
`212`	`212`	`}`
`213`	`213`
`214`	`214`	`override onDestroy(callback: () => void): () => void {`
	`215`	`+ this.assertNotDestroyed();`
`215`	`216`	`this._onDestroyHooks.push(callback);`
`216`	`217`	`return () => this.removeOnDestroy(callback);`
`217`	`218`	`}`
Original file line number	Diff line number	Diff line change
`@@ -95,6 +95,7 @@ export const enum RuntimeErrorCode {`
`95`	`95`	`MISSING_ZONEJS = 908,`
`96`	`96`	`UNEXPECTED_ZONE_STATE = 909,`
`97`	`97`	`UNSAFE_IFRAME_ATTRS = -910,`
	`98`	`+ VIEW_ALREADY_DESTROYED = 911,`
`98`	`99`	`}`
`99`	`100`
`100`	`101`