fix(core): call ngOnDestroy on all services that have it (#23755)

Previously, ngOnDestroy was only called on services which were statically
determined to have ngOnDestroy methods. In some cases, such as with services
instantiated via factory functions, it's not statically known that the service
has an ngOnDestroy method.

This commit changes the runtime to look for ngOnDestroy when instantiating
all DI tokens, and to call the method if it's present.

Fixes #22466
Fixes #22240
Fixes #14818

PR Close #23755

Author: alxhub

packages/core/src/view/ng_module.ts

@@ -150,6 +150,15 @@ function _createProviderInstance(ngModule: NgModuleData, providerDef: NgModulePr
       injectable = providerDef.value;
       break;
   }
+
+  // The read of `ngOnDestroy` here is slightly expensive as it's megamorphic, so it should be
+  // avoided if possible. The sequence of checks here determines whether ngOnDestroy needs to be
+  // checked. It might not if the `injectable` isn't an object or if NodeFlags.OnDestroy is already
+  // set (ngOnDestroy was detected statically).
+  if (injectable !== UNDEFINED_VALUE && injectable != null && typeof injectable === 'object' &&
+      !(providerDef.flags & NodeFlags.OnDestroy) && typeof injectable.ngOnDestroy === 'function') {
+    providerDef.flags |= NodeFlags.OnDestroy;
+  }
   return injectable === undefined ? UNDEFINED_VALUE : injectable;
 }
 
@@ -199,12 +208,17 @@ function _callFactory(ngModule: NgModuleData, factory: any, deps: DepDef[]): any
 
 export function callNgModuleLifecycle(ngModule: NgModuleData, lifecycles: NodeFlags) {
   const def = ngModule._def;
+  const destroyed = new Set<any>();
   for (let i = 0; i < def.providers.length; i++) {
     const provDef = def.providers[i];
     if (provDef.flags & NodeFlags.OnDestroy) {
       const instance = ngModule._providers[i];
       if (instance && instance !== UNDEFINED_VALUE) {
-        instance.ngOnDestroy();
+        const onDestroy: Function|undefined = instance.ngOnDestroy;
+        if (typeof onDestroy === 'function' && !destroyed.has(instance)) {
+          onDestroy.apply(instance);
+          destroyed.add(instance);
+        }
       }
     }
   }

packages/core/test/view/ng_module_spec.ts

@@ -101,6 +101,22 @@ function makeProviders(classes: any[], modules: any[]): NgModuleDefinition {
                     flags: NodeFlags.TypeClassProvider | NodeFlags.LazyProvider, token,
                     value: token,
                   }));
+  return makeModule(modules, providers);
+}
+
+function makeFactoryProviders(
+    factories: {token: any, factory: Function}[], modules: any[]): NgModuleDefinition {
+  const providers = factories.map((factory, index) => ({
+                                    index,
+                                    deps: [],
+                                    flags: NodeFlags.TypeFactoryProvider | NodeFlags.LazyProvider,
+                                    token: factory.token,
+                                    value: factory.factory,
+                                  }));
+  return makeModule(modules, providers);
+}
+
+function makeModule(modules: any[], providers: NgModuleProviderDef[]): NgModuleDefinition {
   const providersByKey: {[key: string]: NgModuleProviderDef} = {};
   providers.forEach(provider => providersByKey[tokenKey(provider.token)] = provider);
   return {factory: null, providers, providersByKey, modules, isRoot: true};
@@ -155,4 +171,58 @@ describe('NgModuleRef_ injector', () => {
 
   it('injects with the current injector always set',
      () => { expect(() => ref.injector.get(UsesInject)).not.toThrow(); });
+
+  it('calls ngOnDestroy on services created via factory', () => {
+    class Module {}
+
+    class Service {
+      static destroyed = 0;
+      ngOnDestroy(): void { Service.destroyed++; }
+    }
+
+    const ref = createNgModuleRef(
+        Module, Injector.NULL, [], makeFactoryProviders(
+                                       [{
+                                         token: Service,
+                                         factory: () => new Service(),
+                                       }],
+                                       [Module]));
+
+    expect(ref.injector.get(Service)).toBeDefined();
+    expect(Service.destroyed).toBe(0);
+    ref.destroy();
+    expect(Service.destroyed).toBe(1);
+  });
+
+  it('only calls ngOnDestroy once per instance', () => {
+    class Module {}
+
+    class Service {
+      static destroyed = 0;
+      ngOnDestroy(): void { Service.destroyed++; }
+    }
+
+    class OtherToken {}
+
+    const instance = new Service();
+    const ref = createNgModuleRef(
+        Module, Injector.NULL, [], makeFactoryProviders(
+                                       [
+                                         {
+                                           token: Service,
+                                           factory: () => instance,
+                                         },
+                                         {
+                                           token: OtherToken,
+                                           factory: () => instance,
+                                         }
+                                       ],
+                                       [Module]));
+
+    expect(ref.injector.get(Service)).toBe(instance);
+    expect(ref.injector.get(OtherToken)).toBe(instance);
+    expect(Service.destroyed).toBe(0);
+    ref.destroy();
+    expect(Service.destroyed).toBe(1);
+  });
 });