mirror of
https://github.com/angular/angular.git
synced 2026-09-14 13:54:52 +08:00
fix(dialog): leaking component instance references (#2875)
Fixes a memory leak in the dialog, which was causing it to keep references to the user's instantiated dialog in memory forever. It was due to: 1. The portal class wasn't invoking the cleanup function if it didn't have attached content. 2. The dialog ref was keeping the reference after the dialog is closed. There is also a leak related to dialog refs which will be addressed separately. Fixes #2734.
This commit is contained in:
committed by
mmalerba
parent
5f3abae3b6
commit
e120e8da92
@@ -70,7 +70,7 @@ export class PortalHostDirective extends BasePortalHost implements OnDestroy {
|
||||
}
|
||||
|
||||
ngOnDestroy() {
|
||||
this.dispose();
|
||||
super.dispose();
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -93,7 +93,7 @@ export class PortalHostDirective extends BasePortalHost implements OnDestroy {
|
||||
componentFactory, viewContainerRef.length,
|
||||
portal.injector || viewContainerRef.parentInjector);
|
||||
|
||||
this.setDisposeFn(() => ref.destroy());
|
||||
super.setDisposeFn(() => ref.destroy());
|
||||
return ref;
|
||||
}
|
||||
|
||||
@@ -105,7 +105,7 @@ export class PortalHostDirective extends BasePortalHost implements OnDestroy {
|
||||
portal.setAttachedHost(this);
|
||||
|
||||
this._viewContainerRef.createEmbeddedView(portal.templateRef);
|
||||
this.setDisposeFn(() => this._viewContainerRef.clear());
|
||||
super.setDisposeFn(() => this._viewContainerRef.clear());
|
||||
|
||||
// TODO(jelbourn): return locals from view
|
||||
return new Map<string, any>();
|
||||
@@ -114,11 +114,11 @@ export class PortalHostDirective extends BasePortalHost implements OnDestroy {
|
||||
/** Detaches the currently attached Portal (if there is one) and attaches the given Portal. */
|
||||
private _replaceAttachedPortal(p: Portal<any>): void {
|
||||
if (this.hasAttached()) {
|
||||
this.detach();
|
||||
super.detach();
|
||||
}
|
||||
|
||||
if (p) {
|
||||
this.attach(p);
|
||||
super.attach(p);
|
||||
this._portal = p;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -278,6 +278,17 @@ describe('Portals', () => {
|
||||
expect(someDomElement.innerHTML)
|
||||
.toBe('', 'Expected the DomPortalHost to be empty after detach');
|
||||
});
|
||||
|
||||
it('should call the dispose function even if the host has no attached content', () => {
|
||||
let spy = jasmine.createSpy('host dispose spy');
|
||||
|
||||
expect(host.hasAttached()).toBe(false, 'Expected host not to have attached content.');
|
||||
|
||||
host.setDisposeFn(spy);
|
||||
host.dispose();
|
||||
|
||||
expect(spy).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -162,12 +162,12 @@ export abstract class BasePortalHost implements PortalHost {
|
||||
private _isDisposed: boolean = false;
|
||||
|
||||
/** Whether this host has an attached portal. */
|
||||
hasAttached() {
|
||||
return this._attachedPortal != null;
|
||||
hasAttached(): boolean {
|
||||
return !!this._attachedPortal;
|
||||
}
|
||||
|
||||
attach(portal: Portal<any>): any {
|
||||
if (portal == null) {
|
||||
if (!portal) {
|
||||
throw new NullPortalError();
|
||||
}
|
||||
|
||||
@@ -195,13 +195,12 @@ export abstract class BasePortalHost implements PortalHost {
|
||||
abstract attachTemplatePortal(portal: TemplatePortal): Map<string, any>;
|
||||
|
||||
detach(): void {
|
||||
if (this._attachedPortal) { this._attachedPortal.setAttachedHost(null); }
|
||||
|
||||
this._attachedPortal = null;
|
||||
if (this._disposeFn != null) {
|
||||
this._disposeFn();
|
||||
this._disposeFn = null;
|
||||
if (this._attachedPortal) {
|
||||
this._attachedPortal.setAttachedHost(null);
|
||||
this._attachedPortal = null;
|
||||
}
|
||||
|
||||
this._invokeDisposeFn();
|
||||
}
|
||||
|
||||
dispose() {
|
||||
@@ -209,10 +208,18 @@ export abstract class BasePortalHost implements PortalHost {
|
||||
this.detach();
|
||||
}
|
||||
|
||||
this._invokeDisposeFn();
|
||||
this._isDisposed = true;
|
||||
}
|
||||
|
||||
setDisposeFn(fn: () => void) {
|
||||
this._disposeFn = fn;
|
||||
}
|
||||
|
||||
private _invokeDisposeFn() {
|
||||
if (this._disposeFn) {
|
||||
this._disposeFn();
|
||||
this._disposeFn = null;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -31,6 +31,7 @@ export class MdDialogRef<T> {
|
||||
this._overlayRef.dispose();
|
||||
this._afterClosed.next(this._result);
|
||||
this._afterClosed.complete();
|
||||
this.componentInstance = null;
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
@@ -345,6 +345,19 @@ describe('MdDialog', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('should not keep a reference to the component after the dialog is closed', async(() => {
|
||||
let dialogRef = dialog.open(PizzaMsg);
|
||||
|
||||
expect(dialogRef.componentInstance).toBeTruthy();
|
||||
|
||||
dialogRef.close();
|
||||
viewContainerFixture.detectChanges();
|
||||
|
||||
viewContainerFixture.whenStable().then(() => {
|
||||
expect(dialogRef.componentInstance).toBeFalsy('Expected reference to have been cleared.');
|
||||
});
|
||||
}));
|
||||
|
||||
describe('disableClose option', () => {
|
||||
it('should prevent closing via clicks on the backdrop', () => {
|
||||
dialog.open(PizzaMsg, {
|
||||
|
||||
Reference in New Issue
Block a user