Cleanup purple buffer during cycle-collector-forget-skippable

RESOLVED FIXED in mozilla12

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Depends on: 1 bug)

12 Branch
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
Attachment #591964 - Flags: review?(continuation)
Blocks: 721543
(Assignee)

Updated

6 years ago
No longer blocks: 721543
(Assignee)

Comment 1

6 years ago
Created attachment 592144 [details] [diff] [review]
up-to-date
Attachment #591964 - Attachment is obsolete: true
Attachment #591964 - Flags: review?(continuation)
Attachment #592144 - Flags: review?(continuation)
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.
Attachment #592144 - Flags: review?(continuation) → review+
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.
(Assignee)

Comment 4

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

Comment 6

6 years ago
That makes sense.
https://hg.mozilla.org/mozilla-central/rev/aa0476948dc0
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla12
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 723081

Updated

5 years ago
Depends on: 738653
You need to log in before you can comment on or make changes to this bug.