From 93f26a90d048c7cc06f380ea1edb2b58dfdaa61b Mon Sep 17 00:00:00 2001 From: Andreas Hocevar Date: Tue, 8 Mar 2016 09:30:09 +0100 Subject: [PATCH 1/4] Do not remove listeners while dispatching event --- src/ol/events/eventtarget.js | 35 +++++++++++++++++++------ test/spec/ol/events.test.js | 2 +- test/spec/ol/events/eventtarget.test.js | 15 +++++++++++ 3 files changed, 43 insertions(+), 9 deletions(-) diff --git a/src/ol/events/eventtarget.js b/src/ol/events/eventtarget.js index f2591ef538..81ac20f993 100644 --- a/src/ol/events/eventtarget.js +++ b/src/ol/events/eventtarget.js @@ -1,5 +1,6 @@ goog.provide('ol.events.EventTarget'); +goog.require('goog.asserts'); goog.require('ol.Disposable'); goog.require('ol.events'); goog.require('ol.events.Event'); @@ -26,6 +27,12 @@ ol.events.EventTarget = function() { goog.base(this); + /** + * @private + * @type {!Object.} + */ + this.pendingRemovals_ = {}; + /** * @private * @type {!Object.>} @@ -46,7 +53,7 @@ ol.events.EventTarget.prototype.addEventListener = function(type, listener) { listeners = this.listeners_[type] = []; } if (listeners.indexOf(listener) === -1) { - listeners.unshift(listener); + listeners.push(listener); } }; @@ -64,12 +71,17 @@ ol.events.EventTarget.prototype.dispatchEvent = function(event) { evt.target = this; var listeners = this.listeners_[type]; if (listeners) { - for (var i = listeners.length - 1; i >= 0; --i) { - if (listeners[i].call(this, evt) === false || - evt.propagationStopped) { + this.pendingRemovals_[type] = 0; + for (var i = 0, ii = listeners.length; i < ii; ++i) { + if (listeners[i].call(this, evt) === false || evt.propagationStopped) { return false; } } + var pendingRemovals = this.pendingRemovals_[type]; + delete this.pendingRemovals_[type]; + while (pendingRemovals--) { + this.removeEventListener(type, ol.nullFunction); + } } }; @@ -84,7 +96,7 @@ ol.events.EventTarget.prototype.disposeInternal = function() { /** * Get the listeners for a specified event type. Listeners are returned in the - * opposite order that they will be called in. + * order that they will be called in. * * @param {string} type Type. * @return {Array.} Listeners. @@ -114,9 +126,16 @@ ol.events.EventTarget.prototype.removeEventListener = function(type, listener) { var listeners = this.listeners_[type]; if (listeners) { var index = listeners.indexOf(listener); - listeners.splice(index, 1); - if (listeners.length === 0) { - delete this.listeners_[type]; + goog.asserts.assert(index != -1, 'listener not found'); + if (type in this.pendingRemovals_) { + // make listener a no-op, and remove later in #dispatchEvent() + listeners[index] = ol.nullFunction; + ++this.pendingRemovals_[type]; + } else { + listeners.splice(index, 1); + if (listeners.length === 0) { + delete this.listeners_[type]; + } } } }; diff --git a/test/spec/ol/events.test.js b/test/spec/ol/events.test.js index 6a9f79dd4c..30f6f5f944 100644 --- a/test/spec/ol/events.test.js +++ b/test/spec/ol/events.test.js @@ -214,7 +214,7 @@ describe('ol.events', function() { var key2 = ol.events.listen(target, 'foo', listener, {}); expect(key1.boundListener).to.not.equal(key2.boundListener); expect(target.getListeners('foo')).to.eql( - [key2.boundListener, key1.boundListener]); + [key1.boundListener, key2.boundListener]); }); }); diff --git a/test/spec/ol/events/eventtarget.test.js b/test/spec/ol/events/eventtarget.test.js index 9fd6619a20..af5cbc3add 100644 --- a/test/spec/ol/events/eventtarget.test.js +++ b/test/spec/ol/events/eventtarget.test.js @@ -115,6 +115,21 @@ describe('ol.events.EventTarget', function() { expect(events[0]).to.equal(event); expect(events[0].target).to.equal(eventTarget); }); + it('is safe to remove listeners in listeners', function() { + eventTarget.addEventListener('foo', spy3); + eventTarget.addEventListener('foo', function() { + eventTarget.removeEventListener('foo', spy1); + eventTarget.removeEventListener('foo', spy2); + eventTarget.removeEventListener('foo', spy3); + }); + eventTarget.addEventListener('foo', spy1); + eventTarget.addEventListener('foo', spy2); + expect(function() { + eventTarget.dispatchEvent('foo'); + }).not.to.throwException(); + expect(called).to.eql([3]); + expect(eventTarget.getListeners('foo')).to.have.length(1); + }); }); describe('#dispose()', function() { From acd76a10b69d135b21db86d6bd9d896dbe63dfbb Mon Sep 17 00:00:00 2001 From: Andreas Hocevar Date: Tue, 8 Mar 2016 15:54:17 +0100 Subject: [PATCH 2/4] Clean up when propagation is stopped --- src/ol/events/eventtarget.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ol/events/eventtarget.js b/src/ol/events/eventtarget.js index 81ac20f993..956b698f16 100644 --- a/src/ol/events/eventtarget.js +++ b/src/ol/events/eventtarget.js @@ -70,11 +70,13 @@ ol.events.EventTarget.prototype.dispatchEvent = function(event) { var type = evt.type; evt.target = this; var listeners = this.listeners_[type]; + var propagate; if (listeners) { this.pendingRemovals_[type] = 0; for (var i = 0, ii = listeners.length; i < ii; ++i) { if (listeners[i].call(this, evt) === false || evt.propagationStopped) { - return false; + propagate = false; + break; } } var pendingRemovals = this.pendingRemovals_[type]; @@ -82,6 +84,7 @@ ol.events.EventTarget.prototype.dispatchEvent = function(event) { while (pendingRemovals--) { this.removeEventListener(type, ol.nullFunction); } + return propagate; } }; From 8a279dd94be0ef067c2b9ee1cbc17e701160ef7d Mon Sep 17 00:00:00 2001 From: Andreas Hocevar Date: Tue, 8 Mar 2016 15:54:51 +0100 Subject: [PATCH 3/4] Do not reset pendingRemovals if event is dispatched again --- src/ol/events/eventtarget.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/ol/events/eventtarget.js b/src/ol/events/eventtarget.js index 956b698f16..a32df51575 100644 --- a/src/ol/events/eventtarget.js +++ b/src/ol/events/eventtarget.js @@ -72,7 +72,9 @@ ol.events.EventTarget.prototype.dispatchEvent = function(event) { var listeners = this.listeners_[type]; var propagate; if (listeners) { - this.pendingRemovals_[type] = 0; + if (!(type in this.pendingRemovals_)) { + this.pendingRemovals_[type] = 0; + } for (var i = 0, ii = listeners.length; i < ii; ++i) { if (listeners[i].call(this, evt) === false || evt.propagationStopped) { propagate = false; From 0377e4a32256c1db3aba75e300460897c9dc5319 Mon Sep 17 00:00:00 2001 From: Andreas Hocevar Date: Tue, 8 Mar 2016 16:52:54 +0100 Subject: [PATCH 4/4] Add tests for corner cases --- test/spec/ol/events/eventtarget.test.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/spec/ol/events/eventtarget.test.js b/test/spec/ol/events/eventtarget.test.js index af5cbc3add..2d0cd19653 100644 --- a/test/spec/ol/events/eventtarget.test.js +++ b/test/spec/ol/events/eventtarget.test.js @@ -130,6 +130,23 @@ describe('ol.events.EventTarget', function() { expect(called).to.eql([3]); expect(eventTarget.getListeners('foo')).to.have.length(1); }); + it('is safe to do weird things in listeners', function() { + eventTarget.addEventListener('foo', spy2); + eventTarget.addEventListener('foo', function weird(evt) { + eventTarget.removeEventListener('foo', weird); + eventTarget.removeEventListener('foo', spy1); + eventTarget.dispatchEvent('foo'); + eventTarget.removeEventListener('foo', spy2); + eventTarget.dispatchEvent('foo'); + evt.preventDefault(); + }); + eventTarget.addEventListener('foo', spy1); + expect(function() { + eventTarget.dispatchEvent('foo'); + }).not.to.throwException(); + expect(called).to.eql([2, 2]); + expect(eventTarget.getListeners('foo')).to.be(undefined); + }); }); describe('#dispose()', function() {