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 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 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.

> 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.

That makes sense.