174320 - When an event handler removes itself the next handler is skipped

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[[e-mail]]

Using Build ID: 2002100920
Many Mozilla components and extensions need to initialize their dynamic overlays
which is typically done using addEventListener("load", myInitFunc, false); but
if myInitFunc contains a call to removeEventListener("load", myInitFunc, false);
the following event handler gets skipped.

Comment 1

[[e-mail]]

Attachment: Test case

This test case uses the "command" event which is easier to demonstrate.
Steps to reproduce problem:
1. Click the "Add" button three times. (OK each alert)
2. Click the "Test" button. OK each alert.
3. Click the "Test" button.

Expected results: 3 alerts at step 2

Actual results: alerts 1 & 3 at step 2; alert 2 at step 3.

Comment 2

[Brian Ryner (not reading)]

taking

Comment 3

[Jens Bannmann]

Note (might be obvious from Neil's comment though): this bug will not occur if
you pass true for addEventListener's capture parameter.

Comment 4

[[e-mail]]

Not true, the test case fails for capturing events too. Note that in the real
world capturing event handlers get hit more frequently so the effect is less
noticable.

Comment 5

[Dorando]

Attachment: Hmm, it actually seems that the next handler is skipped if a handler is removed which was already called in this event phase.

Comment 6

[[e-mail]]

Hixie, do you know what correct behaviour is?

Comment 7

[Hixie (not reading bugmail)]

Did you check the spec before asking me? :-)
   http://www.w3.org/TR/DOM-Level-3-Events/events.html#Events-listeners-removal

Comment 8

[[e-mail]]

Hixie, this bug is about listeners that *aren't* removed...

Comment 9

[Hixie (not reading bugmail)]

Why would the listeners that aren't removed be in any way affected?

Comment 10

[Jens Bannmann]

Obvious answer, Hixie: because it's a *bug* in Mozilla.

Comment 11

[Hixie (not reading bugmail)]

Jens: I was replying to comment 6, and was asking why the _spec_ would say
anything about other listeners.

Comment 12

[MW]

this really *should* be fixed as extensions become more & more popular and it
affects Mozilla Suite as well as FireFox

Comment 13

[Jordan Callicoat]

(In reply to comment #5)

By implication, then, removing the event on the opposite phase should not
manifest the bug (which seems to hold true -- change the third
removeEventListener param in your test-case to false). Add on capture, remove on
bubble (or vice versa) -- then everything seems to work as expected. I guess
this could be seen as a work-around (for most cases) until the bug is squished.

Comment 14

[Jordan Callicoat]

Oops, that should have said:

"...change the third removeEventListener param in your test-case to true..."

Comment 15

[Jeremy Priest]

Apparently these are side effects that I have been running into constantly on
the Avairy build of FireFox (nightlies)... I hope no one minds me posting this
here since I'm very unfamiliar with Bugzilla and appropriateness.

Reference post in EMButtons Extension Thread (also relates to multiple extensions.)
http://forums.mozillazine.org/viewtopic.php?p=724617#724617

Put here for your ease to read...

My post:

Note: Sort is not being used so it is not an event handler bug.

Four Conflicts that I've come across with EMButtons and Three Different
Extensions and FireFox.

First conflict is made reference to here which involves WeatherFox (any version
(tested this)) and as far as I know any version of EMButtons (tested with 0.9.0
and 0.10.1).
(The example is with a developer version of WeatherFox 0.2.5 (found in the
weatherfox.mozdev.org/source.html (through the cvs) (but it conflicts with 0.2.1
as well) and EMButtons 0.10.1)

Second conflict deals with Window-Q 0.1 d11 (metaphor) pass10 what happens in
short is:
When you install EMButtons First and the Install Window-Q 0.1 d11 (metaphor)
pass10 found in the Window-Q thread it causes EMButtons to not use the concise
or performace options. Even if they weren't set before hand. If they were set
before hand they lose them immediately and turning them on or off will not
change anything. So you have to install Window-Q First and then EMButtons.

Third conflict --- I have only been able to get the short-cut overlays and
functions to work in one way... EVER. I have to install Ad-Block first then
installing EMButtons anytime after that and it will work. No matter what. If
Ad-Block is not installed first EMButtons will never retrieve its overlay
functions. Neutral

And Fourth conflict, only when EMButtons is set as a Sidebar or Tab will this
second buggy Extension Manager Window pop up.

I am trying hard not to make a fuss about these these bugs but they are quite
annoying when you Keep running into them.

Moonwolf's reply:

here is no event handler involved in the sorting, not sure why you thought that.
First: As crafteh said in the WeatherFox thread, this is his fault. There's
nothing I can do about it.
Second: Event handler bug. Window-Q is removing an event handler and stopping
the one which applies the enhanced modes in EMbuttons from being loaded. Firefox
loads extensions in reverse order.
Third: Event handler bug. An event handler applies the shortcut keys to the
Tools menu. Have you voted for the bug and posted your problems to it? If it's
irritating you this much you really should.
Fourth: As I said before, the code which loads the EM after a drag and drop is
well hidden. I don't have an unlimited amount of time to work on EMbuttons, and
it's entirely possible that this event is coded in a way that won't allow me to
alter it. Fixing this bug is low priority because it isn't doing any real harm.

Summary/Conclusion:

I find the conflicts listed in my post to extremly annoying and have to
selectively install my extensions in the correct way otherwise I will have
improper installations. I believe many people could run into this and I hope
that this really would be considered fully.

Comment 16

[James Ross]

Whoops, I seem to have fixed this. Patch in a sec.

Comment 17

[James Ross]

Attachment: Use a clone of the list + some magic instead of the live list

Rather than simply looping through the live list of listeners (which causes us
to break in a bunch of interesting ways), this patch clones the list and walks
that instead.

Now, if the DOM Spec didn't want us to care about deleted listeners, that would
be it - but it does, so the code checks it's "in sync" with the live list, and
if it isn't, resyncs itself. All the resync is doing is finding the next
(starting with the current) listener that is still in the live list. This means
additions are ignored and deletions are followed, as per spec.

With this patch both the test cases on this bug pass, as do two test cases I
wrote to check the spec (one for addition, one for deletion). Since event
listeners added during processing are NOT executed after this change, it may
cause some regressions.

Comment 18

[[e-mail]]

Comment on attachment 161541 [details] [diff] [review]
Use a clone of the list + some magic instead of the live list

>+      // Clone the listener array.
>+      nsVoidArray *safeListeners = new nsVoidArray(listeners->Count());
You never free this. Possibly you should create an auto variable instead of a
pointer i.e. nsVoidArray safelisteners(listeners->Count());

>+          while (k < safeListeners->Count()) {
>+            for (int l = 0; listeners && l < listeners->Count(); l++) {
>+              if (listeners->ElementAt(l) == safeListeners->ElementAt(k)) {
>+                synced = PR_TRUE;
>+                break;
>+              }
>+            }
You might want to use listeners->IndexOf to see if the listener still exists.
In fact, if (listeners->IndexOf(ls) == -1) continue; might even work. But I'm
worried that deleting a listener might leave a dangling pointer in your clone.

>+        nsListenerStruct* ls = NS_STATIC_CAST(nsListenerStruct*, safeListeners->ElementAt(k));

Comment 19

[James Ross]

(In reply to comment #18)
> (From update of attachment 161541 [details] [diff] [review])
> >+      // Clone the listener array.
> >+      nsVoidArray *safeListeners = new nsVoidArray(listeners->Count());
> You never free this. Possibly you should create an auto variable instead of a
> pointer i.e. nsVoidArray safelisteners(listeners->Count());

Point, it occured to me when I went to bed that I probably wasn't freeing it
(though it was emptying the array before leaking the object ;) ).

> >+          while (k < safeListeners->Count()) {
> >+            for (int l = 0; listeners && l < listeners->Count(); l++) {
> >+              if (listeners->ElementAt(l) == safeListeners->ElementAt(k)) {
> >+                synced = PR_TRUE;
> >+                break;
> >+              }
> >+            }
> You might want to use listeners->IndexOf to see if the listener still exists.
> In fact, if (listeners->IndexOf(ls) == -1) continue; might even work. But I'm
> worried that deleting a listener might leave a dangling pointer in your clone.

Well, it will do - that's how it can tell if it still needs to call it.

I can attempt a much more complicated version that keeps track of exactly where
it is in the list using a private field that's updated as the list changes, if
that would be better.

Comment 20

[James Ross]

Attachment: Updated patch

Not sure the |safeListeners = nsnull;| line is nessessary, or how I missed
|IndexOf| in nsVoidArray.h before, but this is the patch with changes and still
works. :)

Comment 21

[James Ross]

Attachment: Testcase for removing |this| listener

Comment 22

[James Ross]

Attachment: Testcase for adding new listener

Comment 23

[[e-mail]]

Attachment: Reduced patch

I believe this should have the same effect as attachment 161595 [details] [diff] [review].

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 161618 [details] [diff] [review]
Reduced patch

+      nsVoidArray originalListeners(count);
+      originalListeners = *listeners;

That should be an nsAutoVoidArray to avoid mostly unnecessary malloc overhead.

Comment 25

[Michael Kraft [:morac]]

Is there plans to put the patch (which needs to be updated per the previous
comment) into the core any time soon?  This really should be fixed ASAP as it
can cause extensions to conflict with each other.

Comment 26

[Michael Kraft [:morac]]

Also if the bug is truely in /content/events/src/nsEventListenerManager.cpp then
I think the Component should be listed as "DOM: Events".  As it is currently
set, the bug does not show up in the list Event Bugs.

Comment 27

[Michael Kraft [:morac]]

Ignore my last comment, it accidently got sent when I did not mean for it to.

Comment 28

[Nickolay_Ponomarev]

Asking for blocking-aviary1.1. This bug almost has a patch and it is one of main
causes of extensions conflicts as far as I can tell from reading the MZ forums.

Comment 29

[Michael Kraft [:morac]]

(In reply to comment #28)
> Asking for blocking-aviary1.1. This bug almost has a patch and it is one of main
> causes of extensions conflicts as far as I can tell from reading the MZ forums.

If it blocks 1.1 then I think it should also block 1.8b since the problem exists
in the 1.8 baseline (which will become Mozilla 1.8, Firefox 1.1 and Thunderbird
1.1) and like you said it is nearly has a patch.

See http://www.mozillazine.org/talkback.html?article=6004

Comment 30

[Asa Dotzler [:asa]]

Johnny, what do you think about this one? Can you help get it in for 1.8b? 

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

too late for 1.8b, transferring request to b2, and requesting review from jst

Comment 32

[Johnny Stenback (:jst)]

Comment on attachment 161618 [details] [diff] [review]
Reduced patch

+      nsVoidArray originalListeners(count);

Make that nsAutoVoidArray.

r+sr=jst

Comment 33

[[e-mail]]

I couldn't, they're not assignment compatible.

Comment 34

[Nickolay_Ponomarev]

Thanks James and Neil very much!

Verified using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b)
Gecko/20050218 Firefox/1.0+ - all four testcases pass.

(I would like to reiterate that this patch also fixed another problem not
mentioned in Summary or in the original report - listeners added with
addEventListener while a listener is running will not be called until the event
finishes processing - see attachment 161616 [details] (fourth testcase) and
http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-EventTarget-addEventListener)

Comment 35

[Nickolay_Ponomarev]

*** Bug 290336 has been marked as a duplicate of this bug. ***

Comment 36

[nrlz]

I'm getting this too on Firefox 1.0.3.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414
Firefox/1.0.3

Comment 37

[Jens Bannmann]

(In reply to comment #36)
> I'm getting this too on Firefox 1.0.3.

That's normal - the bug was fixed on the development trunk, meaning it only will
be in Firefox 1.1. New Firefox 1.0.x releases do not pick up all fixes
automatically.