Skip to content

Commit dc04438

Browse files
committed
fix(dialog): leaking component instance references
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.
1 parent 08e9d70 commit dc04438

File tree

5 files changed

+43
-14
lines changed

5 files changed

+43
-14
lines changed

src/lib/core/portal/portal-directives.ts

+5-5
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ export class PortalHostDirective extends BasePortalHost implements OnDestroy {
7070
}
7171

7272
ngOnDestroy() {
73-
this.dispose();
73+
super.dispose();
7474
}
7575

7676
/**
@@ -93,7 +93,7 @@ export class PortalHostDirective extends BasePortalHost implements OnDestroy {
9393
componentFactory, viewContainerRef.length,
9494
portal.injector || viewContainerRef.parentInjector);
9595

96-
this.setDisposeFn(() => ref.destroy());
96+
super.setDisposeFn(() => ref.destroy());
9797
return ref;
9898
}
9999

@@ -105,7 +105,7 @@ export class PortalHostDirective extends BasePortalHost implements OnDestroy {
105105
portal.setAttachedHost(this);
106106

107107
this._viewContainerRef.createEmbeddedView(portal.templateRef);
108-
this.setDisposeFn(() => this._viewContainerRef.clear());
108+
super.setDisposeFn(() => this._viewContainerRef.clear());
109109

110110
// TODO(jelbourn): return locals from view
111111
return new Map<string, any>();
@@ -114,11 +114,11 @@ export class PortalHostDirective extends BasePortalHost implements OnDestroy {
114114
/** Detaches the currently attached Portal (if there is one) and attaches the given Portal. */
115115
private _replaceAttachedPortal(p: Portal<any>): void {
116116
if (this.hasAttached()) {
117-
this.detach();
117+
super.detach();
118118
}
119119

120120
if (p) {
121-
this.attach(p);
121+
super.attach(p);
122122
this._portal = p;
123123
}
124124
}

src/lib/core/portal/portal.spec.ts

+11
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,17 @@ describe('Portals', () => {
278278
expect(someDomElement.innerHTML)
279279
.toBe('', 'Expected the DomPortalHost to be empty after detach');
280280
});
281+
282+
it('should call the dispose function even if the host has no attached content', () => {
283+
let spy = jasmine.createSpy('host dispose spy');
284+
285+
expect(host.hasAttached()).toBe(false, 'Expected host not to have attached content.');
286+
287+
host.setDisposeFn(spy);
288+
host.dispose();
289+
290+
expect(spy).toHaveBeenCalled();
291+
});
281292
});
282293
});
283294

src/lib/core/portal/portal.ts

+16-9
Original file line numberDiff line numberDiff line change
@@ -162,12 +162,12 @@ export abstract class BasePortalHost implements PortalHost {
162162
private _isDisposed: boolean = false;
163163

164164
/** Whether this host has an attached portal. */
165-
hasAttached() {
166-
return this._attachedPortal != null;
165+
hasAttached(): boolean {
166+
return !!this._attachedPortal;
167167
}
168168

169169
attach(portal: Portal<any>): any {
170-
if (portal == null) {
170+
if (!portal) {
171171
throw new NullPortalError();
172172
}
173173

@@ -195,24 +195,31 @@ export abstract class BasePortalHost implements PortalHost {
195195
abstract attachTemplatePortal(portal: TemplatePortal): Map<string, any>;
196196

197197
detach(): void {
198-
if (this._attachedPortal) { this._attachedPortal.setAttachedHost(null); }
199-
200-
this._attachedPortal = null;
201-
if (this._disposeFn != null) {
202-
this._disposeFn();
203-
this._disposeFn = null;
198+
if (this._attachedPortal) {
199+
this._attachedPortal.setAttachedHost(null);
200+
this._attachedPortal = null;
204201
}
202+
203+
this._invokeDisposeFn();
205204
}
206205

207206
dispose() {
208207
if (this.hasAttached()) {
209208
this.detach();
210209
}
211210

211+
this._invokeDisposeFn();
212212
this._isDisposed = true;
213213
}
214214

215215
setDisposeFn(fn: () => void) {
216216
this._disposeFn = fn;
217217
}
218+
219+
private _invokeDisposeFn() {
220+
if (this._disposeFn) {
221+
this._disposeFn();
222+
this._disposeFn = null;
223+
}
224+
}
218225
}

src/lib/dialog/dialog-ref.ts

+1
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ export class MdDialogRef<T> {
2727
this._overlayRef.dispose();
2828
this._afterClosed.next(dialogResult);
2929
this._afterClosed.complete();
30+
this.componentInstance = null;
3031
}
3132

3233
/**

src/lib/dialog/dialog.spec.ts

+10
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,16 @@ describe('MdDialog', () => {
271271
expect(overlayContainerElement.querySelectorAll('md-dialog-container').length).toBe(0);
272272
});
273273

274+
it('should not keep a reference to the component instance after the dialog is closed', () => {
275+
let dialogRef = dialog.open(PizzaMsg);
276+
277+
expect(dialogRef.componentInstance).toBeTruthy();
278+
279+
dialogRef.close();
280+
281+
expect(dialogRef.componentInstance).toBeFalsy('Expected reference to have been cleared.');
282+
});
283+
274284
describe('disableClose option', () => {
275285
it('should prevent closing via clicks on the backdrop', () => {
276286
dialog.open(PizzaMsg, {

0 commit comments

Comments
 (0)