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.
This commit is contained in:
Tobias Bosch 2016-01-25 14:47:25 -08:00
parent 5f0baaac73
commit 0ae77753f3
15 changed files with 104 additions and 46 deletions

View File

@ -112,7 +112,7 @@ interface ViewFactory<EXPRESSION, STATEMENT> {
createElementEventListener(renderer: EXPRESSION, view: EXPRESSION, boundElementIndex: number,
renderNode: EXPRESSION, eventAst: BoundEventAst,
targetStatements: STATEMENT[]);
targetStatements: STATEMENT[]): EXPRESSION;
setElementAttribute(renderer: EXPRESSION, renderNode: EXPRESSION, attrName: string,
attrValue: string, targetStatements: STATEMENT[]);
@ -201,9 +201,11 @@ class CodeGenViewFactory implements ViewFactory<Expression, Statement> {
createElementEventListener(renderer: Expression, appView: Expression, boundElementIndex: number,
renderNode: Expression, eventAst: BoundEventAst,
targetStatements: Statement[]) {
var disposableVar = this._nextDisposableVar();
var eventHandlerExpr = codeGenEventHandler(appView, boundElementIndex, eventAst.fullName);
targetStatements.push(new Statement(
`${renderer.expression}.listen(${renderNode.expression}, ${escapeValue(eventAst.name)}, ${eventHandlerExpr});`));
`var ${disposableVar} = ${renderer.expression}.listen(${renderNode.expression}, ${escapeValue(eventAst.name)}, ${eventHandlerExpr});`));
return new Expression(disposableVar);
}
setElementAttribute(renderer: Expression, renderNode: Expression, attrName: string,
@ -345,9 +347,11 @@ class RuntimeViewFactory implements ViewFactory<any, any> {
}
createElementEventListener(renderer: Renderer, appView: AppView, boundElementIndex: number,
renderNode: any, eventAst: BoundEventAst, targetStatements: any[]) {
renderer.listen(renderNode, eventAst.name, (event) => appView.triggerEventHandlers(
eventAst.fullName, event, boundElementIndex));
renderNode: any, eventAst: BoundEventAst,
targetStatements: any[]): any {
return renderer.listen(
renderNode, eventAst.name,
(event) => appView.triggerEventHandlers(eventAst.fullName, event, boundElementIndex));
}
setElementAttribute(renderer: Renderer, renderNode: any, attrName: string, attrValue: string,
@ -520,14 +524,16 @@ class ViewBuilderVisitor<EXPRESSION, STATEMENT> implements TemplateAstVisitor {
var protoEl = this.protoView.protoElements[elementIndex];
protoEl.renderEvents.forEach((eventAst) => {
var disposable;
if (isPresent(eventAst.target)) {
var disposable = this.factory.createGlobalEventListener(
disposable = this.factory.createGlobalEventListener(
this.renderer, this.view, protoEl.boundElementIndex, eventAst, this.renderStmts);
this.appDisposables.push(disposable);
} else {
this.factory.createElementEventListener(this.renderer, this.view, protoEl.boundElementIndex,
renderNode, eventAst, this.renderStmts);
disposable = this.factory.createElementEventListener(this.renderer, this.view,
protoEl.boundElementIndex, renderNode,
eventAst, this.renderStmts);
}
this.appDisposables.push(disposable);
});
for (var i = 0; i < protoEl.attrNameAndValues.length; i++) {
var attrName = protoEl.attrNameAndValues[i][0];

View File

@ -17,7 +17,7 @@ import {Locals} from './parser/locals';
import {ChangeDetectionStrategy, ChangeDetectorState} from './constants';
import {wtfCreateScope, wtfLeave, WtfScopeFn} from '../profile/profile';
import {isObservable} from './observable_facade';
import {ObservableWrapper} from 'angular2/src/facade/async';
var _scope_check: WtfScopeFn = wtfCreateScope(`ChangeDetector#check(ascii id, bool throwOnChange)`);
@ -40,6 +40,7 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
mode: ChangeDetectionStrategy = null;
pipes: Pipes = null;
propertyBindingIndex: number;
outputSubscriptions: any[];
// This is an experimental feature. Works only in Dart.
subscriptions: any[];
@ -72,7 +73,7 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
handleEvent(eventName: string, elIndex: number, event: any): boolean {
if (!this.hydrated()) {
return true;
this.throwDehydratedError();
}
try {
var locals = new Map<string, any>();
@ -180,6 +181,8 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
this._unsubsribeFromObservables();
}
this._unsubscribeFromOutputs();
this.dispatcher = null;
this.context = null;
this.locals = null;
@ -258,6 +261,15 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
}
}
private _unsubscribeFromOutputs(): void {
if (isPresent(this.outputSubscriptions)) {
for (var i = 0; i < this.outputSubscriptions.length; ++i) {
ObservableWrapper.dispose(this.outputSubscriptions[i]);
this.outputSubscriptions[i] = null;
}
}
}
// This is an experimental feature. Works only in Dart.
observeValue(value: any, index: number): any {
if (isObservable(value)) {

View File

@ -153,6 +153,7 @@ export class CodegenLogicUtil {
genHydrateDirectives(directiveRecords: DirectiveRecord[]): string {
var res = [];
var outputCount = 0;
for (var i = 0; i < directiveRecords.length; ++i) {
var r = directiveRecords[i];
var dirVarName = this._names.getDirectiveName(r.directiveIndex);
@ -160,14 +161,24 @@ export class CodegenLogicUtil {
if (isPresent(r.outputs)) {
r.outputs.forEach(output => {
var eventHandlerExpr = this._genEventHandler(r.directiveIndex.elementIndex, output[1]);
var statementStart =
`this.outputSubscriptions[${outputCount++}] = ${dirVarName}.${output[0]}`;
if (IS_DART) {
res.push(`${dirVarName}.${output[0]}.listen(${eventHandlerExpr});`);
res.push(`${statementStart}.listen(${eventHandlerExpr});`);
} else {
res.push(`${dirVarName}.${output[0]}.subscribe({next: ${eventHandlerExpr}});`);
res.push(`${statementStart}.subscribe({next: ${eventHandlerExpr}});`);
}
});
}
}
if (outputCount > 0) {
var statementStart = 'this.outputSubscriptions';
if (IS_DART) {
res.unshift(`${statementStart} = new List(${outputCount});`);
} else {
res.unshift(`${statementStart} = new Array(${outputCount});`);
}
}
return res.join("\n");
}

View File

@ -114,6 +114,7 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
super.observeDirective(this._getDirectiveFor(index), i);
}
}
this.outputSubscriptions = [];
for (var i = 0; i < this._directiveRecords.length; ++i) {
var r = this._directiveRecords[i];
if (isPresent(r.outputs)) {
@ -122,7 +123,8 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
<any>this._createEventHandler(r.directiveIndex.elementIndex, output[1]);
var directive = this._getDirectiveFor(r.directiveIndex);
var getter = reflector.getter(output[0]);
ObservableWrapper.subscribe(getter(directive), eventHandler);
this.outputSubscriptions.push(
ObservableWrapper.subscribe(getter(directive), eventHandler));
});
}
}

View File

@ -91,7 +91,7 @@ export class ChangeDetectionError extends WrappedException {
* This is an internal Angular error.
*/
export class DehydratedException extends BaseException {
constructor() { super('Attempt to detect changes on a dehydrated detector.'); }
constructor() { super('Attempt to use a dehydrated detector.'); }
}
/**

View File

@ -28,7 +28,7 @@ export abstract class Renderer implements ParentRenderer {
abstract destroyView(hostElement: any, viewAllNodes: any[]);
abstract listen(renderElement: any, name: string, callback: Function);
abstract listen(renderElement: any, name: string, callback: Function): Function;
abstract listenGlobal(target: string, name: string, callback: Function): Function;

View File

@ -155,8 +155,8 @@ export class DomRenderer implements Renderer {
}
}
listen(renderElement: any, name: string, callback: Function) {
this._rootRenderer.eventManager.addEventListener(renderElement, name,
listen(renderElement: any, name: string, callback: Function): Function {
return this._rootRenderer.eventManager.addEventListener(renderElement, name,
decoratePreventDefault(callback));
}

View File

@ -8,10 +8,11 @@ export class DomEventsPlugin extends EventManagerPlugin {
// events.
supports(eventName: string): boolean { return true; }
addEventListener(element: HTMLElement, eventName: string, handler: Function) {
addEventListener(element: HTMLElement, eventName: string, handler: Function): Function {
var zone = this.manager.getZone();
var outsideHandler = (event) => zone.run(() => handler(event));
this.manager.getZone().runOutsideAngular(() => { DOM.on(element, eventName, outsideHandler); });
return this.manager.getZone().runOutsideAngular(
() => DOM.onAndCancel(element, eventName, outsideHandler));
}
addGlobalEventListener(target: string, eventName: string, handler: Function): Function {
@ -19,6 +20,6 @@ export class DomEventsPlugin extends EventManagerPlugin {
var zone = this.manager.getZone();
var outsideHandler = (event) => zone.run(() => handler(event));
return this.manager.getZone().runOutsideAngular(
() => { return DOM.onAndCancel(element, eventName, outsideHandler); });
() => DOM.onAndCancel(element, eventName, outsideHandler));
}
}

View File

@ -16,9 +16,9 @@ export class EventManager {
this._plugins = ListWrapper.reversed(plugins);
}
addEventListener(element: HTMLElement, eventName: string, handler: Function) {
addEventListener(element: HTMLElement, eventName: string, handler: Function): Function {
var plugin = this._findPluginFor(eventName);
plugin.addEventListener(element, eventName, handler);
return plugin.addEventListener(element, eventName, handler);
}
addGlobalEventListener(target: string, eventName: string, handler: Function): Function {
@ -47,7 +47,7 @@ export class EventManagerPlugin {
// That is equivalent to having supporting $event.target
supports(eventName: string): boolean { return false; }
addEventListener(element: HTMLElement, eventName: string, handler: Function) {
addEventListener(element: HTMLElement, eventName: string, handler: Function): Function {
throw "not implemented";
}

View File

@ -15,17 +15,18 @@ export class HammerGesturesPlugin extends HammerGesturesPluginCommon {
return true;
}
addEventListener(element: HTMLElement, eventName: string, handler: Function) {
addEventListener(element: HTMLElement, eventName: string, handler: Function): Function {
var zone = this.manager.getZone();
eventName = eventName.toLowerCase();
zone.runOutsideAngular(function() {
return zone.runOutsideAngular(function() {
// Creating the manager bind events, must be done outside of angular
var mc = new Hammer(element);
mc.get('pinch').set({enable: true});
mc.get('rotate').set({enable: true});
mc.on(eventName, function(eventObj) { zone.run(function() { handler(eventObj); }); });
var handler = function(eventObj) { zone.run(function() { handler(eventObj); }); };
mc.on(eventName, handler);
return () => { mc.off(eventName, handler); };
});
}
}

View File

@ -27,14 +27,15 @@ export class KeyEventsPlugin extends EventManagerPlugin {
return isPresent(KeyEventsPlugin.parseEventName(eventName));
}
addEventListener(element: HTMLElement, eventName: string, handler: Function) {
addEventListener(element: HTMLElement, eventName: string, handler: Function): Function {
var parsedEvent = KeyEventsPlugin.parseEventName(eventName);
var outsideHandler = KeyEventsPlugin.eventCallback(
element, StringMapWrapper.get(parsedEvent, 'fullKey'), handler, this.manager.getZone());
this.manager.getZone().runOutsideAngular(() => {
DOM.on(element, StringMapWrapper.get(parsedEvent, 'domEventName'), outsideHandler);
return this.manager.getZone().runOutsideAngular(() => {
return DOM.onAndCancel(element, StringMapWrapper.get(parsedEvent, 'domEventName'),
outsideHandler);
});
}

View File

@ -66,12 +66,12 @@ export class MessageBasedRenderer {
bind(this._invokeElementMethod, this));
broker.registerMethod("setText", [RenderStoreObject, RenderStoreObject, PRIMITIVE],
bind(this._setText, this));
broker.registerMethod("listen", [RenderStoreObject, RenderStoreObject, PRIMITIVE],
broker.registerMethod("listen", [RenderStoreObject, RenderStoreObject, PRIMITIVE, PRIMITIVE],
bind(this._listen, this));
broker.registerMethod("listenGlobal", [RenderStoreObject, PRIMITIVE, PRIMITIVE, PRIMITIVE],
bind(this._listenGlobal, this));
broker.registerMethod("listenGlobalDone", [RenderStoreObject, RenderStoreObject],
bind(this._listenGlobalDone, this));
broker.registerMethod("listenDone", [RenderStoreObject, RenderStoreObject],
bind(this._listenDone, this));
}
private _renderComponent(renderComponentType: RenderComponentType, rendererId: number) {
@ -155,9 +155,11 @@ export class MessageBasedRenderer {
renderer.setText(renderNode, text);
}
private _listen(renderer: Renderer, renderElement: any, eventName: string) {
renderer.listen(renderElement, eventName, (event) => this._eventDispatcher.dispatchRenderEvent(
private _listen(renderer: Renderer, renderElement: any, eventName: string, unlistenId: number) {
var unregisterCallback = renderer.listen(renderElement, eventName,
(event) => this._eventDispatcher.dispatchRenderEvent(
renderElement, null, eventName, event));
this._renderStore.store(unregisterCallback, unlistenId);
}
private _listenGlobal(renderer: Renderer, eventTarget: string, eventName: string,
@ -168,5 +170,5 @@ export class MessageBasedRenderer {
this._renderStore.store(unregisterCallback, unlistenId);
}
private _listenGlobalDone(renderer: Renderer, unlistenCallback: Function) { unlistenCallback(); }
private _listenDone(renderer: Renderer, unlistenCallback: Function) { unlistenCallback(); }
}

View File

@ -215,10 +215,18 @@ export class WebWorkerRenderer implements Renderer, RenderStoreObject {
[new FnArg(renderNode, RenderStoreObject), new FnArg(text, null)]);
}
listen(renderElement: WebWorkerRenderNode, name: string, callback: Function) {
listen(renderElement: WebWorkerRenderNode, name: string, callback: Function): Function {
renderElement.events.listen(name, callback);
this._runOnService('listen',
[new FnArg(renderElement, RenderStoreObject), new FnArg(name, null)]);
var unlistenCallbackId = this._rootRenderer.allocateId();
this._runOnService('listen', [
new FnArg(renderElement, RenderStoreObject),
new FnArg(name, null),
new FnArg(unlistenCallbackId, null)
]);
return () => {
renderElement.events.unlisten(name, callback);
this._runOnService('listenDone', [new FnArg(unlistenCallbackId, null)]);
};
}
listenGlobal(target: string, name: string, callback: Function): Function {
@ -229,7 +237,7 @@ export class WebWorkerRenderer implements Renderer, RenderStoreObject {
[new FnArg(target, null), new FnArg(name, null), new FnArg(unlistenCallbackId, null)]);
return () => {
this._rootRenderer.globalEvents.unlisten(eventNameWithTarget(target, name), callback);
this._runOnService('listenGlobalDone', [new FnArg(unlistenCallbackId, null)]);
this._runOnService('listenDone', [new FnArg(unlistenCallbackId, null)]);
};
}
}

View File

@ -1267,7 +1267,7 @@ export function main() {
val.changeDetector.dehydrate();
expect(() => {val.changeDetector.detectChanges()})
.toThrowErrorWith("Attempt to detect changes on a dehydrated detector");
.toThrowErrorWith("Attempt to use a dehydrated detector");
expect(val.dispatcher.log).toEqual(['propName=Bob']);
});
});

View File

@ -877,13 +877,22 @@ function declareTests() {
var listener = tc.inject(DirectiveListeningEvent);
expect(listener.msg).toEqual('');
var eventCount = 0;
ObservableWrapper.subscribe(emitter.event, (_) => {
eventCount++;
if (eventCount === 1) {
expect(listener.msg).toEqual('fired !');
fixture.destroy();
emitter.fireEvent('fired again !');
} else {
expect(listener.msg).toEqual('fired !');
async.done();
}
});
emitter.fireEvent('fired !');
});
}));
@ -961,6 +970,11 @@ function declareTests() {
.toEqual(
['domEvent', 'body_domEvent', 'document_domEvent', 'window_domEvent']);
fixture.destroy();
listener.eventTypes = [];
dispatchEvent(tc.nativeElement, 'domEvent');
expect(listener.eventTypes).toEqual([]);
async.done();
});
}));