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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

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.

1- http://dxr.mozilla.org/mozilla-central/source/xpcom/base/CycleCollectedJSRuntime.cpp#876

Comment 1

[Terrence Cole [:terrence]]

Attachment: fix_gc_has_been_run_detection-v0.diff

Is this what you had in mind?

https://tbpl.mozilla.org/?tree=Try&rev=01416539d0b4

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 8379240 [details] [diff] [review]
fix_gc_has_been_run_detection-v0.diff

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

Comment 3

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f1bb2f2f868

Done and done.

Comment 4

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1f1bb2f2f868