Closed Bug 956081 Opened 6 years ago Closed 6 years ago

GGC breaks the CC's GC-has-been-run detection.


(Core :: JavaScript Engine, defect)

Not set





(Reporter: terrence, Assigned: terrence)




(1 file)

The CC uses gcNumber to check if the GC has run[1]. With GGC, we bump the gcNumber outside of "full" GC's, breaking this heuristic. Fortunately, this is only a backup mechanism to prevent manually triggered CC's from running before GC, since normally CC is tied to GC timing.

Since it was added in 2011, the CC has grown js::AreGCGrayBitsValid, which should catch the same case. The flag is initialized false and only set true after a GC. We should double-check the above and then remove the gcNumber check as it is now duplicated.

Also, there is nothing preventing GC number from overflowing. Currently, we GC within 10 seconds of startup anyway, so we're not likely to hit this. However, given how we're changing the GC, this is a loaded and cocked footgun: we should engage the safety.

Is this what you had in mind?
Attachment #8379240 - Flags: review?(continuation)
Comment on attachment 8379240 [details] [diff] [review]

Review of attachment 8379240 [details] [diff] [review]:

r=me with these changes.

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +875,5 @@
>  nsresult
>  CycleCollectedJSRuntime::TraverseRoots(nsCycleCollectionNoteRootCallback &aCb)
>  {
> +  if (!js::AreGCGrayBitsValid()) {

Just change this to something like MOZ_ASSERT(!NeedCollect(), "Cannot cycle collect if GC has not run first!").  The CC is set up in such a way that we should always be okay here.  (NeedCollect is just a wrapper around AreGCGrayBitsValid, but it seems a little higher level.)  Does that sound okay to you?

If you want to test this, start the browser and then immediately go to about:memory and click on the CC button.  (Before the first GC is triggered automatically.)
Attachment #8379240 - Flags: review?(continuation) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: needcollect
You need to log in before you can comment on or make changes to this bug.