Last Comment Bug 721548 - Cleanup purple buffer during cycle-collector-forget-skippable
: Cleanup purple buffer during cycle-collector-forget-skippable
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: mozilla12
Assigned To: Olli Pettay [:smaug] (TPAC)
:
Mentors:
Depends on: 738653
Blocks: 705582 723081
  Show dependency treegraph
 
Reported: 2012-01-26 15:09 PST by Olli Pettay [:smaug] (TPAC)
Modified: 2012-03-23 08:19 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (10.20 KB, patch)
2012-01-26 15:09 PST, Olli Pettay [:smaug] (TPAC)
no flags Details | Diff | Splinter Review
up-to-date (10.20 KB, patch)
2012-01-27 08:20 PST, Olli Pettay [:smaug] (TPAC)
continuation: review+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (TPAC) 2012-01-26 15:09:20 PST
Created attachment 591964 [details] [diff] [review]
patch

This should be easier to review, and less controversial than previous two patches.
Clean up stuff during forgetSkippable, and unmark js stuff gray after gc.

This is currently that last patch I have for CC.

Pushing to tryserver, and hoping I fixed the leak I caused somehow yesterday when
cleaning up all these patches.
Comment 1 Olli Pettay [:smaug] (TPAC) 2012-01-27 08:20:55 PST
Created attachment 592144 [details] [diff] [review]
up-to-date
Comment 2 Andrew McCreight [:mccr8] 2012-01-27 12:13:54 PST
Comment on attachment 592144 [details] [diff] [review]
up-to-date

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

::: content/base/src/nsCCUncollectableMarker.cpp
@@ +105,5 @@
> +static void
> +MarkUserData(void* aNode, nsIAtom* aKey, void* aValue, void* aData)
> +{
> +  nsIDocument* d = static_cast<nsINode*>(aNode)->GetCurrentDoc();
> +  if (d && nsCCUncollectableMarker::InGeneration(d->GetMarkedCCGeneration())) {

We really need a macro or something for this.  Ah well, that can be a followup, as a bunch of places need to be changed.

@@ +106,5 @@
> +MarkUserData(void* aNode, nsIAtom* aKey, void* aValue, void* aData)
> +{
> +  nsIDocument* d = static_cast<nsINode*>(aNode)->GetCurrentDoc();
> +  if (d && nsCCUncollectableMarker::InGeneration(d->GetMarkedCCGeneration())) {
> +    nsGenericElement::MarkUserData(aNode, aKey, aValue, aData);

I'm not a huge fan of having these nsGenericElement functions take dummy arguments, but I'll complain about that in the other bug.  I'm not sure there's any better way.

@@ +120,5 @@
> +  }
> +}
> +
> +static void
> +MarkMessageManagers()

The basic idea here is that there's a top level MM, which holds onto one MM per window, which hold onto one MM per tab?  And there's no further division?  And for every one you do the CC-mark hook you added before?

@@ +133,5 @@
> +  PRUint32 childCount = 0;
> +  globalMM->GetChildCount(&childCount);
> +  for (PRUint32 i = 0; i < childCount; ++i) {
> +    nsCOMPtr<nsITreeItemFrameMessageManager> windowMM;
> +    globalMM->GetChildAt(i, getter_AddRefs(windowMM));

Isn't there an effort to remove callers of GetChildAt?  That you just reviewed a patch for? ;)  Can you use GetNextChild() or whatever here instead?  It looks to me like you are just iterating over all children.  But if there's some reason why you need to do it that way that's okay.

@@ +142,5 @@
> +    PRUint32 tabChildCount = 0;
> +    windowMM->GetChildCount(&tabChildCount);
> +    for (PRUint32 j = 0; j < tabChildCount; ++j) {
> +      nsCOMPtr<nsITreeItemFrameMessageManager> tabMM;
> +      windowMM->GetChildAt(j, getter_AddRefs(tabMM));

Same here with the GetChildAt.

@@ +150,5 @@
> +      tabMM->MarkForCC();
> +      //XXX hack warning, but works, since we know that
> +      //    callback data is frameloader.
> +      void* cb = static_cast<nsFrameMessageManager*>(tabMM.get())->
> +        GetCallbackData();

Somebody else should confirm that this hack is ok, as I don't understand it.  I'll just assume that this line and the next one that casts it to an nsFrameLoader are okay.  Lines 153-155.

@@ +157,5 @@
> +        nsIDOMEventTarget* et = fl->GetTabChildGlobalAsEventTarget();
> +        if (!et) {
> +          continue;
> +        }
> +        static_cast<nsInProcessTabChildGlobal*>(et)->MarkForCC();

This cast is okay to do because we know that the GetTabChildGlobal is really a nsIInProcessContentFrameMessageManager, and we know that the only implementation of that interface is nsInProcessTabChildGlobal?  I see some existing code in nsFrameLoader.cpp that does this, so I guess it is okay.

@@ +177,5 @@
>    }
>  
>    nsIDocument *doc = aViewer->GetDocument();
> +  if (doc &&
> +      doc->GetMarkedCCGeneration() != nsCCUncollectableMarker::sGeneration) {

The idea here is to not mark content in a document more than once for efficiency?  It is a shame you have to do this bare comparison, but oh well.

@@ +315,5 @@
> +
> +  // JS cleanup can be slow. Do it only if there has been a GC.
> +  bool cleanupJS =
> +    !nsJSContext::CleanupSinceLastGC() &&
> +    !strcmp(aTopic, "cycle-collector-forget-skippable");

You could avoid the strcmp here by using !prepareForCC, but I guess that's an irrelevant microoptimization that could break if we add another topic for it to listen to...

@@ +318,5 @@
> +    !nsJSContext::CleanupSinceLastGC() &&
> +    !strcmp(aTopic, "cycle-collector-forget-skippable");
> +
> +  bool prepareForCC = !strcmp(aTopic, "cycle-collector-begin");
> +    

Why do we need to do anything if both cleanupJS and prepareForCC are false?  In that case, the new code you added here won't do anything, and it will be during a forget-skippable, which is a point where we don't currently do any other marking.  Just curious.
Comment 3 Andrew McCreight [:mccr8] 2012-01-27 17:25:19 PST
Comment on attachment 592144 [details] [diff] [review]
up-to-date

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

::: content/base/src/nsCCUncollectableMarker.cpp
@@ +133,5 @@
> +  PRUint32 childCount = 0;
> +  globalMM->GetChildCount(&childCount);
> +  for (PRUint32 i = 0; i < childCount; ++i) {
> +    nsCOMPtr<nsITreeItemFrameMessageManager> windowMM;
> +    globalMM->GetChildAt(i, getter_AddRefs(windowMM));

smaug pointed out to me in IRC that this is a message manager, not a node, so my objections are incorrect.  Oops.

@@ +150,5 @@
> +      tabMM->MarkForCC();
> +      //XXX hack warning, but works, since we know that
> +      //    callback data is frameloader.
> +      void* cb = static_cast<nsFrameMessageManager*>(tabMM.get())->
> +        GetCallbackData();

Okay, so it looks like this first cast is okay because the only subclass of nsITreeItemFrameMessageManager is nsIChromeFrameMessageManager, and the only implementation of that is nsFrameMessageManager.

@@ +151,5 @@
> +      //XXX hack warning, but works, since we know that
> +      //    callback data is frameloader.
> +      void* cb = static_cast<nsFrameMessageManager*>(tabMM.get())->
> +        GetCallbackData();
> +      nsFrameLoader* fl = static_cast<nsFrameLoader*>(cb);

This is okay because all of the calls to SetCallbackData(), which are in nsFrameLoader, pass in an nsFrameLoader.  Seems a little sketchy to me, but okay.
Comment 4 Olli Pettay [:smaug] (TPAC) 2012-01-28 01:47:39 PST
> We really need a macro or something for this.  Ah well, that can be a
> followup, as a bunch of places need to be changed.
Indeed


> The basic idea here is that there's a top level MM, which holds onto one MM
> per window, which hold onto one MM per tab?  And there's no further
> division?  And for every one you do the CC-mark hook you added before?
yes.



> Isn't there an effort to remove callers of GetChildAt?  That you just
> reviewed a patch for? ;)  Can you use GetNextChild() or whatever here
> instead?  It looks to me like you are just iterating over all children.  But
> if there's some reason why you need to do it that way that's okay.
As I mentioned, that is about different GetChildAt


> Why do we need to do anything if both cleanupJS and prepareForCC are false? 
> In that case, the new code you added here won't do anything, and it will be
> during a forget-skippable, which is a point where we don't currently do any
> other marking.  Just curious.
Because black documents need new CCGeneration
Comment 5 Andrew McCreight [:mccr8] 2012-01-28 05:55:57 PST
Sounds good, then.  Though I'd like you to change to a MarkAllUserDataHandler(nsDocument*) and MarkAllUserData(nsDocument*) here instead of using the raw hash table iterators, as discussed in bug 721515.
Comment 6 Olli Pettay [:smaug] (TPAC) 2012-01-28 09:29:03 PST
That makes sense.
Comment 7 Andrew McCreight [:mccr8] 2012-01-30 16:25:15 PST
https://hg.mozilla.org/mozilla-central/rev/aa0476948dc0

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