Skip to content

Commit 3680ad1

Browse files
arturovtmhevery
arturovt
authored and
mhevery
committed
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
1 parent f7e767f commit 3680ad1

File tree

9 files changed

+67
-37
lines changed

9 files changed

+67
-37
lines changed

‎goldens/size-tracking/integration-payloads.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@
3939
"master": {
4040
"uncompressed": {
4141
"runtime-es2015": 2285,
42-
"main-es2015": 241875,
42+
"main-es2015": 242075,
4343
"polyfills-es2015": 36709,
4444
"5-es2015": 745
4545
}

‎packages/core/src/application_ref.ts‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -570,6 +570,7 @@ export class ApplicationRef {
570570
private _runningTick: boolean = false;
571571
private _enforceNoNewChanges: boolean = false;
572572
private _stable = true;
573+
private _onMicrotaskEmptySubscription: Subscription;
573574

574575
/**
575576
* Get a list of component types registered to this application.
@@ -598,7 +599,7 @@ export class ApplicationRef {
598599
private _initStatus: ApplicationInitStatus) {
599600
this._enforceNoNewChanges = isDevMode();
600601

601-
this._zone.onMicrotaskEmpty.subscribe({
602+
this._onMicrotaskEmptySubscription = this._zone.onMicrotaskEmpty.subscribe({
602603
next: () => {
603604
this._zone.run(() => {
604605
this.tick();
@@ -691,15 +692,20 @@ export class ApplicationRef {
691692
isBoundToModule(componentFactory) ? undefined : this._injector.get(NgModuleRef);
692693
const selectorOrNode = rootSelectorOrNode || componentFactory.selector;
693694
const compRef = componentFactory.create(Injector.NULL, [], selectorOrNode, ngModule);
695+
const nativeElement = compRef.location.nativeElement;
696+
const testability = compRef.injector.get(Testability, null);
697+
const testabilityRegistry = testability && compRef.injector.get(TestabilityRegistry);
698+
if (testability && testabilityRegistry) {
699+
testabilityRegistry.registerApplication(nativeElement, testability);
700+
}
694701

695702
compRef.onDestroy(() => {
696-
this._unloadComponent(compRef);
703+
this.detachView(compRef.hostView);
704+
remove(this.components, compRef);
705+
if (testabilityRegistry) {
706+
testabilityRegistry.unregisterApplication(nativeElement);
707+
}
697708
});
698-
const testability = compRef.injector.get(Testability, null);
699-
if (testability) {
700-
compRef.injector.get(TestabilityRegistry)
701-
.registerApplication(compRef.location.nativeElement, testability);
702-
}
703709

704710
this._loadComponent(compRef);
705711
if (isDevMode()) {
@@ -772,14 +778,10 @@ export class ApplicationRef {
772778
listeners.forEach((listener) => listener(componentRef));
773779
}
774780

775-
private _unloadComponent(componentRef: ComponentRef<any>): void {
776-
this.detachView(componentRef.hostView);
777-
remove(this.components, componentRef);
778-
}
779-
780781
/** @internal */
781782
ngOnDestroy() {
782783
this._views.slice().forEach((view) => view.destroy());
784+
this._onMicrotaskEmptySubscription.unsubscribe();
783785
}
784786

785787
/**

‎packages/core/src/render3/component_ref.ts‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,6 @@ export function injectComponentFactoryResolver(): viewEngine_ComponentFactoryRes
246246
*
247247
*/
248248
export class ComponentRef<T> extends viewEngine_ComponentRef<T> {
249-
destroyCbs: (() => void)[]|null = [];
250249
instance: T;
251250
hostView: ViewRef<T>;
252251
changeDetectorRef: ViewEngine_ChangeDetectorRef;
@@ -267,16 +266,10 @@ export class ComponentRef<T> extends viewEngine_ComponentRef<T> {
267266
}
268267

269268
destroy(): void {
270-
if (this.destroyCbs) {
271-
this.destroyCbs.forEach(fn => fn());
272-
this.destroyCbs = null;
273-
!this.hostView.destroyed && this.hostView.destroy();
274-
}
269+
this.hostView.destroy();
275270
}
276271

277272
onDestroy(callback: () => void): void {
278-
if (this.destroyCbs) {
279-
this.destroyCbs.push(callback);
280-
}
273+
this.hostView.onDestroy(callback);
281274
}
282275
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import {assertTNodeType} from '../node_assert';
1919
import {getCurrentDirectiveDef, getCurrentTNode, getLView, getTView} from '../state';
2020
import {getComponentLViewByIndex, getNativeByTNode, unwrapRNode} from '../util/view_utils';
2121

22-
import {getLCleanup, handleError, loadComponentRenderer, markViewDirty} from './shared';
22+
import {getLCleanup, getTViewCleanup, handleError, loadComponentRenderer, markViewDirty} from './shared';
2323

2424

2525

@@ -120,7 +120,7 @@ function listenerInternal(
120120
eventTargetResolver?: GlobalTargetResolver): void {
121121
const isTNodeDirectiveHost = isDirectiveHost(tNode);
122122
const firstCreatePass = tView.firstCreatePass;
123-
const tCleanup: false|any[] = firstCreatePass && (tView.cleanup || (tView.cleanup = []));
123+
const tCleanup: false|any[] = firstCreatePass && getTViewCleanup(tView);
124124

125125
// When the ɵɵlistener instruction was generated and is executed we know that there is either a
126126
// 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‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -752,14 +752,26 @@ export function locateHostElement(
752752
* On the first template pass, saves in TView:
753753
* - Cleanup function
754754
* - Index of context we just saved in LView.cleanupInstances
755+
*
756+
* This function can also be used to store instance specific cleanup fns. In that case the `context`
757+
* is `null` and the function is store in `LView` (rather than it `TView`).
755758
*/
756759
export function storeCleanupWithContext(
757760
tView: TView, lView: LView, context: any, cleanupFn: Function): void {
758761
const lCleanup = getLCleanup(lView);
759-
lCleanup.push(context);
762+
if (context === null) {
763+
// If context is null that this is instance specific callback. These callbacks can only be
764+
// inserted after template shared instances. For this reason in ngDevMode we freeze the TView.
765+
if (ngDevMode) {
766+
Object.freeze(getTViewCleanup(tView));
767+
}
768+
lCleanup.push(cleanupFn);
769+
} else {
770+
lCleanup.push(context);
760771

761-
if (tView.firstCreatePass) {
762-
getTViewCleanup(tView).push(cleanupFn, lCleanup.length - 1);
772+
if (tView.firstCreatePass) {
773+
getTViewCleanup(tView).push(cleanupFn, lCleanup.length - 1);
774+
}
763775
}
764776
}
765777

@@ -1997,7 +2009,7 @@ export function getLCleanup(view: LView): any[] {
19972009
return view[CLEANUP] || (view[CLEANUP] = ngDevMode ? new LCleanup() : []);
19982010
}
19992011

2000-
function getTViewCleanup(tView: TView): any[] {
2012+
export function getTViewCleanup(tView: TView): any[] {
20012013
return tView.cleanup || (tView.cleanup = ngDevMode ? new TCleanup() : []);
20022014
}
20032015

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,8 +163,11 @@ export interface LView extends Array<any> {
163163
*
164164
* These change per LView instance, so they cannot be stored on TView. Instead,
165165
* TView.cleanup saves an index to the necessary context in this array.
166+
*
167+
* After `LView` is created it is possible to attach additional instance specific functions at the
168+
* end of the `lView[CLENUP]` because we know that no more `T` level cleanup functions will be
169+
* addeded here.
166170
*/
167-
// TODO: flatten into LView[]
168171
[CLEANUP]: any[]|null;
169172

170173
/**

‎packages/core/src/render3/node_manipulation.ts‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {ViewEncapsulation} from '../metadata/view';
1010
import {Renderer2} from '../render/api';
1111
import {RendererStyleFlags2} from '../render/api_flags';
1212
import {addToArray, removeFromArray} from '../util/array_utils';
13-
import {assertDefined, assertDomNode, assertEqual, assertString} from '../util/assert';
13+
import {assertDefined, assertDomNode, assertEqual, assertFunction, assertString} from '../util/assert';
1414
import {assertLContainer, assertLView, assertTNodeForLView} from './assert';
1515
import {attachPatchData} from './context_discovery';
1616
import {icuContainerIterate} from './i18n/i18n_tree_shaking';
@@ -418,7 +418,7 @@ function cleanUpView(tView: TView, lView: LView): void {
418418
lView[FLAGS] |= LViewFlags.Destroyed;
419419

420420
executeOnDestroys(tView, lView);
421-
removeListeners(tView, lView);
421+
processCleanups(tView, lView);
422422
// For component views only, the local renderer is destroyed at clean up time.
423423
if (lView[TVIEW].type === TViewType.Component && isProceduralRenderer(lView[RENDERER])) {
424424
ngDevMode && ngDevMode.rendererDestroy++;
@@ -443,38 +443,49 @@ function cleanUpView(tView: TView, lView: LView): void {
443443
}
444444

445445
/** Removes listeners and unsubscribes from output subscriptions */
446-
function removeListeners(tView: TView, lView: LView): void {
446+
function processCleanups(tView: TView, lView: LView): void {
447447
const tCleanup = tView.cleanup;
448+
const lCleanup = lView[CLEANUP]!;
449+
// `LCleanup` contains both share information with `TCleanup` as well as instance specific
450+
// information appended at the end. We need to know where the end of the `TCleanup` information
451+
// is, and we track this with `lastLCleanupIndex`.
452+
let lastLCleanupIndex = -1;
448453
if (tCleanup !== null) {
449-
const lCleanup = lView[CLEANUP]!;
450454
for (let i = 0; i < tCleanup.length - 1; i += 2) {
451455
if (typeof tCleanup[i] === 'string') {
452456
// This is a native DOM listener
453457
const idxOrTargetGetter = tCleanup[i + 1];
454458
const target = typeof idxOrTargetGetter === 'function' ?
455459
idxOrTargetGetter(lView) :
456460
unwrapRNode(lView[idxOrTargetGetter]);
457-
const listener = lCleanup[tCleanup[i + 2]];
461+
const listener = lCleanup[lastLCleanupIndex = tCleanup[i + 2]];
458462
const useCaptureOrSubIdx = tCleanup[i + 3];
459463
if (typeof useCaptureOrSubIdx === 'boolean') {
460464
// native DOM listener registered with Renderer3
461465
target.removeEventListener(tCleanup[i], listener, useCaptureOrSubIdx);
462466
} else {
463467
if (useCaptureOrSubIdx >= 0) {
464468
// unregister
465-
lCleanup[useCaptureOrSubIdx]();
469+
lCleanup[lastLCleanupIndex = useCaptureOrSubIdx]();
466470
} else {
467471
// Subscription
468-
lCleanup[-useCaptureOrSubIdx].unsubscribe();
472+
lCleanup[lastLCleanupIndex = -useCaptureOrSubIdx].unsubscribe();
469473
}
470474
}
471475
i += 2;
472476
} else {
473477
// This is a cleanup function that is grouped with the index of its context
474-
const context = lCleanup[tCleanup[i + 1]];
478+
const context = lCleanup[lastLCleanupIndex = tCleanup[i + 1]];
475479
tCleanup[i].call(context);
476480
}
477481
}
482+
if (lCleanup !== null) {
483+
for (let i = lastLCleanupIndex + 1; i < lCleanup.length; i++) {
484+
const instanceCleanupFn = lCleanup[i];
485+
ngDevMode && assertFunction(instanceCleanupFn, 'Expecting instance cleanup function.');
486+
instanceCleanupFn();
487+
}
488+
}
478489
lView[CLEANUP] = null;
479490
}
480491
}

‎packages/core/src/util/assert.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,12 @@ export function assertString(actual: any, msg: string): asserts actual is string
3131
}
3232
}
3333

34+
export function assertFunction(actual: any, msg: string): asserts actual is Function {
35+
if (!(typeof actual === 'function')) {
36+
throwError(msg, actual === null ? 'null' : typeof actual, 'function', '===');
37+
}
38+
}
39+
3440
export function assertEqual<T>(actual: T, expected: T, msg: string) {
3541
if (!(actual == expected)) {
3642
throwError(msg, actual, expected, '==');

‎packages/core/test/bundling/router/bundle.golden_symbols.json‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1436,6 +1436,9 @@
14361436
{
14371437
"name": "getTView"
14381438
},
1439+
{
1440+
"name": "getTViewCleanup"
1441+
},
14391442
{
14401443
"name": "getToken"
14411444
},

0 commit comments

Comments
 (0)