1249428 - Unlink binding properly from element

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

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

Attachment: xbl_unlink_2.diff

var b = document.createElement("browser");
document.documentElement.appendChild(b);
b.cloneNode();
document.documentElement.removeChild(b);
run in Scratchpad with 'browser' environment leads nsBindingManager::mBoundContentSet to keep the element alive, even though we do traverse that entry.

The issue is that we just set the slot's binding to null, but don't clear the needed hashtable in bindingmanager.

This _does_not_ change xbl dtor handling, since we don't run them currently during/after unlink, and can't really, since xbl dtor requires element to have a wrapper, which we've just unlinked...
If we want to fix that, it needs quite a bit larger changes. This is just fixing a leak.

We could just call SetXBLBinding(nullptr, nullptr); when clearing slots, but I think this a bit larger change makes 
the code easier to follow. The effect should be the same.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92d412abf373


mccr8, if you don't feel comfortable to review this, forward to...bz, I guess.

Comment 1

[Andrew McCreight [:mccr8]]

Comment on attachment 8721009 [details] [diff] [review]
xbl_unlink_2.diff

Review of attachment 8721009 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not too familiar with this, but I guess it looks okay.

::: dom/xbl/nsBindingManager.h
@@ +70,5 @@
> +   eRunDtor,
> +   eDoNotRunDtor
> + };
> +  void RemovedFromDocument(nsIContent* aContent, nsIDocument* aOldDocument,
> +                           DestructorHandling aDestructorHandling)

Maybe give this a default value of eRunDtor? That would avoid most of the changes you make.

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50d07d13c6f1

Comment 3

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/50d07d13c6f1