Bug 1578462 - Mark incoming CCW edges gray when the source is gray r=sfink

This splits marking of incoming CCWs from uncollected zones into two phases dependeing on the mark color of the source object. The targets of black or white wrappers are marked black (white means we don't know what color it should be), and targets of gray wrappers are marked gray. Where gray wrappers are later marked black by gray unmarking, we trigger a read barrier on the targed which will also have the effect of marking it black if it's in a zone we are marking.

I also added a shell function to dump information about cross compartment wrappers that was useful in debugging.

Differential Revision: https://phabricator.services.mozilla.com/D44666

--HG--
extra : moz-landing-system : lando

js/src/debugger/Debugger.cpp

@@ -3550,7 +3550,6 @@ void Debugger::traceCrossCompartmentEdges(JSTracer* trc) {
 void DebugAPI::traceCrossCompartmentEdges(JSTracer* trc) {
   JSRuntime* rt = trc->runtime();
   gc::State state = rt->gc.state();
-  MOZ_ASSERT(state == gc::State::MarkRoots || state == gc::State::Compact);
 
   for (Debugger* dbg : rt->debuggerList()) {
     Zone* zone = MaybeForwarded(dbg->object.get())->zone();

js/src/gc/GC.cpp

@@ -2474,19 +2474,19 @@ void GCRuntime::updateRuntimePointersToRelocatedCells(AutoGCSession& session) {
 
   rt->geckoProfiler().fixupStringsMapAfterMovingGC();
 
+  // Mark roots to update them.
+
   traceRuntimeForMajorGC(&trc, session);
 
-  // Mark roots to update them.
   {
     gcstats::AutoPhase ap2(stats(), gcstats::PhaseKind::MARK_ROOTS);
     DebugAPI::traceAllForMovingGC(&trc);
     DebugAPI::traceCrossCompartmentEdges(&trc);
 
-    // Mark all gray roots, making sure we call the trace callback to get the
-    // current set.
-    if (JSTraceDataOp op = grayRootTracer.op) {
-      (*op)(&trc, grayRootTracer.data);
-    }
+    // Mark all gray roots. We call the trace callback to get the current set.
+    traceEmbeddingGrayRoots(&trc);
+    Compartment::traceIncomingCrossCompartmentEdgesForZoneGC(
+        &trc, Compartment::GrayEdges);
   }
 
   // Sweep everything to fix up weak pointers.
@@ -4172,9 +4172,9 @@ void GCRuntime::markGrayRoots(gcstats::PhaseKind phase) {
     }
   } else {
     MOZ_ASSERT(!isIncremental);
-    if (JSTraceDataOp op = grayRootTracer.op) {
-      (*op)(&marker, grayRootTracer.data);
-    }
+    traceEmbeddingGrayRoots(&marker);
+    Compartment::traceIncomingCrossCompartmentEdgesForZoneGC(
+        &marker, Compartment::GrayEdges);
   }
 }
 

js/src/gc/Marking.cpp

@@ -3631,16 +3631,16 @@ bool UnmarkGrayTracer::onChild(const JS::GCCellPtr& thing) {
 
   TenuredCell& tenured = cell->asTenured();
 
-  // If the cell is in a zone that we're currently marking gray, then it's
-  // possible that it is currently white but will end up gray. To handle this
-  // case, push any cells in zones that are currently being marked onto the
-  // mark stack and they will eventually get marked black.
+  // If the cell is in a zone that we're currently marking, then it's possible
+  // that it is currently white but will end up gray. To handle this case, push
+  // any cells in zones that are currently being marked onto the mark stack and
+  // they will eventually get marked black.
   Zone* zone = tenured.zone();
-  if (zone->isGCMarkingBlackAndGray()) {
+  if (zone->isGCMarking()) {
     if (!cell->isMarkedBlack()) {
       Cell* tmp = cell;
-      TraceManuallyBarrieredGenericPointerEdge(zone->barrierTracer(), &tmp,
-                                               "read barrier");
+      JSTracer* trc = &runtime()->gc.marker;
+      TraceManuallyBarrieredGenericPointerEdge(trc, &tmp, "read barrier");
       MOZ_ASSERT(tmp == cell);
       unmarkedAny = true;
     }

js/src/gc/RootMarking.cpp

@@ -276,7 +276,15 @@ void js::gc::GCRuntime::traceRuntimeForMajorGC(JSTracer* trc,
     traceRuntimeAtoms(trc, session.checkAtomsAccess());
   }
   traceKeptAtoms(trc);
-  Compartment::traceIncomingCrossCompartmentEdgesForZoneGC(trc);
+
+  {
+    // Trace incoming cross compartment edges from uncollected compartments,
+    // skipping gray edges which are traced later.
+    gcstats::AutoPhase ap(stats(), gcstats::PhaseKind::MARK_CCWS);
+    Compartment::traceIncomingCrossCompartmentEdgesForZoneGC(
+        trc, Compartment::NonGrayEdges);
+  }
+
   traceRuntimeCommon(trc, MarkRuntime);
 }
 
@@ -524,9 +532,9 @@ void js::gc::GCRuntime::bufferGrayRoots() {
   }
 
   BufferGrayRootsTracer grayBufferer(rt);
-  if (JSTraceDataOp op = grayRootTracer.op) {
-    (*op)(&grayBufferer, grayRootTracer.data);
-  }
+  traceEmbeddingGrayRoots(&grayBufferer);
+  Compartment::traceIncomingCrossCompartmentEdgesForZoneGC(
+      &grayBufferer, Compartment::GrayEdges);
 
   // Propagate the failure flag from the marker to the runtime.
   if (grayBufferer.failed()) {

js/src/jit-test/tests/gc/bug-1578462.js

@@ -0,0 +1,52 @@
+// |jit-test| skip-if: !('gcstate' in this)
+
+// Check that incoming CCW edges are marked gray in a zone GC if the
+// source is gray.
+
+// Set up a new compartment with a gray object wrapper to this
+// compartment.
+function createOtherCompartment() {
+    let t = {};
+    addMarkObservers([t]);
+    let g = newGlobal({newCompartment: true});
+    g.t = t;
+    g.eval(`grayRoot().push(t);`);
+    g.t = null;
+    t = null;
+    return g;
+}
+
+gczeal(0);
+
+let g = createOtherCompartment();
+
+// The target should be gray in a full GC...
+gc();
+assertEq(getMarks()[0], "gray");
+
+// and subsequently gray in a zone GC of only this compartment.
+gc(this);
+assertEq(getMarks()[0], "gray");
+
+// If a barrier marks the gray wrapper black after the start of the
+// GC, the target ends up black.
+schedulegc(this);
+startgc(1);
+assertEq(getMarks()[0], "unmarked");
+g.eval(`grayRoot()`); // Barrier marks gray roots black.
+assertEq(getMarks()[0], "black");
+finishgc();
+
+// A full collection resets the wrapper to gray.
+gc();
+assertEq(getMarks()[0], "gray");
+
+// If a barrier marks the gray wrapper black after the target has
+// already been marked gray, the target ends up black.
+gczeal(25); // Yield during gray marking.
+schedulegc(this);
+startgc(1);
+assertEq(getMarks()[0], "gray");
+g.eval(`grayRoot()`); // Barrier marks gray roots black.
+assertEq(getMarks()[0], "black");
+finishgc();

js/src/shell/js.cpp

@@ -6323,6 +6323,44 @@ static bool RecomputeWrappers(JSContext* cx, unsigned argc, Value* vp) {
   return true;
 }
 
+static bool DumpObjectWrappers(JSContext* cx, unsigned argc, Value* vp) {
+  CallArgs args = CallArgsFromVp(argc, vp);
+
+  bool printedHeader = false;
+  for (ZonesIter zone(cx->runtime(), WithAtoms); !zone.done(); zone.next()) {
+    bool printedZoneInfo = false;
+    for (CompartmentsInZoneIter comp(zone); !comp.done(); comp.next()) {
+      bool printedCompartmentInfo = false;
+      for (Compartment::ObjectWrapperEnum e(comp); !e.empty(); e.popFront()) {
+        JSObject* wrapper = e.front().value().unbarrieredGet();
+        JSObject* wrapped = e.front().key();
+        if (!printedHeader) {
+          fprintf(stderr, "Cross-compartment object wrappers:\n");
+          printedHeader = true;
+        }
+        if (!printedZoneInfo) {
+          fprintf(stderr, "  Zone %p:\n", zone.get());
+          printedZoneInfo = true;
+        }
+        if (!printedCompartmentInfo) {
+          fprintf(stderr, "    Compartment %p:\n", comp.get());
+          printedCompartmentInfo = true;
+        }
+        fprintf(stderr,
+                "      Object wrapper %p -> %p in zone %p compartment %p\n",
+                wrapper, wrapped, wrapped->zone(), wrapped->compartment());
+      }
+    }
+  }
+
+  if (!printedHeader) {
+    fprintf(stderr, "No cross-compartment object wrappers.\n");
+  }
+
+  args.rval().setUndefined();
+  return true;
+}
+
 static bool GetMaxArgs(JSContext* cx, unsigned argc, Value* vp) {
   CallArgs args = CallArgsFromVp(argc, vp);
   args.rval().setInt32(ARGS_LENGTH_MAX);
@@ -8836,6 +8874,10 @@ JS_FN_HELP("parseBin", BinParse, 1, 0,
 "  and can be used to filter source or target compartments: the unwrapped\n"
 "  object's compartment is used as CompartmentFilter.\n"),
 
+    JS_FN_HELP("dumpObjectWrappers", DumpObjectWrappers, 2, 0,
+"dumpObjectWrappers()",
+"  Print information about cross-compartment object wrappers.\n"),
+
     JS_FN_HELP("wrapWithProto", WrapWithProto, 2, 0,
 "wrapWithProto(obj)",
 "  Wrap an object into a noop wrapper with prototype semantics."),

js/src/vm/Compartment.cpp

@@ -419,7 +419,19 @@ bool Compartment::wrap(JSContext* cx, MutableHandle<GCVector<Value>> vec) {
   return true;
 }
 
-void Compartment::traceWrapperTargetsInCollectedZones(JSTracer* trc) {
+static inline bool ShouldTraceWrapper(JSObject* wrapper,
+                                      Compartment::EdgeSelector whichEdges) {
+  if (whichEdges == Compartment::AllEdges) {
+    return true;
+  }
+
+  bool isGray = wrapper->isMarkedGray();
+  return (whichEdges == Compartment::NonGrayEdges && !isGray) ||
+         (whichEdges == Compartment::GrayEdges && isGray);
+}
+
+void Compartment::traceWrapperTargetsInCollectedZones(JSTracer* trc,
+                                                      EdgeSelector whichEdges) {
   // Trace cross compartment wrapper private pointers into collected zones to
   // either mark or update them. Wrapped object pointers are updated by
   // sweepCrossCompartmentObjectWrappers().
@@ -430,33 +442,39 @@ void Compartment::traceWrapperTargetsInCollectedZones(JSTracer* trc) {
 
   for (WrappedObjectCompartmentEnum c(this); !c.empty(); c.popFront()) {
     Zone* zone = c.front()->zone();
-    if (!zone->isCollecting()) {
+    if (!zone->isCollectingFromAnyThread()) {
       continue;
     }
 
     for (ObjectWrapperEnum e(this, c); !e.empty(); e.popFront()) {
       JSObject* obj = e.front().value().unbarrieredGet();
       ProxyObject* wrapper = &obj->as<ProxyObject>();
-      ProxyObject::traceEdgeToTarget(trc, wrapper);
+      if (ShouldTraceWrapper(wrapper, whichEdges)) {
+        ProxyObject::traceEdgeToTarget(trc, wrapper);
+      }
     }
   }
 }
 
 /* static */
-void Compartment::traceIncomingCrossCompartmentEdgesForZoneGC(JSTracer* trc) {
-  gcstats::AutoPhase ap(trc->runtime()->gc.stats(),
-                        gcstats::PhaseKind::MARK_CCWS);
+void Compartment::traceIncomingCrossCompartmentEdgesForZoneGC(
+    JSTracer* trc, EdgeSelector whichEdges) {
   MOZ_ASSERT(JS::RuntimeHeapIsMajorCollecting());
+
   for (ZonesIter zone(trc->runtime(), SkipAtoms); !zone.done(); zone.next()) {
-    if (zone->isCollecting()) {
+    if (zone->isCollectingFromAnyThread()) {
       continue;
     }
 
     for (CompartmentsInZoneIter c(zone); !c.done(); c.next()) {
-      c->traceWrapperTargetsInCollectedZones(trc);
+      c->traceWrapperTargetsInCollectedZones(trc, whichEdges);
     }
   }
-  DebugAPI::traceCrossCompartmentEdges(trc);
+
+  // Currently we trace all debugger edges as black.
+  if (whichEdges != GrayEdges) {
+    DebugAPI::traceCrossCompartmentEdges(trc);
+  }
 }
 
 void Compartment::sweepAfterMinorGC(JSTracer* trc) {
@@ -482,7 +500,7 @@ void Compartment::fixupCrossCompartmentObjectWrappersAfterMovingGC(
 
   // Trace the wrappers in the map to update their cross-compartment edges
   // to wrapped values in other compartments that may have been moved.
-  traceWrapperTargetsInCollectedZones(trc);
+  traceWrapperTargetsInCollectedZones(trc, AllEdges);
 }
 
 void Compartment::fixupAfterMovingGC(JSTracer* trc) {

js/src/vm/Compartment.h

@@ -412,8 +412,11 @@ class JS::Compartment {
    * dangling (full GCs naturally follow pointers across compartments) and
    * when compacting to update cross-compartment pointers.
    */
-  void traceWrapperTargetsInCollectedZones(JSTracer* trc);
-  static void traceIncomingCrossCompartmentEdgesForZoneGC(JSTracer* trc);
+  enum EdgeSelector { AllEdges, NonGrayEdges, GrayEdges };
+  void traceWrapperTargetsInCollectedZones(JSTracer* trc,
+                                           EdgeSelector whichEdges);
+  static void traceIncomingCrossCompartmentEdgesForZoneGC(
+      JSTracer* trc, EdgeSelector whichEdges);
 
   void sweepRealms(JSFreeOp* fop, bool keepAtleastOne, bool destroyingRuntime);
   void sweepAfterMinorGC(JSTracer* trc);