[heap] Conservatively scan for TracedNode GlobalHandle

v8::TracedReference is supposed to be used from objects allocated on
CppHeap. Such objects can be in construction during garbage
collection, meaning that they are unable to invoke
Trace(v8::TraceReference) as they have not been properly set up.

It is thus necessary to use conservative tracing to find
v8::TracedReference (backed by TracedNode in GlobalHandle) in
in-construction objects.

Change-Id: I5b4ac6e7805ff7ded33f63a405db65ea08d809ad
Bug: v8:13141, chromium:1322114
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3806439
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82188}

src/handles/global-handles.cc

@@ -6,6 +6,7 @@
 
 #include <algorithm>
 #include <atomic>
+#include <cstddef>
 #include <cstdint>
 #include <map>
 
@@ -86,6 +87,9 @@ class GlobalHandles::NodeBlock final {
   NodeBlock* next() const { return next_; }
   NodeBlock* next_used() const { return next_used_; }
 
+  const void* begin_address() const { return nodes_; }
+  const void* end_address() const { return &nodes_[kBlockSize]; }
+
  private:
   NodeType nodes_[kBlockSize];
   NodeBlock* const next_;
@@ -192,6 +196,7 @@ class GlobalHandles::NodeSpace final {
  public:
   using BlockType = NodeBlock<NodeType>;
   using iterator = NodeIterator<BlockType>;
+  using NodeBounds = GlobalHandles::NodeBounds;
 
   static NodeSpace* From(NodeType* node);
   static void Release(NodeType* node);
@@ -208,6 +213,8 @@ class GlobalHandles::NodeSpace final {
   size_t TotalSize() const { return blocks_ * sizeof(NodeType) * kBlockSize; }
   size_t handles_count() const { return handles_count_; }
 
+  NodeBounds GetNodeBlockBounds() const;
+
  private:
   void PutNodesOnFreeList(BlockType* block);
   V8_INLINE void Free(NodeType* node);
@@ -250,6 +257,21 @@ NodeType* GlobalHandles::NodeSpace<NodeType>::Allocate() {
   return node;
 }
 
+template <class NodeType>
+typename GlobalHandles::NodeSpace<NodeType>::NodeBounds
+GlobalHandles::NodeSpace<NodeType>::GetNodeBlockBounds() const {
+  NodeBounds block_bounds;
+  for (BlockType* current = first_used_block_; current;
+       current = current->next_used()) {
+    block_bounds.push_back({current->begin_address(), current->end_address()});
+  }
+  std::sort(block_bounds.begin(), block_bounds.end(),
+            [](const auto& pair1, const auto& pair2) {
+              return pair1.first < pair2.first;
+            });
+  return block_bounds;
+}
+
 template <class NodeType>
 void GlobalHandles::NodeSpace<NodeType>::PutNodesOnFreeList(BlockType* block) {
   for (int32_t i = kBlockSize - 1; i >= 0; --i) {
@@ -1122,12 +1144,27 @@ GlobalHandles* GlobalHandles::From(const TracedNode* node) {
              : NodeBlock<TracedNode>::From(node)->global_handles();
 }
 
+// static
 void GlobalHandles::MarkTraced(Address* location) {
   TracedNode* node = TracedNode::FromLocation(location);
   DCHECK(node->IsInUse());
   node->set_markbit<AccessMode::ATOMIC>();
 }
 
+// static
+Object GlobalHandles::MarkTracedConservatively(
+    Address* inner_location, Address* traced_node_block_base) {
+  // Compute the `TracedNode` address based on its inner pointer.
+  const ptrdiff_t delta = reinterpret_cast<uintptr_t>(inner_location) -
+                          reinterpret_cast<uintptr_t>(traced_node_block_base);
+  const auto index = delta / sizeof(TracedNode);
+  TracedNode& node =
+      reinterpret_cast<TracedNode*>(traced_node_block_base)[index];
+  if (!node.IsInUse()) return Smi::zero();
+  node.set_markbit<AccessMode::ATOMIC>();
+  return node.object();
+}
+
 void GlobalHandles::Destroy(Address* location) {
   if (location != nullptr) {
     NodeSpace<Node>::Release(Node::FromLocation(location));
@@ -1542,8 +1579,11 @@ void GlobalHandles::IterateAllRootsForTesting(
   }
 }
 
-DISABLE_CFI_PERF
+GlobalHandles::NodeBounds GlobalHandles::GetTracedNodeBounds() const {
-void GlobalHandles::IterateTracedNodes(
+  return traced_nodes_->GetNodeBlockBounds();
+}
+
+DISABLE_CFI_PERF void GlobalHandles::IterateTracedNodes(
     v8::EmbedderHeapTracer::TracedGlobalHandleVisitor* visitor) {
   for (TracedNode* node : *traced_nodes_) {
     if (node->IsInUse()) {

src/handles/global-handles.h

@@ -79,6 +79,8 @@ class V8_EXPORT_PRIVATE GlobalHandles final {
   static void CopyTracedReference(const Address* const* from, Address** to);
   static void DestroyTracedReference(Address* location);
   static void MarkTraced(Address* location);
+  static Object MarkTracedConservatively(Address* inner_location,
+                                         Address* traced_node_block_base);
 
   V8_INLINE static Object Acquire(Address* location);
 
@@ -166,6 +168,9 @@ class V8_EXPORT_PRIVATE GlobalHandles final {
 
   void IterateAllRootsForTesting(v8::PersistentHandleVisitor* v);
 
+  using NodeBounds = std::vector<std::pair<const void*, const void*>>;
+  NodeBounds GetTracedNodeBounds() const;
+
 #ifdef DEBUG
   void PrintStats();
   void Print();

src/heap/cppgc-js/cpp-heap.cc

@@ -24,6 +24,7 @@
 #include "src/heap/base/stack.h"
 #include "src/heap/cppgc-js/cpp-marking-state.h"
 #include "src/heap/cppgc-js/cpp-snapshot.h"
+#include "src/heap/cppgc-js/unified-heap-marking-state-inl.h"
 #include "src/heap/cppgc-js/unified-heap-marking-state.h"
 #include "src/heap/cppgc-js/unified-heap-marking-verifier.h"
 #include "src/heap/cppgc-js/unified-heap-marking-visitor.h"
@@ -41,6 +42,7 @@
 #include "src/heap/cppgc/stats-collector.h"
 #include "src/heap/cppgc/sweeper.h"
 #include "src/heap/cppgc/unmarker.h"
+#include "src/heap/cppgc/visitor.h"
 #include "src/heap/embedder-tracing-inl.h"
 #include "src/heap/embedder-tracing.h"
 #include "src/heap/gc-tracer.h"
@@ -245,6 +247,49 @@ void GlobalFatalOutOfMemoryHandlerImpl(const std::string& reason,
   V8::FatalProcessOutOfMemory(nullptr, reason.c_str());
 }
 
+class UnifiedHeapConservativeMarkingVisitor final
+    : public cppgc::internal::ConservativeMarkingVisitor {
+ public:
+  UnifiedHeapConservativeMarkingVisitor(
+      HeapBase& heap, MutatorMarkingState& mutator_marking_state,
+      cppgc::Visitor& visitor, UnifiedHeapMarkingState& marking_state)
+      : ConservativeMarkingVisitor(heap, mutator_marking_state, visitor),
+        marking_state_(marking_state) {}
+  ~UnifiedHeapConservativeMarkingVisitor() override = default;
+
+  void SetTracedNodeBounds(GlobalHandles::NodeBounds traced_node_bounds) {
+    traced_node_bounds_ = std::move(traced_node_bounds);
+  }
+
+  void TraceConservativelyIfNeeded(const void* address) override {
+    ConservativeMarkingVisitor::TraceConservativelyIfNeeded(address);
+    TraceTracedNodesConservatively(address);
+  }
+
+ private:
+  void TraceTracedNodesConservatively(const void* address) {
+    const auto upper_it =
+        std::upper_bound(traced_node_bounds_.begin(), traced_node_bounds_.end(),
+                         address, [](const void* needle, const auto& pair) {
+                           return needle < pair.first;
+                         });
+    // Also checks emptiness as begin() == end() on empty maps.
+    if (upper_it == traced_node_bounds_.begin()) return;
+
+    const auto bounds = std::next(upper_it, -1);
+    if (address < bounds->second) {
+      auto object = GlobalHandles::MarkTracedConservatively(
+          const_cast<Address*>(reinterpret_cast<const Address*>(address)),
+          const_cast<Address*>(
+              reinterpret_cast<const Address*>(bounds->first)));
+      marking_state_.MarkAndPush(object);
+    }
+  }
+
+  GlobalHandles::NodeBounds traced_node_bounds_;
+  UnifiedHeapMarkingState& marking_state_;
+};
+
 }  // namespace
 
 class UnifiedHeapMarker final : public cppgc::internal::MarkerBase {
@@ -269,11 +314,12 @@ class UnifiedHeapMarker final : public cppgc::internal::MarkerBase {
     return mutator_unified_heap_marking_state_;
   }
 
- protected:
+  UnifiedHeapConservativeMarkingVisitor& conservative_visitor() final {
-  cppgc::Visitor& visitor() final { return *marking_visitor_; }
-  cppgc::internal::ConservativeTracingVisitor& conservative_visitor() final {
     return conservative_marking_visitor_;
   }
+
+ protected:
+  cppgc::Visitor& visitor() final { return *marking_visitor_; }
   ::heap::base::StackVisitor& stack_visitor() final {
     return conservative_marking_visitor_;
   }
@@ -281,7 +327,7 @@ class UnifiedHeapMarker final : public cppgc::internal::MarkerBase {
  private:
   UnifiedHeapMarkingState mutator_unified_heap_marking_state_;
   std::unique_ptr<MutatorUnifiedHeapMarkingVisitor> marking_visitor_;
-  cppgc::internal::ConservativeMarkingVisitor conservative_marking_visitor_;
+  UnifiedHeapConservativeMarkingVisitor conservative_marking_visitor_;
 };
 
 UnifiedHeapMarker::UnifiedHeapMarker(Heap* v8_heap,
@@ -298,7 +344,8 @@ UnifiedHeapMarker::UnifiedHeapMarker(Heap* v8_heap,
                                  heap, mutator_marking_state_,
                                  mutator_unified_heap_marking_state_)),
       conservative_marking_visitor_(heap, mutator_marking_state_,
-                                    *marking_visitor_) {
+                                    *marking_visitor_,
+                                    mutator_unified_heap_marking_state_) {
   concurrent_marker_ = std::make_unique<UnifiedHeapConcurrentMarker>(
       heap_, v8_heap, marking_worklists_, schedule_, platform_,
       mutator_unified_heap_marking_state_, config.collection_type);
@@ -650,6 +697,12 @@ bool CppHeap::IsTracingDone() { return marking_done_; }
 void CppHeap::EnterFinalPause(cppgc::EmbedderStackState stack_state) {
   CHECK(!in_disallow_gc_scope());
   in_atomic_pause_ = true;
+  auto* marker = static_cast<UnifiedHeapMarker*>(marker_.get());
+  // Scan global handles conservatively in case we are attached to an Isolate.
+  if (isolate_) {
+    marker->conservative_visitor().SetTracedNodeBounds(
+        isolate()->global_handles()->GetTracedNodeBounds());
+  }
   marker_->EnterAtomicPause(stack_state);
   if (isolate_ && *collection_type_ == CollectionType::kMinor) {
     // Visit V8 -> cppgc references.

src/heap/cppgc-js/unified-heap-marking-state-inl.h

@@ -43,9 +43,15 @@ void UnifiedHeapMarkingState::MarkAndPush(
   // non-empty `TracedReferenceBase` when `CppHeap` is in detached mode.
 
   Object object = BasicTracedReferenceExtractor::GetObjectForMarking(reference);
+  MarkAndPush(object);
+}
+
+void UnifiedHeapMarkingState::MarkAndPush(Object object) {
   if (!object.IsHeapObject()) {
     // The embedder is not aware of whether numbers are materialized as heap
-    // objects are just passed around as Smis.
+    // objects are just passed around as Smis. This branch also filters out
+    // intentionally passed `Smi::zero()` that indicate that there's no object
+    // to mark.
     return;
   }
   HeapObject heap_object = HeapObject::cast(object);

src/heap/cppgc-js/unified-heap-marking-state.h

@@ -25,6 +25,7 @@ class UnifiedHeapMarkingState final {
   void Update(MarkingWorklists::Local*);
 
   V8_INLINE void MarkAndPush(const TracedReferenceBase&);
+  V8_INLINE void MarkAndPush(v8::internal::Object);
 
  private:
   Heap* const heap_;

src/heap/cppgc/visitor.h

@@ -46,7 +46,7 @@ class RootVisitorBase : public RootVisitor {
 };
 
 // Regular visitor that additionally allows for conservative tracing.
-class ConservativeTracingVisitor {
+class V8_EXPORT_PRIVATE ConservativeTracingVisitor {
  public:
   ConservativeTracingVisitor(HeapBase&, PageBackend&, cppgc::Visitor&);
   virtual ~ConservativeTracingVisitor() = default;
@@ -55,18 +55,17 @@ class ConservativeTracingVisitor {
   ConservativeTracingVisitor& operator=(const ConservativeTracingVisitor&) =
       delete;
 
-  void TraceConservativelyIfNeeded(const void*);
+  virtual void TraceConservativelyIfNeeded(const void*);
   void TraceConservativelyIfNeeded(HeapObjectHeader&);
 
  protected:
   using TraceConservativelyCallback = void(ConservativeTracingVisitor*,
                                            const HeapObjectHeader&);
-  virtual void V8_EXPORT_PRIVATE
+  virtual void VisitFullyConstructedConservatively(HeapObjectHeader&);
-  VisitFullyConstructedConservatively(HeapObjectHeader&);
   virtual void VisitInConstructionConservatively(HeapObjectHeader&,
                                                  TraceConservativelyCallback) {}
 
-  void V8_EXPORT_PRIVATE TryTracePointerConservatively(Address address);
+  void TryTracePointerConservatively(Address address);
 
   HeapBase& heap_;
   PageBackend& page_backend_;

test/unittests/heap/cppgc-js/unified-heap-unittest.cc

@@ -20,6 +20,7 @@
 #include "include/v8-traced-handle.h"
 #include "src/api/api-inl.h"
 #include "src/base/platform/time.h"
+#include "src/common/globals.h"
 #include "src/heap/cppgc-js/cpp-heap.h"
 #include "src/heap/cppgc/heap-object-header.h"
 #include "src/heap/cppgc/sweeper.h"
@@ -349,5 +350,44 @@ TEST_F(UnifiedHeapWithCustomSpaceTest, CollectCustomSpaceStatisticsAtLastGC) {
   EXPECT_EQ(4u, StatisticsReceiver::num_calls_);
 }
 
+namespace {
+
+class InConstructionObjectReferringToGlobalHandle final
+    : public cppgc::GarbageCollected<
+          InConstructionObjectReferringToGlobalHandle> {
+ public:
+  InConstructionObjectReferringToGlobalHandle(Heap* heap,
+                                              v8::Local<v8::Object> wrapper)
+      : wrapper_(reinterpret_cast<v8::Isolate*>(heap->isolate()), wrapper) {
+    heap->CollectGarbage(OLD_SPACE, GarbageCollectionReason::kTesting);
+    heap->CollectGarbage(OLD_SPACE, GarbageCollectionReason::kTesting);
+  }
+
+  void Trace(cppgc::Visitor* visitor) const { visitor->Trace(wrapper_); }
+
+  TracedReference<v8::Object>& GetWrapper() { return wrapper_; }
+
+ private:
+  TracedReference<v8::Object> wrapper_;
+};
+
+}  // namespace
+
+TEST_F(UnifiedHeapTest, InConstructionObjectReferringToGlobalHandle) {
+  v8::HandleScope handle_scope(v8_isolate());
+  v8::Local<v8::Context> context = v8::Context::New(v8_isolate());
+  v8::Context::Scope context_scope(context);
+  {
+    v8::HandleScope inner_handle_scope(v8_isolate());
+    auto local = v8::Object::New(v8_isolate());
+    auto* cpp_obj = cppgc::MakeGarbageCollected<
+        InConstructionObjectReferringToGlobalHandle>(
+        allocation_handle(),
+        reinterpret_cast<i::Isolate*>(v8_isolate())->heap(), local);
+    CHECK_NE(kGlobalHandleZapValue,
+             *reinterpret_cast<Address*>(*cpp_obj->GetWrapper()));
+  }
+}
+
 }  // namespace internal
 }  // namespace v8