Bug 1335751 - Add js::CheckGrayMarkingState friend API to check there are black to gray edges r=sfink

js/public/TracingAPI.h

@@ -86,6 +86,7 @@ class JS_PUBLIC_API(JSTracer)
     bool isTenuringTracer() const { return tag_ == TracerKindTag::Tenuring; }
     bool isCallbackTracer() const { return tag_ == TracerKindTag::Callback; }
     inline JS::CallbackTracer* asCallbackTracer();
+    bool traceWeakEdges() const { return traceWeakEdges_; }
 #ifdef DEBUG
     bool checkEdges() { return checkEdges_; }
 #endif
@@ -99,6 +100,7 @@ class JS_PUBLIC_API(JSTracer)
       , checkEdges_(true)
 #endif
       , tag_(tag)
+      , traceWeakEdges_(true)
     {}
 
 #ifdef DEBUG
@@ -117,6 +119,7 @@ class JS_PUBLIC_API(JSTracer)
 
   protected:
     TracerKindTag tag_;
+    bool traceWeakEdges_;
 };
 
 namespace JS {
@@ -232,6 +235,11 @@ class JS_PUBLIC_API(CallbackTracer) : public JSTracer
     void dispatchToOnEdge(js::LazyScript** lazyp) { onLazyScriptEdge(lazyp); }
     void dispatchToOnEdge(js::Scope** scopep) { onScopeEdge(scopep); }
 
+  protected:
+    void setTraceWeakEdges(bool value) {
+        traceWeakEdges_ = value;
+    }
+
   private:
     friend class AutoTracingName;
     const char* contextName_;

js/src/gc/Marking.cpp

@@ -479,9 +479,12 @@ template <typename T>
 void
 js::TraceWeakEdge(JSTracer* trc, WeakRef<T>* thingp, const char* name)
 {
-    // Non-marking tracers treat the edge strongly.
-    if (!trc->isMarkingTracer())
-        return DispatchToTracer(trc, ConvertToBase(thingp->unsafeUnbarrieredForTracing()), name);
+    if (!trc->isMarkingTracer()) {
+        // Non-marking tracers can select whether or not they see weak edges.
+        if (trc->traceWeakEdges())
+            DispatchToTracer(trc, ConvertToBase(thingp->unsafeUnbarrieredForTracing()), name);
+        return;
+    }
 
     NoteWeakEdge(GCMarker::fromTracer(trc),
                  ConvertToBase(thingp->unsafeUnbarrieredForTracing()));

js/src/gc/Verifier.cpp

@@ -449,14 +449,24 @@ js::gc::GCRuntime::finishVerifier()
 
 #endif /* JS_GC_ZEAL */
 
-#ifdef JSGC_HASH_TABLE_CHECKS
+#if defined(JSGC_HASH_TABLE_CHECKS) || defined(DEBUG)
 
-class CheckHeapTracer : public JS::CallbackTracer
+class HeapCheckTracerBase : public JS::CallbackTracer
 {
   public:
-    explicit CheckHeapTracer(JSRuntime* rt);
+    explicit HeapCheckTracerBase(JSRuntime* rt, WeakMapTraceKind weakTraceKind);
     bool init();
-    void check(AutoLockForExclusiveAccess& lock);
+    bool traceHeap(AutoLockForExclusiveAccess& lock);
+    virtual void checkCell(Cell* cell) = 0;
+
+  protected:
+    void dumpCellPath();
+
+    Cell* parentCell() {
+        return parentIndex == -1 ? nullptr : stack[parentIndex].thing.asCell();
+    }
+
+    size_t failures;
 
   private:
     void onChild(const JS::GCCellPtr& thing) override;
@@ -474,17 +484,16 @@ class CheckHeapTracer : public JS::CallbackTracer
 
     JSRuntime* rt;
     bool oom;
-    size_t failures;
     HashSet<Cell*, DefaultHasher<Cell*>, SystemAllocPolicy> visited;
     Vector<WorkItem, 0, SystemAllocPolicy> stack;
     int parentIndex;
 };
 
-CheckHeapTracer::CheckHeapTracer(JSRuntime* rt)
-  : CallbackTracer(rt, TraceWeakMapKeysValues),
+HeapCheckTracerBase::HeapCheckTracerBase(JSRuntime* rt, WeakMapTraceKind weakTraceKind)
+  : CallbackTracer(rt, weakTraceKind),
+    failures(0),
     rt(rt),
     oom(false),
-    failures(0),
     parentIndex(-1)
 {
 #ifdef DEBUG
@@ -493,21 +502,17 @@ CheckHeapTracer::CheckHeapTracer(JSRuntime* rt)
 }
 
 bool
-CheckHeapTracer::init()
+HeapCheckTracerBase::init()
 {
     return visited.init();
 }
 
-inline static bool
-IsValidGCThingPointer(Cell* cell)
-{
-    return (uintptr_t(cell) & CellMask) == 0;
-}
-
 void
-CheckHeapTracer::onChild(const JS::GCCellPtr& thing)
+HeapCheckTracerBase::onChild(const JS::GCCellPtr& thing)
 {
     Cell* cell = thing.asCell();
+    checkCell(cell);
+
     if (visited.lookup(cell))
         return;
 
@@ -516,57 +521,98 @@ CheckHeapTracer::onChild(const JS::GCCellPtr& thing)
         return;
     }
 
-    if (!IsValidGCThingPointer(cell) || !IsGCThingValidAfterMovingGC(cell))
-    {
-        failures++;
-        fprintf(stderr, "Bad pointer %p\n", cell);
-        const char* name = contextName();
-        for (int index = parentIndex; index != -1; index = stack[index].parentIndex) {
-            const WorkItem& parent = stack[index];
-            cell = parent.thing.asCell();
-            fprintf(stderr, "  from %s %p %s edge\n",
-                    GCTraceKindToAscii(cell->getTraceKind()), cell, name);
-            name = parent.name;
-        }
-        fprintf(stderr, "  from root %s\n", name);
-        return;
-    }
-
     // Don't trace into GC things owned by another runtime.
     if (cell->runtimeFromAnyThread() != rt)
         return;
 
+    // Don't trace into GC in zones being used by helper threads.
+    Zone* zone = thing.is<JSObject>() ? thing.as<JSObject>().zone() : cell->asTenured().zone();
+    if (zone->group() && zone->group()->usedByHelperThread)
+        return;
+
     WorkItem item(thing, contextName(), parentIndex);
     if (!stack.append(item))
         oom = true;
 }
 
-void
-CheckHeapTracer::check(AutoLockForExclusiveAccess& lock)
+bool
+HeapCheckTracerBase::traceHeap(AutoLockForExclusiveAccess& lock)
 {
     // The analysis thinks that traceRuntime might GC by calling a GC callback.
     JS::AutoSuppressGCAnalysis nogc;
     if (!rt->isBeingDestroyed())
         rt->gc.traceRuntime(this, lock);
 
-    while (!stack.empty()) {
+    while (!stack.empty() && !oom) {
         WorkItem item = stack.back();
         if (item.processed) {
             stack.popBack();
         } else {
             parentIndex = stack.length() - 1;
-            TraceChildren(this, item.thing);
             stack.back().processed = true;
+            TraceChildren(this, item.thing);
         }
     }
 
-    if (oom)
+    return !oom;
+}
+
+void
+HeapCheckTracerBase::dumpCellPath()
+{
+    const char* name = contextName();
+    for (int index = parentIndex; index != -1; index = stack[index].parentIndex) {
+        const WorkItem& parent = stack[index];
+        Cell* cell = parent.thing.asCell();
+        fprintf(stderr, "  from %s %p %s edge\n",
+                GCTraceKindToAscii(cell->getTraceKind()), cell, name);
+        name = parent.name;
+    }
+    fprintf(stderr, "  from root %s\n", name);
+}
+
+#endif // defined(JSGC_HASH_TABLE_CHECKS) || defined(DEBUG)
+
+#ifdef JSGC_HASH_TABLE_CHECKS
+
+class CheckHeapTracer final : public HeapCheckTracerBase
+{
+  public:
+    explicit CheckHeapTracer(JSRuntime* rt);
+    void check(AutoLockForExclusiveAccess& lock);
+
+  private:
+    void checkCell(Cell* cell) override;
+};
+
+CheckHeapTracer::CheckHeapTracer(JSRuntime* rt)
+  : HeapCheckTracerBase(rt, TraceWeakMapKeysValues)
+{}
+
+inline static bool
+IsValidGCThingPointer(Cell* cell)
+{
+    return (uintptr_t(cell) & CellMask) == 0;
+}
+
+void
+CheckHeapTracer::checkCell(Cell* cell)
+{
+    if (!IsValidGCThingPointer(cell) || !IsGCThingValidAfterMovingGC(cell)) {
+        failures++;
+        fprintf(stderr, "Bad pointer %p\n", cell);
+        dumpCellPath();
+    }
+}
+
+void
+CheckHeapTracer::check(AutoLockForExclusiveAccess& lock)
+{
+    if (!traceHeap(lock))
         return;
 
-    if (failures) {
-        fprintf(stderr, "Heap check: %" PRIuSIZE " failure(s) out of %" PRIu32 " pointers checked\n",
-                failures, visited.count());
-    }
+    if (failures)
+        fprintf(stderr, "Heap check: %" PRIuSIZE " failure(s)\n", failures);
     MOZ_RELEASE_ASSERT(failures == 0);
 }
 
@@ -580,3 +626,69 @@ js::gc::CheckHeapAfterGC(JSRuntime* rt)
 }
 
 #endif /* JSGC_HASH_TABLE_CHECKS */
+
+#ifdef DEBUG
+
+class CheckGrayMarkingTracer final : public HeapCheckTracerBase
+{
+  public:
+    explicit CheckGrayMarkingTracer(JSRuntime* rt);
+    bool check(AutoLockForExclusiveAccess& lock);
+
+  private:
+    void checkCell(Cell* cell) override;
+};
+
+CheckGrayMarkingTracer::CheckGrayMarkingTracer(JSRuntime* rt)
+  : HeapCheckTracerBase(rt, DoNotTraceWeakMaps)
+{
+    // Weak gray->black edges are allowed.
+    setTraceWeakEdges(false);
+}
+
+void
+CheckGrayMarkingTracer::checkCell(Cell* cell)
+{
+    Cell* parent = parentCell();
+    if (!cell->isTenured() || !parent || !parent->isTenured())
+        return;
+
+    TenuredCell* tenuredCell = &cell->asTenured();
+    TenuredCell* tenuredParent = &parent->asTenured();
+    if (tenuredParent->isMarked(BLACK) && !tenuredParent->isMarked(GRAY) &&
+        tenuredCell->isMarked(GRAY))
+    {
+        failures++;
+        fprintf(stderr, "Found black to gray edge %p\n", cell);
+        dumpCellPath();
+    }
+}
+
+bool
+CheckGrayMarkingTracer::check(AutoLockForExclusiveAccess& lock)
+{
+    if (!traceHeap(lock))
+        return true; // Ignore failure.
+
+    return failures == 0;
+}
+
+JS_FRIEND_API(bool)
+js::CheckGrayMarkingState(JSContext* cx)
+{
+    JSRuntime* rt = cx->runtime();
+    MOZ_ASSERT(!JS::CurrentThreadIsHeapCollecting());
+    MOZ_ASSERT(!rt->gc.isIncrementalGCInProgress());
+    if (!rt->gc.areGrayBitsValid())
+        return true;
+
+    gcstats::AutoPhase ap(rt->gc.stats(), gcstats::PHASE_TRACE_HEAP);
+    AutoTraceSession session(rt, JS::HeapState::Tracing);
+    CheckGrayMarkingTracer tracer(rt);
+    if (!tracer.init())
+        return true; // Ignore failure
+
+    return tracer.check(session.lock);
+}
+
+#endif // DEBUG

js/src/jsfriendapi.h

@@ -492,6 +492,16 @@ IterateGrayObjects(JS::Zone* zone, GCThingCallback cellCallback, void* data);
 extern JS_FRIEND_API(void)
 IterateGrayObjectsUnderCC(JS::Zone* zone, GCThingCallback cellCallback, void* data);
 
+#ifdef DEBUG
+// Trace the heap and check there are no black to gray edges. These are
+// not allowed since the cycle collector could throw away the gray thing and
+// leave a dangling pointer.
+//
+// This doesn't trace weak maps as these are handled separately.
+extern JS_FRIEND_API(bool)
+CheckGrayMarkingState(JSContext* cx);
+#endif
+
 #ifdef JS_HAS_CTYPES
 extern JS_FRIEND_API(size_t)
 SizeOfDataIfCDataObject(mozilla::MallocSizeOf mallocSizeOf, JSObject* obj);