cppgc: young-gen: Always execute custom weak callbacks for old objects

Custom callbacks assume that untraced pointers always point to valid,
not freed objects. They must make sure that upon callback completion no
UntracedMembers point to an unreachable object. This may not hold true
if a custom callback for an old object operates with a reference to a
young object that was freed on a minor collection cycle. To maintain
the mentioned invariant, the CL calls custom callbacks for old objects
on every minor collection cycle.

The alternative options could be:
1) Replacing all UntracedMembers with WeakMembers, since WeakMember
   supports tracing and the barrier.
2) Emitting the generational barrier for UntracedMember + tracing
   UntracedMember on minor collection cycles.
The first option requires changing multiple use sites and can bring some
performance regression. The second option requires changing the GC logic
and the semantics of UntracedMember.

Bug: chromium:1029379
Change-Id: I9bb89e4787daf05990feed374dceca940be7be63
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3472499Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Anton Bikineev <bikineev@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79221}

src/heap/cppgc/marker.cc

@@ -319,12 +319,32 @@ void MarkerBase::ProcessWeakness() {
   heap().GetWeakCrossThreadPersistentRegion().Trace(&visitor());
 
   // Call weak callbacks on objects that may now be pointing to dead objects.
-  MarkingWorklists::WeakCallbackItem item;
   LivenessBroker broker = LivenessBrokerFactory::Create();
+#if defined(CPPGC_YOUNG_GENERATION)
+  auto& remembered_set = heap().remembered_set();
+  if (config_.collection_type == MarkingConfig::CollectionType::kMinor) {
+    // Custom callbacks assume that untraced pointers point to not yet freed
+    // objects. They must make sure that upon callback completion no
+    // UntracedMember points to a freed object. This may not hold true if a
+    // custom callback for an old object operates with a reference to a young
+    // object that was freed on a minor collection cycle. To maintain the
+    // invariant that UntracedMembers always point to valid objects, execute
+    // custom callbacks for old objects on each minor collection cycle.
+    remembered_set.ExecuteCustomCallbacks(broker);
+  } else {
+    // For major GCs, just release all the remembered weak callbacks.
+    remembered_set.ReleaseCustomCallbacks();
+  }
+#endif  // defined(CPPGC_YOUNG_GENERATION)
+
+  MarkingWorklists::WeakCallbackItem item;
   MarkingWorklists::WeakCallbackWorklist::Local& local =
       mutator_marking_state_.weak_callback_worklist();
   while (local.Pop(&item)) {
     item.callback(broker, item.parameter);
+#if defined(CPPGC_YOUNG_GENERATION)
+    heap().remembered_set().AddWeakCallback(item);
+#endif  // defined(CPPGC_YOUNG_GENERATION)
   }
 
   // Weak callbacks should not add any new objects for marking.

src/heap/cppgc/remembered-set.cc

@@ -82,6 +82,13 @@ void OldToNewRememberedSet::AddSourceObject(HeapObjectHeader& hoh) {
   remembered_source_objects_.insert(&hoh);
 }
 
+void OldToNewRememberedSet::AddWeakCallback(WeakCallbackItem item) {
+  DCHECK(!BasePage::FromInnerAddress(&heap_, item.parameter)
+              ->ObjectHeaderFromInnerAddress(item.parameter)
+              .IsYoung());
+  remembered_weak_callbacks_.insert(item);
+}
+
 void OldToNewRememberedSet::InvalidateRememberedSlotsInRange(void* begin,
                                                              void* end) {
   // TODO(1029379): The 2 binary walks can be optimized with a custom algorithm.
@@ -109,6 +116,16 @@ void OldToNewRememberedSet::Visit(Visitor& visitor,
   VisitRememberedSourceObjects(remembered_source_objects_, visitor);
 }
 
+void OldToNewRememberedSet::ExecuteCustomCallbacks(LivenessBroker broker) {
+  for (const auto& callback : remembered_weak_callbacks_) {
+    callback.callback(broker, callback.parameter);
+  }
+}
+
+void OldToNewRememberedSet::ReleaseCustomCallbacks() {
+  remembered_weak_callbacks_.clear();
+}
+
 void OldToNewRememberedSet::Reset() {
   remembered_slots_.clear();
   remembered_source_objects_.clear();

src/heap/cppgc/remembered-set.h

@@ -8,10 +8,12 @@
 #include <set>
 
 #include "src/base/macros.h"
+#include "src/heap/cppgc/marking-worklists.h"
 
 namespace cppgc {
 
 class Visitor;
+class LivenessBroker;
 
 namespace internal {
 
@@ -21,27 +23,41 @@ class MutatorMarkingState;
 
 class V8_EXPORT_PRIVATE OldToNewRememberedSet final {
  public:
-  explicit OldToNewRememberedSet(const HeapBase& heap) : heap_(heap) {}
+  using WeakCallbackItem = MarkingWorklists::WeakCallbackItem;
+
+  explicit OldToNewRememberedSet(const HeapBase& heap)
+      : heap_(heap), remembered_weak_callbacks_(compare_parameter) {}
 
   OldToNewRememberedSet(const OldToNewRememberedSet&) = delete;
   OldToNewRememberedSet& operator=(const OldToNewRememberedSet&) = delete;
 
   void AddSlot(void* slot);
   void AddSourceObject(HeapObjectHeader& source_hoh);
+  void AddWeakCallback(WeakCallbackItem);
 
   void InvalidateRememberedSlotsInRange(void* begin, void* end);
   void InvalidateRememberedSourceObject(HeapObjectHeader& source_hoh);
 
   void Visit(Visitor&, MutatorMarkingState&);
 
+  void ExecuteCustomCallbacks(LivenessBroker);
+  void ReleaseCustomCallbacks();
+
   void Reset();
 
  private:
   friend class MinorGCTest;
 
+  static constexpr auto compare_parameter = [](const WeakCallbackItem& lhs,
+                                               const WeakCallbackItem& rhs) {
+    return lhs.parameter < rhs.parameter;
+  };
+
   const HeapBase& heap_;
   std::set<void*> remembered_slots_;
   std::set<HeapObjectHeader*> remembered_source_objects_;
+  std::set<WeakCallbackItem, decltype(compare_parameter)>
+      remembered_weak_callbacks_;
 };
 
 }  // namespace internal

test/unittests/heap/cppgc/minor-gc-unittest.cc

@@ -528,6 +528,43 @@ TYPED_TEST(MinorGCTestForType, GenerationalBarrierDeferredTracing) {
   EXPECT_EQ(0u, remembered_objects.size());
 }
 
+namespace {
+class GCedWithCustomWeakCallback final
+    : public GarbageCollected<GCedWithCustomWeakCallback> {
+ public:
+  static size_t custom_callback_called;
+
+  void CustomWeakCallbackMethod(const LivenessBroker& broker) {
+    custom_callback_called++;
+  }
+
+  void Trace(cppgc::Visitor* visitor) const {
+    visitor->RegisterWeakCallbackMethod<
+        GCedWithCustomWeakCallback,
+        &GCedWithCustomWeakCallback::CustomWeakCallbackMethod>(this);
+  }
+};
+size_t GCedWithCustomWeakCallback::custom_callback_called = 0;
+}  // namespace
+
+TEST_F(MinorGCTest, ReexecuteCustomCallback) {
+  // Create an object with additional kBytesToAllocate bytes.
+  Persistent<GCedWithCustomWeakCallback> old =
+      MakeGarbageCollected<GCedWithCustomWeakCallback>(GetAllocationHandle());
+
+  CollectMinor();
+  EXPECT_EQ(1u, GCedWithCustomWeakCallback::custom_callback_called);
+
+  CollectMinor();
+  EXPECT_EQ(2u, GCedWithCustomWeakCallback::custom_callback_called);
+
+  CollectMinor();
+  EXPECT_EQ(3u, GCedWithCustomWeakCallback::custom_callback_called);
+
+  CollectMajor();
+  // The callback must be called only once.
+  EXPECT_EQ(4u, GCedWithCustomWeakCallback::custom_callback_called);
+}
 }  // namespace internal
 }  // namespace cppgc