Bug 30765 – REGRESSION (r48701): Removing an event listener causes one added after it to not fire

• Component: DOM
• Status: RESOLVED FIXED

[Hanrui]

Created attachment 41850 [details]
Screenshot for your reference.

Other browsers tested:
   Nightly build of Safari: Fail
                  Chrome 4: Fail
  Public Released Safari 4: OK
               Firefox 3.x: OK
                    IE 7/8: OK
                  Chrome 3: OK

What steps will reproduce the problem?
1. Load the url
2. Pay attention to the right area of gray showing "数据载入中..." (The Chinese means "Data loading...")
3. Nightly build of Safari and Chrome 4 cannot load the content of this area while Chrome 3 can (as well as other browsers)

What is the expected result?
Nightly build of Safari and Chrome 4 show the content of the gray area as same as Chrome 3 and other browsers.

What happens instead?
Nightly build of Safari and Chrome 4 cannot load the content in this area.

Please provide any additional information below. Attach a screenshot if possible.
This page adds two event listener to window.onload, and the first one ("cdn_sendData()" ) removes itself in itself.
But this causes Nightly build of Safari and Chrome 4 to remove the second function("loadifrs()") registered to window.onload event unexpectedly.
Chrome 3 and public released Safari 4 have not this bug.

[Hanrui]

Created attachment 41851 [details]
Testcase.

[Mark Rowe (bdash)]

This may be related to bug 29790.

[johnnyding]

According to the testcase Hanrui provided, the bug is because in the first onload event listener, it removed itself from event listeners, but when back to EventTarget::fireEventListeners, the function still re-traversed the listener vector by using old index. Since at this time the index points to new value 0 (which is old index 1), when the for loop continues, the index 0 (old index 1) will be skipped. That causes WebKit misses a event listener handle.

I personally think that if the listener vector was changed in a event listener, we should re-traverse the listener vector in case missing some listeners because of listener order change.

I cooked a patch for fixing this issues without adding new tests. Could anyone give a review? If you are OK with my fix. I will add the test later.
Thanks!

By the way, since now addEventListener logic doesn't notify the increment of listeners like what method removeEventListener notifies for decrement, this patch does not take care the situation about adding new event listeners in a event listener handle.

[johnnyding]

Created attachment 41864 [details]
patch v1 for re-traversing the listener vector when it's changed in a event listener handle

[johnnyding]

Bug 30761 may be related to this bug

[Dimitri Glazkov (Google)]

I think this can be made simpler and without involving an extra stash of listeners. I am working on a patch.

[Darin Adler]

Comment on attachment 41864 [details]
patch v1 for re-traversing the listener vector when it's changed in a event listener handle

Thanks for taking this on!

There is no need to add this extra looping to solve the problem.

A better way to fix this is follow the style already used in the code. We just need to change FiringEventEndIterator to have pointers to both "i" and "end" and have it decrease "i" as well as "end" if the listener being removed is before the index.

There are also some minor style problems with this code: Some of the code is indented two spaces and "old_end" is the wrong style for WebKit.

And the patch needs to include a regression test for the tests directory as well as a bug fix.

[Darin Adler]

(In reply to comment #3)
> By the way, since now addEventListener logic doesn't notify the increment of
> listeners like what method removeEventListener notifies for decrement, this
> patch does not take care the situation about adding new event listeners in a
> event listener handle.

We should make a separate test and separate bug report about this. And make sure we match other browsers in this respect too.

[Geoffrey Garen]

I have a patch for this. Will post after testing.

[Dimitri Glazkov (Google)]

Me too! :)

[Darin Adler]

Oof!

[Dimitri Glazkov (Google)]

Created attachment 41891 [details]
Fix event listener list mutation issues.

[Dimitri Glazkov (Google)]

Here's what I have so far. Still missing:
* expected test results
* nicer ChangeLog descriptions.

[Geoffrey Garen]

Created attachment 41893 [details]
my work in progress - no test case

[Dimitri Glazkov (Google)]

Created attachment 41896 [details]
Fix event listener list mutation issues.

[Dimitri Glazkov (Google)]

Comment on attachment 41891 [details]
Fix event listener list mutation issues.

That wasn't a full patch.

[Geoffrey Garen]

Created attachment 41903 [details]
geoff's patch with dimitri's layout test

Here's a final version of my patch with Dimitri's layout test. Dimitri and I are talking this over on IRC now.

[Geoffrey Garen]

Comment on attachment 41903 [details]
geoff's patch with dimitri's layout test

Posting for review now. I've reviewed the layout test.

[Dimitri Glazkov (Google)]

Comment on attachment 41903 [details]
geoff's patch with dimitri's layout test

r=me on the code.

[Dimitri Glazkov (Google)]

Comment on attachment 41903 [details]
geoff's patch with dimitri's layout test

ggaren can review the test :)

[Geoffrey Garen]

http://trac.webkit.org/changeset/50100

[johnnyding]

Wow, you guys moved really so fast:) and thanks for reviewing my code. I will try to write better patch next time:)

[Darin Adler]

(In reply to comment #22)
> Wow, you guys moved really so fast:) and thanks for reviewing my code. I will
> try to write better patch next time:)

If you'd like to follow up by investigating the situation where someone adds new event listeners in an event listener handler, that would be welcome. I suspect our behavior is already correct and matches other browsers, but it would be good to have a test case even if there's nothing wrong with the code.

[Johnny(Jianning) Ding]

(In reply to comment #23)
> (In reply to comment #22)
> > Wow, you guys moved really so fast:) and thanks for reviewing my code. I will
> > try to write better patch next time:)
> 
> If you'd like to follow up by investigating the situation where someone adds
> new event listeners in an event listener handler, that would be welcome. I
> suspect our behavior is already correct and matches other browsers, but it
> would be good to have a test case even if there's nothing wrong with the code.

Working on that, will file another bug if I find sth unusual

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.