From 2e9fdbde9eb26fea17e3e68e272dc1c2cc9f4fa3 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sat, 27 Jun 2020 19:27:24 +0200 Subject: [PATCH] fix(core): prevent NgModule scope being overwritten in JIT compiler (#37795) In JIT compiled apps, component definitions are compiled upon first access. For a component class `A` that extends component class `B`, the `B` component is also compiled when the `InheritDefinitionFeature` runs during the compilation of `A` before it has finalized. A problem arises when the compilation of `B` would flush the NgModule scoping queue, where the NgModule declaring `A` is still pending. The scope information would be applied to the definition of `A`, but its compilation is still in progress so requesting the component definition would compile `A` again from scratch. This "inner compilation" is correctly assigned the NgModule scope, but once the "outer compilation" of `A` finishes it would overwrite the inner compilation's definition, losing the NgModule scope information. In summary, flushing the NgModule scope queue could trigger a reentrant compilation, where JIT compilation is non-reentrant. To avoid the reentrant compilation, a compilation depth counter is introduced to avoid flushing the NgModule scope during nested compilations. Fixes #37105 PR Close #37795 --- packages/core/src/render3/jit/directive.ts | 43 ++++++++++---- .../test/acceptance/ngmodule_scope_spec.ts | 58 +++++++++++++++++++ 2 files changed, 90 insertions(+), 11 deletions(-) create mode 100644 packages/core/test/acceptance/ngmodule_scope_spec.ts diff --git a/packages/core/src/render3/jit/directive.ts b/packages/core/src/render3/jit/directive.ts index 1b214edd79..6da8acbbe9 100644 --- a/packages/core/src/render3/jit/directive.ts +++ b/packages/core/src/render3/jit/directive.ts @@ -26,7 +26,20 @@ import {angularCoreEnv} from './environment'; import {getJitOptions} from './jit_options'; import {flushModuleScopingQueueAsMuchAsPossible, patchComponentDefWithScope, transitiveScopesFor} from './module'; - +/** + * Keep track of the compilation depth to avoid reentrancy issues during JIT compilation. This + * matters in the following scenario: + * + * Consider a component 'A' that extends component 'B', both declared in module 'M'. During + * the compilation of 'A' the definition of 'B' is requested to capture the inheritance chain, + * potentially triggering compilation of 'B'. If this nested compilation were to trigger + * `flushModuleScopingQueueAsMuchAsPossible` it may happen that module 'M' is still pending in the + * queue, resulting in 'A' and 'B' to be patched with the NgModule scope. As the compilation of + * 'A' is still in progress, this would introduce a circular dependency on its compilation. To avoid + * this issue, the module scope queue is only flushed for compilations at the depth 0, to ensure + * all compilations have finished. + */ +let compilationDepth = 0; /** * Compile an Angular component according to its decorator metadata, and patch the resulting @@ -106,18 +119,26 @@ export function compileComponent(type: Type, metadata: Component): void { interpolation: metadata.interpolation, viewProviders: metadata.viewProviders || null, }; - if (meta.usesInheritance) { - addDirectiveDefToUndecoratedParents(type); + + compilationDepth++; + try { + if (meta.usesInheritance) { + addDirectiveDefToUndecoratedParents(type); + } + ngComponentDef = compiler.compileComponent(angularCoreEnv, templateUrl, meta); + } finally { + // Ensure that the compilation depth is decremented even when the compilation failed. + compilationDepth--; } - ngComponentDef = compiler.compileComponent(angularCoreEnv, templateUrl, meta); - - // When NgModule decorator executed, we enqueued the module definition such that - // it would only dequeue and add itself as module scope to all of its declarations, - // but only if if all of its declarations had resolved. This call runs the check - // to see if any modules that are in the queue can be dequeued and add scope to - // their declarations. - flushModuleScopingQueueAsMuchAsPossible(); + if (compilationDepth === 0) { + // When NgModule decorator executed, we enqueued the module definition such that + // it would only dequeue and add itself as module scope to all of its declarations, + // but only if if all of its declarations had resolved. This call runs the check + // to see if any modules that are in the queue can be dequeued and add scope to + // their declarations. + flushModuleScopingQueueAsMuchAsPossible(); + } // If component compilation is async, then the @NgModule annotation which declares the // component may execute and set an ngSelectorScope property on the component type. This diff --git a/packages/core/test/acceptance/ngmodule_scope_spec.ts b/packages/core/test/acceptance/ngmodule_scope_spec.ts new file mode 100644 index 0000000000..3f6e167b9a --- /dev/null +++ b/packages/core/test/acceptance/ngmodule_scope_spec.ts @@ -0,0 +1,58 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {Component, destroyPlatform, NgModule, Pipe, PipeTransform} from '@angular/core'; +import {BrowserModule} from '@angular/platform-browser'; +import {platformBrowserDynamic} from '@angular/platform-browser-dynamic'; +import {withBody} from '@angular/private/testing'; + +describe('NgModule scopes', () => { + beforeEach(destroyPlatform); + afterEach(destroyPlatform); + + it('should apply NgModule scope to a component that extends another component class', + withBody('', async () => { + // Regression test for https://github.com/angular/angular/issues/37105 + // + // This test reproduces a scenario that used to fail due to a reentrancy issue in Ivy's JIT + // compiler. Extending a component from a decorated baseclass would inadvertently compile + // the subclass twice. NgModule scope information would only be present on the initial + // compilation, but then overwritten during the second compilation. This meant that the + // baseclass did not have a NgModule scope, such that declarations are not available. + // + // The scenario cannot be tested using TestBed as it influences how NgModule + // scopes are applied, preventing the issue from occurring. + + @Pipe({name: 'multiply'}) + class MultiplyPipe implements PipeTransform { + transform(value: number, factor: number): number { + return value * factor; + } + } + + @Component({template: '...'}) + class BaseComponent { + } + + @Component({selector: 'my-app', template: 'App - {{ 3 | multiply:2 }}'}) + class App extends BaseComponent { + } + + @NgModule({ + imports: [BrowserModule], + declarations: [App, BaseComponent, MultiplyPipe], + bootstrap: [App], + }) + class Mod { + } + + const ngModuleRef = await platformBrowserDynamic().bootstrapModule(Mod); + expect(document.body.textContent).toContain('App - 6'); + ngModuleRef.destroy(); + })); +});