Last Comment Bug 286619 - registered event listeners are lost (not firing) after moving XUL elements using DOM appendChild/removeChild methods
: registered event listeners are lost (not firing) after moving XUL elements us...
: testcase
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: x86 Windows XP
-- normal with 4 votes (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: Hixie (not reading bugmail)
: Andrew Overholt [:overholt]
Depends on: 241518 286000 349530
Blocks: 289175 317553 338595
  Show dependency treegraph
Reported: 2005-03-17 12:50 PST by Nickolay_Ponomarev
Modified: 2007-10-09 01:14 PDT (History)
18 users (show)
asqueella: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (1.34 KB, application/vnd.mozilla.xul+xml)
2005-03-17 12:52 PST, Nickolay_Ponomarev
no flags Details
v1 (20.78 KB, patch)
2006-08-09 08:45 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
nsNodeUtils::LastRelease (21.26 KB, patch)
2006-08-15 02:04 PDT, Olli Pettay [:smaug]
jonas: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
-w (19.40 KB, patch)
2006-08-15 02:04 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review

Description User image Nickolay_Ponomarev 2005-03-17 12:50:51 PST
Suppose you have a XUL element with an event listener registered on it via
addEventListener. Everything works fine (the listener fires whenever an
appropriate event is dispatched) until you decide to move this element using DOM
manipulation methods.

After you move it (using removeChild/appendChild), the event listeners seem to
get lost - they no longer fire when they should.

I'm not particularly good at reading specs, but from what I've seen in DOM 2
events, it shouldn't be like this. What made me file this bug is that when you
manipulate HTML elements in the same way, listeners registered on them are not lost.

Handlers specified with on... attributes (e.g. onclick="alert(1)") are not lost

See the testcase for steps for reproduce etc.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050311
Comment 1 User image Nickolay_Ponomarev 2005-03-17 12:52:55 PST
Created attachment 177760 [details]
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2005-03-30 21:42:23 PST
Basically, XUL elements nuke their event listener manager when they get
SetDocument(nsnull) called on them.  This seems very wrong to me (and I have XXX
comments next to that code in my build as a result -- see patch in bug 286000)...

Does anyone know why this was done?  Is it supposed to fix leaks or something? 
Can we just make XUL match whatever HTML does without the world exploding?
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2005-04-06 10:37:04 PDT
Note that if we fixed this we could probably also change the AddListenerFor
thing in BindToTree to only happen when the _owner_ document (not current
document) changes.
Comment 4 User image David Baron :dbaron: ⌚️UTC-8 2005-04-07 09:41:57 PDT
If it was fixing leaks, they might be leaks also fixed by bug 241518.
Comment 5 User image Boris Zbarsky [:bz] (still a bit busy) 2005-04-12 16:53:16 PDT
Note that AddListenerFor also does popup listeners, which probably _should_
depend on the current document....  Perhaps the first step would be splitting
those apart from event listeners that are attached to the node by compiling
certain attributes?
Comment 6 User image Adam Guthrie 2005-11-26 19:51:42 PST
*** Bug 316182 has been marked as a duplicate of this bug. ***
Comment 7 User image John Hansen 2006-04-12 13:22:17 PDT
If it's not possible to patch this in time for Fx 2.0, perhaps the DOM Level 3 eventListenerList attribute could be implemented before the spec ( is complete?

This way we could at least duplicate the events that were lost after removeChild/appendChild operations, while not undoing fixes for the aforementioned memory leak.
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2006-04-13 11:59:58 PDT
I don't think implementing pre-CR stuff like that is a good idea at all.  Quite apart from the API changes it would entail on an API-stable branch.
Comment 9 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-13 14:03:55 PDT
As proven by the fact that it is not in the latest draft for DOM 3 Ev

(you can look at the latest version at the url below. But note that it's not a published draft, just the latest thing in cvs;%20charset=iso-8859-1

Comment 10 User image John Hansen 2006-08-07 13:11:13 PDT
Will this bug possibly be fixed in time for the 2.0 release? Pretty please? =)
Comment 11 User image Boris Zbarsky [:bz] (still a bit busy) 2006-08-07 17:23:23 PDT
Almost certainly not -- this would be a very risky change, sadly.  It's kinda unfortunate that so much of XUL is broken-by-design, but there we are.
Comment 12 User image Olli Pettay [:smaug] 2006-08-07 21:52:31 PDT
Not broken-by-design but just lazy implementators ;)
Comment 13 User image Olli Pettay [:smaug] 2006-08-09 08:45:33 PDT
Created attachment 232914 [details] [diff] [review]

This keeps the ELM alive when XUL elements are moved.
Tooltip and popuplisteners are kept in node properties (to prevent adding them 
twice etc). But to make that work properly, I had to fix also properties 
handling so that properties for a node are deleted when the node is still properly alive - that is the reason for doLastRelease.

Is there still something I didn't think about...
Comment 14 User image Boris Zbarsky [:bz] (still a bit busy) 2006-08-12 10:16:14 PDT
Would it make more sense to put this Destroy method in nsINode instead?

And could we move more of ~nsINode into there (and thus make sure to fix bug 338595)?
Comment 15 User image Olli Pettay [:smaug] 2006-08-14 01:02:46 PDT
We could move more things to Destroy/LastRelease.

The reason why I didn't add nsINode::LastRelease was that I didn't want to add 
a virtual method.
Comment 16 User image Boris Zbarsky [:bz] (still a bit busy) 2006-08-14 07:56:24 PDT
My point was that the static method you added could be a static (non-virtual) method on nsINode.
Comment 17 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-08-14 17:37:06 PDT
Yes, please move the function to nsINode:: or nsNodeUtils::. I actually like the idea of putting it on nsNodeUtils:: since that way we can merge it with the nsNodeUtils::NodeWillBeDestroyed call.

Also, please attach a -w version.
Comment 18 User image Olli Pettay [:smaug] 2006-08-15 02:04:26 PDT
Created attachment 233747 [details] [diff] [review]

Didn't move anything else to LastRelease. I can do that but in a different bug.
Comment 19 User image Olli Pettay [:smaug] 2006-08-15 02:04:58 PDT
Created attachment 233748 [details] [diff] [review]
Comment 20 User image Boris Zbarsky [:bz] (still a bit busy) 2006-08-15 21:23:54 PDT
Comment on attachment 233747 [details] [diff] [review]

>Index: content/xul/content/src/nsXULElement.cpp
>+PopupListenerPropertyDtor(void* aObject, nsIAtom* aPropertyName,
>+  nsCOMPtr<nsIDOMEventTarget> target =
>+    do_QueryInterface(NS_STATIC_CAST(nsIContent*, aObject));

s/nsIContent/nsINode/ here, please.

>     target->AddEventListener(NS_LITERAL_STRING("mousedown"), eventListener, PR_FALSE);
>     target->AddEventListener(NS_LITERAL_STRING("contextmenu"), eventListener, PR_FALSE);
>+    rv = SetProperty(nsXULAtoms::popuplistener, popupListener,
>+                     PopupListenerPropertyDtor);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    NS_ADDREF(popupListener);

I think you should do the SetProperty before calling AddEventListener.

>Index: layout/xul/base/src/nsRootBoxFrame.cpp
> nsRootBoxFrame::AddTooltipSupport(nsIContent* aNode)
>+  listener = new nsXULTooltipListener();

I'd use an nsRefPtr here; see below.

>+  if (NS_SUCCEEDED(listener->Init(aNode))) {

If Init() fails you leak |listener|.

>+    nsresult rv = aNode->SetProperty(nsXULAtoms::tooltiplistener, listener,

If this fails you leak |listener|.

sr=bzbarsky with those nits fixed.
Comment 21 User image Olli Pettay [:smaug] 2006-08-16 07:38:30 PDT
Checked in with 2 fixes related to NS_ADDREF (have to use a real reference, not
nsCOMPtr/nsRefPtr). I also changed one NS_ERROR to NS_WARNING
to fix balsa orange. Even NS_WARNING shouldn't be needed. NS_ERROR was wrong anyway.
tp/tp2/txul/tdhtml/ts look all ok to me.
Balsa doesn't report any new leaks.
Comment 22 User image 2006-08-16 07:52:44 PDT
[Odd, why are popup listeners created using do_CreateInstance when we can create tooltip listeners using new nsXULTooltipListener()?]
Comment 23 User image Nickolay_Ponomarev 2006-09-06 06:14:52 PDT
Yay, thanks! This is indeed fixed in a recent self-built trunk Firefox on windows.
Comment 24 User image Philip Chee 2007-03-17 05:51:31 PDT
Now that this has baked on the trunk for a while, how risky would it be to port this patch to branch?

Note You need to log in before you can comment on or make changes to this bug.