From 5d6052567374c98404be8c582224a359539b82a3 Mon Sep 17 00:00:00 2001 From: arturovt Date: Sun, 29 Nov 2020 02:01:03 +0200 Subject: [PATCH 1/2] fix(core): remove application from the testability registry when the root view is removed In the new behavior Angular removes applications from the testability registry when the root view gets destroyed. This eliminates a memory leak, because before that the TestabilityRegistry holds references to HTML elements, thus they cannot be GCed. PR Close #22106 --- .../size-tracking/integration-payloads.json | 2 +- packages/core/src/application_ref.ts | 26 +++++++++--------- packages/core/src/render3/component_ref.ts | 11 ++------ .../core/src/render3/instructions/listener.ts | 4 +-- .../core/src/render3/instructions/shared.ts | 20 +++++++++++--- packages/core/src/render3/interfaces/view.ts | 5 +++- .../core/src/render3/node_manipulation.ts | 27 +++++++++++++------ packages/core/src/util/assert.ts | 6 +++++ .../router/bundle.golden_symbols.json | 3 +++ 9 files changed, 67 insertions(+), 37 deletions(-) diff --git a/goldens/size-tracking/integration-payloads.json b/goldens/size-tracking/integration-payloads.json index 347229d4b22df..38f8eedc642c7 100644 --- a/goldens/size-tracking/integration-payloads.json +++ b/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 } diff --git a/packages/core/src/application_ref.ts b/packages/core/src/application_ref.ts index cdff12ceef6b8..d7f024bbb2c4b 100644 --- a/packages/core/src/application_ref.ts +++ b/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,11 @@ export class ApplicationRef { listeners.forEach((listener) => listener(componentRef)); } - private _unloadComponent(componentRef: ComponentRef): 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(); } /** diff --git a/packages/core/src/render3/component_ref.ts b/packages/core/src/render3/component_ref.ts index a5d06798990bf..0c4ebfb686195 100644 --- a/packages/core/src/render3/component_ref.ts +++ b/packages/core/src/render3/component_ref.ts @@ -248,7 +248,6 @@ export function injectComponentFactoryResolver(): viewEngine_ComponentFactoryRes * */ export class ComponentRef extends viewEngine_ComponentRef { - destroyCbs: (() => void)[]|null = []; instance: T; hostView: ViewRef; changeDetectorRef: ViewEngine_ChangeDetectorRef; @@ -269,16 +268,10 @@ export class ComponentRef extends viewEngine_ComponentRef { } 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); } } diff --git a/packages/core/src/render3/instructions/listener.ts b/packages/core/src/render3/instructions/listener.ts index 50616cc96244b..046787e22ac19 100644 --- a/packages/core/src/render3/instructions/listener.ts +++ b/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 diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 17a233ed7dc19..c0b11b180a1e5 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/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() : []); } diff --git a/packages/core/src/render3/interfaces/view.ts b/packages/core/src/render3/interfaces/view.ts index 18749018cd4c5..5c5bc78cf2f25 100644 --- a/packages/core/src/render3/interfaces/view.ts +++ b/packages/core/src/render3/interfaces/view.ts @@ -163,8 +163,11 @@ export interface LView extends Array { * * 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; /** diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index c8f572b280da5..8a8cdbe1bae7c 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/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,10 +443,14 @@ 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 @@ -454,7 +458,7 @@ function removeListeners(tView: TView, lView: LView): void { 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 @@ -462,19 +466,26 @@ function removeListeners(tView: TView, lView: LView): void { } 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; } } diff --git a/packages/core/src/util/assert.ts b/packages/core/src/util/assert.ts index 5fdd5bf0b55e4..bf1092a99bd29 100644 --- a/packages/core/src/util/assert.ts +++ b/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(actual: T, expected: T, msg: string) { if (!(actual == expected)) { throwError(msg, actual, expected, '=='); diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index 795e037553550..036c745dba769 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -1436,6 +1436,9 @@ { "name": "getTView" }, + { + "name": "getTViewCleanup" + }, { "name": "getToken" }, From 66e8d75af731ab026c0801b491fc6c7872f56b85 Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Tue, 1 Dec 2020 15:24:04 -0800 Subject: [PATCH 2/2] test(core): verify `onDestroy` callbacks are invoked when ComponentRef is destroyed This commit adds a few tests to verify that the `onDestroy` callbacks are invoked when `ComponentRef` instance is destroyed and the logic is consistent between ViewEngine and Ivy. --- packages/core/src/application_ref.ts | 1 - .../core/test/acceptance/bootstrap_spec.ts | 78 ++++++++++++++++++- .../core/test/acceptance/component_spec.ts | 51 ++++++++++++ 3 files changed, 128 insertions(+), 2 deletions(-) diff --git a/packages/core/src/application_ref.ts b/packages/core/src/application_ref.ts index d7f024bbb2c4b..ace2cec25bc80 100644 --- a/packages/core/src/application_ref.ts +++ b/packages/core/src/application_ref.ts @@ -804,7 +804,6 @@ export class ApplicationRef { /** @internal */ ngOnDestroy() { - // TODO(alxhub): Dispose of the NgZone. this._views.slice().forEach((view) => view.destroy()); this._onMicrotaskEmptySubscription.unsubscribe(); } diff --git a/packages/core/test/acceptance/bootstrap_spec.ts b/packages/core/test/acceptance/bootstrap_spec.ts index 7024c3b6c5693..020470d1a8d83 100644 --- a/packages/core/test/acceptance/bootstrap_spec.ts +++ b/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('', 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('', 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('', 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('', 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'); diff --git a/packages/core/test/acceptance/component_spec.ts b/packages/core/test/acceptance/component_spec.ts index 4023e77118f1d..463759577107d 100644 --- a/packages/core/test/acceptance/component_spec.ts +++ b/packages/core/test/acceptance/component_spec.ts @@ -303,6 +303,57 @@ describe('component', () => { expect(wrapperEls.length).toBe(2); // other elements are preserved }); + it('should invoke `onDestroy` callbacks of dynamically created component', () => { + let wasOnDestroyCalled = false; + @Component({ + selector: '[comp]', + template: 'comp content', + }) + class DynamicComponent { + } + + @NgModule({ + declarations: [DynamicComponent], + entryComponents: [DynamicComponent], // needed only for ViewEngine + }) + class TestModule { + } + + @Component({ + selector: 'button', + template: '
', + }) + class App { + @ViewChild('anchor', {read: ViewContainerRef}) anchor!: ViewContainerRef; + + constructor(private cfr: ComponentFactoryResolver, private injector: Injector) {} + + create() { + const factory = this.cfr.resolveComponentFactory(DynamicComponent); + const componentRef = factory.create(this.injector); + componentRef.onDestroy(() => { + wasOnDestroyCalled = true; + }); + this.anchor.insert(componentRef.hostView); + } + + clear() { + this.anchor.clear(); + } + } + + TestBed.configureTestingModule({imports: [TestModule], declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + // Add ComponentRef to ViewContainerRef instance. + fixture.componentInstance.create(); + // Clear ViewContainerRef to invoke `onDestroy` callbacks on ComponentRef. + fixture.componentInstance.clear(); + + expect(wasOnDestroyCalled).toBeTrue(); + }); + describe('invalid host element', () => { it('should throw when is used as a host element for a Component', () => { @Component({