From ab3f4c9fe143d650b4a1e521d544e0fd4811f995 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sat, 9 May 2020 14:33:54 +0200 Subject: [PATCH] fix(core): infinite loop if injectable using inheritance has a custom decorator (#37022) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #35733. PR Close #37022 --- packages/core/src/render3/di.ts | 35 ++++-- packages/core/test/render3/providers_spec.ts | 123 ++++++++++++++++++- 2 files changed, 146 insertions(+), 12 deletions(-) diff --git a/packages/core/src/render3/di.ts b/packages/core/src/render3/di.ts index 72551166f4..0ce15a35ad 100644 --- a/packages/core/src/render3/di.ts +++ b/packages/core/src/render3/di.ts @@ -657,16 +657,31 @@ export function ɵɵgetFactoryOf(type: Type): FactoryFn|null { */ export function ɵɵgetInheritedFactory(type: Type): (type: Type) => T { return noSideEffects(() => { - const proto = Object.getPrototypeOf(type.prototype).constructor as Type; - const factory = (proto as any)[NG_FACTORY_DEF] || ɵɵgetFactoryOf(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(); }); } diff --git a/packages/core/test/render3/providers_spec.ts b/packages/core/test/render3/providers_spec.ts index 394ddb5896..e41a689833 100644 --- a/packages/core/test/render3/providers_spec.ts +++ b/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 => { + 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[];