The gray marking state isn't always valid so we can't always assert that it is correct. We should add a way to check the entire heap's gray marking state before running a cycle collection.

One thing to keep in mind is that we have a flag to track that we know the state is invalid, and if so, do a GC. So it is okay sometimes for the gray state to be messed up before we CC. See nsCycleCollector::FixGrayBits(). Of course, after we run that the gray state should be valid.

(In reply to Andrew McCreight [:mccr8] from comment #1) Right, good to know! I added the check after the call to FixGrayBits()... and it fails fairly often.

This patch adds an API which traces the heap looking for strong black->gray (weak black->gray edges are fine). This factors out common heap tracing into a base class as there wasn't enough in common with CheckHeapTracer to make that class do both sets of checks.

Patch to check all gray marking state before cycle collection. This factors out the gray mapping fixup logic so it can be used by both the fixup tracer and by the checker. This was green on try for a linux64 only build. I'm testing again now with more platforms.

Comment on attachment 8837673 [details] [diff] [review] bug1335751-check-gray-marking-before-CC Review of attachment 8837673 [details] [diff] [review]: ----------------------------------------------------------------- This is great! ::: xpcom/base/CycleCollectedJSContext.cpp @@ +226,5 @@ > } > } > } > > +// Report whether the key and value of a weak mapping entry are gray but need to nit: should probably be "key or value", not "key and value". @@ +230,5 @@ > +// Report whether the key and value of a weak mapping entry are gray but need to > +// be marked black. > +static void > +ShouldWeakMappingEntryBeBlack(JSObject* aMap, JS::GCCellPtr aKey, JS::GCCellPtr aValue, > + bool* aKeyShouldBeBlack, bool* aValueShouldBeBlack) Maybe move the setting of the return values to false into this method? That is less error prone. @@ +238,5 @@ > + > + // If nothing that could be held alive by this entry is marked gray, return. > + bool keyMightNeedMarking = aKey && JS::GCThingIsMarkedGray(aKey); > + bool valueMightNeedMarking = aValue && JS::GCThingIsMarkedGray(aValue) && > + aValue.kind() != JS::TraceKind::String; It seems odd to me that here and below we check the kind of aValue against specific kinds, rather than calling AddToCCKind, but maybe there's some reason for it... @@ +264,5 @@ > + *aValueShouldBeBlack = true; > + } > +} > + > +// This is based on the logic in ShouldWeakMappingEntryBeBlack. You can drop this comment, given that it literally calls that method, instead of copy-pasting it. :) @@ +1320,5 @@ > + MOZ_ASSERT(mJSContext); > + MOZ_ASSERT(!JS::IsIncrementalGCInProgress(mJSContext), > + "Don't call CheckGrayBits during a GC."); > + MOZ_ASSERT(js::CheckGrayMarkingState(mJSContext)); > + MOZ_ASSERT(CheckWeakMappingGrayBitsTracer::Check(mJSContext)); How long does this take to run in an optimized build? It could be "fun" to run these checks with MOZ_RELEASE_ASSERT after they've settled out a bit in debug builds. ::: xpcom/base/nsCycleCollector.cpp @@ +3527,5 @@ > > +void > +nsCycleCollector::CheckGrayBits() > +{ > + mJSContext->CheckGrayBits(); You need to null check mJSContext here. I think we use the CC in some kind of non-browser tests that don't have a JS context. You can see the same thing in FixGrayBits. I'd also just inline this into BeginCollection as it is a function that is only a few lines, and is only called once.

Comment on attachment 8837672 [details] [diff] [review] bug1335751-add-gray-marking-check-api Review of attachment 8837672 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/Marking.cpp @@ +478,5 @@ > { > // Non-marking tracers treat the edge strongly. > + if (!trc->isMarkingTracer()) { > + if (trc->traceWeakEdges()) { > + if (InternalBarrierMethods<T>::isMarkable(*thingp->unsafeGet())) When does this get called with non-markable cells? @@ +481,5 @@ > + if (trc->traceWeakEdges()) { > + if (InternalBarrierMethods<T>::isMarkable(*thingp->unsafeGet())) > + DispatchToTracer(trc, ConvertToBase(thingp->unsafeUnbarrieredForTracing()), name); > + } > + return; Seems like this would be easier to read as if (!trc->traceWeakEdges()) return; ...

(In reply to Steve Fink [:sfink] [:s:] from comment #6) > When does this get called with non-markable cells? I remember I added it because a test was failing, but I can't find where this happens now. I'll take it out.

(In reply to Andrew McCreight [:mccr8] from comment #5) > It seems odd to me that here and below we check the kind of aValue against > specific kinds, rather than calling AddToCCKind, but maybe there's some > reason for it... I was wondering about that too. > How long does this take to run in an optimized build? It could be "fun" to run these checks with > MOZ_RELEASE_ASSERT after they've settled out a bit in debug builds. Good idea. Unfortunately this patch does detect incorrect gray marking state in Windows builds so I'll need to investigate that before I can land this.

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d3562cfc972 Add js::CheckGrayMarkingState friend API to check there are black to gray edges r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/3c4a6d36149c Check all gray marking state before cycle collection in debug builds r=mccr8

Backed out for mochitest devtools failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/71f7cd1d842f054d2d8b6ebc4dc06b7c4ba0bc54

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4c3e4f77ea2f Add js::CheckGrayMarkingState friend API to check there are black to gray edges r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/c10963d3a687 Check all gray marking state before cycle collection in debug builds r=mccr8

https://hg.mozilla.org/mozilla-central/rev/4c3e4f77ea2f https://hg.mozilla.org/mozilla-central/rev/c10963d3a687