Skip to content

Commit

Permalink
fix(core): memory leak if view container host view is destroyed while…
Browse files Browse the repository at this point in the history 
… view ref is not (#40219)

When we attach a `ViewRef` to a `ViewContainerRef`, we save a reference to the container
onto the `ViewRef` so that we can remove it when the ref is destroyed. The problem is
that if the container's `hostView` is destroyed first, the `ViewRef` has no way of knowing
that it should stop referencing the container.

These changes remove the leak by not saving a reference at all. Instead, when a `ViewRef`
is destroyed, we clean it up through the `LContainer` directly. We don't need to worry
about the case where the container is destroyed before the view, because containers
automatically clean up all of their views upon destruction.

Fixes #38648.

PR Close #40219
  • Loading branch information
@crisbeto @atscott
crisbeto authored and atscott committed Jan 8, 2021
1 parent b3f322f commit f691e85
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 64 deletions.
65 changes: 17 additions & 48 deletions goldens/circular-deps/packages.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,45 +112,6 @@
"packages/compiler/src/render3/view/styling_builder.ts",
"packages/compiler/src/render3/view/template.ts"
],
[
"packages/core/src/change_detection/change_detection.ts",
"packages/core/src/change_detection/change_detector_ref.ts",
"packages/core/src/render3/view_ref.ts",
"packages/core/src/linker/view_container_ref.ts",
"packages/core/src/linker/component_factory.ts"
],
[
"packages/core/src/change_detection/change_detection.ts",
"packages/core/src/change_detection/change_detector_ref.ts",
"packages/core/src/render3/view_ref.ts",
"packages/core/src/linker/view_container_ref.ts",
"packages/core/src/linker/ng_module_factory.ts",
"packages/core/src/linker/component_factory_resolver.ts",
"packages/core/src/linker/component_factory.ts"
],
[
"packages/core/src/change_detection/change_detection.ts",
"packages/core/src/change_detection/change_detector_ref.ts",
"packages/core/src/render3/view_ref.ts",
"packages/core/src/linker/view_container_ref.ts",
"packages/core/src/linker/template_ref.ts",
"packages/core/src/render3/instructions/shared.ts",
"packages/core/src/error_handler.ts",
"packages/core/src/errors.ts",
"packages/core/src/view/types.ts",
"packages/core/src/linker/component_factory.ts"
],
[
"packages/core/src/change_detection/change_detection.ts",
"packages/core/src/change_detection/change_detector_ref.ts",
"packages/core/src/render3/view_ref.ts",
"packages/core/src/linker/view_container_ref.ts",
"packages/core/src/render3/instructions/shared.ts",
"packages/core/src/error_handler.ts",
"packages/core/src/errors.ts",
"packages/core/src/view/types.ts",
"packages/core/src/linker/component_factory.ts"
],
[
"packages/core/src/change_detection/change_detection.ts",
"packages/core/src/change_detection/change_detector_ref.ts",
Expand All @@ -168,8 +129,6 @@
[
"packages/core/src/change_detection/change_detector_ref.ts",
"packages/core/src/render3/view_ref.ts",
"packages/core/src/linker/view_container_ref.ts",
"packages/core/src/linker/template_ref.ts",
"packages/core/src/linker/view_ref.ts"
],
[
Expand Down Expand Up @@ -210,17 +169,27 @@
"packages/core/src/view/types.ts"
],
[
"packages/core/src/linker/component_factory_resolver.ts",
"packages/core/src/linker/ng_module_factory.ts"
],
[
"packages/core/src/error_handler.ts",
"packages/core/src/errors.ts",
"packages/core/src/view/types.ts",
"packages/core/src/linker/template_ref.ts",
"packages/core/src/render3/view_ref.ts",
"packages/core/src/linker/view_container_ref.ts"
"packages/core/src/render3/instructions/shared.ts"
],
[
"packages/core/src/error_handler.ts",
"packages/core/src/errors.ts",
"packages/core/src/view/types.ts",
"packages/core/src/linker/view_container_ref.ts",
"packages/core/src/render3/view_ref.ts"
"packages/core/src/render3/instructions/shared.ts"
],
[
"packages/core/src/linker/component_factory_resolver.ts",
"packages/core/src/linker/component_factory.ts",
"packages/core/src/linker/ng_module_factory.ts"
],
[
"packages/core/src/linker/component_factory_resolver.ts",
"packages/core/src/linker/ng_module_factory.ts"
],
[
"packages/core/src/metadata/directives.ts",
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/linker/view_container_ref.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ const R3ViewContainerRef = class ViewContainerRef extends VE_ViewContainerRef {
addViewToContainer(tView, lContainer[T_HOST], renderer, lView, parentRNode, beforeNode);
}

(viewRef as R3ViewRef<any>).attachToViewContainerRef(this);
(viewRef as R3ViewRef<any>).attachToViewContainerRef();
addToArray(getOrCreateViewRefs(lContainer), adjustedIdx, viewRef);

return viewRef;
Expand Down
38 changes: 24 additions & 14 deletions packages/core/src/render3/view_ref.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,15 @@
*/

import {ChangeDetectorRef as viewEngine_ChangeDetectorRef} from '../change_detection/change_detector_ref';
import {ViewContainerRef as viewEngine_ViewContainerRef} from '../linker/view_container_ref';
import {EmbeddedViewRef as viewEngine_EmbeddedViewRef, InternalViewRef as viewEngine_InternalViewRef, ViewRefTracker} from '../linker/view_ref';
import {removeFromArray} from '../util/array_utils';
import {assertEqual} from '../util/assert';
import {collectNativeNodes} from './collect_native_nodes';
import {checkNoChangesInRootView, checkNoChangesInternal, detectChangesInRootView, detectChangesInternal, markViewDirty, storeCleanupWithContext} from './instructions/shared';
import {CONTEXT, FLAGS, LView, LViewFlags, TVIEW} from './interfaces/view';
import {destroyLView, renderDetachView} from './node_manipulation';
import {CONTAINER_HEADER_OFFSET, VIEW_REFS} from './interfaces/container';
import {isLContainer} from './interfaces/type_checks';
import {CONTEXT, FLAGS, LView, LViewFlags, PARENT, TVIEW} from './interfaces/view';
import {destroyLView, detachView, renderDetachView} from './node_manipulation';



Expand All @@ -24,7 +27,7 @@ export interface viewEngine_ChangeDetectorRef_interface extends viewEngine_Chang
export class ViewRef<T> implements viewEngine_EmbeddedViewRef<T>, viewEngine_InternalViewRef,
viewEngine_ChangeDetectorRef_interface {
private _appRef: ViewRefTracker|null = null;
private _viewContainerRef: viewEngine_ViewContainerRef|null = null;
private _attachedToViewContainer = false;

get rootNodes(): any[] {
const lView = this._lView;
Expand Down Expand Up @@ -65,14 +68,21 @@ export class ViewRef<T> implements viewEngine_EmbeddedViewRef<T>, viewEngine_Int
destroy(): void {
if (this._appRef) {
this._appRef.detachView(this);
} else if (this._viewContainerRef) {
const index = this._viewContainerRef.indexOf(this);

if (index > -1) {
this._viewContainerRef.detach(index);
} else if (this._attachedToViewContainer) {
const parent = this._lView[PARENT];
if (isLContainer(parent)) {
const viewRefs = parent[VIEW_REFS] as ViewRef<unknown>[] | null;
const index = viewRefs ? viewRefs.indexOf(this) : -1;
if (index > -1) {
ngDevMode &&
assertEqual(
index, parent.indexOf(this._lView) - CONTAINER_HEADER_OFFSET,
'An attached view should be in the same position within its container as its ViewRef in the VIEW_REFS array.');
detachView(parent, index);
removeFromArray(viewRefs!, index);
}
}

this._viewContainerRef = null;
this._attachedToViewContainer = false;
}
destroyLView(this._lView[TVIEW], this._lView);
}
Expand Down Expand Up @@ -271,11 +281,11 @@ export class ViewRef<T> implements viewEngine_EmbeddedViewRef<T>, viewEngine_Int
checkNoChangesInternal(this._lView[TVIEW], this._lView, this.context);
}

attachToViewContainerRef(vcRef: viewEngine_ViewContainerRef) {
attachToViewContainerRef() {
if (this._appRef) {
throw new Error('This view is already attached directly to the ApplicationRef!');
}
this._viewContainerRef = vcRef;
this._attachedToViewContainer = true;
}

detachFromAppRef() {
Expand All @@ -284,7 +294,7 @@ export class ViewRef<T> implements viewEngine_EmbeddedViewRef<T>, viewEngine_Int
}

attachToAppRef(appRef: ViewRefTracker) {
if (this._viewContainerRef) {
if (this._attachedToViewContainer) {
throw new Error('This view is already attached to a ViewContainer!');
}
this._appRef = appRef;
Expand Down
78 changes: 77 additions & 1 deletion packages/core/test/acceptance/view_ref_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {ApplicationRef, ChangeDetectorRef, Component, ComponentFactoryResolver, ComponentRef, ElementRef, Injector, NgModule} from '@angular/core';
import {ApplicationRef, ChangeDetectorRef, Component, ComponentFactoryResolver, ComponentRef, ElementRef, EmbeddedViewRef, Injector, NgModule, TemplateRef, ViewChild, ViewContainerRef} from '@angular/core';
import {InternalViewRef} from '@angular/core/src/linker/view_ref';
import {TestBed} from '@angular/core/testing';

Expand Down Expand Up @@ -72,4 +72,80 @@ describe('ViewRef', () => {

expect(called).toBe(true);
});

it('should remove view ref from view container when destroyed', () => {
@Component({template: ''})
class DynamicComponent {
constructor(public viewContainerRef: ViewContainerRef) {}
}

@Component({template: `<ng-template>Hello</ng-template>`})
class App {
@ViewChild(TemplateRef) templateRef!: TemplateRef<any>;
componentRef!: ComponentRef<DynamicComponent>;
viewRef!: EmbeddedViewRef<any>;
constructor(
private _viewContainerRef: ViewContainerRef,
private _componentFactoryResolver: ComponentFactoryResolver) {}

create() {
const factory = this._componentFactoryResolver.resolveComponentFactory(DynamicComponent);
this.viewRef = this.templateRef.createEmbeddedView(null);
this.componentRef = this._viewContainerRef.createComponent(factory);
this.componentRef.instance.viewContainerRef.insert(this.viewRef);
}
}

@NgModule({declarations: [App, DynamicComponent], entryComponents: [DynamicComponent]})
class MyTestModule {
}

TestBed.configureTestingModule({imports: [MyTestModule]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
fixture.componentInstance.create();
const viewContainerRef = fixture.componentInstance.componentRef.instance.viewContainerRef;

expect(viewContainerRef.length).toBe(1);
fixture.componentInstance.viewRef.destroy();
expect(viewContainerRef.length).toBe(0);
});

it('should mark a ViewRef as destroyed when the host view is destroyed', () => {
@Component({template: ''})
class DynamicComponent {
constructor(public viewContainerRef: ViewContainerRef) {}
}

@Component({template: `<ng-template>Hello</ng-template>`})
class App {
@ViewChild(TemplateRef) templateRef!: TemplateRef<any>;
componentRef!: ComponentRef<DynamicComponent>;
viewRef!: EmbeddedViewRef<any>;
constructor(
private _viewContainerRef: ViewContainerRef,
private _componentFactoryResolver: ComponentFactoryResolver) {}

create() {
const factory = this._componentFactoryResolver.resolveComponentFactory(DynamicComponent);
this.viewRef = this.templateRef.createEmbeddedView(null);
this.componentRef = this._viewContainerRef.createComponent(factory);
this.componentRef.instance.viewContainerRef.insert(this.viewRef);
}
}

@NgModule({declarations: [App, DynamicComponent], entryComponents: [DynamicComponent]})
class MyTestModule {
}

TestBed.configureTestingModule({imports: [MyTestModule]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
fixture.componentInstance.create();
const {componentRef, viewRef} = fixture.componentInstance;

expect(viewRef.destroyed).toBe(false);
componentRef.hostView.destroy();
expect(viewRef.destroyed).toBe(true);
});
});

0 comments on commit f691e85

Please sign in to comment.