removeEventListener doesn't handle currently dispatching events #84

Sebmaster · 2015-09-27T08:53:18Z

It seems like browsers remove an event listener even from the currently dispatched event listener list. Explaining it is kinda complicated, but here's a test which fails with an implementation that is (I hope) according to spec:

    var es = [];
    var ls = [];
    var EventListener1 = function() { ls.push(this); }
    var EventListener2 = function() { ls.push(this); }
    EventListener1.prototype.handleEvent = function(event) { _handleEvent(event); }
    EventListener2.prototype.handleEvent = function(event) { _handleEvent(event); }
    var _handleEvent = function(event) {
      es.push(event);
      ls.forEach(function(l){
        event.currentTarget.removeEventListener("foo", l.handleEvent, false);
      })
    }
    // test
    var listener1 = new EventListener1();
    var listener2 = new EventListener2();
    document.addEventListener("foo", listener1.handleEvent, false);
    document.addEventListener("foo", listener2.handleEvent, false);
    var event = document.createEvent("Events");
    event.initEvent("foo",true,false);
    document.dispatchEvent(event);
    console.log(es.length); // returns 1 in Chrome + FF

So, both listeners remove themselves + the other listener, which seems to cause the currently dispatched event to only get to 1 listener since the other one is removed.

In event listener invoke the spec says to copy the event listener list, which should cause it to not be impacted by changes from removeEventListener (I'd assume).

ArkadiuszMichalski · 2015-09-27T10:30:06Z

Hi, some times ago I noticed the same thing, although its correct in IE:
https://bugzilla.mozilla.org/show_bug.cgi?id=906608

Ms2ger · 2015-09-27T14:25:42Z

Somewhat more readable:

<!DOCTYPE html>
<script>
var es = [];
var ls = [
  function(event) { _handleEvent(event); },
  function(event) { _handleEvent(event); },
];
var _handleEvent = function(event) {
  es.push(event);
  ls.forEach(function(l) {
    document.removeEventListener("foo", l);
  })
};
ls.forEach(function(l) {
  document.addEventListener("foo", l);
});

var event = new Event("foo");
document.dispatchEvent(event);
w(es.length); // returns 1 in Chrome + FF
</script>

Ms2ger · 2015-09-27T14:26:35Z

Servo returns 2.

annevk · 2015-09-27T16:38:32Z

I'm pretty sure this was tested. That's why the whole copy clause is there. I wonder what's up.

bzbarsky · 2015-09-28T14:25:56Z

@annevk I thought the copy was to avoid invoking newly added listeners? The behavior in Gecko is certainly that listeners added to the current target during dispatch to one of its listeners are NOT invoked, but listeners removed from it will also not be invoked if they haven't been already. We don't make a list copy internally; we use an iterator along a live list that stops at what used to be the list end when the iterator started.

annevk · 2015-09-28T15:35:53Z

Ah that makes sense. Not entirely sure how to describe iterator-like behavior in prose though. I guess I could "If any event listener is removed, also remove it from listeners" but that seems like a cop-out.

annevk · 2015-11-11T12:05:11Z

The best I can think of is that rather than a copy we create a list of pointers. And then when a listener is removed some kind of "removed flag" gets set that is checked by the invoke algorithm. The concept of iteration over a live list isn't really defined (ideally IDL would define some constructs for us).

Would that work @bzbarsky?

bzbarsky · 2015-11-11T15:41:19Z

That's starting to sound like the setup ES uses for Map and Set, which works as follows:

There is a list of things, each of which is either an entry or a removed-entry marker (e.g. in the case of Map the removed-entry marker is an entry record with [[key]] set to empty; in our case the removed-entry marker could be something like storing an EventListener? and treating null as the removed-entry marker). Operations on the list skip over removed-entry markers, and the removed-entry markers are never explicitly observed (this is important). As a spec device the list never shrinks. Removal of an entry simply replaces it in the list with a removed-entry marker. Implementations, of course, would never implement it this way, but are free to do anything that produces equivalent observable behavior (e.g. having some way to notify all live iterators when a removal happens and actually removing from the list).

It's not quite exactly what we want here, because we want the event dispatch iteration to not consider newly added stuff. But we could do that by grabbing the index to stop at up front in the dispatch and then stopping there, I think.

annevk · 2015-11-11T17:07:00Z

The way to not consider new stuff was to iterate once and only store pointers for the stuff currently in the list. So you wouldn't have any pointers to newly added listeners.

bzbarsky · 2015-11-11T17:37:31Z

Sure, that seems equivalent as long as removal marks the thing the pointer points to as "not a listener anymore" or something. Which specification device is clearer/simpler, I dunno.

annevk · 2015-11-12T11:05:05Z

Thank you @Sebmaster for reporting this. I forgot to add your name in this commit, but that'll be sorted in a future one. Hope that's okay.

Sebmaster · 2015-11-12T14:04:05Z

Sure, no big deal.

annevk · 2015-11-12T14:06:27Z

It's been sorted by the way, have a look: https://dom.spec.whatwg.org/#acks.

Sebmaster · 2015-11-12T15:32:58Z

Sweet 👍 Now let's have a look at this HTML spec... 😄

annevk closed this as completed in 02710dd Nov 12, 2015

ArkadiuszMichalski mentioned this issue Nov 12, 2015

D3E and corelation with DOM4(W3C)/DOM(WHATWAG) and DOM3 Core w3c/uievents#19

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

removeEventListener doesn't handle currently dispatching events #84

removeEventListener doesn't handle currently dispatching events #84

Sebmaster commented Sep 27, 2015

ArkadiuszMichalski commented Sep 27, 2015

Ms2ger commented Sep 27, 2015

Ms2ger commented Sep 27, 2015

annevk commented Sep 27, 2015

bzbarsky commented Sep 28, 2015

annevk commented Sep 28, 2015

annevk commented Nov 11, 2015

bzbarsky commented Nov 11, 2015

annevk commented Nov 11, 2015

bzbarsky commented Nov 11, 2015

annevk commented Nov 12, 2015

Sebmaster commented Nov 12, 2015

annevk commented Nov 12, 2015

Sebmaster commented Nov 12, 2015

removeEventListener doesn't handle currently dispatching events #84

removeEventListener doesn't handle currently dispatching events #84

Comments

Sebmaster commented Sep 27, 2015

ArkadiuszMichalski commented Sep 27, 2015

Ms2ger commented Sep 27, 2015

Ms2ger commented Sep 27, 2015

annevk commented Sep 27, 2015

bzbarsky commented Sep 28, 2015

annevk commented Sep 28, 2015

annevk commented Nov 11, 2015

bzbarsky commented Nov 11, 2015

annevk commented Nov 11, 2015

bzbarsky commented Nov 11, 2015

annevk commented Nov 12, 2015

Sebmaster commented Nov 12, 2015

annevk commented Nov 12, 2015

Sebmaster commented Nov 12, 2015