Bug 1338623 - Add a slower but more exact gray marking check for checking correctness r=sfink r=mccr8

dom/bindings/BindingUtils.h

@@ -1531,7 +1531,7 @@ struct WrapNativeHelper
     JSObject* obj;
     if ((obj = cache->GetWrapper())) {
       // GetWrapper always unmarks gray.
-      MOZ_ASSERT(!JS::ObjectIsMarkedGray(obj));
+      MOZ_ASSERT(JS::ObjectIsNotGray(obj));
       return obj;
     }
 
@@ -1539,11 +1539,11 @@ struct WrapNativeHelper
     if (!CouldBeDOMBinding(parent)) {
       // WrapNativeFallback never returns a gray thing.
       obj = WrapNativeFallback<T>::Wrap(cx, parent, cache);
-      MOZ_ASSERT_IF(obj, !JS::ObjectIsMarkedGray(obj));
+      MOZ_ASSERT(JS::ObjectIsNotGray(obj));
     } else {
       // WrapObject never returns a gray thing.
       obj = parent->WrapObject(cx, nullptr);
-      MOZ_ASSERT_IF(obj, !JS::ObjectIsMarkedGray(obj));
+      MOZ_ASSERT(JS::ObjectIsNotGray(obj));
     }
 
     return obj;
@@ -1565,12 +1565,12 @@ struct WrapNativeHelper<T, false>
                    "Unexpected object in nsWrapperCache");
       obj = rootedObj;
 #endif
-      MOZ_ASSERT(!JS::ObjectIsMarkedGray(obj));
+      MOZ_ASSERT(JS::ObjectIsNotGray(obj));
       return obj;
     }
 
     obj = WrapNativeISupports(cx, parent, cache);
-    MOZ_ASSERT_IF(obj, !JS::ObjectIsMarkedGray(obj));
+    MOZ_ASSERT(JS::ObjectIsNotGray(obj));
     return obj;
   }
 };
@@ -1589,7 +1589,7 @@ FindAssociatedGlobal(JSContext* cx, T* p, nsWrapperCache* cache,
   if (!obj) {
     return nullptr;
   }
-  MOZ_ASSERT(!JS::ObjectIsMarkedGray(obj));
+  MOZ_ASSERT(JS::ObjectIsNotGray(obj));
 
   obj = js::GetGlobalForObjectCrossCompartment(obj);
 
@@ -1606,7 +1606,7 @@ FindAssociatedGlobal(JSContext* cx, T* p, nsWrapperCache* cache,
   JS::Rooted<JSObject*> rootedObj(cx, obj);
   JSObject* xblScope = xpc::GetXBLScope(cx, rootedObj);
   MOZ_ASSERT_IF(xblScope, JS_IsGlobalObject(xblScope));
-  MOZ_ASSERT_IF(xblScope, !JS::ObjectIsMarkedGray(xblScope));
+  MOZ_ASSERT(JS::ObjectIsNotGray(xblScope));
   return xblScope;
 }
 

dom/bindings/CallbackObject.h

@@ -126,7 +126,7 @@ public:
    */
   JS::Handle<JSObject*> CallbackKnownNotGray() const
   {
-    MOZ_ASSERT(!JS::ObjectIsMarkedGray(mCallback));
+    MOZ_ASSERT(JS::ObjectIsNotGray(mCallback));
     return CallbackPreserveColor();
   }
 

dom/bindings/Codegen.py

@@ -3252,7 +3252,7 @@ class CGGetPerInterfaceObject(CGAbstractMethod):
              */
 
             const JS::Heap<JSObject*>& entrySlot = protoAndIfaceCache.EntrySlotMustExist(${id});
-            MOZ_ASSERT_IF(entrySlot, !JS::ObjectIsMarkedGray(entrySlot));
+            MOZ_ASSERT(JS::ObjectIsNotGray(entrySlot));
             return JS::Handle<JSObject*>::fromMarkedLocation(entrySlot.address());
             """,
             id=self.id)
@@ -3769,7 +3769,7 @@ class CGWrapWithCacheMethod(CGAbstractMethod):
               return false;
             }
             MOZ_ASSERT(JS_IsGlobalObject(global));
-            MOZ_ASSERT(!JS::ObjectIsMarkedGray(global));
+            MOZ_ASSERT(JS::ObjectIsNotGray(global));
 
             // That might have ended up wrapping us already, due to the wonders
             // of XBL.  Check for that, and bail out as needed.

js/public/CallArgs.h

@@ -290,7 +290,7 @@ class MOZ_STACK_CLASS CallArgs : public detail::CallArgsBase<detail::IncludeUsed
         args.constructing_ = constructing;
 #ifdef DEBUG
         for (unsigned i = 0; i < argc; ++i)
-            MOZ_ASSERT_IF(argv[i].isGCThing(), !GCThingIsMarkedGray(GCCellPtr(argv[i])));
+            MOZ_ASSERT(ValueIsNotGray(argv[i]));
 #endif
         return args;
     }

js/public/HeapAPI.h

@@ -310,6 +310,11 @@ CellIsMarkedGray(const Cell* cell)
 extern JS_PUBLIC_API(bool)
 CellIsMarkedGrayIfKnown(const Cell* cell);
 
+#ifdef DEBUG
+extern JS_PUBLIC_API(bool)
+CellIsNotGray(const Cell* cell);
+#endif
+
 } /* namespace detail */
 
 MOZ_ALWAYS_INLINE bool

js/public/RootingAPI.h

@@ -323,19 +323,31 @@ ObjectIsMarkedGray(const JS::Heap<JSObject*>& obj)
     return ObjectIsMarkedGray(obj.unbarrieredGet());
 }
 
-static MOZ_ALWAYS_INLINE bool
-ScriptIsMarkedGray(JSScript* script)
+// The following *IsNotGray functions are for use in assertions and take account
+// of the eventual gray marking state at the end of any ongoing incremental GC.
+#ifdef DEBUG
+inline bool
+CellIsNotGray(js::gc::Cell* maybeCell)
 {
-    auto cell = reinterpret_cast<js::gc::Cell*>(script);
-    return js::gc::detail::CellIsMarkedGrayIfKnown(cell);
+    if (!maybeCell)
+        return true;
+
+    return js::gc::detail::CellIsNotGray(maybeCell);
 }
 
-static MOZ_ALWAYS_INLINE bool
-ScriptIsMarkedGray(const Heap<JSScript*>& script)
+inline bool
+ObjectIsNotGray(JSObject* maybeObj)
 {
-    return ScriptIsMarkedGray(script.unbarrieredGet());
+    return CellIsNotGray(reinterpret_cast<js::gc::Cell*>(maybeObj));
 }
 
+inline bool
+ObjectIsNotGray(const JS::Heap<JSObject*>& obj)
+{
+    return ObjectIsNotGray(obj.unbarrieredGet());
+}
+#endif
+
 /**
  * The TenuredHeap<T> class is similar to the Heap<T> class above in that it
  * encapsulates the GC concerns of an on-heap reference to a JS object. However,

js/public/Value.h

@@ -1452,6 +1452,27 @@ template <class S, bool v> struct BoolDefaultAdaptor { static bool defaultValue(
 
 } // namespace js
 
+#ifdef DEBUG
+namespace JS {
+
+MOZ_ALWAYS_INLINE bool
+ValueIsNotGray(const Value& value)
+{
+    if (!value.isGCThing())
+        return true;
+
+    return CellIsNotGray(value.toGCThing());
+}
+
+MOZ_ALWAYS_INLINE bool
+ValueIsNotGray(const Heap<Value>& value)
+{
+    return ValueIsNotGray(value.unbarrieredGet());
+}
+
+} // namespace JS
+#endif
+
 /************************************************************************/
 
 namespace JS {

js/src/gc/Barrier.cpp

@@ -31,7 +31,7 @@ RuntimeFromActiveCooperatingThreadIsHeapMajorCollecting(JS::shadow::Zone* shadow
 #ifdef DEBUG
 
 bool
-IsMarkedBlack(NativeObject* obj)
+IsMarkedBlack(JSObject* obj)
 {
     // Note: we assume conservatively that Nursery things will be live.
     if (!obj->isTenured())
@@ -61,8 +61,7 @@ HeapSlot::assertPreconditionForWriteBarrierPost(NativeObject* obj, Kind kind, ui
     else
         MOZ_ASSERT(static_cast<HeapSlot*>(obj->getDenseElements() + slot)->get() == target);
 
-    MOZ_ASSERT_IF(target.isGCThing() && IsMarkedBlack(obj),
-                  !JS::GCThingIsMarkedGray(JS::GCCellPtr(target)));
+    CheckEdgeIsNotBlackToGray(obj, target);
 }
 
 bool

js/src/gc/Barrier.h

@@ -242,9 +242,15 @@ bool
 CurrentThreadIsGCSweeping();
 
 bool
-IsMarkedBlack(NativeObject* obj);
+IsMarkedBlack(JSObject* obj);
 #endif
 
+MOZ_ALWAYS_INLINE void
+CheckEdgeIsNotBlackToGray(JSObject* src, const Value& dst)
+{
+    MOZ_ASSERT_IF(IsMarkedBlack(src), JS::ValueIsNotGray(dst));
+}
+
 template <typename T>
 struct InternalBarrierMethods {};
 

js/src/gc/Marking.cpp

@@ -2254,7 +2254,7 @@ MarkStackIter::done() const
     return position() == 0;
 }
 
-inline const MarkStack::TaggedPtr&
+inline MarkStack::TaggedPtr
 MarkStackIter::peekPtr() const
 {
     MOZ_ASSERT(!done());
@@ -2286,6 +2286,15 @@ MarkStackIter::nextPtr()
     pos_--;
 }
 
+inline void
+MarkStackIter::next()
+{
+    if (TagIsArrayTag(peekTag()))
+        nextArray();
+    else
+        nextPtr();
+}
+
 inline void
 MarkStackIter::nextArray()
 {
@@ -2559,6 +2568,29 @@ GCMarker::sizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
     return size;
 }
 
+#ifdef DEBUG
+Zone*
+GCMarker::stackContainsCrossZonePointerTo(const TenuredCell* target) const
+{
+    MOZ_ASSERT(!JS::CurrentThreadIsHeapCollecting());
+
+    for (MarkStackIter iter(stack); !iter.done(); iter.next()) {
+        if (iter.peekTag() != MarkStack::ObjectTag)
+            continue;
+
+        auto source = iter.peekPtr().as<JSObject>();
+        if (source->is<ProxyObject>() &&
+            source->as<ProxyObject>().target() == static_cast<const Cell*>(target) &&
+            source->zone() != target->zone())
+        {
+            return source->zone();
+        }
+    }
+
+    return nullptr;
+}
+#endif // DEBUG
+
 
 /*** Tenuring Tracer *****************************************************************************/
 

js/src/gc/Marking.h

@@ -193,7 +193,9 @@ class MarkStackIter
 
     bool done() const;
     MarkStack::Tag peekTag() const;
+    MarkStack::TaggedPtr peekPtr() const;
     MarkStack::ValueArray peekValueArray() const;
+    void next();
     void nextPtr();
     void nextArray();
 
@@ -202,7 +204,6 @@ class MarkStackIter
 
   private:
     size_t position() const;
-    const MarkStack::TaggedPtr& peekPtr() const;
 };
 
 struct WeakKeyTableHashPolicy {
@@ -300,7 +301,11 @@ class GCMarker : public JSTracer
     size_t sizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const;
 
 #ifdef DEBUG
+
     bool shouldCheckCompartments() { return strictCompartmentChecking; }
+
+    Zone* stackContainsCrossZonePointerTo(const gc::TenuredCell* cell) const;
+
 #endif
 
     void markEphemeronValues(gc::Cell* markedCell, gc::WeakEntryVector& entry);

js/src/jsapi-tests/testGCGrayMarking.cpp

@@ -104,6 +104,8 @@ TestMarking()
     CHECK(IsMarkedBlack(sameTarget));
     CHECK(IsMarkedBlack(crossTarget));
 
+    CHECK(!JS::ObjectIsMarkedGray(sameSource));
+
     // Test GC with gray roots marks object gray.
 
     blackRoot1 = nullptr;
@@ -116,6 +118,8 @@ TestMarking()
     CHECK(IsMarkedGray(sameTarget));
     CHECK(IsMarkedGray(crossTarget));
 
+    CHECK(JS::ObjectIsMarkedGray(sameSource));
+
     // Test ExposeToActiveJS marks gray objects black.
 
     JS::ExposeObjectToActiveJS(sameSource);
@@ -478,11 +482,12 @@ TestWatchpoints()
 bool
 TestCCWs()
 {
-    RootedObject target(cx, AllocPlainObject());
+    JSObject* target = AllocPlainObject();
     CHECK(target);
 
     // Test getting a new wrapper doesn't return a gray wrapper.
 
+    RootedObject blackRoot(cx, target);
     JSObject* wrapper = GetCrossCompartmentWrapper(target);
     CHECK(wrapper);
     CHECK(!IsMarkedGray(wrapper));
@@ -519,7 +524,43 @@ TestCCWs()
 
     JS::FinishIncrementalGC(cx, JS::gcreason::API);
 
-    target = nullptr;
+    // Test behaviour of gray CCWs marked black by a barrier during incremental
+    // GC.
+
+    // Initial state: source and target are gray.
+    blackRoot = nullptr;
+    grayRoots.grayRoot1 = wrapper;
+    grayRoots.grayRoot2 = nullptr;
+    JS_GC(cx);
+    CHECK(IsMarkedGray(wrapper));
+    CHECK(IsMarkedGray(target));
+
+    // Incremental zone GC started: the source is now unmarked.
+    JS_SetGCParameter(cx, JSGC_MODE, JSGC_MODE_INCREMENTAL);
+    JS::PrepareZoneForGC(wrapper->zone());
+    budget = js::SliceBudget(js::WorkBudget(1));
+    cx->runtime()->gc.startDebugGC(GC_NORMAL, budget);
+    CHECK(JS::IsIncrementalGCInProgress(cx));
+    CHECK(wrapper->zone()->isGCMarkingBlack());
+    CHECK(!target->zone()->wasGCStarted());
+    CHECK(!IsMarkedBlack(wrapper));
+    CHECK(!IsMarkedGray(wrapper));
+    CHECK(IsMarkedGray(target));
+
+    // Betweeen GC slices: source marked black by barrier, target is still gray.
+    // ObjectIsMarkedGray() and CheckObjectIsNotMarkedGray() should handle this
+    // case and report that target is not marked gray.
+    grayRoots.grayRoot1.get();
+    CHECK(IsMarkedBlack(wrapper));
+    CHECK(IsMarkedGray(target));
+    CHECK(!JS::ObjectIsMarkedGray(target));
+    MOZ_ASSERT(JS::ObjectIsNotGray(target));
+
+    // Final state: source and target are black.
+    JS::FinishIncrementalGC(cx, JS::gcreason::API);
+    CHECK(IsMarkedBlack(wrapper));
+    CHECK(IsMarkedBlack(target));
+
     grayRoots.grayRoot1 = nullptr;
     grayRoots.grayRoot2 = nullptr;
 
@@ -531,8 +572,8 @@ JS::PersistentRootedObject global2;
 
 struct GrayRoots
 {
-    JSObject* grayRoot1;
-    JSObject* grayRoot2;
+    JS::Heap<JSObject*> grayRoot1;
+    JS::Heap<JSObject*> grayRoot2;
 };
 
 GrayRoots grayRoots;
@@ -567,10 +608,8 @@ static void
 TraceGrayRoots(JSTracer* trc, void* data)
 {
     auto grayRoots = static_cast<GrayRoots*>(data);
-    if (grayRoots->grayRoot1)
-        UnsafeTraceManuallyBarrieredEdge(trc, &grayRoots->grayRoot1, "gray root 1");
-    if (grayRoots->grayRoot2)
-        UnsafeTraceManuallyBarrieredEdge(trc, &grayRoots->grayRoot2, "gray root 2");
+    TraceEdge(trc, &grayRoots->grayRoot1, "gray root 1");
+    TraceEdge(trc, &grayRoots->grayRoot2, "gray root 2");
 }
 
 JSObject*

js/src/jscntxtinlines.h

@@ -459,7 +459,7 @@ template <typename T>
 inline void
 JSContext::enterCompartmentOf(const T& target)
 {
-    MOZ_ASSERT(!js::gc::detail::CellIsMarkedGrayIfKnown(target));
+    MOZ_ASSERT(JS::CellIsNotGray(target));
     enterCompartment(target->compartment(), nullptr);
 }
 

js/src/jscompartment.cpp

@@ -462,7 +462,7 @@ JSCompartment::wrap(JSContext* cx, MutableHandleObject obj)
 
     // Anything we're wrapping has already escaped into script, so must have
     // been unmarked-gray at some point in the past.
-    MOZ_ASSERT(!ObjectIsMarkedGray(obj));
+    MOZ_ASSERT(JS::ObjectIsNotGray(obj));
 
     // The passed object may already be wrapped, or may fit a number of special
     // cases that we need to check for and manually correct.

js/src/jscompartmentinlines.h

@@ -116,7 +116,7 @@ JSCompartment::wrap(JSContext* cx, JS::MutableHandleValue vp)
      * that we get the same answer.
      */
 #ifdef DEBUG
-    MOZ_ASSERT(!JS::ObjectIsMarkedGray(&vp.toObject()));
+    MOZ_ASSERT(JS::ValueIsNotGray(vp));
     JS::RootedObject cacheResult(cx);
 #endif
     JS::RootedValue v(cx, vp);

js/src/jsfun.h

@@ -811,8 +811,7 @@ inline void
 JSFunction::initExtendedSlot(size_t which, const js::Value& val)
 {
     MOZ_ASSERT(which < mozilla::ArrayLength(toExtended()->extendedSlots));
-    MOZ_ASSERT_IF(js::IsMarkedBlack(this) && val.isGCThing(),
-                  !JS::GCThingIsMarkedGray(JS::GCCellPtr(val)));
+    js::CheckEdgeIsNotBlackToGray(this, val);
     MOZ_ASSERT(js::IsObjectValueInCompartment(val, compartment()));
     toExtended()->extendedSlots[which].init(val);
 }
@@ -821,8 +820,7 @@ inline void
 JSFunction::setExtendedSlot(size_t which, const js::Value& val)
 {
     MOZ_ASSERT(which < mozilla::ArrayLength(toExtended()->extendedSlots));
-    MOZ_ASSERT_IF(js::IsMarkedBlack(this) && val.isGCThing(),
-                  !JS::GCThingIsMarkedGray(JS::GCCellPtr(val)));
+    js::CheckEdgeIsNotBlackToGray(this, val);
     MOZ_ASSERT(js::IsObjectValueInCompartment(val, compartment()));
     toExtended()->extendedSlots[which] = val;
 }

js/src/jsgc.cpp

@@ -7798,13 +7798,21 @@ js::gc::Cell::dump() const
 }
 #endif
 
-JS_PUBLIC_API(bool)
-js::gc::detail::CellIsMarkedGrayIfKnown(const Cell* cell)
+static inline bool
+CanCheckGrayBits(const Cell* cell)
 {
     MOZ_ASSERT(cell);
     if (!cell->isTenured())
         return false;
 
+    auto tc = &cell->asTenured();
+    auto rt = tc->runtimeFromAnyThread();
+    return CurrentThreadCanAccessRuntime(rt) && rt->gc.areGrayBitsValid();
+}
+
+JS_PUBLIC_API(bool)
+js::gc::detail::CellIsMarkedGrayIfKnown(const Cell* cell)
+{
     // We ignore the gray marking state of cells and return false in the
     // following cases:
     //
@@ -7818,14 +7826,50 @@ js::gc::detail::CellIsMarkedGrayIfKnown(const Cell* cell)
     // call this while parsing, and they are not allowed to inspect the
     // runtime's incremental state. The objects being operated on are not able
     // to be collected and will not be marked any color.
+
+    if (!CanCheckGrayBits(cell))
+        return false;
+
     auto tc = &cell->asTenured();
     auto rt = tc->runtimeFromAnyThread();
-    if (!CurrentThreadCanAccessRuntime(rt) ||
-        !rt->gc.areGrayBitsValid() ||
-        (rt->gc.isIncrementalGCInProgress() && !tc->zone()->wasGCStarted()))
-    {
+    if (rt->gc.isIncrementalGCInProgress() && !tc->zone()->wasGCStarted())
         return false;
-    }
 
     return detail::CellIsMarkedGray(tc);
 }
+
+#ifdef DEBUG
+JS_PUBLIC_API(bool)
+js::gc::detail::CellIsNotGray(const Cell* cell)
+{
+    // Check that a cell is not marked gray.
+    //
+    // Since this is a debug-only check, take account of the eventual mark state
+    // of cells that will be marked black by the next GC slice in an incremental
+    // GC. For performance reasons we don't do this in CellIsMarkedGrayIfKnown.
+
+    // TODO: I'd like to AssertHeapIsIdle() here, but this ends up getting
+    // called while iterating the heap for memory reporting.
+    MOZ_ASSERT(!JS::CurrentThreadIsHeapCollecting());
+    MOZ_ASSERT(!JS::CurrentThreadIsHeapCycleCollecting());
+
+    if (!CanCheckGrayBits(cell))
+        return true;
+
+    auto tc = &cell->asTenured();
+    if (!detail::CellIsMarkedGray(tc))
+        return true;
+
+    auto rt = tc->runtimeFromAnyThread();
+    Zone* sourceZone = nullptr;
+    if (rt->gc.isIncrementalGCInProgress() &&
+        !tc->zone()->wasGCStarted() &&
+        (sourceZone = rt->gc.marker.stackContainsCrossZonePointerTo(tc)) &&
+        sourceZone->wasGCStarted())
+    {
+        return true;
+    }
+
+    return false;
+}
+#endif

js/xpconnect/wrappers/WrapperFactory.cpp

@@ -101,7 +101,7 @@ WrapperFactory::WaiveXray(JSContext* cx, JSObject* objArg)
     if (!waiver) {
         waiver = CreateXrayWaiver(cx, obj);
     }
-    MOZ_ASSERT(!ObjectIsMarkedGray(waiver));
+    MOZ_ASSERT(JS::ObjectIsNotGray(waiver));
     return waiver;
 }
 
@@ -322,7 +322,7 @@ WrapperFactory::PrepareForWrapping(JSContext* cx, HandleObject scope,
 
     obj.set(&v.toObject());
     MOZ_ASSERT(IS_WN_REFLECTOR(obj), "bad object");
-    MOZ_ASSERT(!ObjectIsMarkedGray(obj), "Should never return gray reflectors");
+    MOZ_ASSERT(JS::ObjectIsNotGray(obj), "Should never return gray reflectors");
 
     // Because the underlying native didn't have a PreCreate hook, we had
     // to a new (or possibly pre-existing) XPCWN in our compartment.