Reland "[handles] Remove precise on-stack representation of global handles"

This is a reland of commit 6953b555

The reland fixes tests that retrieved the stack start from a
non-inlined frame's fp. This does not work in certain configurations
as the resulting marker is too low to consider the first local
variables in subsequent calls.

The fix uses an inline frame address for the tests to get an upper
bound of stack addresses to consider.

Original change's description:
> [handles] Remove precise on-stack representation of global handles
>
> Since https://crrev.com/c/3806439 on-stack traced handles are marked
> conservatively when being used in combination with CppHeap.
>
> This change removes the precise on-stack representation of the
> internal traced nodes as they nodes would anyways be marked
> conservatively. The effects are:
> - cheaper representation (just a single node space);
> - uniform handling: no checks to distinguish on-stack vs on-heap;
> - no brittleness around cleaning on-stack handles when the event loop
>  is empty;
>
> Change-Id: Id859623bfed77a66bdd064ea8065536264515eae
> Bug: v8:13141
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3812039
> Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#82306}

Bug: v8:13141
Change-Id: I53ece36220e99d02be6df18f83c18450e5d5037b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3820585Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82342}

BUILD.bazel

@@ -1450,6 +1450,8 @@ filegroup(
         "src/heap/gc-tracer.cc",
         "src/heap/gc-tracer-inl.h",
         "src/heap/gc-tracer.h",
+        "src/heap/global-handle-marking-visitor.cc",
+        "src/heap/global-handle-marking-visitor.h",
         "src/heap/heap-allocator-inl.h",
         "src/heap/heap-allocator.cc",
         "src/heap/heap-allocator.h",

BUILD.gn

@@ -3121,6 +3121,7 @@ v8_header_set("v8_internal_headers") {
     "src/heap/gc-idle-time-handler.h",
     "src/heap/gc-tracer-inl.h",
     "src/heap/gc-tracer.h",
+    "src/heap/global-handle-marking-visitor.h",
     "src/heap/heap-allocator-inl.h",
     "src/heap/heap-allocator.h",
     "src/heap/heap-controller.h",
@@ -4506,6 +4507,7 @@ v8_source_set("v8_base_without_compiler") {
     "src/heap/free-list.cc",
     "src/heap/gc-idle-time-handler.cc",
     "src/heap/gc-tracer.cc",
+    "src/heap/global-handle-marking-visitor.cc",
     "src/heap/heap-allocator.cc",
     "src/heap/heap-controller.cc",
     "src/heap/heap-layout-tracer.cc",

src/api/api.cc

@@ -10352,7 +10352,7 @@ void HeapProfiler::SetGetDetachednessCallback(GetDetachednessCallback callback,
 void EmbedderHeapTracer::SetStackStart(void* stack_start) {
   CHECK(v8_isolate_);
   reinterpret_cast<i::Isolate*>(v8_isolate_)
-      ->global_handles()
+      ->heap()
       ->SetStackStart(stack_start);
 }
 

src/base/platform/platform.h

@@ -39,6 +39,10 @@
 #include "src/base/qnx-math.h"
 #endif
 
+#if V8_CC_MSVC
+#include <intrin.h>
+#endif  // V8_CC_MSVC
+
 #if V8_OS_FUCHSIA
 #include <zircon/types.h>
 #endif  // V8_OS_FUCHSIA
@@ -619,11 +623,22 @@ class V8_BASE_EXPORT Stack {
   static StackSlot GetStackStart();
 
   // Returns the current stack top. Works correctly with ASAN and SafeStack.
+  //
   // GetCurrentStackPosition() should not be inlined, because it works on stack
   // frames if it were inlined into a function with a huge stack frame it would
   // return an address significantly above the actual current stack position.
   static V8_NOINLINE StackSlot GetCurrentStackPosition();
 
+  // Same as `GetCurrentStackPosition()` with the difference that it is always
+  // inlined and thus always returns the current frame's stack top.
+  static V8_INLINE StackSlot GetCurrentFrameAddress() {
+#if V8_CC_MSVC
+    return _AddressOfReturnAddress();
+#else
+    return __builtin_frame_address(0);
+#endif
+  }
+
   // Returns the real stack frame if slot is part of a fake frame, and slot
   // otherwise.
   static StackSlot GetRealStackAddressForSlot(StackSlot slot) {

src/handles/global-handles.h

@@ -94,9 +94,6 @@ class V8_EXPORT_PRIVATE GlobalHandles final {
   template <typename T>
   inline Handle<T> Create(T value);
 
-  Handle<Object> CreateTraced(Object value, Address* slot,
-                              GlobalHandleStoreMode store_mode,
-                              bool is_on_stack);
   Handle<Object> CreateTraced(Object value, Address* slot,
                               GlobalHandleStoreMode store_mode);
   Handle<Object> CreateTraced(Address value, Address* slot,
@@ -112,7 +109,6 @@ class V8_EXPORT_PRIVATE GlobalHandles final {
       GarbageCollector collector, const v8::GCCallbackFlags gc_callback_flags);
 
   void IterateStrongRoots(RootVisitor* v);
-  void IterateStrongStackRoots(RootVisitor* v);
   void IterateWeakRoots(RootVisitor* v);
   void IterateAllRoots(RootVisitor* v);
   void IterateAllYoungRoots(RootVisitor* v);
@@ -157,15 +153,9 @@ class V8_EXPORT_PRIVATE GlobalHandles final {
 
   size_t TotalSize() const;
   size_t UsedSize() const;
-
   // Number of global handles.
   size_t handles_count() const;
 
-  void SetStackStart(void* stack_start);
-  void NotifyEmptyEmbedderStack();
-  void CleanupOnStackReferencesBelowCurrentStackPosition();
-  size_t NumberOfOnStackHandlesForTesting();
-
   using NodeBounds = std::vector<std::pair<const void*, const void*>>;
   NodeBounds GetTracedNodeBounds() const;
 
@@ -185,7 +175,6 @@ class V8_EXPORT_PRIVATE GlobalHandles final {
   class NodeSpace;
   class PendingPhantomCallback;
   class TracedNode;
-  class OnStackTracedNodeSpace;
 
   static GlobalHandles* From(const TracedNode*);
 
@@ -213,7 +202,6 @@ class V8_EXPORT_PRIVATE GlobalHandles final {
 
   std::unique_ptr<NodeSpace<TracedNode>> traced_nodes_;
   std::vector<TracedNode*> traced_young_nodes_;
-  std::unique_ptr<OnStackTracedNodeSpace> on_stack_nodes_;
 
   std::vector<std::pair<Node*, PendingPhantomCallback>>
       regular_pending_phantom_callbacks_;

src/heap/base/stack.cc

@@ -163,17 +163,5 @@ void Stack::IteratePointersUnsafe(StackVisitor* visitor,
   IteratePointersImpl(this, visitor, reinterpret_cast<intptr_t*>(stack_end));
 }
 
-const void* Stack::GetCurrentStackPointerForLocalVariables() {
-#if defined(__has_feature)
-#if __has_feature(safe_stack)
-  return __builtin___get_unsafe_stack_ptr();
-#else   // __has_feature(safe_stack)
-  return v8::base::Stack::GetCurrentStackPosition();
-#endif  // __has_feature(safe_stack)
-#else   // defined(__has_feature)
-  return v8::base::Stack::GetCurrentStackPosition();
-#endif  // defined(__has_feature)
-}
-
 }  // namespace base
 }  // namespace heap

src/heap/base/stack.h

@@ -7,8 +7,7 @@
 
 #include "src/base/macros.h"
 
-namespace heap {
-namespace base {
+namespace heap::base {
 
 class StackVisitor {
  public:
@@ -47,17 +46,10 @@ class V8_EXPORT_PRIVATE Stack final {
   // Returns the start of the stack.
   const void* stack_start() const { return stack_start_; }
 
-  // Get the current stack pointer for the stack, on which local variables are
-  // stored. In case the safe-stack is enabled (-fsanitize=safe-stack), this
-  // will return the stack pointer for the unsafe-stack. Otherwise, the function
-  // returns the stack pointer for the native stack.
-  static const void* GetCurrentStackPointerForLocalVariables();
-
  private:
   const void* stack_start_;
 };
 
-}  // namespace base
-}  // namespace heap
+}  // namespace heap::base
 
 #endif  // V8_HEAP_BASE_STACK_H_

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

@@ -46,6 +46,7 @@
 #include "src/heap/embedder-tracing-inl.h"
 #include "src/heap/embedder-tracing.h"
 #include "src/heap/gc-tracer.h"
+#include "src/heap/global-handle-marking-visitor.h"
 #include "src/heap/marking-worklist.h"
 #include "src/heap/sweeper.h"
 #include "src/init/v8.h"
@@ -252,42 +253,26 @@ class UnifiedHeapConservativeMarkingVisitor final
  public:
   UnifiedHeapConservativeMarkingVisitor(
       HeapBase& heap, MutatorMarkingState& mutator_marking_state,
-      cppgc::Visitor& visitor, UnifiedHeapMarkingState& marking_state)
-      : ConservativeMarkingVisitor(heap, mutator_marking_state, visitor),
-        marking_state_(marking_state) {}
+      cppgc::Visitor& visitor)
+      : ConservativeMarkingVisitor(heap, mutator_marking_state, visitor) {}
   ~UnifiedHeapConservativeMarkingVisitor() override = default;
 
-  void SetTracedNodeBounds(GlobalHandles::NodeBounds traced_node_bounds) {
-    traced_node_bounds_ = std::move(traced_node_bounds);
+  void SetGlobalHandlesMarkingVisitor(
+      std::unique_ptr<GlobalHandleMarkingVisitor>
+          global_handle_marking_visitor) {
+    global_handle_marking_visitor_ = std::move(global_handle_marking_visitor);
   }
 
   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);
+    if (global_handle_marking_visitor_) {
+      global_handle_marking_visitor_->VisitPointer(address);
     }
   }
 
-  GlobalHandles::NodeBounds traced_node_bounds_;
-  UnifiedHeapMarkingState& marking_state_;
+ private:
+  std::unique_ptr<GlobalHandleMarkingVisitor> global_handle_marking_visitor_ =
+      nullptr;
 };
 
 }  // namespace
@@ -344,8 +329,7 @@ UnifiedHeapMarker::UnifiedHeapMarker(Heap* v8_heap,
                                  heap, mutator_marking_state_,
                                  mutator_unified_heap_marking_state_)),
       conservative_marking_visitor_(heap, mutator_marking_state_,
-                                    *marking_visitor_,
-                                    mutator_unified_heap_marking_state_) {
+                                    *marking_visitor_) {
   concurrent_marker_ = std::make_unique<UnifiedHeapConcurrentMarker>(
       heap_, v8_heap, marking_worklists_, schedule_, platform_,
       mutator_unified_heap_marking_state_, config.collection_type);
@@ -531,7 +515,7 @@ void CppHeap::AttachIsolate(Isolate* isolate) {
         &CppGraphBuilder::Run, this);
   }
   SetMetricRecorder(std::make_unique<MetricRecorderAdapter>(*this));
-  isolate_->global_handles()->SetStackStart(base::Stack::GetStackStart());
+  isolate_->heap()->SetStackStart(base::Stack::GetStackStart());
   oom_handler().SetCustomHandler(&FatalOutOfMemoryHandlerImpl);
   no_gc_scope_--;
 }
@@ -701,8 +685,11 @@ void CppHeap::EnterFinalPause(cppgc::EmbedderStackState stack_state) {
   auto& marker = marker_.get()->To<UnifiedHeapMarker>();
   // Scan global handles conservatively in case we are attached to an Isolate.
   if (isolate_) {
-    marker.conservative_visitor().SetTracedNodeBounds(
-        isolate()->global_handles()->GetTracedNodeBounds());
+    auto& heap = *isolate()->heap();
+    marker.conservative_visitor().SetGlobalHandlesMarkingVisitor(
+        std::make_unique<GlobalHandleMarkingVisitor>(
+            heap, *heap.mark_compact_collector()->marking_state(),
+            *heap.mark_compact_collector()->local_marking_worklists()));
   }
   marker.EnterAtomicPause(stack_state);
   if (isolate_ && *collection_type_ == CollectionType::kMinor) {

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

@@ -43,10 +43,6 @@ 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. This branch also filters out

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

@@ -25,7 +25,6 @@ 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/embedder-tracing.cc

@@ -175,16 +175,6 @@ void LocalEmbedderHeapTracer::StartIncrementalMarkingIfNeeded() {
   }
 }
 
-void LocalEmbedderHeapTracer::NotifyEmptyEmbedderStack() {
-  auto* overriden_stack_state = isolate_->heap()->overriden_stack_state();
-  if (overriden_stack_state &&
-      (*overriden_stack_state ==
-       cppgc::EmbedderStackState::kMayContainHeapPointers))
-    return;
-
-  isolate_->global_handles()->NotifyEmptyEmbedderStack();
-}
-
 void LocalEmbedderHeapTracer::EmbedderWriteBarrier(Heap* heap,
                                                    JSObject js_object) {
   DCHECK(InUse());

src/heap/embedder-tracing.h

@@ -152,8 +152,6 @@ class V8_EXPORT_PRIVATE LocalEmbedderHeapTracer final {
     return default_embedder_roots_handler_;
   }
 
-  void NotifyEmptyEmbedderStack();
-
   EmbedderHeapTracer::EmbedderStackState embedder_stack_state() const {
     return embedder_stack_state_;
   }

src/heap/global-handle-marking-visitor.cc

+// Copyright 2022 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/heap/global-handle-marking-visitor.h"
+
+#include "src/heap/marking-worklist-inl.h"
+
+namespace v8 {
+namespace internal {
+
+GlobalHandleMarkingVisitor::GlobalHandleMarkingVisitor(
+    Heap& heap, MarkingState& marking_state,
+    MarkingWorklists::Local& local_marking_worklist)
+    : heap_(heap),
+      marking_state_(marking_state),
+      local_marking_worklist_(local_marking_worklist),
+      traced_node_bounds_(
+          heap.isolate()->global_handles()->GetTracedNodeBounds()) {}
+
+void GlobalHandleMarkingVisitor::VisitPointer(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 bounds.
+  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)));
+    if (!object.IsHeapObject()) {
+      // The embedder is not aware of whether numbers are materialized as heap
+      // 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);
+    if (marking_state_.WhiteToGrey(heap_object)) {
+      local_marking_worklist_.Push(heap_object);
+    }
+    if (V8_UNLIKELY(FLAG_track_retaining_path)) {
+      heap_.AddRetainingRoot(Root::kWrapperTracing, heap_object);
+    }
+  }
+}
+
+}  // namespace internal
+}  // namespace v8

src/heap/global-handle-marking-visitor.h

+// Copyright 2022 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef V8_HEAP_GLOBAL_HANDLE_MARKING_VISITOR_H_
+#define V8_HEAP_GLOBAL_HANDLE_MARKING_VISITOR_H_
+
+#include "src/handles/global-handles.h"
+#include "src/heap/base/stack.h"
+#include "src/heap/heap.h"
+#include "src/heap/mark-compact.h"
+
+namespace v8 {
+namespace internal {
+
+// Root marking visitor for conservatively marking traced global handles.
+// The visitor assumes that on-stack pointers may point into global handle nodes
+// which requires them to be kept alive.
+class GlobalHandleMarkingVisitor final : public ::heap::base::StackVisitor {
+ public:
+  GlobalHandleMarkingVisitor(Heap&, MarkingState&, MarkingWorklists::Local&);
+  ~GlobalHandleMarkingVisitor() override = default;
+
+  void VisitPointer(const void*) override;
+
+ private:
+  Heap& heap_;
+  MarkingState& marking_state_;
+  MarkingWorklists::Local& local_marking_worklist_;
+  GlobalHandles::NodeBounds traced_node_bounds_;
+};
+
+#endif  // V8_HEAP_GLOBAL_HANDLE_MARKING_VISITOR_H_
+
+}  // namespace internal
+}  // namespace v8

src/heap/heap.cc

@@ -36,6 +36,7 @@
 #include "src/execution/vm-state-inl.h"
 #include "src/handles/global-handles-inl.h"
 #include "src/heap/array-buffer-sweeper.h"
+#include "src/heap/base/stack.h"
 #include "src/heap/basic-memory-chunk.h"
 #include "src/heap/code-object-registry.h"
 #include "src/heap/code-range.h"
@@ -1819,11 +1820,6 @@ bool Heap::CollectGarbage(AllocationSpace space,
         this, IsYoungGenerationCollector(collector) ? "MinorGC" : "MajorGC",
         GarbageCollectionReasonToString(gc_reason));
 
-    // Filter on-stack reference below this method.
-    isolate()
-        ->global_handles()
-        ->CleanupOnStackReferencesBelowCurrentStackPosition();
-
     if (collector == GarbageCollector::MARK_COMPACTOR && cpp_heap()) {
       // CppHeap needs a stack marker at the top of all entry points to allow
       // deterministic passes over the stack. E.g., a verifier that should only
@@ -5100,10 +5096,7 @@ void Heap::IterateBuiltins(RootVisitor* v) {
   static_assert(Builtins::AllBuiltinsAreIsolateIndependent());
 }
 
-void Heap::IterateStackRoots(RootVisitor* v) {
-  isolate_->Iterate(v);
-  isolate_->global_handles()->IterateStrongStackRoots(v);
-}
+void Heap::IterateStackRoots(RootVisitor* v) { isolate_->Iterate(v); }
 
 namespace {
 size_t GlobalMemorySizeFromV8Size(size_t v8_size) {
@@ -5793,6 +5786,7 @@ void Heap::SetUpSpaces(LinearAllocationArea& new_allocation_info,
   tracer_.reset(new GCTracer(this));
   array_buffer_sweeper_.reset(new ArrayBufferSweeper(this));
   gc_idle_time_handler_.reset(new GCIdleTimeHandler());
+  stack_ = std::make_unique<::heap::base::Stack>();
   memory_measurement_.reset(new MemoryMeasurement(isolate()));
   memory_reducer_.reset(new MemoryReducer(this));
   if (V8_UNLIKELY(TracingFlags::is_gc_stats_enabled())) {
@@ -5993,6 +5987,12 @@ const cppgc::EmbedderStackState* Heap::overriden_stack_state() const {
   return cpp_heap ? cpp_heap->override_stack_state() : nullptr;
 }
 
+void Heap::SetStackStart(void* stack_start) {
+  stack_->SetStackStart(stack_start);
+}
+
+::heap::base::Stack& Heap::stack() { return *stack_.get(); }
+
 void Heap::RegisterExternallyReferencedObject(Address* location) {
   GlobalHandles::MarkTraced(location);
   Object object(*location);
@@ -6114,6 +6114,7 @@ void Heap::TearDown() {
   concurrent_marking_.reset();
 
   gc_idle_time_handler_.reset();
+  stack_.reset();
   memory_measurement_.reset();
   allocation_tracker_for_debugging_.reset();
 
@@ -7581,8 +7582,6 @@ EmbedderStackStateScope::EmbedderStackStateScope(
   }
 
   local_tracer_->embedder_stack_state_ = stack_state;
-  if (EmbedderHeapTracer::EmbedderStackState::kNoHeapPointers == stack_state)
-    local_tracer_->NotifyEmptyEmbedderStack();
 }
 
 // static
@@ -7598,8 +7597,6 @@ EmbedderStackStateScope::EmbedderStackStateScope(
     : local_tracer_(local_tracer),
       old_stack_state_(local_tracer_->embedder_stack_state_) {
   local_tracer_->embedder_stack_state_ = stack_state;
-  if (EmbedderHeapTracer::EmbedderStackState::kNoHeapPointers == stack_state)
-    local_tracer_->NotifyEmptyEmbedderStack();
 }
 
 EmbedderStackStateScope::~EmbedderStackStateScope() {

src/heap/heap.h

@@ -28,6 +28,7 @@
 #include "src/common/globals.h"
 #include "src/heap/allocation-observer.h"
 #include "src/heap/allocation-result.h"
+#include "src/heap/base/stack.h"
 #include "src/heap/heap-allocator.h"
 #include "src/init/heap-symbols.h"
 #include "src/objects/allocation-site.h"
@@ -51,6 +52,12 @@ class ClassNameAsHeapObjectNameScope;
 }  // namespace internal
 }  // namespace cppgc
 
+namespace heap {
+namespace base {
+class Stack;
+}  // namespace base
+}  // namespace heap
+
 namespace v8 {
 
 namespace debug {
@@ -60,6 +67,7 @@ using OutOfMemoryCallback = void (*)(void* data);
 namespace internal {
 
 namespace heap {
+
 class HeapTester;
 class TestMemoryAllocatorScope;
 }  // namespace heap
@@ -1198,11 +1206,16 @@ class Heap {
 
   const cppgc::EmbedderStackState* overriden_stack_state() const;
 
+  V8_EXPORT_PRIVATE void SetStackStart(void* stack_start);
+
+  ::heap::base::Stack& stack();
+
   // ===========================================================================
   // Embedder roots optimizations. =============================================
   // ===========================================================================
 
-  V8_EXPORT_PRIVATE void SetEmbedderRootsHandler(EmbedderRootsHandler* handler);
+  V8_EXPORT_PRIVATE
+  void SetEmbedderRootsHandler(EmbedderRootsHandler* handler);
 
   EmbedderRootsHandler* GetEmbedderRootsHandler() const;
 
@@ -2334,6 +2347,7 @@ class Heap {
   std::unique_ptr<LocalEmbedderHeapTracer> local_embedder_heap_tracer_;
   std::unique_ptr<AllocationTrackerForDebugging>
       allocation_tracker_for_debugging_;
+  std::unique_ptr<::heap::base::Stack> stack_;
 
   // This object controls virtual space reserved for code on the V8 heap. This
   // is only valid for 64-bit architectures where kRequiresCodeRange.
@@ -2453,6 +2467,7 @@ class Heap {
   friend class EvacuateVisitorBase;
   friend class GCCallbacksScope;
   friend class GCTracer;
+  friend class GlobalHandleMarkingVisitor;
   friend class HeapAllocator;
   friend class HeapObjectIterator;
   friend class ScavengeTaskObserver;

src/heap/mark-compact.cc

@@ -27,6 +27,7 @@
 #include "src/heap/evacuation-allocator-inl.h"
 #include "src/heap/gc-tracer-inl.h"
 #include "src/heap/gc-tracer.h"
+#include "src/heap/global-handle-marking-visitor.h"
 #include "src/heap/heap.h"
 #include "src/heap/incremental-marking-inl.h"
 #include "src/heap/index-generator.h"
@@ -2082,6 +2083,20 @@ void MarkCompactCollector::MarkRoots(RootVisitor* root_visitor,
           ProcessTopOptimizedFrame(custom_root_body_visitor, client);
         });
   }
+
+  if (!heap_->cpp_heap() && heap_->local_embedder_heap_tracer()->InUse()) {
+    // Conservative global handle scanning is necessary for keeping
+    // v8::TracedReference alive from the stack. This is only needed when using
+    // `EmbedderHeapTracer` and not using `CppHeap`.
+    auto& stack = heap()->stack();
+    if (stack.stack_start() &&
+        heap_->local_embedder_heap_tracer()->embedder_stack_state() ==
+            cppgc::EmbedderStackState::kMayContainHeapPointers) {
+      GlobalHandleMarkingVisitor global_handles_marker(
+          *heap_, marking_state_, *local_marking_worklists_);
+      stack.IteratePointers(&global_handles_marker);
+    }
+  }
 }
 
 #ifdef V8_ENABLE_INNER_POINTER_RESOLUTION_MB

test/unittests/heap/cppgc-js/traced-reference-unittest.cc

@@ -224,7 +224,7 @@ TEST_F(TracedReferenceTest, NoWriteBarrierOnConstruction) {
   }
 }
 
-TEST_F(TracedReferenceTest, WriteBarrierOnHeapReset) {
+TEST_F(TracedReferenceTest, WriteBarrierForOnHeapReset) {
   if (!FLAG_incremental_marking)
     GTEST_SKIP() << "Write barrier tests require incremental marking";
 
@@ -239,14 +239,15 @@ TEST_F(TracedReferenceTest, WriteBarrierOnHeapReset) {
     MarkingState state(i_isolate());
     ASSERT_TRUE(state.IsWhite(HeapObject::cast(*Utils::OpenHandle(*local))));
     ref->Reset(v8_isolate(), local);
-    EXPECT_TRUE(state.IsGrey(HeapObject::cast(*Utils::OpenHandle(*local))));
+    EXPECT_FALSE(state.IsWhite(HeapObject::cast(*Utils::OpenHandle(*local))));
   }
 }
 
-TEST_F(TracedReferenceTest, NoWriteBarrierOnStackReset) {
-  if (!FLAG_incremental_marking) return;
+TEST_F(TracedReferenceTest, WriteBarrierForOnStackReset) {
+  if (!FLAG_incremental_marking)
+    GTEST_SKIP() << "Write barrier tests require incremental marking";
 
-  isolate()->global_handles()->SetStackStart(base::Stack::GetStackStart());
+  heap()->SetStackStart(base::Stack::GetStackStart());
 
   v8::Local<v8::Context> context = v8::Context::New(v8_isolate());
   v8::Context::Scope context_scope(context);
@@ -259,7 +260,7 @@ TEST_F(TracedReferenceTest, NoWriteBarrierOnStackReset) {
     MarkingState state(i_isolate());
     ASSERT_TRUE(state.IsWhite(HeapObject::cast(*Utils::OpenHandle(*local))));
     ref.Reset(v8_isolate(), local);
-    EXPECT_TRUE(state.IsWhite(HeapObject::cast(*Utils::OpenHandle(*local))));
+    EXPECT_FALSE(state.IsWhite(HeapObject::cast(*Utils::OpenHandle(*local))));
   }
 }
 
@@ -281,14 +282,15 @@ TEST_F(TracedReferenceTest, WriteBarrierOnHeapCopy) {
     ASSERT_TRUE(state.IsWhite(HeapObject::cast(*Utils::OpenHandle(*local))));
     *ref_to = *ref_from;
     EXPECT_TRUE(!ref_from->IsEmpty());
-    EXPECT_TRUE(state.IsGrey(HeapObject::cast(*Utils::OpenHandle(*local))));
+    EXPECT_FALSE(state.IsWhite(HeapObject::cast(*Utils::OpenHandle(*local))));
   }
 }
 
-TEST_F(TracedReferenceTest, NoWriteBarrierOnStackCopy) {
-  if (!FLAG_incremental_marking) return;
+TEST_F(TracedReferenceTest, WriteBarrierForOnStackCopy) {
+  if (!FLAG_incremental_marking)
+    GTEST_SKIP() << "Write barrier tests require incremental marking";
 
-  isolate()->global_handles()->SetStackStart(base::Stack::GetStackStart());
+  heap()->SetStackStart(base::Stack::GetStackStart());
 
   v8::Local<v8::Context> context = v8::Context::New(v8_isolate());
   v8::Context::Scope context_scope(context);
@@ -304,11 +306,11 @@ TEST_F(TracedReferenceTest, NoWriteBarrierOnStackCopy) {
     ASSERT_TRUE(state.IsWhite(HeapObject::cast(*Utils::OpenHandle(*local))));
     ref_to = *ref_from;
     EXPECT_TRUE(!ref_from->IsEmpty());
-    EXPECT_TRUE(state.IsWhite(HeapObject::cast(*Utils::OpenHandle(*local))));
+    EXPECT_FALSE(state.IsWhite(HeapObject::cast(*Utils::OpenHandle(*local))));
   }
 }
 
-TEST_F(TracedReferenceTest, WriteBarrierOnMove) {
+TEST_F(TracedReferenceTest, WriteBarrierForOnHeapMove) {
   if (!FLAG_incremental_marking)
     GTEST_SKIP() << "Write barrier tests require incremental marking";
 
@@ -326,15 +328,15 @@ TEST_F(TracedReferenceTest, WriteBarrierOnMove) {
     ASSERT_TRUE(state.IsWhite(HeapObject::cast(*Utils::OpenHandle(*local))));
     *ref_to = std::move(*ref_from);
     ASSERT_TRUE(ref_from->IsEmpty());
-    EXPECT_TRUE(state.IsGrey(HeapObject::cast(*Utils::OpenHandle(*local))));
+    EXPECT_FALSE(state.IsWhite(HeapObject::cast(*Utils::OpenHandle(*local))));
   }
 }
 
-TEST_F(TracedReferenceTest, NoWriteBarrierOnStackMove) {
+TEST_F(TracedReferenceTest, WriteBarrierForOnStackMove) {
   if (!FLAG_incremental_marking)
     GTEST_SKIP() << "Write barrier tests require incremental marking";
 
-  isolate()->global_handles()->SetStackStart(base::Stack::GetStackStart());
+  heap()->SetStackStart(base::Stack::GetStackStart());
 
   v8::Local<v8::Context> context = v8::Context::New(v8_isolate());
   v8::Context::Scope context_scope(context);
@@ -350,7 +352,7 @@ TEST_F(TracedReferenceTest, NoWriteBarrierOnStackMove) {
     ASSERT_TRUE(state.IsWhite(HeapObject::cast(*Utils::OpenHandle(*local))));
     ref_to = std::move(*ref_from);
     ASSERT_TRUE(ref_from->IsEmpty());
-    EXPECT_TRUE(state.IsWhite(HeapObject::cast(*Utils::OpenHandle(*local))));
+    EXPECT_FALSE(state.IsWhite(HeapObject::cast(*Utils::OpenHandle(*local))));
   }
 }
 

test/unittests/heap/embedder-tracing-unittest.cc

@@ -788,6 +788,8 @@ TEST_F(EmbedderTracingTest, BasicTracedReference) {
   v8::HandleScope scope(v8_isolate());
   TestEmbedderHeapTracer tracer;
   heap::TemporaryEmbedderHeapTracerScope tracer_scope(v8_isolate(), &tracer);
+  tracer.SetStackStart(
+      static_cast<void*>(base::Stack::GetCurrentFrameAddress()));
   i::GlobalHandles* global_handles = i_isolate()->global_handles();
 
   const size_t initial_count = global_handles->handles_count();
@@ -804,8 +806,17 @@ TEST_F(EmbedderTracingTest, BasicTracedReference) {
   }
   traced->~TracedReference<v8::Value>();
   EXPECT_EQ(initial_count + 1, global_handles->handles_count());
-  // GC should clear the handle.
-  FullGC();
+  {
+    // Conservative scanning may find stale pointers to on-stack handles.
+    // Disable scanning, assuming the slots are overwritten.
+    EmbedderStackStateScope scope =
+        EmbedderStackStateScope::ExplicitScopeForTesting(
+            reinterpret_cast<i::Isolate*>(v8_isolate())
+                ->heap()
+                ->local_embedder_heap_tracer(),
+            EmbedderHeapTracer::EmbedderStackState::kNoHeapPointers);
+    FullGC();
+  }
   EXPECT_EQ(initial_count, global_handles->handles_count());
   delete[] memory;
 }
@@ -923,20 +934,33 @@ V8_NOINLINE void OnStackTest(v8::Isolate* v8_isolate,
   EXPECT_FALSE(observer.IsEmpty());
 }
 
+}  // namespace
+
+TEST_F(EmbedderTracingTest, TracedReferenceOnStack) {
+  ManualGCScope manual_gc(i_isolate());
+  TestEmbedderHeapTracer tracer;
+  heap::TemporaryEmbedderHeapTracerScope tracer_scope(v8_isolate(), &tracer);
+  tracer.SetStackStart(
+      static_cast<void*>(base::Stack::GetCurrentFrameAddress()));
+  OnStackTest<v8::TracedReference<v8::Value>>(v8_isolate(), &tracer);
+}
+
+namespace {
+
 enum class Operation {
   kCopy,
   kMove,
 };
 
 template <typename T>
-void PerformOperation(Operation op, T* lhs, T* rhs) {
+V8_NOINLINE void PerformOperation(Operation op, T* target, T* source) {
   switch (op) {
     case Operation::kMove:
-      *lhs = std::move(*rhs);
+      *target = std::move(*source);
       break;
     case Operation::kCopy:
-      *lhs = *rhs;
-      rhs->Reset();
+      *target = *source;
+      source->Reset();
       break;
   }
 }
@@ -979,12 +1003,22 @@ V8_NOINLINE void StackToHeapTest(v8::Isolate* v8_isolate,
   tracer->AddReferenceForTracing(heap_handle);
   FullGC(v8_isolate);
   EXPECT_FALSE(observer.IsEmpty());
-  tracer->AddReferenceForTracing(heap_handle);
   PerformOperation(op, heap_handle, &stack_handle);
+  tracer->AddReferenceForTracing(heap_handle);
   FullGC(v8_isolate);
   EXPECT_FALSE(observer.IsEmpty());
-  FullGC(v8_isolate);
-  EXPECT_TRUE(observer.IsEmpty());
+  {
+    // Conservative scanning may find stale pointers to on-stack handles.
+    // Disable scanning, assuming the slots are overwritten.
+    EmbedderStackStateScope scope =
+        EmbedderStackStateScope::ExplicitScopeForTesting(
+            reinterpret_cast<i::Isolate*>(v8_isolate)
+                ->heap()
+                ->local_embedder_heap_tracer(),
+            EmbedderHeapTracer::EmbedderStackState::kNoHeapPointers);
+    FullGC(v8_isolate);
+  }
+  ASSERT_TRUE(observer.IsEmpty());
   delete heap_handle;
 }
 
@@ -1067,46 +1101,14 @@ V8_NOINLINE void StackToStackTest(v8::Isolate* v8_isolate,
   EXPECT_TRUE(observer.IsEmpty());
 }
 
-V8_NOINLINE void TracedReferenceCleanedTest(v8::Isolate* v8_isolate,
-                                            TestEmbedderHeapTracer* tracer) {
-  v8::HandleScope scope(v8_isolate);
-  v8::Local<v8::Object> object(ConstructTraceableJSApiObject(
-      v8_isolate->GetCurrentContext(), nullptr, nullptr));
-  const size_t before = reinterpret_cast<Isolate*>(v8_isolate)
-                            ->global_handles()
-                            ->NumberOfOnStackHandlesForTesting();
-  for (int i = 0; i < 100; i++) {
-    v8::TracedReference<v8::Value> stack_handle;
-    stack_handle.Reset(v8_isolate, object);
-  }
-  EXPECT_EQ(before + 1, reinterpret_cast<Isolate*>(v8_isolate)
-                            ->global_handles()
-                            ->NumberOfOnStackHandlesForTesting());
-}
-
 }  // namespace
 
-TEST_F(EmbedderTracingTest, TracedReferenceOnStack) {
-  ManualGCScope manual_gc(i_isolate());
-  TestEmbedderHeapTracer tracer;
-  heap::TemporaryEmbedderHeapTracerScope tracer_scope(v8_isolate(), &tracer);
-  tracer.SetStackStart(&manual_gc);
-  OnStackTest<v8::TracedReference<v8::Value>>(v8_isolate(), &tracer);
-}
-
-TEST_F(EmbedderTracingTest, TracedReferenceCleaned) {
-  ManualGCScope manual_gc(i_isolate());
-  TestEmbedderHeapTracer tracer;
-  heap::TemporaryEmbedderHeapTracerScope tracer_scope(v8_isolate(), &tracer);
-  tracer.SetStackStart(&manual_gc);
-  TracedReferenceCleanedTest(v8_isolate(), &tracer);
-}
-
 TEST_F(EmbedderTracingTest, TracedReferenceMove) {
   ManualGCScope manual_gc(i_isolate());
   TestEmbedderHeapTracer tracer;
   heap::TemporaryEmbedderHeapTracerScope tracer_scope(v8_isolate(), &tracer);
-  tracer.SetStackStart(&manual_gc);
+  tracer.SetStackStart(
+      static_cast<void*>(base::Stack::GetCurrentFrameAddress()));
   StackToHeapTest(v8_isolate(), &tracer, Operation::kMove,
                   TargetHandling::kNonInitialized);
   StackToHeapTest(v8_isolate(), &tracer, Operation::kMove,
@@ -1131,7 +1133,8 @@ TEST_F(EmbedderTracingTest, TracedReferenceCopy) {
   ManualGCScope manual_gc(i_isolate());
   TestEmbedderHeapTracer tracer;
   heap::TemporaryEmbedderHeapTracerScope tracer_scope(v8_isolate(), &tracer);
-  tracer.SetStackStart(&manual_gc);
+  tracer.SetStackStart(
+      static_cast<void*>(base::Stack::GetCurrentFrameAddress()));
   StackToHeapTest(v8_isolate(), &tracer, Operation::kCopy,
                   TargetHandling::kNonInitialized);
   StackToHeapTest(v8_isolate(), &tracer, Operation::kCopy,
@@ -1165,27 +1168,34 @@ V8_NOINLINE void CreateTracedReferenceInDeepStack(
   observer->SetWeak();
 }
 
-V8_NOINLINE void TracedReferenceNotifyEmptyStackTest(
+V8_NOINLINE void TracedReferenceOnStackReferencesAreTemporaryTest(
     v8::Isolate* v8_isolate, TestEmbedderHeapTracer* tracer) {
   v8::Global<v8::Object> observer;
   CreateTracedReferenceInDeepStack(v8_isolate, &observer);
   EXPECT_FALSE(observer.IsEmpty());
-  reinterpret_cast<i::Isolate*>(v8_isolate)
-      ->heap()
-      ->local_embedder_heap_tracer()
-      ->NotifyEmptyEmbedderStack();
-  FullGC(v8_isolate);
+  {
+    // Conservative scanning may find stale pointers to on-stack handles.
+    // Disable scanning, assuming the slots are overwritten.
+    EmbedderStackStateScope scope =
+        EmbedderStackStateScope::ExplicitScopeForTesting(
+            reinterpret_cast<i::Isolate*>(v8_isolate)
+                ->heap()
+                ->local_embedder_heap_tracer(),
+            EmbedderHeapTracer::EmbedderStackState::kNoHeapPointers);
+    FullGC(v8_isolate);
+  }
   EXPECT_TRUE(observer.IsEmpty());
 }
 
 }  // namespace
 
-TEST_F(EmbedderTracingTest, NotifyEmptyStack) {
+TEST_F(EmbedderTracingTest, OnStackReferencesAreTemporary) {
   ManualGCScope manual_gc(i_isolate());
   TestEmbedderHeapTracer tracer;
   heap::TemporaryEmbedderHeapTracerScope tracer_scope(v8_isolate(), &tracer);
-  tracer.SetStackStart(&manual_gc);
-  TracedReferenceNotifyEmptyStackTest(v8_isolate(), &tracer);
+  tracer.SetStackStart(
+      static_cast<void*>(base::Stack::GetCurrentFrameAddress()));
+  TracedReferenceOnStackReferencesAreTemporaryTest(v8_isolate(), &tracer);
 }
 
 }  // namespace heap