Last Comment Bug 730639 - Blast DOM owned subtrees during canSkip
: Blast DOM owned subtrees during canSkip
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 12 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla16
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
Depends on: 730581
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-25 18:23 PST by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2012-07-02 16:26 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
untested WIP (5.77 KB, patch)
2012-02-25 18:23 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
v2 (5.99 KB, patch)
2012-02-26 13:15 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
WIP3 (7.49 KB, patch)
2012-02-29 00:32 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
v6 (6.83 KB, patch)
2012-03-01 06:35 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
v7, a bit more conservative approach (6.96 KB, patch)
2012-06-26 03:03 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
v8 (6.85 KB, patch)
2012-06-26 06:08 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
v9 (9.36 KB, patch)
2012-06-26 10:14 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
v10 (10.02 KB, patch)
2012-06-26 11:19 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
v11 (10.25 KB, patch)
2012-06-27 02:30 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
v12 (10.12 KB, patch)
2012-06-27 03:47 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
continuation: review+
Details | Diff | Review
patch (11.94 KB, patch)
2012-06-27 15:08 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-25 18:23:35 PST
Created attachment 600736 [details] [diff] [review]
untested WIP

https://tbpl.mozilla.org/?tree=Try&rev=f7e09c0fe522
Comment 1 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-26 07:59:06 PST
Try shows some leaks, which I can't reproduce locally :(
Comment 2 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-26 13:15:50 PST
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.
Comment 3 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-26 14:44:59 PST
Still leaky :(
Comment 4 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-26 14:47:17 PST
Er, that was known crasher, not leak. Still waiting...
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-26 14:59:03 PST
Comment on attachment 600809 [details] [diff] [review]
v2

Leaking
Comment 6 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-29 00:32:54 PST
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
Comment 7 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-03-01 06:35:43 PST
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.
Comment 8 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-03-01 11:49:06 PST
I'll try M2 first, as it is shorter.
Comment 9 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-03-01 11:49:52 PST
Oops, that only leaked on 32 bit, so I'll try M1.
Comment 10 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-03-27 14:58:25 PDT
Andrew, could you perhaps try to reproduce the leak using M1.
Comment 11 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-03-27 15:01:44 PDT
Sure.
Comment 12 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-26 03:03:45 PDT
Created attachment 636644 [details] [diff] [review]
v7, a bit more conservative approach
Comment 13 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-26 03:03:57 PDT
https://tbpl.mozilla.org/?tree=Try&rev=518c85ee438e
Comment 14 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-26 06:08:16 PDT
Created attachment 636675 [details] [diff] [review]
v8

https://tbpl.mozilla.org/?tree=Try&rev=e4384ec23245
Comment 15 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-26 10:14:37 PDT
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.
Comment 16 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-26 11:19:42 PDT
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.
Comment 17 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-26 11:20:32 PDT
s/witch/which/ :)
Comment 18 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-27 02:30:45 PDT
Created attachment 637046 [details] [diff] [review]
v11

Put back the unmarkpurple part I removed in some version.
Comment 19 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-27 02:32:36 PDT
(I'll push still another patch to try to test wrapper handling, but feel free to review v11)
Comment 20 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-27 03:47:44 PDT
Created attachment 637062 [details] [diff] [review]
v12

As I expected this a tiny bit simpler patch works too.
Comment 21 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-06-27 14:27:31 PDT
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] (high review load, please consider other reviewers) 2012-06-27 14:35:10 PDT
(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] (high review load, please consider other reviewers) 2012-06-27 14:44:23 PDT
 > 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 (PTO-ish through 6-29) [:mccr8] 2012-06-27 14:47:46 PDT
(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 (PTO-ish through 6-29) [:mccr8] 2012-06-27 14:48:12 PDT
Specific mentions of what might be the case could be useful, but maybe it doesn't really matter.
Comment 26 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-27 15:08:00 PDT
Created attachment 637280 [details] [diff] [review]
patch
Comment 27 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-27 15:21:27 PDT
https://hg.mozilla.org/mozilla-central/rev/d254c07f3301

Note You need to log in before you can comment on or make changes to this bug.