Bug 1509923 - Check weak map marking state in debug builds and when enabled with a zeal mode r=sfink
☠☠ backed out by 387f770bf58c ☠ ☠
authorJon Coppeard <jcoppeard@mozilla.com>
Thu, 06 Dec 2018 16:27:21 -0500
changeset 449671 98f8e4e44c103044d3a6d9a27bd2e8586fe4f05e
parent 449670 4c4645fdcf1cd1a18e03a0689fcc5528e0836a01
child 449672 387f770bf58c175b69b5803b3c0d4a7ea55d18c4
push id35179
push useraciure@mozilla.com
push dateSun, 09 Dec 2018 21:43:27 +0000
treeherdermozilla-central@53fd96ca5aa4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1509923
milestone65.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 1509923 - Check weak map marking state in debug builds and when enabled with a zeal mode r=sfink
js/src/gc/GC.cpp
js/src/gc/GCEnum.h
js/src/gc/Verifier.cpp
js/src/gc/WeakMap-inl.h
js/src/gc/WeakMap.cpp
js/src/gc/WeakMap.h
js/src/vm/Debugger.h
--- a/js/src/gc/GC.cpp
+++ b/js/src/gc/GC.cpp
@@ -1013,17 +1013,19 @@ const char gc::ZealModeHelpText[] =
     "    21: (YieldBeforeSweepingObjects) Incremental GC in two slices that "
     "yields\n"
     "        before sweeping foreground finalized objects\n"
     "    22: (YieldBeforeSweepingNonObjects) Incremental GC in two slices that "
     "yields\n"
     "        before sweeping non-object GC things\n"
     "    23: (YieldBeforeSweepingShapeTrees) Incremental GC in two slices that "
     "yields\n"
-    "        before sweeping shape trees\n";
+    "        before sweeping shape trees\n"
+    "    24: (CheckWeakMapMarking) Check weak map marking invariants after "
+    "every GC\n";
 
 // The set of zeal modes that control incremental slices. These modes are
 // mutually exclusive.
 static const mozilla::EnumSet<ZealMode> IncrementalSliceZealModes = {
     ZealMode::YieldBeforeMarking,
     ZealMode::YieldBeforeSweeping,
     ZealMode::IncrementalMultipleSlices,
     ZealMode::YieldBeforeSweepingAtoms,
@@ -5211,16 +5213,36 @@ void js::NotifyGCPostSwap(JSObject* a, J
   if (removedFlags & JS_GC_SWAP_OBJECT_A_REMOVED) {
     DelayCrossCompartmentGrayMarking(b);
   }
   if (removedFlags & JS_GC_SWAP_OBJECT_B_REMOVED) {
     DelayCrossCompartmentGrayMarking(a);
   }
 }
 
+static inline void MaybeCheckWeakMapMarking(GCRuntime* gc)
+{
+#if defined(JS_GC_ZEAL) || defined(DEBUG)
+
+  bool shouldCheck;
+#if defined(DEBUG)
+  shouldCheck = true;
+#else
+  shouldCheck = gc->hasZealMode(ZealMode::CheckWeakMapMarking);
+#endif
+
+  if (shouldCheck) {
+    for (SweepGroupZonesIter zone(gc->rt); !zone.done(); zone.next()) {
+      MOZ_RELEASE_ASSERT(WeakMapBase::checkMarkingForZone(zone));
+    }
+  }
+
+#endif
+}
+
 IncrementalProgress GCRuntime::endMarkingSweepGroup(FreeOp* fop,
                                                     SliceBudget& budget) {
   gcstats::AutoPhase ap(stats(), gcstats::PhaseKind::SWEEP_MARK);
 
   // Mark any incoming black pointers from previously swept compartments
   // whose referents are not marked. This can occur when gray cells become
   // black by the action of UnmarkGray.
   markIncomingCrossCompartmentPointers(MarkColor::Black);
@@ -5248,16 +5270,18 @@ IncrementalProgress GCRuntime::endMarkin
   for (SweepGroupZonesIter zone(rt); !zone.done(); zone.next()) {
     zone->changeGCState(Zone::MarkGray, Zone::Mark);
   }
   MOZ_ASSERT(marker.isDrained());
 
   // We must not yield after this point before we start sweeping the group.
   safeToYield = false;
 
+  MaybeCheckWeakMapMarking(this);
+
   return Finished;
 }
 
 // Causes the given WeakCache to be swept when run.
 class ImmediateSweepWeakCacheTask
     : public GCParallelTaskHelper<ImmediateSweepWeakCacheTask> {
   JS::detail::WeakCacheBase& cache;
 
--- a/js/src/gc/GCEnum.h
+++ b/js/src/gc/GCEnum.h
@@ -68,17 +68,18 @@ enum class AbortReason {
   D(CheckHeapAfterGC, 15)              \
   D(CheckNursery, 16)                  \
   D(YieldBeforeSweepingAtoms, 17)      \
   D(CheckGrayMarking, 18)              \
   D(YieldBeforeSweepingCaches, 19)     \
   D(YieldBeforeSweepingTypes, 20)      \
   D(YieldBeforeSweepingObjects, 21)    \
   D(YieldBeforeSweepingNonObjects, 22) \
-  D(YieldBeforeSweepingShapeTrees, 23)
+  D(YieldBeforeSweepingShapeTrees, 23) \
+  D(CheckWeakMapMarking, 24)
 
 enum class ZealMode {
 #define ZEAL_MODE(name, value) name = value,
   JS_FOR_EACH_ZEAL_MODE(ZEAL_MODE)
 #undef ZEAL_MODE
       Count,
   Limit = Count - 1
 };
--- a/js/src/gc/Verifier.cpp
+++ b/js/src/gc/Verifier.cpp
@@ -9,28 +9,31 @@
 #include "mozilla/Sprintf.h"
 
 #ifdef MOZ_VALGRIND
 #include <valgrind/memcheck.h>
 #endif
 
 #include "gc/GCInternals.h"
 #include "gc/PublicIterators.h"
+#include "gc/WeakMap.h"
 #include "gc/Zone.h"
 #include "js/HashTable.h"
 #include "vm/JSContext.h"
 
 #include "gc/ArenaList-inl.h"
 #include "gc/GC-inl.h"
 #include "gc/Marking-inl.h"
 #include "vm/JSContext-inl.h"
 
 using namespace js;
 using namespace js::gc;
 
+using mozilla::DebugOnly;
+
 #ifdef JS_GC_ZEAL
 
 /*
  * Write barrier verification
  *
  * The next few functions are for write barrier verification.
  *
  * The VerifyBarriers function is a shorthand. It checks if a verification phase
@@ -442,17 +445,51 @@ void js::gc::GCRuntime::finishVerifier()
   if (verifyPreData) {
     js_delete(verifyPreData.ref());
     verifyPreData = nullptr;
   }
 }
 
 #endif /* JS_GC_ZEAL */
 
-#if defined(JSGC_HASH_TABLE_CHECKS) || defined(DEBUG)
+#if defined(JS_GC_ZEAL) || defined(DEBUG)
+
+// Like gc::MarkColor but allows the possibility of the cell being
+// unmarked. Order is important here, with white being 'least marked'
+// and black being 'most marked'.
+enum class CellColor : uint8_t { White = 0, Gray = 1, Black = 2 };
+
+static CellColor GetCellColor(Cell* cell) {
+  if (cell->isMarkedBlack()) {
+    return CellColor::Black;
+  }
+
+  if (cell->isMarkedGray()) {
+    return CellColor::Gray;
+  }
+
+  return CellColor::White;
+}
+
+static const char* CellColorName(CellColor color) {
+  switch (color) {
+    case CellColor::White:
+      return "white";
+    case CellColor::Black:
+      return "black";
+    case CellColor::Gray:
+      return "gray";
+    default:
+      MOZ_CRASH("Unexpected cell color");
+  }
+}
+
+static const char* GetCellColorName(Cell* cell) {
+  return CellColorName(GetCellColor(cell));
+}
 
 class HeapCheckTracerBase : public JS::CallbackTracer {
  public:
   explicit HeapCheckTracerBase(JSRuntime* rt, WeakMapTraceKind weakTraceKind);
   bool traceHeap(AutoTraceSession& session);
   virtual void checkCell(Cell* cell) = 0;
 
  protected:
@@ -554,26 +591,16 @@ bool HeapCheckTracerBase::traceHeap(Auto
       stack.back().processed = true;
       TraceChildren(this, item.thing);
     }
   }
 
   return !oom;
 }
 
-static const char* GetCellColorName(Cell* cell) {
-  if (cell->isMarkedBlack()) {
-    return "black";
-  }
-  if (cell->isMarkedGray()) {
-    return "gray";
-  }
-  return "white";
-}
-
 void HeapCheckTracerBase::dumpCellInfo(Cell* cell) {
   auto kind = cell->getTraceKind();
   JSObject* obj =
       kind == JS::TraceKind::Object ? static_cast<JSObject*>(cell) : nullptr;
 
   fprintf(stderr, "%s %s", GetCellColorName(cell), GCTraceKindToAscii(kind));
   if (obj) {
     fprintf(stderr, " %s", obj->getClass()->name);
@@ -592,20 +619,16 @@ void HeapCheckTracerBase::dumpCellPath()
     fprintf(stderr, "  from ");
     dumpCellInfo(cell);
     fprintf(stderr, " %s edge\n", 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:
   enum GCType { Moving, NonMoving };
 
   explicit CheckHeapTracer(JSRuntime* rt, GCType type);
   void check(AutoTraceSession& session);
 
  private:
@@ -651,20 +674,16 @@ void js::gc::CheckHeapAfterGC(JSRuntime*
   } else {
     gcType = CheckHeapTracer::GCType::NonMoving;
   }
 
   CheckHeapTracer tracer(rt, gcType);
   tracer.check(session);
 }
 
-#endif /* JSGC_HASH_TABLE_CHECKS */
-
-#if defined(JS_GC_ZEAL) || defined(DEBUG)
-
 class CheckGrayMarkingTracer final : public HeapCheckTracerBase {
  public:
   explicit CheckGrayMarkingTracer(JSRuntime* rt);
   bool check(AutoTraceSession& session);
 
  private:
   void checkCell(Cell* cell) override;
 };
@@ -715,9 +734,93 @@ JS_FRIEND_API bool js::CheckGrayMarkingS
 
   gcstats::AutoPhase ap(rt->gc.stats(), gcstats::PhaseKind::TRACE_HEAP);
   AutoTraceSession session(rt);
   CheckGrayMarkingTracer tracer(rt);
 
   return tracer.check(session);
 }
 
+static Zone* GetCellZone(Cell* cell) {
+  if (cell->is<JSObject>()) {
+    return cell->as<JSObject>()->zone();
+  }
+
+  return cell->asTenured().zone();
+}
+
+static JSObject* MaybeGetDelegate(Cell* cell) {
+  if (!cell->is<JSObject>()) {
+    return nullptr;
+  }
+
+  JSObject* object = cell->as<JSObject>();
+  JSWeakmapKeyDelegateOp op = object->getClass()->extWeakmapKeyDelegateOp();
+  if (!op) {
+    return nullptr;
+  }
+
+  return op(object);
+}
+
+bool js::gc::CheckWeakMapEntryMarking(const WeakMapBase* map, Cell* key,
+                                      Cell* value) {
+  Zone* zone = map->zone();
+
+  JSObject* object = map->memberOf;
+  MOZ_ASSERT_IF(object, object->zone() == zone);
+
+  // Debugger weak maps can have keys in different zones.
+  DebugOnly<Zone*> keyZone = GetCellZone(key);
+  MOZ_ASSERT_IF(!map->allowKeysInOtherZones(),
+                keyZone == zone || keyZone->isAtomsZone());
+
+  DebugOnly<Zone*> valueZone = GetCellZone(value);
+  MOZ_ASSERT(valueZone == zone || valueZone->isAtomsZone());
+
+  // We may not know the color of the map, but we know that it's
+  // alive so it must at least be marked gray.
+  CellColor mapColor = object ? GetCellColor(object) : CellColor::Gray;
+
+  CellColor keyColor = GetCellColor(key);
+  CellColor valueColor = GetCellColor(value);
+
+  if (valueColor < Min(mapColor, keyColor)) {
+    fprintf(stderr, "WeakMap value is less marked than map and key\n");
+    fprintf(stderr, "(map %p is %s, key %p is %s, value %p is %s)\n", map,
+            CellColorName(mapColor), key, CellColorName(keyColor), value,
+            CellColorName(valueColor));
+    return false;
+  }
+
+  // Debugger weak maps map have keys in zones that are not or are
+  // no longer collecting. We can't make guarantees about the mark
+  // state of these keys.
+  if (map->allowKeysInOtherZones() &&
+      !(keyZone->isGCMarking() || keyZone->isGCSweeping())) {
+    return true;
+  }
+
+  JSObject* delegate = MaybeGetDelegate(key);
+  if (!delegate) {
+    return true;
+  }
+
+  CellColor delegateColor;
+  if (delegate->zone()->isGCMarking() || delegate->zone()->isGCSweeping()) {
+    delegateColor = GetCellColor(delegate);
+  } else {
+    // IsMarked() assumes cells in uncollected zones are marked.
+    delegateColor = CellColor::Black;
+  }
+
+  if (keyColor < Min(mapColor, delegateColor)) {
+    fprintf(stderr, "WeakMap key is less marked than map and delegate\n");
+    fprintf(stderr, "(map %p is %s, delegate %p is %s, key %p is %s)\n", map,
+            CellColorName(mapColor), delegate, CellColorName(delegateColor),
+            key, CellColorName(keyColor));
+    return false;
+  }
+
+  return true;
+}
+
 #endif  // defined(JS_GC_ZEAL) || defined(DEBUG)
--- a/js/src/gc/WeakMap-inl.h
+++ b/js/src/gc/WeakMap-inl.h
@@ -242,11 +242,28 @@ void WeakMap<K, V>::assertEntriesNotAbou
     K k(r.front().key());
     MOZ_ASSERT(!gc::IsAboutToBeFinalized(&k));
     MOZ_ASSERT(!gc::IsAboutToBeFinalized(&r.front().value()));
     MOZ_ASSERT(k == r.front().key());
   }
 }
 #endif
 
+#ifdef JS_GC_ZEAL
+template <class K, class V>
+bool WeakMap<K, V>::checkMarking() const {
+  bool ok = true;
+  for (Range r = Base::all(); !r.empty(); r.popFront()) {
+    gc::Cell* key = gc::ToMarkable(r.front().key());
+    gc::Cell* value = gc::ToMarkable(r.front().value());
+    if (key && value) {
+      if (!gc::CheckWeakMapEntryMarking(this, key, value)) {
+        ok = false;
+      }
+    }
+  }
+  return ok;
+}
+#endif
+
 } /* namespace js */
 
 #endif /* gc_WeakMap_inl_h */
--- a/js/src/gc/WeakMap.cpp
+++ b/js/src/gc/WeakMap.cpp
@@ -40,16 +40,32 @@ void WeakMapBase::unmarkZone(JS::Zone* z
 void WeakMapBase::traceZone(JS::Zone* zone, JSTracer* tracer) {
   MOZ_ASSERT(tracer->weakMapAction() != DoNotTraceWeakMaps);
   for (WeakMapBase* m : zone->gcWeakMapList()) {
     m->trace(tracer);
     TraceNullableEdge(tracer, &m->memberOf, "memberOf");
   }
 }
 
+#if defined(JS_GC_ZEAL) || defined(DEBUG)
+bool WeakMapBase::checkMarkingForZone(JS::Zone* zone) {
+  // This is called at the end of marking.
+  MOZ_ASSERT(zone->isGCMarking());
+
+  bool ok = true;
+  for (WeakMapBase* m : zone->gcWeakMapList()) {
+    if (m->marked && !m->checkMarking()) {
+      ok = false;
+    }
+  }
+
+  return ok;
+}
+#endif
+
 bool WeakMapBase::markZoneIteratively(JS::Zone* zone, GCMarker* marker) {
   bool markedAny = false;
   for (WeakMapBase* m : zone->gcWeakMapList()) {
     if (m->marked && m->markIteratively(marker)) {
       markedAny = true;
     }
   }
   return markedAny;
--- a/js/src/gc/WeakMap.h
+++ b/js/src/gc/WeakMap.h
@@ -20,17 +20,24 @@ class Zone;
 
 namespace js {
 
 class GCMarker;
 class WeakMapBase;
 struct WeakMapTracer;
 
 namespace gc {
+
 struct WeakMarkable;
+
+#if defined(JS_GC_ZEAL) || defined(DEBUG)
+// Check whether a weak map entry is marked correctly.
+bool CheckWeakMapEntryMarking(const WeakMapBase* map, Cell* key, Cell* value);
+#endif
+
 }  // namespace gc
 
 // A subclass template of js::HashMap whose keys and values may be
 // garbage-collected. When a key is collected, the table entry disappears,
 // dropping its reference to the value.
 //
 // More precisely:
 //
@@ -57,60 +64,70 @@ class WeakMapBase : public mozilla::Link
 
   JS::Zone* zone() const { return zone_; }
 
   // Garbage collector entry points.
 
   // Unmark all weak maps in a zone.
   static void unmarkZone(JS::Zone* zone);
 
-  // Mark all the weakmaps in a zone.
+  // Trace all the weakmaps in a zone.
   static void traceZone(JS::Zone* zone, JSTracer* tracer);
 
   // Check all weak maps in a zone that have been marked as live in this garbage
   // collection, and mark the values of all entries that have become strong
   // references to them. Return true if we marked any new values, indicating
   // that we need to make another pass. In other words, mark my marked maps'
   // marked members' mid-collection.
   static bool markZoneIteratively(JS::Zone* zone, GCMarker* marker);
 
   // Add zone edges for weakmaps with key delegates in a different zone.
   static bool findInterZoneEdges(JS::Zone* zone);
 
   // Sweep the weak maps in a zone, removing dead weak maps and removing
   // entries of live weak maps whose keys are dead.
   static void sweepZone(JS::Zone* zone);
 
-  // Trace all delayed weak map bindings. Used by the cycle collector.
+  // Trace all weak map bindings. Used by the cycle collector.
   static void traceAllMappings(WeakMapTracer* tracer);
 
   // Save information about which weak maps are marked for a zone.
   static bool saveZoneMarkedWeakMaps(JS::Zone* zone,
                                      WeakMapSet& markedWeakMaps);
 
   // Restore information about which weak maps are marked for many zones.
   static void restoreMarkedWeakMaps(WeakMapSet& markedWeakMaps);
 
+#if defined(JS_GC_ZEAL) || defined(DEBUG)
+  static bool checkMarkingForZone(JS::Zone* zone);
+#endif
+
  protected:
   // Instance member functions called by the above. Instantiations of WeakMap
   // override these with definitions appropriate for their Key and Value types.
   virtual void trace(JSTracer* tracer) = 0;
   virtual bool findZoneEdges() = 0;
   virtual void sweep() = 0;
   virtual void traceMappings(WeakMapTracer* tracer) = 0;
   virtual void clearAndCompact() = 0;
 
   // Any weakmap key types that want to participate in the non-iterative
   // ephemeron marking must override this method.
   virtual void markEntry(GCMarker* marker, gc::Cell* markedCell,
                          JS::GCCellPtr l) = 0;
 
   virtual bool markIteratively(GCMarker* marker) = 0;
 
- protected:
+#ifdef JS_GC_ZEAL
+  virtual bool checkMarking() const = 0;
+  virtual bool allowKeysInOtherZones() const { return false; }
+  friend bool gc::CheckWeakMapEntryMarking(const WeakMapBase*, gc::Cell*,
+                                           gc::Cell*);
+#endif
+
   // Object that this weak map is part of, if any.
   GCPtrObject memberOf;
 
   // Zone containing this weak map.
   JS::Zone* zone_;
 
   // Whether this object has been traced during garbage collection.
   bool marked;
@@ -195,16 +212,20 @@ class WeakMap
   // memberOf can be nullptr, which means that the map is not part of a
   // JSObject.
   void traceMappings(WeakMapTracer* tracer) override;
 
  protected:
 #if DEBUG
   void assertEntriesNotAboutToBeFinalized();
 #endif
+
+#ifdef JS_GC_ZEAL
+  bool checkMarking() const override;
+#endif
 };
 
 class ObjectValueMap : public WeakMap<HeapPtr<JSObject*>, HeapPtr<Value>> {
  public:
   ObjectValueMap(JSContext* cx, JSObject* obj) : WeakMap(cx, obj) {}
 
   bool findZoneEdges() override;
 };
--- a/js/src/vm/Debugger.h
+++ b/js/src/vm/Debugger.h
@@ -250,16 +250,22 @@ class DebuggerWeakMap
     CountMap::Ptr p = zoneCounts.lookup(zone);
     MOZ_ASSERT(p);
     MOZ_ASSERT(p->value() > 0);
     --p->value();
     if (p->value() == 0) {
       zoneCounts.remove(zone);
     }
   }
+
+#ifdef JS_GC_ZEAL
+  // Let the weak map marking verifier know that this map can
+  // contain keys in other zones.
+  virtual bool allowKeysInOtherZones() const override { return true; }
+#endif
 };
 
 class LeaveDebuggeeNoExecute;
 
 // Suppresses all debuggee NX checks, i.e., allow all execution. Used to allow
 // certain whitelisted operations to execute code.
 //
 // WARNING