From 3e70d88d7a8e79583fb94db877a4ab33abe15107 Mon Sep 17 00:00:00 2001
From: Jon Coppeard <jcoppeard@mozilla.com>
Date: Wed, 24 Jul 2024 09:19:33 +0000
Subject: [PATCH] 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       | 12 ++----------
 js/src/gc/GCRuntime.h  |  1 +
 js/src/gc/Sweeping.cpp | 40 +++++++++++++++++++++++++++++-----------
 3 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/js/src/gc/GC.cpp b/js/src/gc/GC.cpp
index 33665e9b453e..e8af5a64685d 100644
--- a/js/src/gc/GC.cpp
+++ b/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) {
diff --git a/js/src/gc/GCRuntime.h b/js/src/gc/GCRuntime.h
index 53361140209a..179e189765c0 100644
--- a/js/src/gc/GCRuntime.h
+++ b/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();
diff --git a/js/src/gc/Sweeping.cpp b/js/src/gc/Sweeping.cpp
index 80f4a20e7402..1c236f6e4e99 100644
--- a/js/src/gc/Sweeping.cpp
+++ b/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.