From 5581e97d2ac6b232cc7402370e681eb909bd63e7 Mon Sep 17 00:00:00 2001
From: Alex Rickabaugh <alx+alxhub@alxandria.net>
Date: Mon, 7 May 2018 10:47:59 -0700
Subject: [PATCH] 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
---
 packages/core/src/view/ng_module.ts       | 16 +++++-
 packages/core/test/view/ng_module_spec.ts | 70 +++++++++++++++++++++++
 2 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/packages/core/src/view/ng_module.ts b/packages/core/src/view/ng_module.ts
index 4a7d6a19ad..9e6bd20305 100644
--- a/packages/core/src/view/ng_module.ts
+++ b/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);
+        }
       }
     }
   }
diff --git a/packages/core/test/view/ng_module_spec.ts b/packages/core/test/view/ng_module_spec.ts
index 4e89b95ae0..6e3c1fa9a1 100644
--- a/packages/core/test/view/ng_module_spec.ts
+++ b/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);
+  });
 });