api, heap: Fix move of on-stack TracedReference

Previously, V8 was just relinking nodes which broke when a move involves
an on-stack reference as such nodes have different semantics.

The solution is to create new internal nodes when necessary.

Bug: chromium:1040038
Change-Id: Ia5b3866ae68d014beb30972c4266aa5bae6559fc
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2002546
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65823}

include/v8.h

@@ -10766,14 +10766,9 @@ TracedGlobal<T>& TracedGlobal<T>::operator=(const TracedGlobal<S>& rhs) {
 template <class T>
 TracedGlobal<T>& TracedGlobal<T>::operator=(TracedGlobal&& rhs) {
   if (this != &rhs) {
-    this->Reset();
-    if (rhs.val_ != nullptr) {
-      this->val_ = rhs.val_;
       V8::MoveTracedGlobalReference(
           reinterpret_cast<internal::Address**>(&rhs.val_),
           reinterpret_cast<internal::Address**>(&this->val_));
-      rhs.val_ = nullptr;
-    }
   }
   return *this;
 }
@@ -10821,14 +10816,9 @@ TracedReference<T>& TracedReference<T>::operator=(
 template <class T>
 TracedReference<T>& TracedReference<T>::operator=(TracedReference&& rhs) {
   if (this != &rhs) {
-    this->Reset();
-    if (rhs.val_ != nullptr) {
-      this->val_ = rhs.val_;
-      V8::MoveTracedGlobalReference(
-          reinterpret_cast<internal::Address**>(&rhs.val_),
-          reinterpret_cast<internal::Address**>(&this->val_));
-      rhs.val_ = nullptr;
-    }
+    V8::MoveTracedGlobalReference(
+        reinterpret_cast<internal::Address**>(&rhs.val_),
+        reinterpret_cast<internal::Address**>(&this->val_));
   }
   return *this;
 }

src/handles/global-handles.cc

@@ -625,6 +625,7 @@ class GlobalHandles::TracedNode final
   // instead of move ctor.)
   TracedNode(TracedNode&& other) V8_NOEXCEPT = default;
   TracedNode(const TracedNode& other) V8_NOEXCEPT = default;
+  TracedNode& operator=(const TracedNode& other) V8_NOEXCEPT = default;
 
   enum State { FREE = 0, NORMAL, NEAR_DEATH };
 
@@ -703,6 +704,7 @@ class GlobalHandles::TracedNode final
     set_markbit();
     callback_ = nullptr;
     set_is_on_stack(false);
+    set_has_destructor(false);
   }
 
   void CheckImplFieldsAreCleared() const {
@@ -714,8 +716,6 @@ class GlobalHandles::TracedNode final
   WeakCallbackInfo<void>::Callback callback_;
 
   friend class NodeBase<GlobalHandles::TracedNode>;
-
-  DISALLOW_ASSIGN(TracedNode);
 };
 
 // Space to keep track of on-stack handles (e.g. TracedReference). Such
@@ -726,6 +726,11 @@ class GlobalHandles::TracedNode final
 // Design doc: http://bit.ly/on-stack-traced-reference
 class GlobalHandles::OnStackTracedNodeSpace final {
  public:
+  static GlobalHandles* GetGlobalHandles(const TracedNode* on_stack_node) {
+    DCHECK(on_stack_node->is_on_stack());
+    return reinterpret_cast<const NodeEntry*>(on_stack_node)->global_handles;
+  }
+
   explicit OnStackTracedNodeSpace(GlobalHandles* global_handles)
       : global_handles_(global_handles) {}
 
@@ -771,6 +776,7 @@ class GlobalHandles::OnStackTracedNodeSpace final {
   std::map<uintptr_t, NodeEntry> on_stack_nodes_;
   uintptr_t stack_start_ = 0;
   GlobalHandles* global_handles_ = nullptr;
+  size_t acquire_count_ = 0;
 };
 
 uintptr_t GlobalHandles::OnStackTracedNodeSpace::GetStackAddressForSlot(
@@ -778,10 +784,12 @@ uintptr_t GlobalHandles::OnStackTracedNodeSpace::GetStackAddressForSlot(
 #ifdef V8_USE_ADDRESS_SANITIZER
   void* fake_stack = __asan_get_current_fake_stack();
   if (fake_stack) {
-    void* real_slot = __asan_addr_is_in_fake_stack(
-        fake_stack, reinterpret_cast<void*>(slot), nullptr, nullptr);
-    if (real_slot) {
-      return reinterpret_cast<uintptr_t>(real_slot);
+    void* fake_frame_start;
+    void* real_frame = __asan_addr_is_in_fake_stack(
+        fake_stack, reinterpret_cast<void*>(slot), &fake_frame_start, nullptr);
+    if (real_frame) {
+      return reinterpret_cast<uintptr_t>(real_frame) +
+             (slot - reinterpret_cast<uintptr_t>(fake_frame_start));
     }
   }
 #endif  // V8_USE_ADDRESS_SANITIZER
@@ -806,8 +814,13 @@ void GlobalHandles::OnStackTracedNodeSpace::Iterate(RootVisitor* v) {
 
 GlobalHandles::TracedNode* GlobalHandles::OnStackTracedNodeSpace::Acquire(
     Object value, uintptr_t slot) {
+  constexpr size_t kAcquireCleanupThresholdLog2 = 8;
+  constexpr size_t kAcquireCleanupThresholdMask =
+      (size_t{1} << kAcquireCleanupThresholdLog2) - 1;
   DCHECK(IsOnStack(slot));
-  CleanupBelowCurrentStackPosition();
+  if (((acquire_count_++) & kAcquireCleanupThresholdMask) == 0) {
+    CleanupBelowCurrentStackPosition();
+  }
   NodeEntry entry;
   entry.node.Free(nullptr);
   entry.global_handles = global_handles_;
@@ -871,21 +884,28 @@ Handle<Object> GlobalHandles::Create(Address value) {
 
 Handle<Object> GlobalHandles::CreateTraced(Object value, Address* slot,
                                            bool has_destructor) {
+  return CreateTraced(
+      value, slot, has_destructor,
+      on_stack_nodes_->IsOnStack(reinterpret_cast<uintptr_t>(slot)));
+}
+
+Handle<Object> GlobalHandles::CreateTraced(Object value, Address* slot,
+                                           bool has_destructor,
+                                           bool is_on_stack) {
   GlobalHandles::TracedNode* result;
-  const uintptr_t slot_address = reinterpret_cast<uintptr_t>(slot);
-  if (on_stack_nodes_->IsOnStack(slot_address)) {
+  if (is_on_stack) {
     CHECK_WITH_MSG(!has_destructor,
                    "TracedGlobal is prohibited from on-stack usage.");
-    result = on_stack_nodes_->Acquire(value, slot_address);
+    result = on_stack_nodes_->Acquire(value, reinterpret_cast<uintptr_t>(slot));
   } else {
     result = traced_nodes_->Acquire(value);
     if (ObjectInYoungGeneration(value) && !result->is_in_young_list()) {
       traced_young_nodes_.push_back(result);
       result->set_in_young_list(true);
     }
+    result->set_parameter(slot);
+    result->set_has_destructor(has_destructor);
   }
-  result->set_parameter(slot);
-  result->set_has_destructor(has_destructor);
   return result->handle();
 }
 
@@ -917,10 +937,7 @@ void GlobalHandles::CopyTracedGlobal(const Address* const* from, Address** to) {
   CHECK(!node->HasFinalizationCallback());
 
   GlobalHandles* global_handles =
-      (node->is_on_stack())
-          ? *reinterpret_cast<GlobalHandles**>(const_cast<TracedNode*>(node) +
-                                               1)
-          : NodeBlock<TracedNode>::From(node)->global_handles();
+      GlobalHandles::From(const_cast<TracedNode*>(node));
   Handle<Object> o = global_handles->CreateTraced(
       node->object(), reinterpret_cast<Address*>(to), node->has_destructor());
   *to = o.location();
@@ -946,18 +963,65 @@ void GlobalHandles::MoveGlobal(Address** from, Address** to) {
 }
 
 void GlobalHandles::MoveTracedGlobal(Address** from, Address** to) {
-  DCHECK_NOT_NULL(*from);
-  DCHECK_NOT_NULL(*to);
-  DCHECK_EQ(*from, *to);
-  TracedNode* node = TracedNode::FromLocation(*from);
-  // Only set the backpointer for clearing a phantom handle when there is no
-  // finalization callback attached. As soon as a callback is attached to a node
-  // the embedder is on its own when resetting a handle.
-  if (!node->HasFinalizationCallback()) {
-    node->set_parameter(to);
+  // Fast path for moving from an empty reference.
+  if (!*from) {
+    DestroyTraced(*to);
+    *to = nullptr;
+    return;
+  }
+
+  // Determining whether from or to are on stack.
+  TracedNode* from_node = TracedNode::FromLocation(*from);
+  TracedNode* to_node = TracedNode::FromLocation(*to);
+  GlobalHandles* global_handles = nullptr;
+  bool from_on_stack = from_node->is_on_stack();
+  bool to_on_stack = false;
+  if (!to_node) {
+    // Figure out whether stack or heap to allow fast path for heap->heap move.
+    global_handles = GlobalHandles::From(from_node);
+    to_on_stack = global_handles->on_stack_nodes_->IsOnStack(
+        reinterpret_cast<uintptr_t>(to));
+  }
+
+  // Moving.
+  if (from_on_stack || to_on_stack) {
+    // Move involving a stack slot.
+    DCHECK(!from_node->has_destructor());
+    DCHECK(!from_node->HasFinalizationCallback());
+    if (!to_node) {
+      DCHECK(global_handles);
+      Handle<Object> o = global_handles->CreateTraced(
+          from_node->object(), reinterpret_cast<Address*>(to), false,
+          to_on_stack);
+      *to = o.location();
+      DCHECK(TracedNode::FromLocation(*to)->markbit());
+    } else {
+      // To node already exists, just copy fields.
+      *TracedNode::FromLocation(*to) = *from_node;
+    }
+    DestroyTraced(*from);
+    *from = nullptr;
+  } else {
+    // Pure heap move.
+    DestroyTraced(*to);
+    *to = *from;
+    DCHECK_NOT_NULL(*from);
+    DCHECK_NOT_NULL(*to);
+    DCHECK_EQ(*from, *to);
+    if (!from_node->HasFinalizationCallback()) {
+      from_node->set_parameter(to);
+    }
+    *from = nullptr;
   }
 }
 
+// static
+GlobalHandles* GlobalHandles::From(const TracedNode* node) {
+  return node->is_on_stack()
+             ? OnStackTracedNodeSpace::GetGlobalHandles(node)
+             : NodeBlock<TracedNode>::From(node)->global_handles();
+}
+
 void GlobalHandles::MarkTraced(Address* location) {
   TracedNode* node = TracedNode::FromLocation(location);
   node->set_markbit();

src/handles/global-handles.h

@@ -104,6 +104,8 @@ class V8_EXPORT_PRIVATE GlobalHandles final {
     return Handle<T>::cast(Create(Object(value)));
   }
 
+  Handle<Object> CreateTraced(Object value, Address* slot, bool has_destructor,
+                              bool is_on_stack);
   Handle<Object> CreateTraced(Object value, Address* slot, bool has_destructor);
   Handle<Object> CreateTraced(Address value, Address* slot,
                               bool has_destructor);
@@ -203,6 +205,8 @@ class V8_EXPORT_PRIVATE GlobalHandles final {
   class TracedNode;
   class OnStackTracedNodeSpace;
 
+  static GlobalHandles* From(const TracedNode*);
+
   bool InRecursiveGC(unsigned gc_processing_counter);
 
   void InvokeSecondPassPhantomCallbacksFromTask();

test/cctest/heap/test-embedder-tracing.cc

@@ -948,8 +948,13 @@ V8_NOINLINE void TracedReferenceNotifyEmptyStack(
   CHECK(observer.IsEmpty());
 }
 
-V8_NOINLINE void TracedReferenceCopyStackToHeapTest(
-    TestEmbedderHeapTracer* tracer) {
+enum class Operation {
+  kCopy,
+  kMove,
+};
+
+V8_NOINLINE void TracedReferenceStackToHeapTest(TestEmbedderHeapTracer* tracer,
+                                                Operation op) {
   v8::Isolate* isolate = CcTest::isolate();
   v8::Global<v8::Object> observer;
   v8::TracedReference<v8::Value> stack_handle;
@@ -967,8 +972,15 @@ V8_NOINLINE void TracedReferenceCopyStackToHeapTest(
   heap::InvokeMarkSweep();
   CHECK(!observer.IsEmpty());
   tracer->AddReferenceForTracing(heap_handle);
-  *heap_handle = stack_handle;
-  stack_handle.Reset();
+  switch (op) {
+    case Operation::kMove:
+      *heap_handle = std::move(stack_handle);
+      break;
+    case Operation::kCopy:
+      *heap_handle = stack_handle;
+      stack_handle.Reset();
+      break;
+  }
   heap::InvokeMarkSweep();
   CHECK(!observer.IsEmpty());
   heap::InvokeMarkSweep();
@@ -976,8 +988,8 @@ V8_NOINLINE void TracedReferenceCopyStackToHeapTest(
   delete heap_handle;
 }
 
-V8_NOINLINE void TracedReferenceCopyHeapToStackTest(
-    TestEmbedderHeapTracer* tracer) {
+V8_NOINLINE void TracedReferenceHeapToStackTest(TestEmbedderHeapTracer* tracer,
+                                                Operation op) {
   v8::Isolate* isolate = CcTest::isolate();
   v8::Global<v8::Object> observer;
   v8::TracedReference<v8::Value> stack_handle;
@@ -995,7 +1007,14 @@ V8_NOINLINE void TracedReferenceCopyHeapToStackTest(
   tracer->AddReferenceForTracing(heap_handle);
   heap::InvokeMarkSweep();
   CHECK(!observer.IsEmpty());
-  stack_handle = *heap_handle;
+  switch (op) {
+    case Operation::kMove:
+      stack_handle = std::move(*heap_handle);
+      break;
+    case Operation::kCopy:
+      stack_handle = *heap_handle;
+      break;
+  }
   delete heap_handle;
   heap::InvokeMarkSweep();
   CHECK(!observer.IsEmpty());
@@ -1004,6 +1023,39 @@ V8_NOINLINE void TracedReferenceCopyHeapToStackTest(
   CHECK(observer.IsEmpty());
 }
 
+V8_NOINLINE void TracedReferenceStackToStackTest(TestEmbedderHeapTracer* tracer,
+                                                 Operation op) {
+  v8::Isolate* isolate = CcTest::isolate();
+  v8::Global<v8::Object> observer;
+  v8::TracedReference<v8::Value> stack_handle1;
+  v8::TracedReference<v8::Value> stack_handle2;
+  {
+    v8::HandleScope scope(isolate);
+    v8::Local<v8::Object> object(ConstructTraceableJSApiObject(
+        isolate->GetCurrentContext(), nullptr, nullptr));
+    stack_handle1.Reset(isolate, object);
+    observer.Reset(isolate, object);
+    observer.SetWeak();
+  }
+  CHECK(!observer.IsEmpty());
+  heap::InvokeMarkSweep();
+  CHECK(!observer.IsEmpty());
+  switch (op) {
+    case Operation::kMove:
+      stack_handle2 = std::move(stack_handle1);
+      break;
+    case Operation::kCopy:
+      stack_handle2 = stack_handle1;
+      stack_handle1.Reset();
+      break;
+  }
+  heap::InvokeMarkSweep();
+  CHECK(!observer.IsEmpty());
+  stack_handle2.Reset();
+  heap::InvokeMarkSweep();
+  CHECK(observer.IsEmpty());
+}
+
 V8_NOINLINE void TracedReferenceCleanedTest(TestEmbedderHeapTracer* tracer) {
   v8::Isolate* isolate = CcTest::isolate();
   v8::HandleScope scope(isolate);
@@ -1049,7 +1101,7 @@ TEST(TracedReferenceCopyStackToHeap) {
   heap::TemporaryEmbedderHeapTracerScope tracer_scope(CcTest::isolate(),
                                                       &tracer);
   tracer.SetStackStart(&manual_gc);
-  TracedReferenceCopyStackToHeapTest(&tracer);
+  TracedReferenceStackToHeapTest(&tracer, Operation::kCopy);
 }
 
 TEST(TracedReferenceCopyHeapToStack) {
@@ -1059,7 +1111,47 @@ TEST(TracedReferenceCopyHeapToStack) {
   heap::TemporaryEmbedderHeapTracerScope tracer_scope(CcTest::isolate(),
                                                       &tracer);
   tracer.SetStackStart(&manual_gc);
-  TracedReferenceCopyHeapToStackTest(&tracer);
+  TracedReferenceHeapToStackTest(&tracer, Operation::kCopy);
+}
+
+TEST(TracedReferenceCopyStackToStack) {
+  ManualGCScope manual_gc;
+  CcTest::InitializeVM();
+  TestEmbedderHeapTracer tracer;
+  heap::TemporaryEmbedderHeapTracerScope tracer_scope(CcTest::isolate(),
+                                                      &tracer);
+  tracer.SetStackStart(&manual_gc);
+  TracedReferenceStackToStackTest(&tracer, Operation::kCopy);
+}
+
+TEST(TracedReferenceMoveStackToHeap) {
+  ManualGCScope manual_gc;
+  CcTest::InitializeVM();
+  TestEmbedderHeapTracer tracer;
+  heap::TemporaryEmbedderHeapTracerScope tracer_scope(CcTest::isolate(),
+                                                      &tracer);
+  tracer.SetStackStart(&manual_gc);
+  TracedReferenceStackToHeapTest(&tracer, Operation::kMove);
+}
+
+TEST(TracedReferenceMoveHeapToStack) {
+  ManualGCScope manual_gc;
+  CcTest::InitializeVM();
+  TestEmbedderHeapTracer tracer;
+  heap::TemporaryEmbedderHeapTracerScope tracer_scope(CcTest::isolate(),
+                                                      &tracer);
+  tracer.SetStackStart(&manual_gc);
+  TracedReferenceHeapToStackTest(&tracer, Operation::kMove);
+}
+
+TEST(TracedReferenceMoveStackToStack) {
+  ManualGCScope manual_gc;
+  CcTest::InitializeVM();
+  TestEmbedderHeapTracer tracer;
+  heap::TemporaryEmbedderHeapTracerScope tracer_scope(CcTest::isolate(),
+                                                      &tracer);
+  tracer.SetStackStart(&manual_gc);
+  TracedReferenceStackToStackTest(&tracer, Operation::kMove);
 }
 
 TEST(TracedReferenceCleaned) {