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 idunknown
push userunknown
push dateunknown
reviewerssfink
bugs1509923
milestone65.0a1
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