fix(core): infinite loop if injectable using inheritance has a custom decorator (angular#37022)

If we detect that an injectable class is inheriting from another injectable, we generate code that looks something like this:

```
const baseFactory = ɵɵgetInheritedFactory(Child);

@Injectable()
class Parent {}

@Injectable()
class Child extends Parent {
  static ɵfac = (t) => baseFactory(t || Child)
}
```

This usually works fine, because the `ɵɵgetInheritedFactory` resolves to the factory of `Parent`, but the logic can break down if the `Child` class has a custom decorator. Custom decorators can return a new class that extends the original once, which means that the `ɵɵgetInheritedFactory` call will now resolve to the factory of the `Child`, causing an infinite loop.

These changes fix the issue by changing the inherited factory resolution logic so that it walks up the prototype chain class-by-class, while skipping classes that have the same factory as the class that was passed in.

Fixes angular#35733.

PR Close angular#37022

Author: crisbeto

packages/core/src/render3/di.ts

@@ -657,16 +657,31 @@ export function ɵɵgetFactoryOf<T>(type: Type<any>): FactoryFn<T>|null {
  */
 export function ɵɵgetInheritedFactory<T>(type: Type<any>): (type: Type<T>) => T {
   return noSideEffects(() => {
-    const proto = Object.getPrototypeOf(type.prototype).constructor as Type<any>;
-    const factory = (proto as any)[NG_FACTORY_DEF] || ɵɵgetFactoryOf<T>(proto);
-    if (factory !== null) {
-      return factory;
-    } else {
-      // There is no factory defined. Either this was improper usage of inheritance
-      // (no Angular decorator on the superclass) or there is no constructor at all
-      // in the inheritance chain. Since the two cases cannot be distinguished, the
-      // latter has to be assumed.
-      return (t) => new t();
+    const ownConstructor = type.prototype.constructor;
+    const ownFactory = ownConstructor[NG_FACTORY_DEF] || ɵɵgetFactoryOf(ownConstructor);
+    const objectPrototype = Object.prototype;
+    let parent = Object.getPrototypeOf(type.prototype).constructor;
+
+    // Go up the prototype until we hit `Object`.
+    while (parent && parent !== objectPrototype) {
+      const factory = parent[NG_FACTORY_DEF] || ɵɵgetFactoryOf(parent);
+
+      // If we hit something that has a factory and the factory isn't the same as the type,
+      // we've found the inherited factory. Note the check that the factory isn't the type's
+      // own factory is redundant in most cases, but if the user has custom decorators on the
+      // class, this lookup will start one level down in the prototype chain, causing us to
+      // find the own factory first and potentially triggering an infinite loop downstream.
+      if (factory && factory !== ownFactory) {
+        return factory;
+      }
+
+      parent = Object.getPrototypeOf(parent);
     }
+
+    // There is no factory defined. Either this was improper usage of inheritance
+    // (no Angular decorator on the superclass) or there is no constructor at all
+    // in the inheritance chain. Since the two cases cannot be distinguished, the
+    // latter has to be assumed.
+    return t => new t();
   });
 }

packages/core/test/render3/providers_spec.ts

@@ -6,10 +6,10 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Component as _Component, ComponentFactoryResolver, ElementRef, Injectable as _Injectable, InjectFlags, InjectionToken, InjectorType, Provider, RendererFactory2, ViewContainerRef, ɵNgModuleDef as NgModuleDef, ɵɵdefineInjectable, ɵɵdefineInjector, ɵɵinject} from '../../src/core';
+import {Component as _Component, ComponentFactoryResolver, ElementRef, Injectable as _Injectable, InjectFlags, InjectionToken, InjectorType, Provider, RendererFactory2, Type, ViewContainerRef, ɵNgModuleDef as NgModuleDef, ɵɵdefineInjectable, ɵɵdefineInjector, ɵɵinject} from '../../src/core';
 import {forwardRef} from '../../src/di/forward_ref';
 import {createInjector} from '../../src/di/r3_injector';
-import {injectComponentFactoryResolver, ɵɵdefineComponent, ɵɵdefineDirective, ɵɵdirectiveInject, ɵɵelement, ɵɵelementEnd, ɵɵelementStart, ɵɵProvidersFeature, ɵɵtext, ɵɵtextInterpolate1} from '../../src/render3/index';
+import {injectComponentFactoryResolver, ɵɵdefineComponent, ɵɵdefineDirective, ɵɵdirectiveInject, ɵɵelement, ɵɵelementEnd, ɵɵelementStart, ɵɵgetInheritedFactory, ɵɵProvidersFeature, ɵɵtext, ɵɵtextInterpolate1} from '../../src/render3/index';
 import {RenderFlags} from '../../src/render3/interfaces/definition';
 import {NgModuleFactory} from '../../src/render3/ng_module_ref';
 import {getInjector} from '../../src/render3/util/discovery_utils';
@@ -1282,7 +1282,126 @@ describe('providers', () => {
       expect(injector.get(Some).location).toEqual('From app component');
     });
   });
+
+  // Note: these tests check the behavior of `getInheritedFactory` specifically.
+  // Since `getInheritedFactory` is only generated in AOT, the tests can't be
+  // ported directly to TestBed while running in JIT mode.
+  describe('getInheritedFactory on class with custom decorator', () => {
+    function addFoo() {
+      return (constructor: Type<any>): any => {
+        const decoratedClass = class Extender extends constructor { foo = 'bar'; };
+
+        // On IE10 child classes don't inherit static properties by default. If we detect
+        // such a case, try to account for it so the tests are consistent between browsers.
+        if (Object.getPrototypeOf(decoratedClass) !== constructor) {
+          decoratedClass.prototype = constructor.prototype;
+        }
+
+        return decoratedClass;
+      };
+    }
+
+    it('should find the correct factories if a parent class has a custom decorator', () => {
+      class GrandParent {
+        static ɵfac = function GrandParent_Factory() {};
+      }
+
+      @addFoo()
+      class Parent extends GrandParent {
+        static ɵfac = function Parent_Factory() {};
+      }
+
+      class Child extends Parent {
+        static ɵfac = function Child_Factory() {};
+      }
+
+      expect(ɵɵgetInheritedFactory(Child).name).toBe('Parent_Factory');
+      expect(ɵɵgetInheritedFactory(Parent).name).toBe('GrandParent_Factory');
+      expect(ɵɵgetInheritedFactory(GrandParent).name).toBeFalsy();
+    });
+
+    it('should find the correct factories if a child class has a custom decorator', () => {
+      class GrandParent {
+        static ɵfac = function GrandParent_Factory() {};
+      }
+
+      class Parent extends GrandParent {
+        static ɵfac = function Parent_Factory() {};
+      }
+
+      @addFoo()
+      class Child extends Parent {
+        static ɵfac = function Child_Factory() {};
+      }
+
+      expect(ɵɵgetInheritedFactory(Child).name).toBe('Parent_Factory');
+      expect(ɵɵgetInheritedFactory(Parent).name).toBe('GrandParent_Factory');
+      expect(ɵɵgetInheritedFactory(GrandParent).name).toBeFalsy();
+    });
+
+    it('should find the correct factories if a grandparent class has a custom decorator', () => {
+      @addFoo()
+      class GrandParent {
+        static ɵfac = function GrandParent_Factory() {};
+      }
+
+      class Parent extends GrandParent {
+        static ɵfac = function Parent_Factory() {};
+      }
+
+      class Child extends Parent {
+        static ɵfac = function Child_Factory() {};
+      }
+
+      expect(ɵɵgetInheritedFactory(Child).name).toBe('Parent_Factory');
+      expect(ɵɵgetInheritedFactory(Parent).name).toBe('GrandParent_Factory');
+      expect(ɵɵgetInheritedFactory(GrandParent).name).toBeFalsy();
+    });
+
+    it('should find the correct factories if all classes have a custom decorator', () => {
+      @addFoo()
+      class GrandParent {
+        static ɵfac = function GrandParent_Factory() {};
+      }
+
+      @addFoo()
+      class Parent extends GrandParent {
+        static ɵfac = function Parent_Factory() {};
+      }
+
+      @addFoo()
+      class Child extends Parent {
+        static ɵfac = function Child_Factory() {};
+      }
+
+      expect(ɵɵgetInheritedFactory(Child).name).toBe('Parent_Factory');
+      expect(ɵɵgetInheritedFactory(Parent).name).toBe('GrandParent_Factory');
+      expect(ɵɵgetInheritedFactory(GrandParent).name).toBeFalsy();
+    });
+
+    it('should find the correct factories if parent and grandparent classes have a custom decorator',
+       () => {
+         @addFoo()
+         class GrandParent {
+           static ɵfac = function GrandParent_Factory() {};
+         }
+
+         @addFoo()
+         class Parent extends GrandParent {
+           static ɵfac = function Parent_Factory() {};
+         }
+
+         class Child extends Parent {
+           static ɵfac = function Child_Factory() {};
+         }
+
+         expect(ɵɵgetInheritedFactory(Child).name).toBe('Parent_Factory');
+         expect(ɵɵgetInheritedFactory(Parent).name).toBe('GrandParent_Factory');
+         expect(ɵɵgetInheritedFactory(GrandParent).name).toBeFalsy();
+       });
+  });
 });
+
 interface ComponentTest {
   providers?: Provider[];
   viewProviders?: Provider[];