996151 - Fix incomplete unlinking of nsBindingManager references to elements

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Clear binding manager references in FragmentOrElement::Unlink. WIP.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Check that FragmentOrElement actually unlinks the binding manager stuff. not for landing.

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Clear binding manager references in FragmentOrElement::Unlink. r=mrbkap

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Check that FragmentOrElement actually unlinks the binding manager stuff. Not for landing.

Comment 6

[Andrew McCreight [:mccr8]]

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

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8413458 [details] [diff] [review]
Clear binding manager references in FragmentOrElement::Unlink. r=mrbkap

Let me know what you think, too, William.

Comment 8

[William Chen [: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.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 10

[William Chen [:wchen]]

Attachment: Clear out XBL binding and insertion parent of content even if nsBindingManager is destroyed.

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.

Comment 11

[Andrew McCreight [:mccr8]]

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.

Comment 12

[Andrew McCreight [:mccr8]]

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.

Comment 13

[William Chen [:wchen]]

Attachment: Clear out XBL binding and insertion parent of content even if nsBindingManager is destroyed.

Added comments from c11

Comment 14

[William Chen [:wchen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de19c62cbc6b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de19c62cbc6b