fix(ivy): component destroy hook called twice when configured as provider (#28470)

Fixes the `ngOnDestroy` hook on a component or directive being called twice, if the type is also registered as a provider.

This PR resolves FW-1010.

PR Close #28470

integration/_payload-limits.json

@@ -21,7 +21,7 @@
     "master": {
       "uncompressed": {
         "runtime": 1440,
-        "main": 207765,
+        "main": 209904,
         "polyfills": 38390
       }
     }

packages/core/src/di/r3_injector.ts

@@ -467,6 +467,10 @@ export function isTypeProvider(value: SingleProvider): value is TypeProvider {
   return typeof value === 'function';
 }
 
+export function isClassProvider(value: SingleProvider): value is ClassProvider {
+  return !!(value as StaticClassProvider | ClassProvider).useClass;
+}
+
 function hasDeps(value: ClassProvider | ConstructorProvider | StaticClassProvider):
     value is ClassProvider&{deps: any[]} {
   return !!(value as any).deps;

packages/core/src/render3/di.ts

@@ -520,10 +520,6 @@ export function getNodeInjectable(
     setTNodeAndViewData(tNode, lData);
     try {
       value = lData[index] = factory.factory(null, tData, lData, tNode);
-      const tView = lData[TVIEW];
-      if (value && factory.isProvider && value.ngOnDestroy) {
-        (tView.destroyHooks || (tView.destroyHooks = [])).push(index, value.ngOnDestroy);
-      }
     } finally {
       if (factory.injectImpl) setInjectImplementation(previousInjectImplementation);
       setIncludeViewProviders(previousIncludeViewProviders);

packages/core/src/render3/di_setup.ts

@@ -8,8 +8,8 @@
 
 
 import {resolveForwardRef} from '../di/forward_ref';
-import {Provider} from '../di/interface/provider';
-import {isTypeProvider, providerToFactory} from '../di/r3_injector';
+import {ClassProvider, Provider} from '../di/interface/provider';
+import {isClassProvider, isTypeProvider, providerToFactory} from '../di/r3_injector';
 
 import {DirectiveDef} from '.';
 import {diPublicInInjector, getNodeInjectable, getOrCreateNodeInjectorForNode} from './di';
@@ -81,10 +81,19 @@ 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) {
+        const tView = lView[TVIEW];
+        (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, true, directiveInject);
+      const factory = new NodeInjectorFactory(providerFactory, isViewProvider, directiveInject);
       const existingFactoryIndex = indexOf(
           token, tInjectables, isViewProvider ? beginIndex : beginIndex + cptViewProvidersCount,
           endIndex);
@@ -246,7 +255,7 @@ function multiFactory(
         this: NodeInjectorFactory, _: null, tData: TData, lData: LView, tNode: TElementNode) => any,
     index: number, isViewProvider: boolean, isComponent: boolean,
     f: () => any): NodeInjectorFactory {
-  const factory = new NodeInjectorFactory(factoryFn, isViewProvider, true, directiveInject);
+  const factory = new NodeInjectorFactory(factoryFn, isViewProvider, directiveInject);
   factory.multi = [];
   factory.index = index;
   factory.componentProviders = 0;

packages/core/src/render3/instructions.ts

@@ -2067,8 +2067,7 @@ function baseResolveDirective<T>(
     tView: TView, viewData: LView, def: DirectiveDef<T>,
     directiveFactory: (t: Type<T>| null) => any) {
   tView.data.push(def);
-  const nodeInjectorFactory =
-      new NodeInjectorFactory(directiveFactory, isComponentDef(def), false, null);
+  const nodeInjectorFactory = new NodeInjectorFactory(directiveFactory, isComponentDef(def), null);
   tView.blueprint.push(nodeInjectorFactory);
   viewData.push(nodeInjectorFactory);
 }

packages/core/src/render3/interfaces/injector.ts

@@ -235,10 +235,6 @@ export class NodeInjectorFactory {
        * Set to `true` if the token is declared in `viewProviders` (or if it is component).
        */
       isViewProvider: boolean,
-      /**
-       * Set to `true` if the token is a provider, and not a directive.
-       */
-      public isProvider: boolean,
       injectImplementation: null|(<T>(token: Type<T>|InjectionToken<T>, flags: InjectFlags) => T)) {
     this.canSeeViewProviders = isViewProvider;
     this.injectImpl = injectImplementation;

packages/core/src/render3/node_manipulation.ts

@@ -9,9 +9,9 @@
 import {ViewEncapsulation} from '../metadata/view';
 
 import {attachPatchData} from './context_discovery';
-import {callHooks} from './hooks';
 import {LContainer, NATIVE, VIEWS, unusedValueExportToPlacateAjd as unused1} from './interfaces/container';
 import {ComponentDef} from './interfaces/definition';
+import {NodeInjectorFactory} from './interfaces/injector';
 import {TContainerNode, TElementContainerNode, TElementNode, TNode, TNodeFlags, TNodeType, TViewNode, unusedValueExportToPlacateAjd as unused2} from './interfaces/node';
 import {unusedValueExportToPlacateAjd as unused3} from './interfaces/projection';
 import {ProceduralRenderer3, RComment, RElement, RNode, RText, Renderer3, isProceduralRenderer, unusedValueExportToPlacateAjd as unused4} from './interfaces/renderer';
@@ -495,8 +495,16 @@ function removeListeners(lView: LView): void {
 function executeOnDestroys(view: LView): void {
   const tView = view[TVIEW];
   let destroyHooks: HookData|null;
+
   if (tView != null && (destroyHooks = tView.destroyHooks) != null) {
-    callHooks(view, destroyHooks);
+    for (let i = 0; i < destroyHooks.length; i += 2) {
+      const context = view[destroyHooks[i] as number];
+
+      // Only call the destroy hook if the context has been requested.
+      if (!(context instanceof NodeInjectorFactory)) {
+        (destroyHooks[i + 1] as() => void).call(context);
+      }
+    }
   }
 }
 

packages/core/test/acceptance/component_spec.ts

@@ -0,0 +1,52 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {Component, InjectionToken} from '@angular/core';
+import {TestBed} from '@angular/core/testing';
+
+
+describe('component', () => {
+  describe('view destruction', () => {
+    it('should invoke onDestroy only once when a component is registered as a provider', () => {
+      const testToken = new InjectionToken<ParentWithOnDestroy>('testToken');
+      let destroyCalls = 0;
+
+      @Component({
+        selector: 'comp-with-on-destroy',
+        template: '',
+        providers: [{provide: testToken, useExisting: ParentWithOnDestroy}]
+      })
+      class ParentWithOnDestroy {
+        ngOnDestroy() { destroyCalls++; }
+      }
+
+      @Component({selector: 'child', template: ''})
+      class ChildComponent {
+        // We need to inject the parent so the provider is instantiated.
+        constructor(_parent: ParentWithOnDestroy) {}
+      }
+
+      @Component({
+        template: `
+          <comp-with-on-destroy>
+            <child></child>
+          </comp-with-on-destroy>
+        `
+      })
+      class App {
+      }
+
+      TestBed.configureTestingModule({declarations: [App, ParentWithOnDestroy, ChildComponent]});
+      const fixture = TestBed.createComponent(App);
+      fixture.detectChanges();
+      fixture.destroy();
+
+      expect(destroyCalls).toBe(1, 'Expected `ngOnDestroy` to only be called once.');
+    });
+  });
+});

packages/core/test/acceptance/providers_spec.ts

@@ -0,0 +1,184 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {Component, Injectable} from '@angular/core';
+import {TestBed} from '@angular/core/testing';
+import {onlyInIvy} from '@angular/private/testing';
+
+
+describe('providers', () => {
+  describe('lifecycles', () => {
+    it('should inherit ngOnDestroy hooks on providers', () => {
+      const logs: string[] = [];
+
+      @Injectable()
+      class SuperInjectableWithDestroyHook {
+        ngOnDestroy() { logs.push('OnDestroy'); }
+      }
+
+      @Injectable()
+      class SubInjectableWithDestroyHook extends SuperInjectableWithDestroyHook {
+      }
+
+      @Component({template: '', providers: [SubInjectableWithDestroyHook]})
+      class App {
+        constructor(foo: SubInjectableWithDestroyHook) {}
+      }
+
+      TestBed.configureTestingModule({declarations: [App]});
+      const fixture = TestBed.createComponent(App);
+      fixture.detectChanges();
+      fixture.destroy();
+
+      expect(logs).toEqual(['OnDestroy']);
+    });
+
+    it('should not call ngOnDestroy for providers that have not been requested', () => {
+      const logs: string[] = [];
+
+      @Injectable()
+      class InjectableWithDestroyHook {
+        ngOnDestroy() { logs.push('OnDestroy'); }
+      }
+
+      @Component({template: '', providers: [InjectableWithDestroyHook]})
+      class App {
+      }
+
+      TestBed.configureTestingModule({declarations: [App]});
+      const fixture = TestBed.createComponent(App);
+      fixture.detectChanges();
+      fixture.destroy();
+
+      expect(logs).toEqual([]);
+    });
+
+    it('should only call ngOnDestroy once for multiple instances', () => {
+      const logs: string[] = [];
+
+      @Injectable()
+      class InjectableWithDestroyHook {
+        ngOnDestroy() { logs.push('OnDestroy'); }
+      }
+
+      @Component({selector: 'my-cmp', template: ''})
+      class MyComponent {
+        constructor(foo: InjectableWithDestroyHook) {}
+      }
+
+      @Component({
+        template: `
+          <my-cmp></my-cmp>
+          <my-cmp></my-cmp>
+        `,
+        providers: [InjectableWithDestroyHook]
+      })
+      class App {
+      }
+
+      TestBed.configureTestingModule({declarations: [App, MyComponent]});
+      const fixture = TestBed.createComponent(App);
+      fixture.detectChanges();
+      fixture.destroy();
+
+      expect(logs).toEqual(['OnDestroy']);
+    });
+
+    it('should call ngOnDestroy when providing same token via useClass', () => {
+      const logs: string[] = [];
+
+      @Injectable()
+      class InjectableWithDestroyHook {
+        ngOnDestroy() { logs.push('OnDestroy'); }
+      }
+
+      @Component({
+        template: '',
+        providers: [{provide: InjectableWithDestroyHook, useClass: InjectableWithDestroyHook}]
+      })
+      class App {
+        constructor(foo: InjectableWithDestroyHook) {}
+      }
+
+      TestBed.configureTestingModule({declarations: [App]});
+      const fixture = TestBed.createComponent(App);
+      fixture.detectChanges();
+      fixture.destroy();
+
+      expect(logs).toEqual(['OnDestroy']);
+    });
+
+    onlyInIvy('Destroy hook of useClass provider is invoked correctly')
+        .it('should only call ngOnDestroy of value when providing via useClass', () => {
+          const logs: string[] = [];
+
+          @Injectable()
+          class InjectableWithDestroyHookToken {
+            ngOnDestroy() { logs.push('OnDestroy Token'); }
+          }
+
+          @Injectable()
+          class InjectableWithDestroyHookValue {
+            ngOnDestroy() { logs.push('OnDestroy Value'); }
+          }
+
+          @Component({
+            template: '',
+            providers: [
+              {provide: InjectableWithDestroyHookToken, useClass: InjectableWithDestroyHookValue}
+            ]
+          })
+          class App {
+            constructor(foo: InjectableWithDestroyHookToken) {}
+          }
+
+          TestBed.configureTestingModule({declarations: [App]});
+          const fixture = TestBed.createComponent(App);
+          fixture.detectChanges();
+          fixture.destroy();
+
+          expect(logs).toEqual(['OnDestroy Value']);
+        });
+
+    it('should only call ngOnDestroy of value when providing via useExisting', () => {
+      const logs: string[] = [];
+
+      @Injectable()
+      class InjectableWithDestroyHookToken {
+        ngOnDestroy() { logs.push('OnDestroy Token'); }
+      }
+
+      @Injectable()
+      class InjectableWithDestroyHookExisting {
+        ngOnDestroy() { logs.push('OnDestroy Existing'); }
+      }
+
+      @Component({
+        template: '',
+        providers: [
+          InjectableWithDestroyHookExisting, {
+            provide: InjectableWithDestroyHookToken,
+            useExisting: InjectableWithDestroyHookExisting
+          }
+        ]
+      })
+      class App {
+        constructor(foo1: InjectableWithDestroyHookExisting, foo2: InjectableWithDestroyHookToken) {
+        }
+      }
+
+      TestBed.configureTestingModule({declarations: [App]});
+      const fixture = TestBed.createComponent(App);
+      fixture.detectChanges();
+      fixture.destroy();
+
+      expect(logs).toEqual(['OnDestroy Existing']);
+    });
+
+  });
+});

packages/core/test/render3/component_spec.ts

@@ -6,9 +6,9 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ViewEncapsulation, defineInjectable, defineInjector} from '../../src/core';
+import {InjectionToken, ViewEncapsulation, defineInjectable, defineInjector} from '../../src/core';
 
-import {AttributeMarker, ComponentFactory, LifecycleHooksFeature, defineComponent, directiveInject, markDirty, template, getRenderedText} from '../../src/render3/index';
+import {AttributeMarker, ComponentFactory, LifecycleHooksFeature, defineComponent, directiveInject, markDirty, template, getRenderedText, ProvidersFeature} from '../../src/render3/index';
 import {bind, container, containerRefreshEnd, containerRefreshStart, element, elementEnd, elementProperty, elementStart, embeddedViewEnd, embeddedViewStart, nextContext, text, textBinding, tick} from '../../src/render3/instructions';
 import {ComponentDef, RenderFlags} from '../../src/render3/interfaces/definition';
 
@@ -669,22 +669,22 @@ describe('recursive components', () => {
 });
 
 describe('view destruction', () => {
-  let wasOnDestroyCalled = false;
-
-  class ComponentWithOnDestroy {
-    static ngComponentDef = defineComponent({
-      selectors: [['comp-with-destroy']],
-      type: ComponentWithOnDestroy,
-      consts: 0,
-      vars: 0,
-      factory: () => new ComponentWithOnDestroy(),
-      template: (rf: any, ctx: any) => {},
-    });
-
-    ngOnDestroy() { wasOnDestroyCalled = true; }
-  }
-
   it('should invoke onDestroy when directly destroying a root view', () => {
+    let wasOnDestroyCalled = false;
+
+    class ComponentWithOnDestroy {
+      static ngComponentDef = defineComponent({
+        selectors: [['comp-with-destroy']],
+        type: ComponentWithOnDestroy,
+        consts: 0,
+        vars: 0,
+        factory: () => new ComponentWithOnDestroy(),
+        template: (rf: any, ctx: any) => {},
+      });
+
+      ngOnDestroy() { wasOnDestroyCalled = true; }
+    }
+
     // This test asserts that the view tree is set up correctly based on the knowledge that this
     // tree is used during view destruction. If the child view is not correctly attached as a
     // child of the root view, then the onDestroy hook on the child view will never be called
@@ -704,4 +704,5 @@ describe('view destruction', () => {
             true,
             'Expected component onDestroy method to be called when its parent view is destroyed');
   });
+
 });

packages/core/test/render3/providers_spec.ts

@@ -1355,6 +1355,7 @@ describe('providers', () => {
       expect(fixture.html).toEqual('<div></div>');
       expect(logs).toEqual(['Injectable OnDestroy']);
     });
+
   });
 });
 interface ComponentTest {

tools/material-ci/angular_material_test_blocklist.js

@@ -237,46 +237,6 @@ window.testBlocklist = {
     "error": "TypeError: Cannot read property 'accordion' of undefined",
     "notes": "Unknown"
   },
-  "CdkDrag standalone draggable should enable native drag interactions when there is a drag handle": {
-    "error": "TypeError: Cannot read property 'removeEventListener' of null",
-    "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider"
-  },
-  "CdkDrag draggable with a handle should not be able to drag the entire element if it has a handle": {
-    "error": "TypeError: Cannot read property 'removeEventListener' of null",
-    "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider"
-  },
-  "CdkDrag draggable with a handle should be able to drag an element using its handle": {
-    "error": "TypeError: Cannot read property 'removeEventListener' of null",
-    "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider"
-  },
-  "CdkDrag draggable with a handle should not be able to drag the element if the handle is disabled": {
-    "error": "TypeError: Cannot read property 'removeEventListener' of null",
-    "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider"
-  },
-  "CdkDrag draggable with a handle should not be able to drag using the handle if the element is disabled": {
-    "error": "TypeError: Cannot read property 'removeEventListener' of null",
-    "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider"
-  },
-  "CdkDrag draggable with a handle should be able to use a handle that was added after init": {
-    "error": "TypeError: Cannot read property 'removeEventListener' of null",
-    "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider"
-  },
-  "CdkDrag draggable with a handle should be able to use more than one handle to drag the element": {
-    "error": "TypeError: Cannot read property 'removeEventListener' of null",
-    "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider"
-  },
-  "CdkDrag draggable with a handle should be able to drag with a handle that is not a direct descendant": {
-    "error": "TypeError: Cannot read property 'removeEventListener' of null",
-    "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider"
-  },
-  "CdkDrag draggable with a handle should disable the tap highlight while dragging via the handle": {
-    "error": "TypeError: Cannot read property 'removeEventListener' of null",
-    "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider"
-  },
-  "CdkDrag draggable with a handle should preserve any existing `webkitTapHighlightColor`": {
-    "error": "TypeError: Cannot read property 'removeEventListener' of null",
-    "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider"
-  },
   "CdkDrag in a drop container should be able to customize the preview element": {
     "error": "Error: Expected cdk-drag cdk-drag-preview to contain 'custom-preview'.",
     "notes": "Unknown"