Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Too much recursion because of @Injectable conflicts with other decorators #35733

Closed
tamtakoe opened this issue Feb 27, 2020 · 8 comments
Closed
Assignees
Labels
area: core Issues related to the framework runtime core: di freq1: low regression Indicates than the issue relates to something that worked in a previous version state: confirmed state: has PR type: bug/fix
Milestone

Comments

@tamtakoe
Copy link

🐞 bug report

Affected Package

Looks like @angular/core

Is this a regression?

Yes, the versions before 9 work correct (https://stackblitz.com/edit/angular-3sutb9 - works, because stackblitz is not provided Ivy-rendering)

Description

@injectable conflicts with other decorators in this example:

@Injectable()
export class TestServiceA {}

@Injectable()
@testDecorator()
export class TestServiceB extends TestServiceA {}

@Injectable()
export class TestService extends TestServiceB {}

@Component({
  selector: 'my-app',
  templateUrl: './app.component.html',
  providers: [TestService]
})
export class AppComponent  {
  constructor(private testService: TestService) {}
}

🔬 Minimal Reproduction

You can just clone the repo and run locally (stackblitz doesn't work with last versions of Angular)
https://github.com/tamtakoe/angular-max-call-stack-size

🔥 Exception or Error

core.js:3866 ERROR RangeError: Maximum call stack size exceeded
    at TestServiceB_Factory (app.component.ts:30) ...

or  ERROR InternalError: "too much recursion" ... in Firefox etc.

🌍 Your Environment

Angular Version:

Angular CLI: 9.0.3
Node: 12.4.0
OS: darwin x64

Angular: 9.0.4
... common, compiler, compiler-cli, core, forms
... language-service, localize, platform-browser
... platform-browser-dynamic, router
Ivy Workspace: Yes

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.900.3
@angular-devkit/build-angular     0.900.3
@angular-devkit/build-optimizer   0.900.3
@angular-devkit/build-webpack     0.900.3
@angular-devkit/core              9.0.3
@angular-devkit/schematics        9.0.3
@angular/cli                      9.0.3
@ngtools/webpack                  9.0.3
@schematics/angular               9.0.3
@schematics/update                0.900.3
rxjs                              6.5.4
typescript                        3.7.5
webpack                           4.41.2
@AndrewKushnir AndrewKushnir added area: core Issues related to the framework runtime comp: ivy labels Feb 27, 2020
@ngbot ngbot bot added this to the needsTriage milestone Feb 27, 2020
@PapaNappa
Copy link

I have a similar error after upgrading to 9.0.4/0.900.4:

Compiling @angular/… : es2015 as esm2015

chunk {} runtime.689ba4fd6cadb82c1ac2.js (runtime) 1.45 kB [entry] [rendered]
chunk {1} main.9d65be46ac259c9df33b.js (main) 700 kB [initial] [rendered]
chunk {2} polyfills.da2c6c4849ef9aea9de5.js (polyfills) 35.6 kB [initial] [rendered]
chunk {3} styles.1b30f95df1bf800225e4.css (styles) 62.7 kB [initial] [rendered]
Date: 2020-02-28T12:40:47.476Z - Hash: 5d47462af19da00a8547 - Time: 94639ms

ERROR in Error when flattening the source-map "<path1>.d.ts.map" for "<path1>.d.ts": RangeError: Maximum call stack size exceeded
ERROR in Error when flattening the source-map "<path2>.d.ts.map" for "<path2>.d.ts": RangeError: Maximum call stack size exceeded
ERROR in Error when flattening the source-map "<path3>.d.ts.map" for "<path3>.d.ts": RangeError: Maximum call stack size exceeded
ERROR in Error when flattening the source-map "<path4>.d.ts.map" for "<path4>.d.ts": RangeError: Maximum call stack size exceeded
…

Unfortunately, I was not yet able to reproduce this issue.
This appears to happen only once after upgrading an existing Angular <9.0.4/3 (not sure which one) project to 9.0.3/4. I can run ng build again and then it works flawlessly. Even with npm ci I cannot reproduce the error. :/

@pkozlowski-opensource
Copy link
Member

This seems to be a legit report of the behaviour change in ivy with custom decorators (there were multiple similar bugs reported previously), here is a live repro: https://ng-run.com/edit/IneGzHSECoI3LtdEXPWh

@pkozlowski-opensource pkozlowski-opensource added freq1: low regression Indicates than the issue relates to something that worked in a previous version type: bug/fix labels Mar 2, 2020
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Mar 2, 2020
@JoostK
Copy link
Member

JoostK commented Mar 20, 2020

There's several issues here.

First, this line introduces the infinite loop:

newConstructor.prototype = Object.create(target.prototype);

This effectively sets up target to act as a superclass of newConstructor, as far as I understand it. Then, when the runtime looks up the factory for TestServiceB it delegates to the parent (as there's no own constructor) here:

const proto = Object.getPrototypeOf(type.prototype).constructor as Type<any>;

Here, type.prototype === newConstructor.prototype, so using Object.getPrototypeOf we arrive at target.prototype which is TestServiceB. Therefore, the superclass' factory that is obtained for TestServiceB is TestServiceB again, causing the infinite loop.

Disclaimer: I am always confused by prototype stuff in JS, so there could be nonsense in the above.


Secondly, the compiled definitions that are present as static property on the class are not copied over to newConstructor. This is a problem for DI to work correctly, as factory functions etc will not be available. You don't currently see the effect of this as none of the constructors have parameters that need to be injected.

@crisbeto crisbeto self-assigned this May 9, 2020
crisbeto added a commit to crisbeto/angular that referenced this issue May 9, 2020
… decorator

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 breaks down if the `Child` class has a custom decorator. The way decorators work is by taking a class and returning a different class that extends it. This 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.
crisbeto added a commit to crisbeto/angular that referenced this issue May 9, 2020
… decorator

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 breaks down if the `Child` class has a custom decorator. The way decorators work is by taking a class and returning a different class that extends it. This 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.
crisbeto added a commit to crisbeto/angular that referenced this issue May 9, 2020
… decorator

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.
crisbeto added a commit to crisbeto/angular that referenced this issue May 9, 2020
… decorator

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.
crisbeto added a commit to crisbeto/angular that referenced this issue May 9, 2020
… decorator

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.
crisbeto added a commit to crisbeto/angular that referenced this issue May 13, 2020
… decorator

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.
atscott pushed a commit that referenced this issue Jun 3, 2020
… decorator (#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 #35733.

PR Close #37022
@atscott atscott closed this as completed in ab3f4c9 Jun 3, 2020
@splincode
Copy link
Contributor

@atscott Is this added backport to Angular 9?

I see only add to Angular 10

image

@atscott
Copy link
Contributor

atscott commented Jun 12, 2020

@splincode This change wasn't merged into the 9.1.x branch. At the time the fix was created, master and patch branches would have referred to 10.0 and 9.1 respectively. As of today (and 9 days ago when it was merged), the two branches were 10.1 and 10.0. Thanks for bringing this up. @crisbeto - Should we create a new PR to target 9.1?

crisbeto added a commit to crisbeto/angular that referenced this issue Jun 12, 2020
… decorator

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.
@atscott
Copy link
Contributor

atscott commented Jun 12, 2020

@splincode The fix will be included in the 9.1.12 release next week.

@splincode
Copy link
Contributor

@atscott 11 days ago 9.1.12 not found))

ngwattcos pushed a commit to ngwattcos/angular that referenced this issue Jun 25, 2020
… 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
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 24, 2020
profanis pushed a commit to profanis/angular that referenced this issue Sep 5, 2020
… 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime core: di freq1: low regression Indicates than the issue relates to something that worked in a previous version state: confirmed state: has PR type: bug/fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants