Skip to content

Commit 0ae7775

Browse files
tboschtbosch
committed
fix(core): always remove DOM listeners and stream subscriptions
This is needed to prevent memory leaks. The DOM listeners don’t need to be removed for simple examples, but a big internal app shows memory leaks because of them. BREAKING CHANGE: - `Renderer.listen` now has to return a function that removes the event listener.
1 parent 5f0baaa commit 0ae7775

15 files changed

Lines changed: 104 additions & 46 deletions

File tree

‎modules/angular2/src/compiler/view_compiler.ts‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ interface ViewFactory<EXPRESSION, STATEMENT> {
112112

113113
createElementEventListener(renderer: EXPRESSION, view: EXPRESSION, boundElementIndex: number,
114114
renderNode: EXPRESSION, eventAst: BoundEventAst,
115-
targetStatements: STATEMENT[]);
115+
targetStatements: STATEMENT[]): EXPRESSION;
116116

117117
setElementAttribute(renderer: EXPRESSION, renderNode: EXPRESSION, attrName: string,
118118
attrValue: string, targetStatements: STATEMENT[]);
@@ -201,9 +201,11 @@ class CodeGenViewFactory implements ViewFactory<Expression, Statement> {
201201
createElementEventListener(renderer: Expression, appView: Expression, boundElementIndex: number,
202202
renderNode: Expression, eventAst: BoundEventAst,
203203
targetStatements: Statement[]) {
204+
var disposableVar = this._nextDisposableVar();
204205
var eventHandlerExpr = codeGenEventHandler(appView, boundElementIndex, eventAst.fullName);
205206
targetStatements.push(new Statement(
206-
`${renderer.expression}.listen(${renderNode.expression}, ${escapeValue(eventAst.name)}, ${eventHandlerExpr});`));
207+
`var ${disposableVar} = ${renderer.expression}.listen(${renderNode.expression}, ${escapeValue(eventAst.name)}, ${eventHandlerExpr});`));
208+
return new Expression(disposableVar);
207209
}
208210

209211
setElementAttribute(renderer: Expression, renderNode: Expression, attrName: string,
@@ -345,9 +347,11 @@ class RuntimeViewFactory implements ViewFactory<any, any> {
345347
}
346348

347349
createElementEventListener(renderer: Renderer, appView: AppView, boundElementIndex: number,
348-
renderNode: any, eventAst: BoundEventAst, targetStatements: any[]) {
349-
renderer.listen(renderNode, eventAst.name, (event) => appView.triggerEventHandlers(
350-
eventAst.fullName, event, boundElementIndex));
350+
renderNode: any, eventAst: BoundEventAst,
351+
targetStatements: any[]): any {
352+
return renderer.listen(
353+
renderNode, eventAst.name,
354+
(event) => appView.triggerEventHandlers(eventAst.fullName, event, boundElementIndex));
351355
}
352356

353357
setElementAttribute(renderer: Renderer, renderNode: any, attrName: string, attrValue: string,
@@ -520,14 +524,16 @@ class ViewBuilderVisitor<EXPRESSION, STATEMENT> implements TemplateAstVisitor {
520524
var protoEl = this.protoView.protoElements[elementIndex];
521525

522526
protoEl.renderEvents.forEach((eventAst) => {
527+
var disposable;
523528
if (isPresent(eventAst.target)) {
524-
var disposable = this.factory.createGlobalEventListener(
529+
disposable = this.factory.createGlobalEventListener(
525530
this.renderer, this.view, protoEl.boundElementIndex, eventAst, this.renderStmts);
526-
this.appDisposables.push(disposable);
527531
} else {
528-
this.factory.createElementEventListener(this.renderer, this.view, protoEl.boundElementIndex,
529-
renderNode, eventAst, this.renderStmts);
532+
disposable = this.factory.createElementEventListener(this.renderer, this.view,
533+
protoEl.boundElementIndex, renderNode,
534+
eventAst, this.renderStmts);
530535
}
536+
this.appDisposables.push(disposable);
531537
});
532538
for (var i = 0; i < protoEl.attrNameAndValues.length; i++) {
533539
var attrName = protoEl.attrNameAndValues[i][0];

‎modules/angular2/src/core/change_detection/abstract_change_detector.ts‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import {Locals} from './parser/locals';
1717
import {ChangeDetectionStrategy, ChangeDetectorState} from './constants';
1818
import {wtfCreateScope, wtfLeave, WtfScopeFn} from '../profile/profile';
1919
import {isObservable} from './observable_facade';
20-
20+
import {ObservableWrapper} from 'angular2/src/facade/async';
2121

2222
var _scope_check: WtfScopeFn = wtfCreateScope(`ChangeDetector#check(ascii id, bool throwOnChange)`);
2323

@@ -40,6 +40,7 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
4040
mode: ChangeDetectionStrategy = null;
4141
pipes: Pipes = null;
4242
propertyBindingIndex: number;
43+
outputSubscriptions: any[];
4344

4445
// This is an experimental feature. Works only in Dart.
4546
subscriptions: any[];
@@ -72,7 +73,7 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
7273

7374
handleEvent(eventName: string, elIndex: number, event: any): boolean {
7475
if (!this.hydrated()) {
75-
return true;
76+
this.throwDehydratedError();
7677
}
7778
try {
7879
var locals = new Map<string, any>();
@@ -180,6 +181,8 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
180181
this._unsubsribeFromObservables();
181182
}
182183

184+
this._unsubscribeFromOutputs();
185+
183186
this.dispatcher = null;
184187
this.context = null;
185188
this.locals = null;
@@ -258,6 +261,15 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
258261
}
259262
}
260263

264+
private _unsubscribeFromOutputs(): void {
265+
if (isPresent(this.outputSubscriptions)) {
266+
for (var i = 0; i < this.outputSubscriptions.length; ++i) {
267+
ObservableWrapper.dispose(this.outputSubscriptions[i]);
268+
this.outputSubscriptions[i] = null;
269+
}
270+
}
271+
}
272+
261273
// This is an experimental feature. Works only in Dart.
262274
observeValue(value: any, index: number): any {
263275
if (isObservable(value)) {

‎modules/angular2/src/core/change_detection/codegen_logic_util.ts‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,21 +153,32 @@ export class CodegenLogicUtil {
153153

154154
genHydrateDirectives(directiveRecords: DirectiveRecord[]): string {
155155
var res = [];
156+
var outputCount = 0;
156157
for (var i = 0; i < directiveRecords.length; ++i) {
157158
var r = directiveRecords[i];
158159
var dirVarName = this._names.getDirectiveName(r.directiveIndex);
159160
res.push(`${dirVarName} = ${this._genReadDirective(i)};`);
160161
if (isPresent(r.outputs)) {
161162
r.outputs.forEach(output => {
162163
var eventHandlerExpr = this._genEventHandler(r.directiveIndex.elementIndex, output[1]);
164+
var statementStart =
165+
`this.outputSubscriptions[${outputCount++}] = ${dirVarName}.${output[0]}`;
163166
if (IS_DART) {
164-
res.push(`${dirVarName}.${output[0]}.listen(${eventHandlerExpr});`);
167+
res.push(`${statementStart}.listen(${eventHandlerExpr});`);
165168
} else {
166-
res.push(`${dirVarName}.${output[0]}.subscribe({next: ${eventHandlerExpr}});`);
169+
res.push(`${statementStart}.subscribe({next: ${eventHandlerExpr}});`);
167170
}
168171
});
169172
}
170173
}
174+
if (outputCount > 0) {
175+
var statementStart = 'this.outputSubscriptions';
176+
if (IS_DART) {
177+
res.unshift(`${statementStart} = new List(${outputCount});`);
178+
} else {
179+
res.unshift(`${statementStart} = new Array(${outputCount});`);
180+
}
181+
}
171182
return res.join("\n");
172183
}
173184

‎modules/angular2/src/core/change_detection/dynamic_change_detector.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
114114
super.observeDirective(this._getDirectiveFor(index), i);
115115
}
116116
}
117+
this.outputSubscriptions = [];
117118
for (var i = 0; i < this._directiveRecords.length; ++i) {
118119
var r = this._directiveRecords[i];
119120
if (isPresent(r.outputs)) {
@@ -122,7 +123,8 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
122123
<any>this._createEventHandler(r.directiveIndex.elementIndex, output[1]);
123124
var directive = this._getDirectiveFor(r.directiveIndex);
124125
var getter = reflector.getter(output[0]);
125-
ObservableWrapper.subscribe(getter(directive), eventHandler);
126+
this.outputSubscriptions.push(
127+
ObservableWrapper.subscribe(getter(directive), eventHandler));
126128
});
127129
}
128130
}

‎modules/angular2/src/core/change_detection/exceptions.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ export class ChangeDetectionError extends WrappedException {
9191
* This is an internal Angular error.
9292
*/
9393
export class DehydratedException extends BaseException {
94-
constructor() { super('Attempt to detect changes on a dehydrated detector.'); }
94+
constructor() { super('Attempt to use a dehydrated detector.'); }
9595
}
9696

9797
/**

‎modules/angular2/src/core/render/api.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ export abstract class Renderer implements ParentRenderer {
2828

2929
abstract destroyView(hostElement: any, viewAllNodes: any[]);
3030

31-
abstract listen(renderElement: any, name: string, callback: Function);
31+
abstract listen(renderElement: any, name: string, callback: Function): Function;
3232

3333
abstract listenGlobal(target: string, name: string, callback: Function): Function;
3434

‎modules/angular2/src/platform/dom/dom_renderer.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -155,9 +155,9 @@ export class DomRenderer implements Renderer {
155155
}
156156
}
157157

158-
listen(renderElement: any, name: string, callback: Function) {
159-
this._rootRenderer.eventManager.addEventListener(renderElement, name,
160-
decoratePreventDefault(callback));
158+
listen(renderElement: any, name: string, callback: Function): Function {
159+
return this._rootRenderer.eventManager.addEventListener(renderElement, name,
160+
decoratePreventDefault(callback));
161161
}
162162

163163
listenGlobal(target: string, name: string, callback: Function): Function {

‎modules/angular2/src/platform/dom/events/dom_events.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,18 @@ export class DomEventsPlugin extends EventManagerPlugin {
88
// events.
99
supports(eventName: string): boolean { return true; }
1010

11-
addEventListener(element: HTMLElement, eventName: string, handler: Function) {
11+
addEventListener(element: HTMLElement, eventName: string, handler: Function): Function {
1212
var zone = this.manager.getZone();
1313
var outsideHandler = (event) => zone.run(() => handler(event));
14-
this.manager.getZone().runOutsideAngular(() => { DOM.on(element, eventName, outsideHandler); });
14+
return this.manager.getZone().runOutsideAngular(
15+
() => DOM.onAndCancel(element, eventName, outsideHandler));
1516
}
1617

1718
addGlobalEventListener(target: string, eventName: string, handler: Function): Function {
1819
var element = DOM.getGlobalEventTarget(target);
1920
var zone = this.manager.getZone();
2021
var outsideHandler = (event) => zone.run(() => handler(event));
2122
return this.manager.getZone().runOutsideAngular(
22-
() => { return DOM.onAndCancel(element, eventName, outsideHandler); });
23+
() => DOM.onAndCancel(element, eventName, outsideHandler));
2324
}
2425
}

‎modules/angular2/src/platform/dom/events/event_manager.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,9 @@ export class EventManager {
1616
this._plugins = ListWrapper.reversed(plugins);
1717
}
1818

19-
addEventListener(element: HTMLElement, eventName: string, handler: Function) {
19+
addEventListener(element: HTMLElement, eventName: string, handler: Function): Function {
2020
var plugin = this._findPluginFor(eventName);
21-
plugin.addEventListener(element, eventName, handler);
21+
return plugin.addEventListener(element, eventName, handler);
2222
}
2323

2424
addGlobalEventListener(target: string, eventName: string, handler: Function): Function {
@@ -47,7 +47,7 @@ export class EventManagerPlugin {
4747
// That is equivalent to having supporting $event.target
4848
supports(eventName: string): boolean { return false; }
4949

50-
addEventListener(element: HTMLElement, eventName: string, handler: Function) {
50+
addEventListener(element: HTMLElement, eventName: string, handler: Function): Function {
5151
throw "not implemented";
5252
}
5353

‎modules/angular2/src/platform/dom/events/hammer_gestures.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,17 +15,18 @@ export class HammerGesturesPlugin extends HammerGesturesPluginCommon {
1515
return true;
1616
}
1717

18-
addEventListener(element: HTMLElement, eventName: string, handler: Function) {
18+
addEventListener(element: HTMLElement, eventName: string, handler: Function): Function {
1919
var zone = this.manager.getZone();
2020
eventName = eventName.toLowerCase();
2121

22-
zone.runOutsideAngular(function() {
22+
return zone.runOutsideAngular(function() {
2323
// Creating the manager bind events, must be done outside of angular
2424
var mc = new Hammer(element);
2525
mc.get('pinch').set({enable: true});
2626
mc.get('rotate').set({enable: true});
27-
28-
mc.on(eventName, function(eventObj) { zone.run(function() { handler(eventObj); }); });
27+
var handler = function(eventObj) { zone.run(function() { handler(eventObj); }); };
28+
mc.on(eventName, handler);
29+
return () => { mc.off(eventName, handler); };
2930
});
3031
}
3132
}

0 commit comments

Comments
 (0)