fix(core): stop reusing provider definitions across NgModuleRef instances (#25022)

Fixes #25018.

Instantiating a NgModuleRef from NgModuleFactory reuses the NgModuleDefinition if it is already present. However the NgModuleDefinition has a providers array which modified when tree shakable providers are instantiated. This corrupts the provider definitions the next time the same factory is used to create a new NgModuleRef - Two provider definitions can end up with the same index anf the injector could potentially return a completely wrong object for a provider token.

This scenario is more likely on the server where the same NgModuleFactory is reused across requests.

The fix clones the cached NgModuleDefinition so that any tree shakable providers added later do not affect the cached copy.

PR Close #25022

packages/core/src/view/entrypoint.ts

@@ -12,7 +12,7 @@ import {NgModuleFactory, NgModuleRef} from '../linker/ng_module_factory';
 import {Type} from '../type';
 
 import {initServicesIfNeeded} from './services';
-import {NgModuleDefinitionFactory, ProviderOverride, Services, ViewDefinition} from './types';
+import {NgModuleDefinition, NgModuleDefinitionFactory, NgModuleProviderDef, ProviderOverride, Services, ViewDefinition} from './types';
 import {resolveDefinition} from './util';
 
 export function overrideProvider(override: ProviderOverride) {
@@ -38,6 +38,20 @@ export function createNgModuleFactory(
   return new NgModuleFactory_(ngModuleType, bootstrapComponents, defFactory);
 }
 
+function cloneNgModuleDefinition(def: NgModuleDefinition): NgModuleDefinition {
+  const providers = Array.from(def.providers);
+  const modules = Array.from(def.modules);
+  const providersByKey: {[tokenKey: string]: NgModuleProviderDef} = {};
+  for (const key in def.providersByKey) {
+    providersByKey[key] = def.providersByKey[key];
+  }
+
+  return {
+    factory: def.factory,
+    isRoot: def.isRoot, providers, modules, providersByKey,
+  };
+}
+
 class NgModuleFactory_ extends NgModuleFactory<any> {
   constructor(
       public readonly moduleType: Type<any>, private _bootstrapComponents: Type<any>[],
@@ -49,7 +63,10 @@ class NgModuleFactory_ extends NgModuleFactory<any> {
 
   create(parentInjector: Injector|null): NgModuleRef<any> {
     initServicesIfNeeded();
-    const def = resolveDefinition(this._ngModuleDefFactory);
+    // Clone the NgModuleDefinition so that any tree shakeable provider definition
+    // added to this instance of the NgModuleRef doesn't affect the cached copy.
+    // See https://github.com/angular/angular/issues/25018.
+    const def = cloneNgModuleDefinition(resolveDefinition(this._ngModuleDefFactory));
     return Services.createNgModuleRef(
         this.moduleType, parentInjector || Injector.NULL, this._bootstrapComponents, def);
   }

packages/core/test/bundling/hello_world_r2/bundle.golden_symbols.json

@@ -2414,6 +2414,9 @@
   {
     "name": "checkStable"
   },
+  {
+    "name": "cloneNgModuleDefinition"
+  },
   {
     "name": "collectReferences"
   },

packages/core/test/linker/ng_module_integration_spec.ts

@@ -8,10 +8,13 @@
 
 import {ANALYZE_FOR_ENTRY_COMPONENTS, CUSTOM_ELEMENTS_SCHEMA, Compiler, Component, ComponentFactoryResolver, Directive, HostBinding, Inject, Injectable, InjectionToken, Injector, Input, NgModule, NgModuleRef, Optional, Pipe, Provider, Self, Type, forwardRef, getModuleFactory} from '@angular/core';
 import {Console} from '@angular/core/src/console';
+import {InjectableDef, defineInjectable} from '@angular/core/src/di/defs';
+import {NgModuleData} from '@angular/core/src/view/types';
+import {tokenKey} from '@angular/core/src/view/util';
 import {ComponentFixture, TestBed, inject} from '@angular/core/testing';
 import {expect} from '@angular/platform-browser/testing/src/matchers';
 
-import {InternalNgModuleRef} from '../../src/linker/ng_module_factory';
+import {InternalNgModuleRef, NgModuleFactory} from '../../src/linker/ng_module_factory';
 import {clearModulesForTest} from '../../src/linker/ng_module_factory_loader';
 import {stringify} from '../../src/util';
 
@@ -117,9 +120,13 @@ function declareTests({useJit}: {useJit: boolean}) {
       injector = _injector;
     }));
 
+    function createModuleFactory<T>(moduleType: Type<T>): NgModuleFactory<T> {
+      return compiler.compileModuleSync(moduleType);
+    }
+
     function createModule<T>(
         moduleType: Type<T>, parentInjector?: Injector | null): NgModuleRef<T> {
-      return compiler.compileModuleSync(moduleType).create(parentInjector || null);
+      return createModuleFactory(moduleType).create(parentInjector || null);
     }
 
     function createComp<T>(compType: Type<T>, moduleType: Type<any>): ComponentFixture<T> {
@@ -1290,6 +1297,37 @@ function declareTests({useJit}: {useJit: boolean}) {
                   `Invalid provider for the NgModule 'ImportedModule1' - only instances of Provider and Type are allowed, got: [?broken?]`);
         });
       });
+
+      describe('tree shakable providers', () => {
+        it('definition should not persist across NgModuleRef instances', () => {
+          @NgModule()
+          class SomeModule {
+          }
+
+          class Bar {
+            static ngInjectableDef: InjectableDef<Bar> = defineInjectable({
+              factory: () => new Bar(),
+              providedIn: SomeModule,
+            });
+          }
+
+          const factory = createModuleFactory(SomeModule);
+          const ngModuleRef1 = factory.create(null);
+
+          // Inject a tree shakeable provider token.
+          ngModuleRef1.injector.get(Bar);
+
+          // Tree Shakeable provider definition should get added to the NgModule data.
+          const providerDef1 = (ngModuleRef1 as NgModuleData)._def.providersByKey[tokenKey(Bar)];
+          expect(providerDef1).not.toBeUndefined();
+
+          // Instantiate the same module. The tree shakeable provider
+          // definition should not already be present.
+          const ngModuleRef2 = factory.create(null);
+          const providerDef2 = (ngModuleRef2 as NgModuleData)._def.providersByKey[tokenKey(Bar)];
+          expect(providerDef2).toBeUndefined();
+        });
+      });
     });
   });
 }