Closed Bug 1249428 Opened 4 years ago Closed 4 years ago

Unlink binding properly from element

Categories

(Core :: XBL, defect)

36 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: smaug, Assigned: smaug)

Details

(Whiteboard: dom-triaged btpp-active)

Attachments

(1 file)

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.
Attachment #8721009 - Flags: review?(continuation)
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.
Attachment #8721009 - Flags: review?(continuation) → review+
Whiteboard: dom-triaged btpp-active
https://hg.mozilla.org/mozilla-central/rev/50d07d13c6f1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.