The default bug view has changed. See this FAQ.

When an event handler removes itself the next handler is skipped

VERIFIED FIXED

Status

()

Core
Event Handling
--
critical
VERIFIED FIXED
15 years ago
11 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
Points:
---
Bug Flags:
blocking1.7 -
blocking-aviary1.0 -
blocking1.8b -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments, 1 obsolete attachment)

(Assignee)

Description

15 years ago
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.
(Assignee)

Comment 1

15 years ago
Created attachment 102778 [details]
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.
taking
Assignee: joki → bryner

Updated

15 years ago
QA Contact: rakeshmishra → trix

Comment 3

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

Comment 4

14 years ago
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

13 years ago
Created attachment 141310 [details]
Hmm, it actually seems that the next handler is skipped if a handler is removed which was already called in this event phase.
(Assignee)

Comment 6

13 years ago
Hixie, do you know what correct behaviour is?
Did you check the spec before asking me? :-)
   http://www.w3.org/TR/DOM-Level-3-Events/events.html#Events-listeners-removal
(Assignee)

Comment 8

13 years ago
Hixie, this bug is about listeners that *aren't* removed...
Why would the listeners that aren't removed be in any way affected?

Comment 10

13 years ago
Obvious answer, Hixie: because it's a *bug* in Mozilla.
Jens: I was replying to comment 6, and was asking why the _spec_ would say
anything about other listeners.

Comment 12

13 years ago
this really *should* be fixed as extensions become more & more popular and it
affects Mozilla Suite as well as FireFox
Flags: blocking1.7+

Comment 13

13 years ago
(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

13 years ago
Oops, that should have said:

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

Updated

13 years ago
Flags: blocking1.7+

Updated

13 years ago
Flags: blocking1.7+

Updated

13 years ago
Flags: blocking1.7+ → blocking1.7?

Updated

13 years ago
Flags: blocking1.7? → blocking1.7-

Updated

13 years ago
Flags: blocking-aviary1.0?

Comment 15

13 years ago
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.

Updated

13 years ago
Flags: blocking-aviary1.0? → blocking-aviary1.0-

Comment 16

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

Comment 17

13 years ago
Created attachment 161541 [details] [diff] [review]
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.
(Assignee)

Comment 18

13 years ago
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

13 years ago
(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

13 years ago
Created attachment 161595 [details] [diff] [review]
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. :)
Attachment #161541 - Attachment is obsolete: true

Comment 21

13 years ago
Created attachment 161615 [details]
Testcase for removing |this| listener

Comment 22

13 years ago
Created attachment 161616 [details]
Testcase for adding new listener
(Assignee)

Comment 23

13 years ago
Created attachment 161618 [details] [diff] [review]
Reduced patch

I believe this should have the same effect as attachment 161595 [details] [diff] [review].
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.

Updated

12 years ago
Assignee: bryner → neil.parkwaycc.co.uk
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.
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.
Ignore my last comment, it accidently got sent when I did not mean for it to.

Comment 28

12 years ago
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.
Flags: blocking-aviary1.1?
(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
Flags: blocking1.8b?

Comment 30

12 years ago
Johnny, what do you think about this one? Can you help get it in for 1.8b? 
Attachment #161618 - Flags: review?(jst)
too late for 1.8b, transferring request to b2, and requesting review from jst
Flags: blocking1.8b?
Flags: blocking1.8b2?
Flags: blocking1.8b-
Comment on attachment 161618 [details] [diff] [review]
Reduced patch

+      nsVoidArray originalListeners(count);

Make that nsAutoVoidArray.

r+sr=jst
Attachment #161618 - Flags: superreview+
Attachment #161618 - Flags: review?(jst)
Attachment #161618 - Flags: review+
(Assignee)

Comment 33

12 years ago
I couldn't, they're not assignment compatible.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 34

12 years ago
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)
Status: RESOLVED → VERIFIED
Flags: blocking-aviary1.1?

Updated

12 years ago
Flags: blocking1.8b2?

Comment 35

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

Comment 36

12 years ago
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

12 years ago
(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.
Blocks: 305217
You need to log in before you can comment on or make changes to this bug.