Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 3680ad1

Browse filesBrowse 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
Copy full SHA for 3680ad1

File tree

9 files changed

+67
-37
lines changed
Filter options

9 files changed

+67
-37
lines changed

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

Copy file name to clipboardExpand all lines: goldens/size-tracking/integration-payloads.json
+1-1Lines 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

Copy file name to clipboardExpand all lines: packages/core/src/application_ref.ts
+14-12Lines 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

Copy file name to clipboardExpand all lines: packages/core/src/render3/component_ref.ts
+2-9Lines 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

Copy file name to clipboardExpand all lines: packages/core/src/render3/instructions/listener.ts
+2-2Lines 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

Copy file name to clipboardExpand all lines: packages/core/src/render3/instructions/shared.ts
+16-4Lines 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

Copy file name to clipboardExpand all lines: packages/core/src/render3/interfaces/view.ts
+4-1Lines 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

Copy file name to clipboardExpand all lines: packages/core/src/render3/node_manipulation.ts
+19-8Lines 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

Copy file name to clipboardExpand all lines: packages/core/src/util/assert.ts
+6Lines 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

Copy file name to clipboardExpand all lines: packages/core/test/bundling/router/bundle.golden_symbols.json
+3Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.