Blast DOM owned subtrees during canSkip

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

12 Branch
mozilla16
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 9 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 600736 [details] [diff] [review]
untested WIP

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

Comment 1

6 years ago
Try shows some leaks, which I can't reproduce locally :(
(Assignee)

Comment 2

6 years ago
Created attachment 600809 [details] [diff] [review]
v2

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

Let's see if being a bit more strict helps.
Attachment #600736 - Attachment is obsolete: true
(Assignee)

Comment 3

6 years ago
Still leaky :(
(Assignee)

Comment 4

6 years ago
Er, that was known crasher, not leak. Still waiting...
(Assignee)

Comment 5

6 years ago
Comment on attachment 600809 [details] [diff] [review]
v2

Leaking
Attachment #600809 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
Created attachment 601550 [details] [diff] [review]
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
(Assignee)

Comment 7

6 years ago
Created attachment 601944 [details] [diff] [review]
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.
Attachment #601550 - Attachment is obsolete: true
I'll try M2 first, as it is shorter.
Oops, that only leaked on 32 bit, so I'll try M1.
(Assignee)

Comment 10

6 years ago
Andrew, could you perhaps try to reproduce the leak using M1.
Sure.
(Assignee)

Comment 12

5 years ago
Created attachment 636644 [details] [diff] [review]
v7, a bit more conservative approach
(Assignee)

Comment 13

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=518c85ee438e
(Assignee)

Comment 14

5 years ago
Created attachment 636675 [details] [diff] [review]
v8

https://tbpl.mozilla.org/?tree=Try&rev=e4384ec23245
Attachment #636644 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Created attachment 636765 [details] [diff] [review]
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.
(Assignee)

Comment 16

5 years ago
Created attachment 636797 [details] [diff] [review]
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.
Attachment #601944 - Attachment is obsolete: true
Attachment #636675 - Attachment is obsolete: true
Attachment #636765 - Attachment is obsolete: true
Attachment #636797 - Flags: review?(continuation)
(Assignee)

Comment 17

5 years ago
s/witch/which/ :)
(Assignee)

Comment 18

5 years ago
Created attachment 637046 [details] [diff] [review]
v11

Put back the unmarkpurple part I removed in some version.
Attachment #636797 - Attachment is obsolete: true
Attachment #636797 - Flags: review?(continuation)
Attachment #637046 - Flags: review?(continuation)
(Assignee)

Comment 19

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

Comment 20

5 years ago
Created attachment 637062 [details] [diff] [review]
v12

As I expected this a tiny bit simpler patch works too.
Attachment #637046 - Attachment is obsolete: true
Attachment #637046 - Flags: review?(continuation)
Attachment #637062 - Flags: review?(continuation)
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".
Attachment #637062 - Flags: review?(continuation) → review+
(Assignee)

Comment 22

5 years ago
(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.
(Assignee)

Comment 23

5 years ago
 > 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.
(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."
Specific mentions of what might be the case could be useful, but maybe it doesn't really matter.
(Assignee)

Comment 26

5 years ago
Created attachment 637280 [details] [diff] [review]
patch
(Assignee)

Comment 27

5 years ago
https://hg.mozilla.org/mozilla-central/rev/d254c07f3301
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.