fix(core): allow onDestroy unregistration while destroying (#50237)

Ensure that all onDestroy callbacks are called by allowing unregistering of onDestroy hooks while destroying.

Fixes #50221

PR Close #50237

packages/core/src/di/r3_injector.ts

@@ -199,15 +199,18 @@ export class R3Injector extends EnvironmentInjector {
       for (const service of this._ngOnDestroyHooks) {
         service.ngOnDestroy();
       }
-      for (const hook of this._onDestroyHooks) {
+      const onDestroyHooks = this._onDestroyHooks;
+      // Reset the _onDestroyHooks array before iterating over it to prevent hooks that unregister
+      // themselves from mutating the array during iteration.
+      this._onDestroyHooks = [];
+      for (const hook of onDestroyHooks) {
         hook();
       }
     } finally {
       // Release all references.
       this.records.clear();
       this._ngOnDestroyHooks.clear();
       this.injectorDefTypes.clear();
-      this._onDestroyHooks.length = 0;
     }
   }
 

packages/core/src/render3/node_manipulation.ts

@@ -467,12 +467,14 @@ function processCleanups(tView: TView, lView: LView): void {
   }
   const destroyHooks = lView[ON_DESTROY_HOOKS];
   if (destroyHooks !== null) {
+    // Reset the ON_DESTROY_HOOKS array before iterating over it to prevent hooks that unregister
+    // themselves from mutating the array during iteration.
+    lView[ON_DESTROY_HOOKS] = null;
     for (let i = 0; i < destroyHooks.length; i++) {
       const destroyHooksFn = destroyHooks[i];
       ngDevMode && assertFunction(destroyHooksFn, 'Expecting destroy hook to be a function.');
       destroyHooksFn();
     }
-    lView[ON_DESTROY_HOOKS] = null;
   }
 }
 

packages/core/test/acceptance/destroy_ref_spec.ts

@@ -260,4 +260,31 @@ describe('DestroyRef', () => {
       destroyRef.onDestroy(() => {});
     }).toThrowError('NG0911: View has already been destroyed.');
   });
+
+  it('should allow unregistration while destroying', () => {
+    const destroyedLog: string[] = [];
+
+    @Component({
+      selector: 'test',
+      standalone: true,
+      template: ``,
+    })
+    class TestCmp {
+      constructor(destroyCtx: DestroyRef) {
+        const unregister = destroyCtx.onDestroy(() => {
+          destroyedLog.push('first');
+          unregister();
+        });
+        destroyCtx.onDestroy(() => {
+          destroyedLog.push('second');
+        });
+      }
+    }
+
+    const fixture = TestBed.createComponent(TestCmp);
+    expect(destroyedLog).toEqual([]);
+
+    fixture.componentRef.destroy();
+    expect(destroyedLog).toEqual(['first', 'second']);
+  });
 });

packages/core/test/acceptance/environment_injector_spec.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Component, createComponent, createEnvironmentInjector, ENVIRONMENT_INITIALIZER, EnvironmentInjector, inject, InjectFlags, InjectionToken, INJECTOR, Injector, NgModuleRef, ViewContainerRef} from '@angular/core';
+import {Component, createComponent, createEnvironmentInjector, DestroyRef, ENVIRONMENT_INITIALIZER, EnvironmentInjector, inject, InjectFlags, InjectionToken, INJECTOR, Injector, NgModuleRef, ViewContainerRef} from '@angular/core';
 import {R3Injector} from '@angular/core/src/di/r3_injector';
 import {RuntimeError, RuntimeErrorCode} from '@angular/core/src/errors';
 import {TestBed} from '@angular/core/testing';
@@ -27,6 +27,27 @@ describe('environment injector', () => {
     expect(destroyed).toBeTrue();
   });
 
+  it('should allow unregistration while destroying', () => {
+    const destroyedLog: string[] = [];
+
+    const parentEnvInjector = TestBed.inject(EnvironmentInjector);
+    const envInjector = createEnvironmentInjector([], parentEnvInjector);
+    const destroyRef = envInjector.get(DestroyRef);
+
+    const unregister = destroyRef.onDestroy(() => {
+      destroyedLog.push('first');
+      unregister();
+    });
+    destroyRef.onDestroy(() => {
+      destroyedLog.push('second');
+    });
+
+    expect(destroyedLog).toEqual([]);
+
+    envInjector.destroy();
+    expect(destroyedLog).toEqual(['first', 'second']);
+  });
+
   it('should see providers from a parent EnvInjector', () => {
     class Service {}