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

Author: JoostK

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<any>, metadata: Component): void {
           interpolation: metadata.interpolation,
           viewProviders: metadata.viewProviders || null,
         };
-        if (meta.usesInheritance) {
-          addDirectiveDefToUndecoratedParents(type);
-        }
 
-        ngComponentDef = compiler.compileComponent(angularCoreEnv, templateUrl, meta);
+        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--;
+        }
 
-        // 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

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('<my-app></my-app>', 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();
+     }));
+});