Closed Bug 996151 Opened 10 years ago Closed 10 years ago

Fix incomplete unlinking of nsBindingManager references to elements

Categories

(Core :: XBL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mccr8, Assigned: wchen)

References

Details

Attachments

(2 files, 4 obsolete files)

FragmentOrElement does this in its Traverse:
  tmp->OwnerDoc()->BindingManager()->Traverse(tmp, cc);
This deals with various references to the fragment or element.  In investigating bug 957109 (without the patch in that bug) I saw a case where a handful of elements were identified as garbage, but they were being held alive by this binding edge.  Maybe even with deoptimizing shadow DOM stuff this is still a problem.
Summary: Incomplete unlinking of nsBindingManager references to elements → Fix incomplete unlinking of nsBindingManager references to elements
Depends on: 1001562
I confirmed that without the clearing patch we hit the assert, and that with it, we don't, when running the webcomponents subdirectory at least.
Attachment #8406339 - Attachment is obsolete: true
Attachment #8406340 - Attachment is obsolete: true
Comment on attachment 8413458 [details] [diff] [review]
Clear binding manager references in FragmentOrElement::Unlink. r=mrbkap

I'm not entirely sure this is right, but it worked better than the other things I tried.  This is supposed to clear out the reference traversed in the line: |tmp->OwnerDoc()->BindingManager()->Traverse(tmp, cb);|

try run, no asserts:
  https://tbpl.mozilla.org/?tree=Try&rev=00aafe8bcef0
try run with asserts in the second patch:
  https://tbpl.mozilla.org/?tree=Try&rev=9f4d86c48972
try run with ICC enabled that shows that this seems to fix the intermittent M4 webcomponents leak:
  https://tbpl.mozilla.org/?tree=Try&rev=98d6571b6f4c
Attachment #8413458 - Flags: review?(mrbkap)
Comment on attachment 8413458 [details] [diff] [review]
Clear binding manager references in FragmentOrElement::Unlink. r=mrbkap

Let me know what you think, too, William.
Attachment #8413458 - Flags: feedback?(wchen)
Comment on attachment 8413458 [details] [diff] [review]
Clear binding manager references in FragmentOrElement::Unlink. r=mrbkap

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

::: content/base/src/FragmentOrElement.cpp
@@ +1249,5 @@
>  
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(FragmentOrElement)
>    nsINode::Unlink(tmp);
>  
> +  tmp->SetXBLBinding(nullptr, nullptr);

You don't need the second argument.

This looks fine to me, I was worried about the binding's destructor being called but it looks like that happens elsewhere.
Attachment #8413458 - Flags: feedback?(wchen) → feedback+
Comment on attachment 8413458 [details] [diff] [review]
Clear binding manager references in FragmentOrElement::Unlink. r=mrbkap

I talked with wchen about this, and I think that we've found a cleaner way of doing this.

In particular, because Unlink already calls UnbindFromTree, we're already creating runnables to remove the bindings from elements when they're unlinked I think this patch should be redundant. wchen has a patch that might fix this in a slightly cleaner way.
Attachment #8413458 - Flags: review?(mrbkap)
mccr8: Can you try this fix instead of the one you attached? It allows the RemoveFromBindingManager runnable to null out the XBL binding reference even if the binding manager was destroyed.
Thanks!  I'll do a try run and let you know how it goes. (Which involves a lot of M4 XP retriggers.)
  https://tbpl.mozilla.org/?tree=Try&rev=0c3be5715ae3

It would be nice in a final version of the patch to have a comment in Unlink about where the bindingmanager reference is being cleared out to make it easier to see that the Unlink matches the Traverse.
I retriggered M4 XP about 20 times on my run with ICC enabled, and there was no leak, so it looks like the patch works, thanks.  I'll assign this over to you then.
Assignee: continuation → wchen
Attachment #8413458 - Attachment is obsolete: true
Attachment #8413459 - Flags: checkin-
Added comments from c11
Attachment #8414076 - Attachment is obsolete: true
Attachment #8414664 - Flags: review?(mrbkap)
Attachment #8414664 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/de19c62cbc6b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.