[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/+/3812039Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82306}

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

@@ -10350,7 +10350,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/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/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
@@ -1196,11 +1204,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;
 
@@ -2336,6 +2349,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.
@@ -2455,6 +2469,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(const_cast<void*>(
+      ::heap::base::Stack::GetCurrentStackPointerForLocalVariables()));
   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;
 }
@@ -929,14 +940,14 @@ enum class Operation {
 };
 
 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;
   }
 }
@@ -980,12 +991,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;
 }
 
@@ -1070,46 +1091,23 @@ 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);
+  tracer.SetStackStart(const_cast<void*>(
+      ::heap::base::Stack::GetCurrentStackPointerForLocalVariables()));
   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(const_cast<void*>(
+      ::heap::base::Stack::GetCurrentStackPointerForLocalVariables()));
   StackToHeapTest(v8_isolate(), &tracer, Operation::kMove,
                   TargetHandling::kNonInitialized);
   StackToHeapTest(v8_isolate(), &tracer, Operation::kMove,
@@ -1134,7 +1132,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(const_cast<void*>(
+      ::heap::base::Stack::GetCurrentStackPointerForLocalVariables()));
   StackToHeapTest(v8_isolate(), &tracer, Operation::kCopy,
                   TargetHandling::kNonInitialized);
   StackToHeapTest(v8_isolate(), &tracer, Operation::kCopy,
@@ -1168,27 +1167,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(const_cast<void*>(
+      ::heap::base::Stack::GetCurrentStackPointerForLocalVariables()));
+  TracedReferenceOnStackReferencesAreTemporaryTest(v8_isolate(), &tracer);
 }
 
 }  // namespace heap