Bug 1463462 - Make weak map marking take account of the fact that black and gray marking can now be interleaved r=sfink

js/src/gc/GCMarker.h

@@ -271,6 +271,21 @@ class GCMarker : public JSTracer {
   void setMarkColor(gc::MarkColor newColor);
   gc::MarkColor markColor() const { return color; }
 
+  // Return whether a cell is marked relative to the current marking color. If
+  // the cell is black then this returns true, but if it's gray it will return
+  // false if the mark color is black.
+  template <typename T>
+  bool isMarked(T* thingp) {
+    return color == gc::MarkColor::Black ? gc::IsMarkedBlack(runtime(), thingp)
+                                         : gc::IsMarked(runtime(), thingp);
+  }
+  template <typename T>
+  bool isMarkedUnbarriered(T* thingp) {
+    return color == gc::MarkColor::Black
+               ? gc::IsMarkedBlackUnbarriered(runtime(), thingp)
+               : gc::IsMarkedUnbarriered(runtime(), thingp);
+  }
+
   void enterWeakMarkingMode();
   void leaveWeakMarkingMode();
   void abortLinearWeakMarking() {

js/src/gc/Marking.cpp

@@ -616,9 +616,15 @@ struct ImplicitEdgeHolderType<JSScript*> {
 
 void GCMarker::markEphemeronValues(gc::Cell* markedCell,
                                    WeakEntryVector& values) {
-  size_t initialLen = values.length();
-  for (size_t i = 0; i < initialLen; i++) {
-    values[i].weakmap->markEntry(this, markedCell, values[i].key);
+  DebugOnly<size_t> initialLen = values.length();
+
+  for (const auto& markable : values) {
+    if (color == gc::MarkColor::Black &&
+        markable.weakmap->markColor == gc::MarkColor::Gray) {
+      continue;
+    }
+
+    markable.weakmap->markEntry(this, markedCell, markable.key);
   }
 
   // The vector should not be appended to during iteration because the key is
@@ -3241,22 +3247,22 @@ static inline void CheckIsMarkedThing(T* thingp) {
 }
 
 template <typename T>
-static bool IsMarkedInternalCommon(T* thingp) {
+static inline bool ShouldCheckMarkState(JSRuntime* rt, T** thingp) {
   CheckIsMarkedThing(thingp);
   MOZ_ASSERT(!IsInsideNursery(*thingp));
 
   TenuredCell& thing = (*thingp)->asTenured();
   Zone* zone = thing.zoneFromAnyThread();
   if (!zone->isCollectingFromAnyThread() || zone->isGCFinished()) {
-    return true;
+    return false;
   }
 
   if (zone->isGCCompacting() && IsForwarded(*thingp)) {
     *thingp = Forwarded(*thingp);
-    return true;
+    return false;
   }
 
-  return thing.isMarkedAny();
+  return true;
 }
 
 template <typename T>
@@ -3277,7 +3283,30 @@ bool js::gc::IsMarkedInternal(JSRuntime* rt, T** thingp) {
     return Nursery::getForwardedPointer(cellp);
   }
 
-  return IsMarkedInternalCommon(thingp);
+  if (!ShouldCheckMarkState(rt, thingp)) {
+    return true;
+  }
+
+  return (*thingp)->asTenured().isMarkedAny();
+}
+
+template <typename T>
+bool js::gc::IsMarkedBlackInternal(JSRuntime* rt, T** thingp) {
+  if (IsOwnedByOtherRuntime(rt, *thingp)) {
+    return true;
+  }
+
+  if (MightBeNurseryAllocated<T>::value && IsInsideNursery(*thingp)) {
+    MOZ_ASSERT(CurrentThreadCanAccessRuntime(rt));
+    Cell** cellp = reinterpret_cast<Cell**>(thingp);
+    return Nursery::getForwardedPointer(cellp);
+  }
+
+  if (!ShouldCheckMarkState(rt, thingp)) {
+    return true;
+  }
+
+  return (*thingp)->asTenured().isMarkedBlack();
 }
 
 template <typename S>
@@ -3296,6 +3325,22 @@ bool js::gc::IsMarkedInternal(JSRuntime* rt, T* thingp) {
   return rv;
 }
 
+template <typename S>
+struct IsMarkedBlackFunctor : public IdentityDefaultAdaptor<S> {
+  template <typename T>
+  S operator()(T* t, JSRuntime* rt, bool* rv) {
+    *rv = IsMarkedBlackInternal(rt, &t);
+    return js::gc::RewrapTaggedPointer<S, T>::wrap(t);
+  }
+};
+
+template <typename T>
+bool js::gc::IsMarkedBlackInternal(JSRuntime* rt, T* thingp) {
+  bool rv = true;
+  *thingp = DispatchTyped(IsMarkedBlackFunctor<T>(), *thingp, rt, &rv);
+  return rv;
+}
+
 bool js::gc::IsAboutToBeFinalizedDuringSweep(TenuredCell& tenured) {
   MOZ_ASSERT(!IsInsideNursery(&tenured));
   MOZ_ASSERT(tenured.zoneFromAnyThread()->isGCSweeping());
@@ -3368,8 +3413,9 @@ FOR_EACH_PUBLIC_GC_POINTER_TYPE(INSTANTIATE_ALL_VALID_HEAP_TRACE_FUNCTIONS)
 FOR_EACH_PUBLIC_TAGGED_GC_POINTER_TYPE(
     INSTANTIATE_ALL_VALID_HEAP_TRACE_FUNCTIONS)
 
-#define INSTANTIATE_INTERNAL_MARKING_FUNCTIONS(type)          \
-  template bool IsMarkedInternal(JSRuntime* rt, type* thing); \
+#define INSTANTIATE_INTERNAL_MARKING_FUNCTIONS(type)               \
+  template bool IsMarkedInternal(JSRuntime* rt, type* thing);      \
+  template bool IsMarkedBlackInternal(JSRuntime* rt, type* thing); \
   template bool IsAboutToBeFinalizedInternal(type* thingp);
 
 #define INSTANTIATE_INTERNAL_MARKING_FUNCTIONS_FROM_TRACEKIND(_1, type, _2) \

js/src/gc/Marking.h

@@ -67,28 +67,46 @@ bool IsMarkedInternal(JSRuntime* rt, T* thing);
 template <typename T>
 bool IsMarkedInternal(JSRuntime* rt, T** thing);
 
+template <typename T>
+bool IsMarkedBlackInternal(JSRuntime* rt, T* thing);
+template <typename T>
+bool IsMarkedBlackInternal(JSRuntime* rt, T** thing);
+
 template <typename T>
 bool IsAboutToBeFinalizedInternal(T* thingp);
 template <typename T>
 bool IsAboutToBeFinalizedInternal(T** thingp);
 
-// Report whether a thing has been marked.  Things which are in zones that are
-// not currently being collected or are owned by another runtime are always
-// reported as being marked.
+// Report whether a GC thing has been marked with any color. Things which are in
+// zones that are not currently being collected or are owned by another runtime
+// are always reported as being marked.
 template <typename T>
 inline bool IsMarkedUnbarriered(JSRuntime* rt, T* thingp) {
   return IsMarkedInternal(rt, ConvertToBase(thingp));
 }
 
-// Report whether a thing has been marked.  Things which are in zones that are
-// not currently being collected or are owned by another runtime are always
-// reported as being marked.
+// Report whether a GC thing has been marked with any color. Things which are in
+// zones that are not currently being collected or are owned by another runtime
+// are always reported as being marked.
 template <typename T>
 inline bool IsMarked(JSRuntime* rt, WriteBarrieredBase<T>* thingp) {
   return IsMarkedInternal(rt,
                           ConvertToBase(thingp->unsafeUnbarrieredForTracing()));
 }
 
+// Report whether a GC thing has been marked black.
+template <typename T>
+inline bool IsMarkedBlackUnbarriered(JSRuntime* rt, T* thingp) {
+  return IsMarkedBlackInternal(rt, ConvertToBase(thingp));
+}
+
+// Report whether a GC thing has been marked black.
+template <typename T>
+inline bool IsMarkedBlack(JSRuntime* rt, WriteBarrieredBase<T>* thingp) {
+  return IsMarkedBlackInternal(
+      rt, ConvertToBase(thingp->unsafeUnbarrieredForTracing()));
+}
+
 template <typename T>
 inline bool IsAboutToBeFinalizedUnbarriered(T* thingp) {
   return IsAboutToBeFinalizedInternal(ConvertToBase(thingp));

js/src/gc/Verifier.cpp

@@ -778,9 +778,9 @@ bool js::gc::CheckWeakMapEntryMarking(const WeakMapBase* map, Cell* key,
   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 mapColor = map->markColor == MarkColor::Black ? CellColor::Black
+                                                          : CellColor::Gray;
+  MOZ_ASSERT_IF(object, GetCellColor(object) == mapColor);
 
   CellColor keyColor = GetCellColor(key);
   CellColor valueColor = valueZone->isGCMarking() ? GetCellColor(value)

js/src/gc/WeakMap-inl.h

@@ -34,7 +34,10 @@ WeakMap<K, V>::WeakMap(JSContext* cx, JSObject* memOf)
                 "Object's TraceKind should be added to CC graph.");
 
   zone()->gcWeakMapList().insertFront(this);
-  marked = JS::IsIncrementalGCInProgress(TlsContext.get());
+  if (zone()->wasGCStarted()) {
+    marked = true;
+    markColor = gc::MarkColor::Black;
+  }
 }
 
 // Trace a WeakMap entry based on 'markedCell' getting marked, where 'origKey'
@@ -58,9 +61,9 @@ void WeakMap<K, V>::markEntry(GCMarker* marker, gc::Cell* markedCell,
   K key(p->key());
   MOZ_ASSERT((markedCell == extractUnbarriered(key)) ||
              (markedCell == getDelegate(key)));
-  if (gc::IsMarked(marker->runtime(), &key)) {
+  if (marker->isMarked(&key)) {
     TraceEdge(marker, &p->value(), "ephemeron value");
-  } else if (keyNeedsMark(key)) {
+  } else if (keyNeedsMark(marker, key)) {
     TraceEdge(marker, &p->value(), "WeakMap ephemeron value");
     TraceEdge(marker, &key, "proxy-preserved WeakMap ephemeron key");
     MOZ_ASSERT(key == p->key());  // No moving
@@ -76,8 +79,10 @@ void WeakMap<K, V>::trace(JSTracer* trc) {
 
   if (trc->isMarkingTracer()) {
     MOZ_ASSERT(trc->weakMapAction() == ExpandWeakMaps);
+    auto marker = GCMarker::fromTracer(trc);
     marked = true;
-    (void)markIteratively(GCMarker::fromTracer(trc));
+    markColor = marker->markColor();
+    (void)markIteratively(marker);
     return;
   }
 
@@ -122,13 +127,17 @@ template <class K, class V>
 template <class K, class V>
 bool WeakMap<K, V>::markIteratively(GCMarker* marker) {
   MOZ_ASSERT(marked);
+  if (marker->markColor() == gc::MarkColor::Black &&
+      markColor == gc::MarkColor::Gray) {
+    return false;
+  }
 
   bool markedAny = false;
 
   for (Enum e(*this); !e.empty(); e.popFront()) {
     // If the entry is live, ensure its key and value are marked.
-    bool keyIsMarked = gc::IsMarked(marker->runtime(), &e.front().mutableKey());
-    if (!keyIsMarked && keyNeedsMark(e.front().key())) {
+    bool keyIsMarked = marker->isMarked(&e.front().mutableKey());
+    if (!keyIsMarked && keyNeedsMark(marker, e.front().key())) {
       TraceEdge(marker, &e.front().mutableKey(),
                 "proxy-preserved WeakMap entry key");
       keyIsMarked = true;
@@ -136,7 +145,7 @@ bool WeakMap<K, V>::markIteratively(GCMarker* marker) {
     }
 
     if (keyIsMarked) {
-      if (!gc::IsMarked(marker->runtime(), &e.front().value())) {
+      if (!marker->isMarked(&e.front().value())) {
         TraceEdge(marker, &e.front().value(), "WeakMap entry value");
         markedAny = true;
       }
@@ -186,23 +195,24 @@ inline JSObject* WeakMap<K, V>::getDelegate(LazyScript* script) const {
 }
 
 template <class K, class V>
-inline bool WeakMap<K, V>::keyNeedsMark(JSObject* key) const {
+inline bool WeakMap<K, V>::keyNeedsMark(GCMarker* marker, JSObject* key) const {
   JSObject* delegate = getDelegate(key);
   /*
    * Check if the delegate is marked with any color to properly handle
    * gray marking when the key's delegate is black and the map is gray.
    */
-  return delegate &&
-         gc::IsMarkedUnbarriered(zone()->runtimeFromMainThread(), &delegate);
+  return delegate && marker->isMarkedUnbarriered(&delegate);
 }
 
 template <class K, class V>
-inline bool WeakMap<K, V>::keyNeedsMark(JSScript* script) const {
+inline bool WeakMap<K, V>::keyNeedsMark(GCMarker* marker,
+                                        JSScript* script) const {
   return false;
 }
 
 template <class K, class V>
-inline bool WeakMap<K, V>::keyNeedsMark(LazyScript* script) const {
+inline bool WeakMap<K, V>::keyNeedsMark(GCMarker* marker,
+                                        LazyScript* script) const {
   return false;
 }
 

js/src/gc/WeakMap.cpp

@@ -23,7 +23,7 @@ using namespace js;
 using namespace js::gc;
 
 WeakMapBase::WeakMapBase(JSObject* memOf, Zone* zone)
-    : memberOf(memOf), zone_(zone), marked(false) {
+    : memberOf(memOf), zone_(zone), marked(false), markColor(MarkColor::Black) {
   MOZ_ASSERT_IF(memberOf, memberOf->compartment()->zone() == zone);
 }
 

js/src/gc/WeakMap.h

@@ -129,8 +129,10 @@ class WeakMapBase : public mozilla::LinkedListElement<WeakMapBase> {
   // Zone containing this weak map.
   JS::Zone* zone_;
 
-  // Whether this object has been traced during garbage collection.
+  // Whether this object has been marked during garbage collection and which
+  // color it was marked.
   bool marked;
+  gc::MarkColor markColor;
 };
 
 template <class Key, class Value>
@@ -193,9 +195,9 @@ class WeakMap
     JS::ExposeObjectToActiveJS(obj);
   }
 
-  bool keyNeedsMark(JSObject* key) const;
-  bool keyNeedsMark(JSScript* script) const;
-  bool keyNeedsMark(LazyScript* script) const;
+  bool keyNeedsMark(GCMarker* marker, JSObject* key) const;
+  bool keyNeedsMark(GCMarker* marker, JSScript* script) const;
+  bool keyNeedsMark(GCMarker* marker, LazyScript* script) const;
 
   bool findZoneEdges() override {
     // This is overridden by ObjectValueMap.