fix(core): remove application from the testability registry when the root view is removed

goldens/size-tracking/integration-payloads.json

@@ -39,7 +39,7 @@
     "master": {
       "uncompressed": {
         "runtime-es2015": 2285,
-        "main-es2015": 241879,
+        "main-es2015": 242455,
         "polyfills-es2015": 36709,
         "5-es2015": 745
       }

packages/core/src/application_ref.ts

@@ -594,6 +594,7 @@ export class ApplicationRef {
   private _runningTick: boolean = false;
   private _enforceNoNewChanges: boolean = false;
   private _stable = true;
+  private _onMicrotaskEmptySubscription: Subscription;
 
   /**
    * Get a list of component types registered to this application.
@@ -622,7 +623,7 @@ export class ApplicationRef {
       private _initStatus: ApplicationInitStatus) {
     this._enforceNoNewChanges = isDevMode();
 
-    this._zone.onMicrotaskEmpty.subscribe({
+    this._onMicrotaskEmptySubscription = this._zone.onMicrotaskEmpty.subscribe({
       next: () => {
         this._zone.run(() => {
           this.tick();
@@ -715,15 +716,20 @@ export class ApplicationRef {
         isBoundToModule(componentFactory) ? undefined : this._injector.get(NgModuleRef);
     const selectorOrNode = rootSelectorOrNode || componentFactory.selector;
     const compRef = componentFactory.create(Injector.NULL, [], selectorOrNode, ngModule);
+    const nativeElement = compRef.location.nativeElement;
+    const testability = compRef.injector.get(Testability, null);
+    const testabilityRegistry = testability && compRef.injector.get(TestabilityRegistry);
+    if (testability && testabilityRegistry) {
+      testabilityRegistry.registerApplication(nativeElement, testability);
+    }
 
     compRef.onDestroy(() => {
-      this._unloadComponent(compRef);
+      this.detachView(compRef.hostView);
+      remove(this.components, compRef);
+      if (testabilityRegistry) {
+        testabilityRegistry.unregisterApplication(nativeElement);
+      }
     });
-    const testability = compRef.injector.get(Testability, null);
-    if (testability) {
-      compRef.injector.get(TestabilityRegistry)
-          .registerApplication(compRef.location.nativeElement, testability);
-    }
 
     this._loadComponent(compRef);
     if (isDevMode()) {
@@ -796,15 +802,10 @@ export class ApplicationRef {
     listeners.forEach((listener) => listener(componentRef));
   }
 
-  private _unloadComponent(componentRef: ComponentRef<any>): void {
-    this.detachView(componentRef.hostView);
-    remove(this.components, componentRef);
-  }
-
   /** @internal */
   ngOnDestroy() {
-    // TODO(alxhub): Dispose of the NgZone.
     this._views.slice().forEach((view) => view.destroy());
+    this._onMicrotaskEmptySubscription.unsubscribe();
   }
 
   /**

packages/core/src/render3/component_ref.ts

@@ -248,7 +248,6 @@ export function injectComponentFactoryResolver(): viewEngine_ComponentFactoryRes
  *
  */
 export class ComponentRef<T> extends viewEngine_ComponentRef<T> {
-  destroyCbs: (() => void)[]|null = [];
   instance: T;
   hostView: ViewRef<T>;
   changeDetectorRef: ViewEngine_ChangeDetectorRef;
@@ -269,16 +268,10 @@ export class ComponentRef<T> extends viewEngine_ComponentRef<T> {
   }
 
   destroy(): void {
-    if (this.destroyCbs) {
-      this.destroyCbs.forEach(fn => fn());
-      this.destroyCbs = null;
-      !this.hostView.destroyed && this.hostView.destroy();
-    }
+    this.hostView.destroy();
   }
 
   onDestroy(callback: () => void): void {
-    if (this.destroyCbs) {
-      this.destroyCbs.push(callback);
-    }
+    this.hostView.onDestroy(callback);
   }
 }

packages/core/src/render3/instructions/listener.ts

@@ -19,7 +19,7 @@ import {assertTNodeType} from '../node_assert';
 import {getCurrentDirectiveDef, getCurrentTNode, getLView, getTView} from '../state';
 import {getComponentLViewByIndex, getNativeByTNode, unwrapRNode} from '../util/view_utils';
 
-import {getLCleanup, handleError, loadComponentRenderer, markViewDirty} from './shared';
+import {getLCleanup, getTViewCleanup, handleError, loadComponentRenderer, markViewDirty} from './shared';
 
 
 
@@ -120,7 +120,7 @@ function listenerInternal(
     eventTargetResolver?: GlobalTargetResolver): void {
   const isTNodeDirectiveHost = isDirectiveHost(tNode);
   const firstCreatePass = tView.firstCreatePass;
-  const tCleanup: false|any[] = firstCreatePass && (tView.cleanup || (tView.cleanup = []));
+  const tCleanup: false|any[] = firstCreatePass && getTViewCleanup(tView);
 
   // When the ɵɵlistener instruction was generated and is executed we know that there is either a
   // native listener or a directive output on this element. As such we we know that we will have to

packages/core/src/render3/instructions/shared.ts

@@ -752,14 +752,26 @@ export function locateHostElement(
  * On the first template pass, saves in TView:
  * - Cleanup function
  * - Index of context we just saved in LView.cleanupInstances
+ *
+ * This function can also be used to store instance specific cleanup fns. In that case the `context`
+ * is `null` and the function is store in `LView` (rather than it `TView`).
  */
 export function storeCleanupWithContext(
     tView: TView, lView: LView, context: any, cleanupFn: Function): void {
   const lCleanup = getLCleanup(lView);
-  lCleanup.push(context);
+  if (context === null) {
+    // If context is null that this is instance specific callback. These callbacks can only be
+    // inserted after template shared instances. For this reason in ngDevMode we freeze the TView.
+    if (ngDevMode) {
+      Object.freeze(getTViewCleanup(tView));
+    }
+    lCleanup.push(cleanupFn);
+  } else {
+    lCleanup.push(context);
 
-  if (tView.firstCreatePass) {
-    getTViewCleanup(tView).push(cleanupFn, lCleanup.length - 1);
+    if (tView.firstCreatePass) {
+      getTViewCleanup(tView).push(cleanupFn, lCleanup.length - 1);
+    }
   }
 }
 
@@ -1997,7 +2009,7 @@ export function getLCleanup(view: LView): any[] {
   return view[CLEANUP] || (view[CLEANUP] = ngDevMode ? new LCleanup() : []);
 }
 
-function getTViewCleanup(tView: TView): any[] {
+export function getTViewCleanup(tView: TView): any[] {
   return tView.cleanup || (tView.cleanup = ngDevMode ? new TCleanup() : []);
 }
 

packages/core/src/render3/interfaces/view.ts

@@ -163,8 +163,11 @@ export interface LView extends Array<any> {
    *
    * These change per LView instance, so they cannot be stored on TView. Instead,
    * TView.cleanup saves an index to the necessary context in this array.
+   *
+   * After `LView` is created it is possible to attach additional instance specific functions at the
+   * end of the `lView[CLENUP]` because we know that no more `T` level cleanup functions will be
+   * addeded here.
    */
-  // TODO: flatten into LView[]
   [CLEANUP]: any[]|null;
 
   /**

packages/core/src/render3/node_manipulation.ts

@@ -10,7 +10,7 @@ import {ViewEncapsulation} from '../metadata/view';
 import {Renderer2} from '../render/api';
 import {RendererStyleFlags2} from '../render/api_flags';
 import {addToArray, removeFromArray} from '../util/array_utils';
-import {assertDefined, assertDomNode, assertEqual, assertString} from '../util/assert';
+import {assertDefined, assertDomNode, assertEqual, assertFunction, assertString} from '../util/assert';
 import {assertLContainer, assertLView, assertTNodeForLView} from './assert';
 import {attachPatchData} from './context_discovery';
 import {icuContainerIterate} from './i18n/i18n_tree_shaking';
@@ -418,7 +418,7 @@ function cleanUpView(tView: TView, lView: LView): void {
     lView[FLAGS] |= LViewFlags.Destroyed;
 
     executeOnDestroys(tView, lView);
-    removeListeners(tView, lView);
+    processCleanups(tView, lView);
     // For component views only, the local renderer is destroyed at clean up time.
     if (lView[TVIEW].type === TViewType.Component && isProceduralRenderer(lView[RENDERER])) {
       ngDevMode && ngDevMode.rendererDestroy++;
@@ -443,38 +443,49 @@ function cleanUpView(tView: TView, lView: LView): void {
 }
 
 /** Removes listeners and unsubscribes from output subscriptions */
-function removeListeners(tView: TView, lView: LView): void {
+function processCleanups(tView: TView, lView: LView): void {
   const tCleanup = tView.cleanup;
+  const lCleanup = lView[CLEANUP]!;
+  // `LCleanup` contains both share information with `TCleanup` as well as instance specific
+  // information appended at the end. We need to know where the end of the `TCleanup` information
+  // is, and we track this with `lastLCleanupIndex`.
+  let lastLCleanupIndex = -1;
   if (tCleanup !== null) {
-    const lCleanup = lView[CLEANUP]!;
     for (let i = 0; i < tCleanup.length - 1; i += 2) {
       if (typeof tCleanup[i] === 'string') {
         // This is a native DOM listener
         const idxOrTargetGetter = tCleanup[i + 1];
         const target = typeof idxOrTargetGetter === 'function' ?
             idxOrTargetGetter(lView) :
             unwrapRNode(lView[idxOrTargetGetter]);
-        const listener = lCleanup[tCleanup[i + 2]];
+        const listener = lCleanup[lastLCleanupIndex = tCleanup[i + 2]];
         const useCaptureOrSubIdx = tCleanup[i + 3];
         if (typeof useCaptureOrSubIdx === 'boolean') {
           // native DOM listener registered with Renderer3
           target.removeEventListener(tCleanup[i], listener, useCaptureOrSubIdx);
         } else {
           if (useCaptureOrSubIdx >= 0) {
             // unregister
-            lCleanup[useCaptureOrSubIdx]();
+            lCleanup[lastLCleanupIndex = useCaptureOrSubIdx]();
           } else {
             // Subscription
-            lCleanup[-useCaptureOrSubIdx].unsubscribe();
+            lCleanup[lastLCleanupIndex = -useCaptureOrSubIdx].unsubscribe();
           }
         }
         i += 2;
       } else {
         // This is a cleanup function that is grouped with the index of its context
-        const context = lCleanup[tCleanup[i + 1]];
+        const context = lCleanup[lastLCleanupIndex = tCleanup[i + 1]];
         tCleanup[i].call(context);
       }
     }
+    if (lCleanup !== null) {
+      for (let i = lastLCleanupIndex + 1; i < lCleanup.length; i++) {
+        const instanceCleanupFn = lCleanup[i];
+        ngDevMode && assertFunction(instanceCleanupFn, 'Expecting instance cleanup function.');
+        instanceCleanupFn();
+      }
+    }
     lView[CLEANUP] = null;
   }
 }

packages/core/src/util/assert.ts

@@ -31,6 +31,12 @@ export function assertString(actual: any, msg: string): asserts actual is string
   }
 }
 
+export function assertFunction(actual: any, msg: string): asserts actual is Function {
+  if (!(typeof actual === 'function')) {
+    throwError(msg, actual === null ? 'null' : typeof actual, 'function', '===');
+  }
+}
+
 export function assertEqual<T>(actual: T, expected: T, msg: string) {
   if (!(actual == expected)) {
     throwError(msg, actual, expected, '==');

packages/core/test/acceptance/bootstrap_spec.ts

@@ -6,7 +6,8 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {COMPILER_OPTIONS, Component, destroyPlatform, NgModule, ViewEncapsulation} from '@angular/core';
+import {ApplicationRef, COMPILER_OPTIONS, Component, destroyPlatform, NgModule, TestabilityRegistry, ViewEncapsulation} from '@angular/core';
+import {expect} from '@angular/core/testing/src/testing_internal';
 import {BrowserModule} from '@angular/platform-browser';
 import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
 import {onlyInIvy, withBody} from '@angular/private/testing';
@@ -151,6 +152,81 @@ describe('bootstrap', () => {
          ngModuleRef.destroy();
        }));
 
+    describe('ApplicationRef cleanup', () => {
+      it('should cleanup ApplicationRef when Injector is destroyed',
+         withBody('<my-app></my-app>', async () => {
+           const TestModule = createComponentAndModule();
+
+           const ngModuleRef = await platformBrowserDynamic().bootstrapModule(TestModule);
+           const appRef = ngModuleRef.injector.get(ApplicationRef);
+           const testabilityRegistry = ngModuleRef.injector.get(TestabilityRegistry);
+
+           expect(appRef.components.length).toBe(1);
+           expect(testabilityRegistry.getAllRootElements().length).toBe(1);
+
+           ngModuleRef.destroy();  // also destroys an Injector instance.
+
+           expect(appRef.components.length).toBe(0);
+           expect(testabilityRegistry.getAllRootElements().length).toBe(0);
+         }));
+
+      it('should cleanup ApplicationRef when ComponentRef is destroyed',
+         withBody('<my-app></my-app>', async () => {
+           const TestModule = createComponentAndModule();
+
+           const ngModuleRef = await platformBrowserDynamic().bootstrapModule(TestModule);
+           const appRef = ngModuleRef.injector.get(ApplicationRef);
+           const testabilityRegistry = ngModuleRef.injector.get(TestabilityRegistry);
+           const componentRef = appRef.components[0];
+
+           expect(appRef.components.length).toBe(1);
+           expect(testabilityRegistry.getAllRootElements().length).toBe(1);
+
+           componentRef.destroy();
+
+           expect(appRef.components.length).toBe(0);
+           expect(testabilityRegistry.getAllRootElements().length).toBe(0);
+         }));
+
+      it('should not throw in case ComponentRef is destroyed and Injector is destroyed after that',
+         withBody('<my-app></my-app>', async () => {
+           const TestModule = createComponentAndModule();
+
+           const ngModuleRef = await platformBrowserDynamic().bootstrapModule(TestModule);
+           const appRef = ngModuleRef.injector.get(ApplicationRef);
+           const testabilityRegistry = ngModuleRef.injector.get(TestabilityRegistry);
+           const componentRef = appRef.components[0];
+
+           expect(appRef.components.length).toBe(1);
+           expect(testabilityRegistry.getAllRootElements().length).toBe(1);
+
+           componentRef.destroy();
+           ngModuleRef.destroy();  // also destroys an Injector instance.
+
+           expect(appRef.components.length).toBe(0);
+           expect(testabilityRegistry.getAllRootElements().length).toBe(0);
+         }));
+
+      it('should not throw in case Injector is destroyed and ComponentRef is destroyed after that',
+         withBody('<my-app></my-app>', async () => {
+           const TestModule = createComponentAndModule();
+
+           const ngModuleRef = await platformBrowserDynamic().bootstrapModule(TestModule);
+           const appRef = ngModuleRef.injector.get(ApplicationRef);
+           const testabilityRegistry = ngModuleRef.injector.get(TestabilityRegistry);
+           const componentRef = appRef.components[0];
+
+           expect(appRef.components.length).toBe(1);
+           expect(testabilityRegistry.getAllRootElements().length).toBe(1);
+
+           ngModuleRef.destroy();  // also destroys an Injector instance.
+           componentRef.destroy();
+
+           expect(appRef.components.length).toBe(0);
+           expect(testabilityRegistry.getAllRootElements().length).toBe(0);
+         }));
+    });
+
     onlyInIvy('options cannot be changed in Ivy').describe('changing bootstrap options', () => {
       beforeEach(() => {
         spyOn(console, 'error');