286619 - registered event listeners are lost (not firing) after moving XUL elements using DOM appendChild/removeChild methods

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Nickolay_Ponomarev]

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]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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:]

If it was fixing leaks, they might be leaks also fixed by bug 241518.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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]

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

Comment 7

[John Hansen]

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 [:bzbarsky]]

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]

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]

Will this bug possibly be fixed in time for the 2.0 release? Pretty please? =)

Comment 11

[Boris Zbarsky [:bzbarsky]]

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][[e-mail]]]

Not broken-by-design but just lazy implementators ;)

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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 [:bzbarsky]]

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][[e-mail]]]

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 [:bzbarsky]]

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]

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][[e-mail]]]

Attachment: nsNodeUtils::LastRelease

Didn't move anything else to LastRelease. I can do that but in a different bug.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Attachment: -w

Comment 20

[Boris Zbarsky [:bzbarsky]]

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][[e-mail]]]

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

[[e-mail]]

[Odd, why are popup listeners created using do_CreateInstance when we can create tooltip listeners using new nsXULTooltipListener()?]

Comment 23

[Nickolay_Ponomarev]

Yay, thanks! This is indeed fixed in a recent self-built trunk Firefox on windows.

Comment 24

[Philip Chee]

Now that this has baked on the trunk for a while, how risky would it be to port this patch to branch?