Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED

Status

()

Core
DOM: Events
VERIFIED FIXED
13 years ago
10 years ago

People

(Reporter: Nickolay_Ponomarev, Assigned: smaug)

Tracking

({testcase})

Trunk
x86
Windows XP
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

13 years ago
Created attachment 177760 [details]
testcase

Comment 2

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

Updated

13 years ago
Blocks: 289175

Comment 3

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

Updated

13 years ago
No longer blocks: 286000
Depends on: 286000
If it was fixing leaks, they might be leaks also fixed by bug 241518.
Depends on: 241518

Comment 5

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

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

Comment 7

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

11 years ago
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
)


Comment 10

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

Comment 11

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

Comment 12

11 years ago
Not broken-by-design but just lazy implementators ;)
Assignee: events → Olli.Pettay
(Assignee)

Comment 13

11 years ago
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...
Attachment #232914 - Flags: review?(bugmail)

Comment 14

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

Comment 15

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

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

Comment 18

11 years ago
Created attachment 233747 [details] [diff] [review]
nsNodeUtils::LastRelease

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)
(Assignee)

Comment 19

11 years ago
Created attachment 233748 [details] [diff] [review]
-w
Attachment #233747 - Flags: review?(bugmail) → review+
(Assignee)

Updated

11 years ago
Attachment #233747 - Flags: superreview?(bzbarsky)

Comment 20

11 years ago
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+
(Assignee)

Comment 21

11 years ago
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
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 22

11 years ago
[Odd, why are popup listeners created using do_CreateInstance when we can create tooltip listeners using new nsXULTooltipListener()?]
Depends on: 349530
(Assignee)

Updated

11 years ago
Blocks: 317553
(Reporter)

Comment 23

11 years ago
Yay, thanks! This is indeed fixed in a recent self-built trunk Firefox on windows.
Status: RESOLVED → VERIFIED

Comment 24

11 years ago
Now that this has baked on the trunk for a while, how risky would it be to port this patch to branch?
(Reporter)

Updated

10 years ago
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.