730639 - Blast DOM owned subtrees during canSkip

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

Attachment: untested WIP

https://tbpl.mozilla.org/?tree=Try&rev=f7e09c0fe522

Comment 1

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

Try shows some leaks, which I can't reproduce locally :(

Comment 2

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

Attachment: v2

https://tbpl.mozilla.org/?tree=Try&rev=c99a4ba7c677

Let's see if being a bit more strict helps.

Comment 3

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

Still leaky :(

Comment 4

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

Er, that was known crasher, not leak. Still waiting...

Comment 5

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

Comment on attachment 600809 [details] [diff] [review]
v2

Leaking

Comment 6

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

Attachment: WIP3

At least I shouldn't Addref an object which is being iterated in the purple buffer.
https://tbpl.mozilla.org/?tree=Try&rev=d5cf765f5f07

Comment 7

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

Attachment: v6

This should still leak
https://tbpl.mozilla.org/?tree=Try&rev=2dd0a5df1e71

Andrew, could you try the patch on OSX and see if you can reproduce the
leak. M1 should at least leak.

Comment 8

[Andrew McCreight [:mccr8]]

I'll try M2 first, as it is shorter.

Comment 9

[Andrew McCreight [:mccr8]]

Oops, that only leaked on 32 bit, so I'll try M1.

Comment 10

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

Andrew, could you perhaps try to reproduce the leak using M1.

Comment 11

[Andrew McCreight [:mccr8]]

Sure.

Comment 12

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

Attachment: v7, a bit more conservative approach

Comment 13

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

https://tbpl.mozilla.org/?tree=Try&rev=518c85ee438e

Comment 14

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

Attachment: v8

https://tbpl.mozilla.org/?tree=Try&rev=e4384ec23245

Comment 15

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

Attachment: v9

This doesn't seem to leak, but doesn't behave too good with 
http://mozilla.pettay.fi/moztests/innerhtml_nonsimple_nostyle_test.html for example.

Comment 16

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

Attachment: v10

This one seems to work.
Need to clear pending to-be-blasted subtrees before CC, so that ContentUnbind
objects don't end up causing unknown edges.

So the idea is to blast DOM subtrees if we know that they are owned only
by DOM itself (and don't have wrappers)

http://mozilla.pettay.fi/moztests/innerhtml_nonsimple_nostyle_test.html
is a pathological case with witch the patch helps quite a bit.

nsGenericElement::ClearContentUnbinder is a bit ugly, but I didn't want to
move ContentUnbinder to .h.

Comment 17

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

s/witch/which/ :)

Comment 18

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

Attachment: v11

Put back the unmarkpurple part I removed in some version.

Comment 19

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

(I'll push still another patch to try to test wrapper handling, but feel free to review v11)

Comment 20

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

Attachment: v12

As I expected this a tiny bit simpler patch works too.

Comment 21

[Andrew McCreight [:mccr8]]

Comment on attachment 637062 [details] [diff] [review]
v12

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

Does this code get used during Mochitests at least a little?  I suppose you were having problems with leaks on Try so it must.

One thing that is unfortunate about this patch is that the PurpleRoot flag doesn't just mean that it has been found to be a purple root any more.  It is a more general flag to indicate that the cycle collector optimization shouldn't look at it again.  Oh well, probably not worth the hassle of changing it everywhere.

::: content/base/public/nsIContent.h
@@ +45,5 @@
>  
>  // IID for the nsIContent interface
>  #define NS_ICONTENT_IID \
> +{ 0x98fb308d, 0xc6dd, 0x4c6d, \
> +  { 0xb7, 0x7c, 0x91, 0x18, 0x0c, 0xf0, 0x6f, 0x23 } }

Do any subtypes of nsIContent need their IIDs changed?  I don't know how that all works.

::: content/base/src/nsGenericDOMDataNode.h
@@ +314,5 @@
> +    PRUint32 rc = mRefCnt.get();
> +    if (GetParent()) {
> +      --rc;
> +    }
> +    return rc == 0;

Maybe you could change this to
  return GetParent() && mRefCnt.get() == 1?
Do you want to support the case where there's no parent and the refcnt is 0?

::: content/base/src/nsGenericElement.cpp
@@ +2808,5 @@
>      }
>      return NS_OK;
>    }
>  
> +  static void UnbindAll()

Kind of funny you didn't need this before. Maybe because you were always scheduling at the end of the previous CC, there was enough time to clear it out before the next CC, whereas now things can be added at any point, even right before a CC.

@@ +3098,2 @@
>  
>  void ClearPurpleRoots()

Please rename this function now that it does something else, too.  Maybe ClearCycleCollectorCleanupData, or something less awkward...

@@ +3236,5 @@
> +    } else {
> +      domOnlyCycle = domOnlyCycle && node->OwnedOnlyByTheDOMTree();
> +      if (ShouldClearPurple(node)) {
> +        // Collect interesting nodes which we can clear if we find that
> +        // they are kept alive in a black tree.

This comment should be updated to say something like "or are in a DOM-only cycle".

Comment 22

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

(In reply to Andrew McCreight [:mccr8] from comment #21) 
> Does this code get used during Mochitests at least a little? 
Yes. Almost always when someone sets innerHTML value to an element which has child nodes.

> > +{ 0x98fb308d, 0xc6dd, 0x4c6d, \
> > +  { 0xb7, 0x7c, 0x91, 0x18, 0x0c, 0xf0, 0x6f, 0x23 } }
> 
> Do any subtypes of nsIContent need their IIDs changed?  I don't know how
> that all works.
Hmm, that is not done usually.
I could update NS_ELEMENT_IID too.
> Maybe you could change this to
>   return GetParent() && mRefCnt.get() == 1?
Indeed, this is better.


> Kind of funny you didn't need this before. Maybe because you were always
> scheduling at the end of the previous CC, there was enough time to clear it
> out before the next CC, whereas now things can be added at any point, even
> right before a CC.
Right.


> Please rename this function now that it does something else, too.  Maybe
> ClearCycleCollectorCleanupData, or something less awkward...
Ok

> This comment should be updated to say something like "or are in a DOM-only
> cycle".
Indeed.

Comment 23

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

 > One thing that is unfortunate about this patch is that the PurpleRoot flag
> doesn't just mean that it has been found to be a purple root any more. 
The patch doesn't change that.

Comment 24

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #23)
> The patch doesn't change that.

I suppose that's true.

This comment should probably be updated:

// Subtree has been traversed already, and aNode
// wasn't removed from purple buffer. No need to do more here.

Maybe just something like "Subtree has been traversed already, and aNode has been handled in a way that doesn't require revisiting it."

Comment 25

[Andrew McCreight [:mccr8]]

Specific mentions of what might be the case could be useful, but maybe it doesn't really matter.

Comment 26

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

Attachment: patch

Comment 27

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

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