From df27027ecb0ac96d503eb97a02f606023f62f60a Mon Sep 17 00:00:00 2001 From: arturovt Date: Sun, 29 Nov 2020 02:01:03 +0200 Subject: [PATCH] fix(core): remove application from the testability registry when the root view is removed (#39876) 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 PR Close #39876 --- .../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" },