83635 - XBL binding not deleted on removal from document tree

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Tim Dewhirst]

A problem has been noted with regards to removing an XBL binding from a 
document.

If a simple binding is constructed, which simply dumps a message on 
construction and destruction, and added to the document and subsequently 
deleted, the destructor is not called. 

The destructor is only called at browser shutdown if the binding is not removed 
from the tree.

A test case attachment follows.

Comment 1

[Tim Dewhirst]

Attachment: test case (.xul/.xml/.css)

Comment 2

[Peter Trudelle]

->moz1.0

Comment 3

[Boris Zbarsky [:bzbarsky]]

So the basic problem is that we only detach bindings when a document is unloaded, and only for those bindings that are in the binding manager's table.  When we remove a node from the document, we null out the binding in the document's binding manager and nuke the anonymous content, but do NOT call BindingDetached().

Frankly, I think we should just call BindingDetached either in that code, or in ~nsXBLBinding. Thoughts?

Note that this bug causes various memory leaks if people removeChild a node with a binding attached to it; we have workarounds for some of these in our tabbrowser bindings last I checked...

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is there a risk that that would break a bunch of XUL because we'd behave differently for nodes that are moved out of the tree?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Maybe.  If people remove nodes from the DOM and then call XBL methods on them, the methods will not work.  Right now they sorta kinda work, maybe.

One more note: our current behavior is that if you remove a node from the DOM we do nothing. If you reinsert it, we fire the constructor (a second time, with no destructor firing in between).  Whether you reinsert it or not, if the destructor did any sort of cleanup we probably leak the node (more likely if you don't reinsert)....

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Marking wanted for fixing any mlk involved; correctness isn't blocking 1.9.

Comment 7

[Asaf Romano (gone)]

(In reply to comment #5)
> Maybe.  If people remove nodes from the DOM and then call XBL methods on them,
> the methods will not work.  Right now they sorta kinda work, maybe.
> 

No.  It does not work.  Although the binding wasn't destroyed, one cannot access the binding implementation after the node is removed (see my patch on bug 528006).

IMO, this negates the compatibility argument for keeping this behavior until XBL2.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> No.  It does not work.

Indeed; that got changed in bug 398135.

Comment 9

[Asaf Romano (gone)]

OK.  What sort of compatibility issue remains here then?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Probably none.

Comment 11

[Asaf Romano (gone)]

Re-evauating for blocking then.

Drivers: This bug is causing various leaks related to toolbar customization, along with non-trivial workarounds, like the one in 528006.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

jst, you marked this beta3+ 10 days ago, no progress since then. Do we expect this to hit code freeze on Monday, Aug 2, 23:00 PT?

Comment 13

[Johnny Stenback (:jst)]

No. Moving to beta4.

Comment 14

[Shawn Wilsher :sdwilsh]

This isn't a regression from anything remotely recent, and we still have no progress on it AFAICT.  It's not clear to me why this should block.  At a maximum it feels like we just need beta coverage, but I'm leaning to "not a blocker".  Setting to betaN for now, but please justify why this is blocking.

Comment 15

[Johnny Stenback (:jst)]

After looking at more details in this bug and talking this over with Jonas I agree with Shawn, this is not a blocker.

Comment 16

[Johnny Stenback (:jst)]

... but I assigned this to Mounir and he will try out the easy fix and see if that fixes the leak issue here, and if it does we should take the fix for fx4, assuming it seems like a safe fix.

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think I'd prefer to call BindingDetached when the binding is removed. The dtor can run at more random times due to CC, so that's more error prone.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 479555 [details] [diff] [review]
Patch v1

Removing review request while waiting

Comment 20

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Is that what you are expecting?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 483942 [details] [diff] [review]
Patch v2

Yes. Lets get this in sooner rather than later. All XBL changes scare me.

Comment 22

[Mounir Lamouri (:mounir)]

Actually, this patch is breaking a lot of tests:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1287457169.1287459276.21119.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1287449088.1287449484.10175.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1287449139.1287449728.11664.gz

I will have a look at that later.

Comment 23

[Mounir Lamouri (:mounir)]

So, the problem is, when calling BindingDetached, the destructor is called but when called in that situation, all methods inside the destructor will be undefined. I have no idea why :-/

I have a test case here: http://oldworld.fr/mozilla/xbl-test.xul (you need Remote XUL Manager extension to set the permissions). With the patch applied, if you remove a child, the bindings will be detached and _foo() will try to be called but the call will fail. Without the patch, the destructor will be called later and _foo() will be correctly called.

Comment 24

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

Ok, I should detach before changing the document actually.

Comment 25

[Mounir Lamouri (:mounir)]

Gasp, still have one failure on MacOS X only:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1287600826.1287601188.9198.gz

Comment 26

[Mounir Lamouri (:mounir)]

This is ready to be landed but according to Boris this has to be landed for b7 because it might break some extensions. For example, radio widget has been broken.

Should I push that in b7 branch?

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You'll need beltzners approval.

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

If this is an API change that would break people, then yes, please put it on the b7 relbranch. I've moved this bug to be a b7 blocker to reflect that.

The relbranch is GECKO20b7pre_20101006_RELBRANCH

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 484683 [details] [diff] [review]
Patch v2.1

a=beltzner for mozilla-central default and b7 relbranch

Comment 30

[Mounir Lamouri (:mounir)]

Pushed on trunk:
http://hg.mozilla.org/mozilla-central/rev/11591f86a117

And on b7 branch:
http://hg.mozilla.org/mozilla-central/rev/4cb683e8c0f7