Skip to content

Commit

Permalink
fix(dialog): leaking component instance references
Browse files Browse the repository at this point in the history
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 angular#2734.
  • Loading branch information
crisbeto committed Jan 31, 2017
1 parent 08e9d70 commit dc04438
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 14 deletions.
10 changes: 5 additions & 5 deletions src/lib/core/portal/portal-directives.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export class PortalHostDirective extends BasePortalHost implements OnDestroy {
}

ngOnDestroy() {
this.dispose();
super.dispose();
}

/**
Expand All @@ -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;
}

Expand All @@ -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>();
Expand All @@ -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;
}
}
Expand Down
11 changes: 11 additions & 0 deletions src/lib/core/portal/portal.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
});

Expand Down
25 changes: 16 additions & 9 deletions src/lib/core/portal/portal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down Expand Up @@ -195,24 +195,31 @@ 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() {
if (this.hasAttached()) {
this.detach();
}

this._invokeDisposeFn();
this._isDisposed = true;
}

setDisposeFn(fn: () => void) {
this._disposeFn = fn;
}

private _invokeDisposeFn() {
if (this._disposeFn) {
this._disposeFn();
this._disposeFn = null;
}
}
}
1 change: 1 addition & 0 deletions src/lib/dialog/dialog-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export class MdDialogRef<T> {
this._overlayRef.dispose();
this._afterClosed.next(dialogResult);
this._afterClosed.complete();
this.componentInstance = null;
}

/**
Expand Down
10 changes: 10 additions & 0 deletions src/lib/dialog/dialog.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,16 @@ describe('MdDialog', () => {
expect(overlayContainerElement.querySelectorAll('md-dialog-container').length).toBe(0);
});

it('should not keep a reference to the component instance after the dialog is closed', () => {
let dialogRef = dialog.open(PizzaMsg);

expect(dialogRef.componentInstance).toBeTruthy();

dialogRef.close();

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, {
Expand Down

0 comments on commit dc04438

Please sign in to comment.