The CC can run a GC beforehand to make sure that the gray bits are set correctly. We're supposed to set a flag when this happens, but this flag is not set if helper thread zones are present because the GC sees that we are not collecting all zones and concludes that we haven't performed a full GC. In reality this probably fine. I don't think anything in a helper thread zone is ever reachable from the main heap so the CC doesn't need gray bits to be correct in these zones.

Here's a patch to set the 'gray bits are valid' flag even if we don't perform a full GC. As described in the comments we ignore helper thread zones, empty zones and the atoms zone. The purpose of this is so that when the CC requests a full GC because the gray marking state is invalid, we do end up with the flag set at the end.

Comment on attachment 8843353 [details] [diff] [review] bug1343986-gray-bits-valid-flag Review of attachment 8843353 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense to me. If there were gray cycles going through a helper zone, I think it would mean we're messing up our thread safety anyway. :-) ::: js/src/jsgc.cpp @@ +5440,5 @@ > } > } > > +bool > +GCRuntime::grayBitsHaveBecomeValid() const I wonder if this should be allGrayableZonesWereCollected (if you'll forgive the mangling of the English language)? @@ +5461,5 @@ > + for (ZonesIter zone(rt, WithAtoms); !zone.done(); zone.next()) { > + if (!zone->isCollecting() && > + !zone->usedByHelperThread() && > + !zone->isAtomsZone() && > + !zone->arenas.arenaListsAreEmpty()) Do you prefer it this way, or SkipAtoms and a shorter list of conditionals? (I'm fine either way.)

(In reply to Steve Fink [:sfink] [:s:] from comment #2) > I wonder if this should be allGrayableZonesWereCollected (if you'll forgive > the mangling of the English language)? I'm going to go with 'allCCVisibleZonesWereCollected'. > Do you prefer it this way, or SkipAtoms and a shorter list of conditionals? > (I'm fine either way.) Ugh, yeah it's a bit strange to include the atoms zone only to explicitly disallow it.

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b2caae037c26 Fix setting of gray bits valid flag after GC r=sfink

Is this worth backporting?

This might mean that we do some extra GCs before CCing that aren't strictly necessary. I don't think it's worth backporting.