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...
Status: VERIFIED FIXED
: 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] (vacation Aug 25-28)
: Hixie (not reading bugmail)
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
nsNodeUtils::LastRelease (21.26 KB, patch)
2006-08-15 02:04 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
jonas: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
-w (19.40 KB, patch)
2006-08-15 02:04 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review

Description 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
either.

See the testcase for steps for reproduce etc.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050311
Firefox/1.0+
Comment 1 Nickolay_Ponomarev 2005-03-17 12:52:55 PST
Created attachment 177760 [details]
testcase
Comment 2 Boris Zbarsky [:bz] 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 Boris Zbarsky [:bz] 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 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-04-07 09:41:57 PDT
If it was fixing leaks, they might be leaks also fixed by bug 241518.
Comment 5 Boris Zbarsky [:bz] 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 Adam Guthrie 2005-11-26 19:51:42 PST
*** Bug 316182 has been marked as a duplicate of this bug. ***
Comment 7 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 (http://www.w3.org/TR/2002/WD-DOM-Level-3-Events-20020208/changes.html) 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 Boris Zbarsky [:bz] 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 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

http://dev.w3.org/cvsweb/~checkout~/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html?content-type=text/html;%20charset=iso-8859-1
)


Comment 10 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 Boris Zbarsky [:bz] 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 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-08-07 21:52:31 PDT
Not broken-by-design but just lazy implementators ;)
Comment 13 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-08-09 08:45:33 PDT
Created attachment 232914 [details] [diff] [review]
v1

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 Boris Zbarsky [:bz] 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 Olli Pettay [:smaug] (vacation Aug 25-28) 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 Boris Zbarsky [:bz] 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 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 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-08-15 02:04:26 PDT
Created attachment 233747 [details] [diff] [review]
nsNodeUtils::LastRelease

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

>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 Olli Pettay [:smaug] (vacation Aug 25-28) 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
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/xul/base/src&command=DIFF_FRAMESET&file=nsXULTooltipListener.cpp&rev1=1.50&rev2=1.51&root=/cvsroot
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 neil@parkwaycc.co.uk 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 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 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.