Bug 1871303 - Ensure there is no more marking at the start of sweeping (ESR115) a=dmeehan

There are two issues here. The first is that there unexpected marking work at
the start of sweeping after entering from the mark phase without yielding. We
previously called assertNoMarkingWork() after markUntilBudgetExhausted() in the
marking phase so something since then must have added it.

As far as I can tell this must be the conditional call to
collectNurseryFromMajorGC(), where a post barrier for a pointer cleared during
finalization (e.g. for Maps in mapObject::sweepAfterMinorGC) ends up marking
something. I'm not sure such barriers are necessary, but for now the safest
thing to do is to move this nursery collection to the start of the slice so
that it happens before we drain the mark stack.

The second issue is that we check the budget and conditionally yield if we
enter from the marking state. The comment above this code states that this is
not safe since we have not yet started sweeping a sweep group. This check was
added in bug 1865383 but was not the main part of the fix. I think we should
remove this.

I wasn't able to come up with a test case to reproduce this.

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

js/src/gc/GC.cpp

@@ -3598,10 +3598,7 @@ void GCRuntime::incrementalSlice(SliceBudget& budget, JS::GCReason reason,
 
     case State::Mark:
       if (mightSweepInThisSlice(budget.isUnlimited())) {
-        // Trace wrapper rooters before marking if we might start sweeping in
-        // this slice.
-        rt->mainContextFromOwnThread()->traceWrapperGCRooters(
-            marker().tracer());
+        prepareForSweepSlice(reason);
       }
 
       {
@@ -3650,13 +3647,8 @@ void GCRuntime::incrementalSlice(SliceBudget& budget, JS::GCReason reason,
       [[fallthrough]];
 
     case State::Sweep:
-      if (storeBuffer().mayHavePointersToDeadCells()) {
-        collectNurseryFromMajorGC(reason);
-      }
-
       if (initialState == State::Sweep) {
-        rt->mainContextFromOwnThread()->traceWrapperGCRooters(
-            marker().tracer());
+        prepareForSweepSlice(reason);
       }
 
       if (performSweepActions(budget) == NotFinished) {

js/src/gc/GCRuntime.h

@@ -875,6 +875,7 @@ class GCRuntime {
   void sweepBackgroundThings(ZoneList& zones);
   void backgroundFinalize(JS::GCContext* gcx, Zone* zone, AllocKind kind,
                           Arena** empty);
+  void prepareForSweepSlice(JS::GCReason reason);
   void assertBackgroundSweepingFinished();
 
   bool allCCVisibleZonesWereCollected();

js/src/gc/Sweeping.cpp

@@ -2269,7 +2269,26 @@ bool GCRuntime::initSweepActions() {
   return sweepActions != nullptr;
 }
 
+void GCRuntime::prepareForSweepSlice(JS::GCReason reason) {
+  // Work that must be done at the start of each slice where we sweep.
+  //
+  // Since this must happen at the start of the slice, it must be called in
+  // marking slices before any sweeping happens. Therefore it is called
+  // conservatively since we may not always transition to sweeping from marking.
+
+  // Clear out whole cell store buffer entries to unreachable cells.
+  if (storeBuffer().mayHavePointersToDeadCells()) {
+    collectNurseryFromMajorGC(reason);
+  }
+
+  // Trace wrapper rooters before marking if we might start sweeping in
+  // this slice.
+  rt->mainContextFromOwnThread()->traceWrapperGCRooters(marker().tracer());
+}
+
 IncrementalProgress GCRuntime::performSweepActions(SliceBudget& budget) {
+  MOZ_ASSERT(!storeBuffer().mayHavePointersToDeadCells());
+
   AutoMajorGCProfilerEntry s(this);
   gcstats::AutoPhase ap(stats(), gcstats::PhaseKind::SWEEP);
 
@@ -2283,20 +2302,19 @@ IncrementalProgress GCRuntime::performSweepActions(SliceBudget& budget) {
   // Drain the mark stack, possibly in a parallel task if we're in a part of
   // sweeping that allows it.
   //
-  // In the first sweep slice where we must not yield to the mutator until we've
-  // starting sweeping a sweep group but in that case the stack must be empty
-  // already.
+  // The first time we enter the sweep phase we must not yield to the mutator
+  // until we've starting sweeping a sweep group but in that case the stack must
+  // be empty already.
 
-#ifdef DEBUG
   MOZ_ASSERT(initialState <= State::Sweep);
-  if (initialState != State::Sweep) {
-    assertNoMarkingWork();
-  }
-#endif
+  bool startOfSweeping = initialState < State::Sweep;
 
-  if (initialState == State::Sweep &&
-      markDuringSweeping(gcx, budget) == NotFinished) {
-    return NotFinished;
+  if (startOfSweeping) {
+    assertNoMarkingWork();
+  } else {
+    if (markDuringSweeping(gcx, budget) == NotFinished) {
+      return NotFinished;
+    }
   }
 
   // Then continue running sweep actions.