cppgc: Fix marking of ephemerons with keys in construction

mlippautz · mlippautz · commit 4feccb9b4e4a · 2021-10-16T09:26:26.000Z
Consider in-construction keys as live during the final GC pause. Bug: chromium:1259587 Change-Id: Ia8c05923db6e5827b68b17a51561fbc8b2c4b467 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3221153 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Anton Bikineev <bikineev@chromium.org> Cr-Commit-Position: refs/heads/main@{#77386} (cherry picked from commit 32a09a6) Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3226785 Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
diff --git a/src/heap/cppgc/marker.cc b/src/heap/cppgc/marker.cc
@@ -243,6 +243,7 @@ void MarkerBase::EnterAtomicPause(MarkingConfig::StackState stack_state) {
   }
   config_.stack_state = stack_state;
   config_.marking_type = MarkingConfig::MarkingType::kAtomic;
+  mutator_marking_state_.set_in_atomic_pause();
 
   {
     // VisitRoots also resets the LABs.
diff --git a/src/heap/cppgc/marking-state.h b/src/heap/cppgc/marking-state.h
@@ -9,6 +9,7 @@
 
 #include "include/cppgc/trace-trait.h"
 #include "include/cppgc/visitor.h"
+#include "src/base/logging.h"
 #include "src/heap/cppgc/compaction-worklists.h"
 #include "src/heap/cppgc/globals.h"
 #include "src/heap/cppgc/heap-object-header.h"
@@ -123,6 +124,8 @@ class MarkingStateBase {
     discovered_new_ephemeron_pairs_ = false;
   }
 
+  void set_in_atomic_pause() { in_atomic_pause_ = true; }
+
  protected:
   inline void MarkAndPush(HeapObjectHeader&, TraceDescriptor);
 
@@ -160,6 +163,7 @@ class MarkingStateBase {
   size_t marked_bytes_ = 0;
   bool in_ephemeron_processing_ = false;
   bool discovered_new_ephemeron_pairs_ = false;
+  bool in_atomic_pause_ = false;
 };
 
 MarkingStateBase::MarkingStateBase(HeapBase& heap,
@@ -300,12 +304,19 @@ void MarkingStateBase::ProcessEphemeron(const void* key, const void* value,
   // 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.
-  if (!HeapObjectHeader::FromObject(key)
-           .IsInConstruction<AccessMode::kAtomic>() &&
-      HeapObjectHeader::FromObject(key).IsMarked<AccessMode::kAtomic>()) {
+  // Keys are considered live even in incremental/concurrent marking settings
+  // because the write barrier for WeakMember ensures that any newly set value
+  // after this point is kept alive and does not require the callback.
+  const bool key_in_construction =
+      HeapObjectHeader::FromObject(key).IsInConstruction<AccessMode::kAtomic>();
+  const bool key_considered_as_live =
+      key_in_construction
+          ? in_atomic_pause_
+          : HeapObjectHeader::FromObject(key).IsMarked<AccessMode::kAtomic>();
+  DCHECK_IMPLIES(
+      key_in_construction && in_atomic_pause_,
+      HeapObjectHeader::FromObject(key).IsMarked<AccessMode::kAtomic>());
+  if (key_considered_as_live) {
     if (value_desc.base_object_payload) {
       MarkAndPush(value_desc.base_object_payload, value_desc);
     } else {
diff --git a/test/unittests/heap/cppgc/ephemeron-pair-unittest.cc b/test/unittests/heap/cppgc/ephemeron-pair-unittest.cc
@@ -242,5 +242,50 @@ TEST_F(EphemeronPairTest, EphemeronPairWithEmptyMixinValue) {
   FinishMarking();
 }
 
+namespace {
+
+class KeyWithCallback final : public GarbageCollected<KeyWithCallback> {
+ public:
+  template <typename Callback>
+  explicit KeyWithCallback(Callback callback) {
+    callback(this);
+  }
+  void Trace(Visitor*) const {}
+};
+
+class EphemeronHolderForKeyWithCallback final
+    : public GarbageCollected<EphemeronHolderForKeyWithCallback> {
+ public:
+  EphemeronHolderForKeyWithCallback(KeyWithCallback* key, GCed* value)
+      : ephemeron_pair_(key, value) {}
+  void Trace(cppgc::Visitor* visitor) const { visitor->Trace(ephemeron_pair_); }
+
+ private:
+  const EphemeronPair<KeyWithCallback, GCed> ephemeron_pair_;
+};
+
+}  // namespace
+
+TEST_F(EphemeronPairTest, EphemeronPairWithKeyInConstruction) {
+  GCed* value = MakeGarbageCollected<GCed>(GetAllocationHandle());
+  Persistent<EphemeronHolderForKeyWithCallback> holder;
+  InitializeMarker(*Heap::From(GetHeap()), GetPlatformHandle().get());
+  FinishSteps();
+  MakeGarbageCollected<KeyWithCallback>(
+      GetAllocationHandle(), [this, &holder, value](KeyWithCallback* thiz) {
+        // The test doesn't use conservative stack scanning to retain key to
+        // avoid retaining value as a side effect.
+        EXPECT_TRUE(HeapObjectHeader::FromObject(thiz).TryMarkAtomic());
+        holder = MakeGarbageCollected<EphemeronHolderForKeyWithCallback>(
+            GetAllocationHandle(), thiz, value);
+        // Finishing marking at this point will leave an ephemeron pair
+        // reachable where the key is still in construction. The GC needs to
+        // mark the value for such pairs as live in the atomic pause as they key
+        // is considered live.
+        FinishMarking();
+      });
+  EXPECT_TRUE(HeapObjectHeader::FromObject(value).IsMarked());
+}
+
 }  // namespace internal
 }  // namespace cppgc

Original file line number	Diff line number	Diff line change
`@@ -243,6 +243,7 @@ void MarkerBase::EnterAtomicPause(MarkingConfig::StackState stack_state) {`
`243`	`243`	`}`
`244`	`244`	`config_.stack_state = stack_state;`
`245`	`245`	`config_.marking_type = MarkingConfig::MarkingType::kAtomic;`
	`246`	`+ mutator_marking_state_.set_in_atomic_pause();`
`246`	`247`
`247`	`248`	`{`
`248`	`249`	`// VisitRoots also resets the LABs.`