fix(ivy): wrong context passed to ngOnDestroy when resolved multiple times (#35249)

When the same provider is resolved multiple times on the same node, the first invocation had the correct context, but all subsequent ones were incorrect because we were registering the hook multiple times under different indexes in `destroyHooks`.

Fixes #35167.

PR Close #35249

Author: crisbeto

packages/core/src/render3/di_setup.ts

@@ -81,26 +81,18 @@ function resolveProvider(
     const cptViewProvidersCount =
         tNode.providerIndexes >> TNodeProviderIndexes.CptViewProvidersCountShift;
 
-    if (isClassProvider(provider) || isTypeProvider(provider)) {
-      const prototype = ((provider as ClassProvider).useClass || provider).prototype;
-      const ngOnDestroy = prototype.ngOnDestroy;
-
-      if (ngOnDestroy) {
-        (tView.destroyHooks || (tView.destroyHooks = [])).push(tInjectables.length, ngOnDestroy);
-      }
-    }
-
     if (isTypeProvider(provider) || !provider.multi) {
       // Single provider case: the factory is created and pushed immediately
       const factory = new NodeInjectorFactory(providerFactory, isViewProvider, ɵɵdirectiveInject);
       const existingFactoryIndex = indexOf(
           token, tInjectables, isViewProvider ? beginIndex : beginIndex + cptViewProvidersCount,
           endIndex);
-      if (existingFactoryIndex == -1) {
+      if (existingFactoryIndex === -1) {
         diPublicInInjector(
             getOrCreateNodeInjectorForNode(
                 tNode as TElementNode | TContainerNode | TElementContainerNode, lView),
             tView, token);
+        registerDestroyHooksIfSupported(tView, provider, tInjectables.length);
         tInjectables.push(token);
         tNode.directiveStart++;
         tNode.directiveEnd++;
@@ -157,6 +149,7 @@ function resolveProvider(
         if (!isViewProvider && doesViewProvidersFactoryExist) {
           lInjectablesBlueprint[existingViewProvidersFactoryIndex].providerFactory = factory;
         }
+        registerDestroyHooksIfSupported(tView, provider, tInjectables.length);
         tInjectables.push(token);
         tNode.directiveStart++;
         tNode.directiveEnd++;
@@ -167,6 +160,10 @@ function resolveProvider(
         lView.push(factory);
       } else {
         // Cases 1.b and 2.b
+        registerDestroyHooksIfSupported(
+            tView, provider, existingProvidersFactoryIndex > -1 ?
+                existingProvidersFactoryIndex :
+                existingViewProvidersFactoryIndex);
         multiFactoryAdd(
             lInjectablesBlueprint ![isViewProvider ? existingViewProvidersFactoryIndex : existingProvidersFactoryIndex],
             providerFactory, !isViewProvider && isComponent);
@@ -178,6 +175,24 @@ function resolveProvider(
   }
 }
 
+/**
+ * Registers the `ngOnDestroy` hook of a provider, if the provider supports destroy hooks.
+ * @param tView `TView` in which to register the hook.
+ * @param provider Provider whose hook should be registered.
+ * @param contextIndex Index under which to find the context for the hook when it's being invoked.
+ */
+function registerDestroyHooksIfSupported(
+    tView: TView, provider: Exclude<Provider, any[]>, contextIndex: number) {
+  const providerIsTypeProvider = isTypeProvider(provider);
+  if (providerIsTypeProvider || isClassProvider(provider)) {
+    const prototype = ((provider as ClassProvider).useClass || provider).prototype;
+    const ngOnDestroy = prototype.ngOnDestroy;
+    if (ngOnDestroy) {
+      (tView.destroyHooks || (tView.destroyHooks = [])).push(contextIndex, ngOnDestroy);
+    }
+  }
+}
+
 /**
  * Add a factory in a multi factory.
  */

packages/core/test/acceptance/providers_spec.ts

@@ -223,6 +223,129 @@ describe('providers', () => {
       expect(logs).toEqual(['OnDestroy Existing']);
     });
 
+    it('should invoke ngOnDestroy with the correct context when providing a type provider multiple times on the same node',
+       () => {
+         const resolvedServices: (DestroyService | undefined)[] = [];
+         const destroyContexts: (DestroyService | undefined)[] = [];
+         let parentService: DestroyService|undefined;
+         let childService: DestroyService|undefined;
+
+         @Injectable()
+         class DestroyService {
+           constructor() { resolvedServices.push(this); }
+           ngOnDestroy() { destroyContexts.push(this); }
+         }
+
+         @Directive({selector: '[dir-one]', providers: [DestroyService]})
+         class DirOne {
+           constructor(service: DestroyService) { childService = service; }
+         }
+
+         @Directive({selector: '[dir-two]', providers: [DestroyService]})
+         class DirTwo {
+           constructor(service: DestroyService) { childService = service; }
+         }
+
+         @Component({template: '<div dir-one dir-two></div>', providers: [DestroyService]})
+         class App {
+           constructor(service: DestroyService) { parentService = service; }
+         }
+
+         TestBed.configureTestingModule({declarations: [App, DirOne, DirTwo]});
+         const fixture = TestBed.createComponent(App);
+         fixture.detectChanges();
+         fixture.destroy();
+
+         expect(parentService).toBeDefined();
+         expect(childService).toBeDefined();
+         expect(parentService).not.toBe(childService);
+         expect(resolvedServices).toEqual([parentService, childService]);
+         expect(destroyContexts).toEqual([parentService, childService]);
+       });
+
+    onlyInIvy('Destroy hook of useClass provider is invoked correctly')
+        .it('should invoke ngOnDestroy with the correct context when providing a class provider multiple times on the same node',
+            () => {
+              const resolvedServices: (DestroyService | undefined)[] = [];
+              const destroyContexts: (DestroyService | undefined)[] = [];
+              const token = new InjectionToken<any>('token');
+              let parentService: DestroyService|undefined;
+              let childService: DestroyService|undefined;
+
+              @Injectable()
+              class DestroyService {
+                constructor() { resolvedServices.push(this); }
+                ngOnDestroy() { destroyContexts.push(this); }
+              }
+
+              @Directive(
+                  {selector: '[dir-one]', providers: [{provide: token, useClass: DestroyService}]})
+              class DirOne {
+                constructor(@Inject(token) service: DestroyService) { childService = service; }
+              }
+
+              @Directive(
+                  {selector: '[dir-two]', providers: [{provide: token, useClass: DestroyService}]})
+              class DirTwo {
+                constructor(@Inject(token) service: DestroyService) { childService = service; }
+              }
+
+              @Component({
+                template: '<div dir-one dir-two></div>',
+                providers: [{provide: token, useClass: DestroyService}]
+              })
+              class App {
+                constructor(@Inject(token) service: DestroyService) { parentService = service; }
+              }
+
+              TestBed.configureTestingModule({declarations: [App, DirOne, DirTwo]});
+              const fixture = TestBed.createComponent(App);
+              fixture.detectChanges();
+              fixture.destroy();
+
+              expect(parentService).toBeDefined();
+              expect(childService).toBeDefined();
+              expect(parentService).not.toBe(childService);
+              expect(resolvedServices).toEqual([parentService, childService]);
+              expect(destroyContexts).toEqual([parentService, childService]);
+            });
+
+    onlyInIvy('ngOnDestroy hooks for multi providers were not supported in ViewEngine')
+        .it('should not invoke ngOnDestroy on multi providers', () => {
+          // TODO(FW-1866): currently we only assert that the hook was called,
+          // but we should also be checking that the correct context was passed in.
+          let destroyCalls = 0;
+          const SERVICES = new InjectionToken<any>('SERVICES');
+
+          @Injectable()
+          class DestroyService {
+            ngOnDestroy() { destroyCalls++; }
+          }
+
+          @Injectable()
+          class OtherDestroyService {
+            ngOnDestroy() { destroyCalls++; }
+          }
+
+          @Component({
+            template: '<div></div>',
+            providers: [
+              {provide: SERVICES, useClass: DestroyService, multi: true},
+              {provide: SERVICES, useClass: OtherDestroyService, multi: true},
+            ]
+          })
+          class App {
+            constructor(@Inject(SERVICES) s: any) {}
+          }
+
+          TestBed.configureTestingModule({declarations: [App]});
+          const fixture = TestBed.createComponent(App);
+          fixture.detectChanges();
+          fixture.destroy();
+
+          expect(destroyCalls).toBe(2);
+        });
+
   });
 
   describe('components and directives', () => {