cppgc: Fix ephemeron iterations

If processing the marking worklists found new ephemeron pairs, but
processing the existing ephemeron pairs didn't mark new objects, marking
would stop and the newly discovered ephemeron pairs would not be
processed. This can lead to a marked key with an unmarked value.

Bug: chromium:1252878
Change-Id: I0f158f6f64490f1f06961520b4ba57fa204bd867
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3199872
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#77197}

src/heap/cppgc/marker.cc

@@ -432,7 +432,9 @@ bool MarkerBase::ProcessWorklistsWithDeadline(
     size_t marked_bytes_deadline, v8::base::TimeTicks time_deadline) {
   StatsCollector::EnabledScope stats_scope(
       heap().stats_collector(), StatsCollector::kMarkTransitiveClosure);
+  bool saved_did_discover_new_ephemeron_pairs;
   do {
+    mutator_marking_state_.ResetDidDiscoverNewEphemeronPairs();
     if ((config_.marking_type == MarkingConfig::MarkingType::kAtomic) ||
         schedule_.ShouldFlushEphemeronPairs()) {
       mutator_marking_state_.FlushDiscoveredEphemeronPairs();
@@ -520,6 +522,8 @@ bool MarkerBase::ProcessWorklistsWithDeadline(
       }
     }
 
+    saved_did_discover_new_ephemeron_pairs =
+        mutator_marking_state_.DidDiscoverNewEphemeronPairs();
     {
       StatsCollector::EnabledScope inner_stats_scope(
           heap().stats_collector(), StatsCollector::kMarkProcessEphemerons);
@@ -533,7 +537,8 @@ bool MarkerBase::ProcessWorklistsWithDeadline(
         return false;
       }
     }
-  } while (!mutator_marking_state_.marking_worklist().IsLocalAndGlobalEmpty());
+  } while (!mutator_marking_state_.marking_worklist().IsLocalAndGlobalEmpty() ||
+           saved_did_discover_new_ephemeron_pairs);
   return true;
 }
 

src/heap/cppgc/marking-state.h

@@ -115,6 +115,14 @@ class MarkingStateBase {
     movable_slots_worklist_.reset();
   }
 
+  bool DidDiscoverNewEphemeronPairs() const {
+    return discovered_new_ephemeron_pairs_;
+  }
+
+  void ResetDidDiscoverNewEphemeronPairs() {
+    discovered_new_ephemeron_pairs_ = false;
+  }
+
  protected:
   inline void MarkAndPush(HeapObjectHeader&, TraceDescriptor);
 
@@ -150,6 +158,8 @@ class MarkingStateBase {
       movable_slots_worklist_;
 
   size_t marked_bytes_ = 0;
+  bool in_ephemeron_processing_ = false;
+  bool discovered_new_ephemeron_pairs_ = false;
 };
 
 MarkingStateBase::MarkingStateBase(HeapBase& heap,
@@ -286,6 +296,10 @@ void MarkingStateBase::ProcessWeakContainer(const void* object,
 void MarkingStateBase::ProcessEphemeron(const void* key, const void* value,
                                         TraceDescriptor value_desc,
                                         Visitor& visitor) {
+  // ProcessEphemeron is not expected to find new ephemerons recursively, which
+  // would break the main marking loop.
+  DCHECK(!in_ephemeron_processing_);
+  in_ephemeron_processing_ = true;
   // Filter out already marked keys. The write barrier for WeakMember
   // ensures that any newly set value after this point is kept alive and does
   // not require the callback.
@@ -299,9 +313,11 @@ void MarkingStateBase::ProcessEphemeron(const void* key, const void* value,
       // should be immediately traced.
       value_desc.callback(&visitor, value);
     }
-    return;
+  } else {
+    discovered_ephemeron_pairs_worklist_.Push({key, value, value_desc});
+    discovered_new_ephemeron_pairs_ = true;
   }
-  discovered_ephemeron_pairs_worklist_.Push({key, value, value_desc});
+  in_ephemeron_processing_ = false;
 }
 
 void MarkingStateBase::AccountMarkedBytes(const HeapObjectHeader& header) {

test/unittests/heap/cppgc/marker-unittest.cc

@@ -7,12 +7,14 @@
 #include <memory>
 
 #include "include/cppgc/allocation.h"
+#include "include/cppgc/ephemeron-pair.h"
 #include "include/cppgc/internal/pointer-policies.h"
 #include "include/cppgc/member.h"
 #include "include/cppgc/persistent.h"
 #include "include/cppgc/trace-trait.h"
 #include "src/heap/cppgc/heap-object-header.h"
 #include "src/heap/cppgc/marking-visitor.h"
+#include "src/heap/cppgc/object-allocator.h"
 #include "src/heap/cppgc/stats-collector.h"
 #include "test/unittests/heap/cppgc/tests.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -44,6 +46,8 @@ class MarkerTest : public testing::TestWithHeap {
 
   Marker* marker() const { return marker_.get(); }
 
+  void ResetMarker() { marker_.reset(); }
+
  private:
   std::unique_ptr<Marker> marker_;
 };
@@ -346,6 +350,50 @@ TEST_F(MarkerTest, SentinelNotClearedOnWeakPersistentHandling) {
   EXPECT_EQ(kSentinelPointer, root->weak_child());
 }
 
+namespace {
+
+class SimpleObject final : public GarbageCollected<SimpleObject> {
+ public:
+  void Trace(Visitor*) const {}
+};
+
+class ObjectWithEphemeronPair final
+    : public GarbageCollected<ObjectWithEphemeronPair> {
+ public:
+  explicit ObjectWithEphemeronPair(AllocationHandle& handle)
+      : ephemeron_pair_(MakeGarbageCollected<SimpleObject>(handle),
+                        MakeGarbageCollected<SimpleObject>(handle)) {}
+
+  void Trace(Visitor* visitor) const {
+    // First trace the ephemeron pair. The key is not yet marked as live, so the
+    // pair should be recorded for later processing. Then strongly mark the key.
+    // Marking the key will not trigger another worklist processing iteration,
+    // as it merely continues the same loop for regular objects and will leave
+    // the main marking worklist empty. If recording the ephemeron pair doesn't
+    // as well, we will get a crash when destroying the marker.
+    visitor->Trace(ephemeron_pair_);
+    visitor->Trace(const_cast<const SimpleObject*>(ephemeron_pair_.key.Get()));
+  }
+
+ private:
+  const EphemeronPair<SimpleObject, SimpleObject> ephemeron_pair_;
+};
+
+}  // namespace
+
+TEST_F(MarkerTest, MarkerProcessesAllEphemeronPairs) {
+  static const Marker::MarkingConfig config = {
+      MarkingConfig::CollectionType::kMajor,
+      MarkingConfig::StackState::kNoHeapPointers,
+      MarkingConfig::MarkingType::kAtomic};
+  Persistent<ObjectWithEphemeronPair> obj =
+      MakeGarbageCollected<ObjectWithEphemeronPair>(GetAllocationHandle(),
+                                                    GetAllocationHandle());
+  InitializeMarker(*Heap::From(GetHeap()), GetPlatformHandle().get(), config);
+  marker()->FinishMarking(MarkingConfig::StackState::kNoHeapPointers);
+  ResetMarker();
+}
+
 // Incremental Marking
 
 class IncrementalMarkingTest : public testing::TestWithHeap {