Closed Bug 286619 Opened 19 years ago Closed 18 years ago

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

Categories

(Core :: DOM: Events, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: asqueella, Assigned: smaug)

References

Details

(Keywords: testcase)

Attachments

(3 files, 1 obsolete file)

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+
Attached file testcase
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?
Blocks: 286000
Blocks: 289175
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.
No longer blocks: 286000
Depends on: 286000
If it was fixing leaks, they might be leaks also fixed by bug 241518.
Depends on: 241518
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?
*** Bug 316182 has been marked as a duplicate of this bug. ***
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.
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.
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
)


Will this bug possibly be fixed in time for the 2.0 release? Pretty please? =)
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.
Not broken-by-design but just lazy implementators ;)
Assignee: events → Olli.Pettay
Attached patch v1 (obsolete) — Splinter 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...
Attachment #232914 - Flags: review?(bugmail)
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)?
Blocks: 338595
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.
My point was that the static method you added could be a static (non-virtual) method on nsINode.
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.
Didn't move anything else to LastRelease. I can do that but in a different bug.
Attachment #232914 - Attachment is obsolete: true
Attachment #233747 - Flags: review?(bugmail)
Attachment #232914 - Flags: review?(bugmail)
Attached patch -wSplinter Review
Attachment #233747 - Flags: superreview?(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.
Attachment #233747 - Flags: superreview?(bzbarsky) → superreview+
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.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
[Odd, why are popup listeners created using do_CreateInstance when we can create tooltip listeners using new nsXULTooltipListener()?]
Depends on: 349530
Blocks: 317553
Yay, thanks! This is indeed fixed in a recent self-built trunk Firefox on windows.
Status: RESOLVED → VERIFIED
Now that this has baked on the trunk for a while, how risky would it be to port this patch to branch?
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.