Bug 1335751 - Add js::CheckGrayMarkingState friend API to check there are black to gray edges r=sfink
authorJon Coppeard <jcoppeard@mozilla.com>
Sun, 05 Mar 2017 09:23:09 +0000
changeset 394493 4c3e4f77ea2fc477936ba06fc4488a6e4468a6d2
parent 394492 5a1e069911faeeaf1f3d3a6047251464a17b3e31
child 394494 c10963d3a6872149f753738c265ef56b4394dda0
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1335751
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1335751 - Add js::CheckGrayMarkingState friend API to check there are black to gray edges r=sfink
js/public/TracingAPI.h
js/src/gc/Marking.cpp
js/src/gc/Verifier.cpp
js/src/jsfriendapi.h
--- a/js/public/TracingAPI.h
+++ b/js/public/TracingAPI.h
@@ -81,29 +81,31 @@ class JS_PUBLIC_API(JSTracer)
         // Traversing children is the responsibility of the callback.
         Callback
     };
     bool isMarkingTracer() const { return tag_ == TracerKindTag::Marking || tag_ == TracerKindTag::WeakMarking; }
     bool isWeakMarkingTracer() const { return tag_ == TracerKindTag::WeakMarking; }
     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
 
   protected:
     JSTracer(JSRuntime* rt, TracerKindTag tag,
              WeakMapTraceKind weakTraceKind = TraceWeakMapValues)
       : runtime_(rt)
       , weakMapAction_(weakTraceKind)
 #ifdef DEBUG
       , checkEdges_(true)
 #endif
       , tag_(tag)
+      , traceWeakEdges_(true)
     {}
 
 #ifdef DEBUG
     // Set whether to check edges are valid in debug builds.
     void setCheckEdges(bool check) {
         checkEdges_ = check;
     }
 #endif
@@ -112,16 +114,17 @@ class JS_PUBLIC_API(JSTracer)
     JSRuntime* runtime_;
     WeakMapTraceKind weakMapAction_;
 #ifdef DEBUG
     bool checkEdges_;
 #endif
 
   protected:
     TracerKindTag tag_;
+    bool traceWeakEdges_;
 };
 
 namespace JS {
 
 class AutoTracingName;
 class AutoTracingIndex;
 class AutoTracingCallback;
 
@@ -227,16 +230,21 @@ class JS_PUBLIC_API(CallbackTracer) : pu
     void dispatchToOnEdge(JSScript** scriptp) { onScriptEdge(scriptp); }
     void dispatchToOnEdge(js::Shape** shapep) { onShapeEdge(shapep); }
     void dispatchToOnEdge(js::ObjectGroup** groupp) { onObjectGroupEdge(groupp); }
     void dispatchToOnEdge(js::BaseShape** basep) { onBaseShapeEdge(basep); }
     void dispatchToOnEdge(js::jit::JitCode** codep) { onJitCodeEdge(codep); }
     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_;
 
     friend class AutoTracingIndex;
     size_t contextIndex_;
 
     friend class AutoTracingDetails;
--- a/js/src/gc/Marking.cpp
+++ b/js/src/gc/Marking.cpp
@@ -474,19 +474,22 @@ js::UnsafeTraceManuallyBarrieredEdge(JST
 {
     DispatchToTracer(trc, ConvertToBase(thingp), name);
 }
 
 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()));
 }
 
 template <typename T>
 void
 js::TraceRoot(JSTracer* trc, T* thingp, const char* name)
--- a/js/src/gc/Verifier.cpp
+++ b/js/src/gc/Verifier.cpp
@@ -444,24 +444,34 @@ js::gc::GCRuntime::finishVerifier()
     if (verifyPreData) {
         js_delete(verifyPreData.ref());
         verifyPreData = nullptr;
     }
 }
 
 #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;
 
     struct WorkItem {
         WorkItem(JS::GCCellPtr thing, const char* name, int parentIndex)
           : thing(thing), name(name), parentIndex(parentIndex), processed(false)
         {}
@@ -469,114 +479,216 @@ class CheckHeapTracer : public JS::Callb
         JS::GCCellPtr thing;
         const char* name;
         int parentIndex;
         bool processed;
     };
 
     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
     setCheckEdges(false);
 #endif
 }
 
 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;
 
     if (!visited.put(cell)) {
         oom = true;
         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;
+            stack.back().processed = true;
             TraceChildren(this, item.thing);
-            stack.back().processed = true;
         }
     }
 
-    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);
 }
 
 void
 js::gc::CheckHeapAfterGC(JSRuntime* rt)
 {
     AutoTraceSession session(rt, JS::HeapState::Tracing);
     CheckHeapTracer tracer(rt);
     if (tracer.init())
         tracer.check(session.lock);
 }
 
 #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
--- a/js/src/jsfriendapi.h
+++ b/js/src/jsfriendapi.h
@@ -487,16 +487,26 @@ IterateGrayObjects(JS::Zone* zone, GCThi
 
 /**
  * Invoke cellCallback on every gray JSObject in the given zone while cycle
  * collection is in progress.
  */
 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);
 #endif
 
 extern JS_FRIEND_API(JSCompartment*)
 GetAnyCompartmentInZone(JS::Zone* zone);