heap: Fixes for copying/moving traced references

- Fix copying of already initialized nodes
- Add better verification
- Add tests for moving/copying onto already initialized nodes

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

src/handles/global-handles.cc

@@ -4,6 +4,7 @@
 
 #include "src/handles/global-handles.h"
 
+#include <algorithm>
 #include <map>
 
 #include "src/api/api-inl.h"
@@ -626,7 +627,6 @@ 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 };
 
@@ -662,6 +662,8 @@ class GlobalHandles::TracedNode final
   }
   bool HasFinalizationCallback() const { return callback_ != nullptr; }
 
+  void CopyObjectReference(const TracedNode& other) { object_ = other.object_; }
+
   void CollectPhantomCallbackData(
       std::vector<std::pair<TracedNode*, PendingPhantomCallback>>*
           pending_phantom_callbacks) {
@@ -691,11 +693,7 @@ class GlobalHandles::TracedNode final
     DCHECK(!IsInUse());
   }
 
-  void Verify() {
-    DCHECK(IsInUse());
-    DCHECK_IMPLIES(!has_destructor(), nullptr == parameter());
-    DCHECK_IMPLIES(has_destructor() && !HasFinalizationCallback(), parameter());
-  }
+  static void Verify(GlobalHandles* global_handles, const Address* const* slot);
 
  protected:
   using NodeState = base::BitField8<State, 0, 2>;
@@ -844,6 +842,31 @@ void GlobalHandles::OnStackTracedNodeSpace::CleanupBelowCurrentStackPosition() {
   on_stack_nodes_.erase(on_stack_nodes_.begin(), it);
 }
 
+// static
+void GlobalHandles::TracedNode::Verify(GlobalHandles* global_handles,
+                                       const Address* const* slot) {
+#ifdef DEBUG
+  const TracedNode* node = FromLocation(*slot);
+  DCHECK(node->IsInUse());
+  DCHECK_IMPLIES(!node->has_destructor(), nullptr == node->parameter());
+  DCHECK_IMPLIES(node->has_destructor() && !node->HasFinalizationCallback(),
+                 node->parameter());
+  bool slot_on_stack = global_handles->on_stack_nodes_->IsOnStack(
+      reinterpret_cast<uintptr_t>(slot));
+  DCHECK_EQ(slot_on_stack, node->is_on_stack());
+  if (!node->is_on_stack()) {
+    // On-heap nodes have seprate lists for young generation processing.
+    bool is_young_gen_object = ObjectInYoungGeneration(node->object());
+    DCHECK_IMPLIES(is_young_gen_object, node->is_in_young_list());
+  }
+  bool in_young_list =
+      std::find(global_handles->traced_young_nodes_.begin(),
+                global_handles->traced_young_nodes_.end(),
+                node) != global_handles->traced_young_nodes_.end();
+  DCHECK_EQ(in_young_list, node->is_in_young_list());
+#endif  // DEBUG
+}
+
 void GlobalHandles::CleanupOnStackReferencesBelowCurrentStackPosition() {
   on_stack_nodes_->CleanupBelowCurrentStackPosition();
 }
@@ -940,6 +963,8 @@ void GlobalHandles::CopyTracedGlobal(const Address* const* from, Address** to) {
   Handle<Object> o = global_handles->CreateTraced(
       node->object(), reinterpret_cast<Address*>(to), node->has_destructor());
   *to = o.location();
+  TracedNode::Verify(global_handles, from);
+  TracedNode::Verify(global_handles, to);
 #ifdef VERIFY_HEAP
   if (i::FLAG_verify_heap) {
     Object(**to).ObjectVerify(global_handles->isolate());
@@ -971,8 +996,12 @@ void GlobalHandles::MoveTracedGlobal(Address** from, Address** to) {
 
   // Determining whether from or to are on stack.
   TracedNode* from_node = TracedNode::FromLocation(*from);
+  DCHECK(from_node->IsInUse());
   TracedNode* to_node = TracedNode::FromLocation(*to);
   GlobalHandles* global_handles = nullptr;
+#ifdef DEBUG
+  global_handles = GlobalHandles::From(from_node);
+#endif  // DEBUG
   bool from_on_stack = from_node->is_on_stack();
   bool to_on_stack = false;
   if (!to_node) {
@@ -980,6 +1009,8 @@ void GlobalHandles::MoveTracedGlobal(Address** from, Address** to) {
     global_handles = GlobalHandles::From(from_node);
     to_on_stack = global_handles->on_stack_nodes_->IsOnStack(
         reinterpret_cast<uintptr_t>(to));
+  } else {
+    to_on_stack = to_node->is_on_stack();
   }
 
   // Moving a traced handle with finalization callback is prohibited because
@@ -1005,16 +1036,17 @@ void GlobalHandles::MoveTracedGlobal(Address** from, Address** to) {
       to_node = TracedNode::FromLocation(*to);
       DCHECK(to_node->markbit());
     } else {
-      // To node already exists, just copy fields.
-      *TracedNode::FromLocation(*to) = *from_node;
-      // Fixup back reference for destructor.
-      if (to_node->has_destructor()) {
-        to_node->set_parameter(to);
+      DCHECK(to_node->IsInUse());
+      to_node->CopyObjectReference(*from_node);
+      if (!to_node->is_on_stack() && !to_node->is_in_young_list() &&
+          ObjectInYoungGeneration(to_node->object())) {
+        global_handles = GlobalHandles::From(from_node);
+        global_handles->traced_young_nodes_.push_back(to_node);
+        to_node->set_in_young_list(true);
       }
     }
     DestroyTraced(*from);
     *from = nullptr;
-    to_node->Verify();
   } else {
     // Pure heap move.
     DestroyTraced(*to);
@@ -1028,8 +1060,8 @@ void GlobalHandles::MoveTracedGlobal(Address** from, Address** to) {
       to_node->set_parameter(to);
     }
     *from = nullptr;
-    to_node->Verify();
   }
+  TracedNode::Verify(global_handles, to);
 }
 
 // static

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

@@ -967,12 +967,31 @@ void PerformOperation(Operation op, T* lhs, T* rhs) {
   }
 }
 
+enum class TargetHandling {
+  kNonInitialized,
+  kInitializedYoungGen,
+  kInitializedOldGen
+};
+
 template <typename T>
-V8_NOINLINE void StackToHeapTest(TestEmbedderHeapTracer* tracer, Operation op) {
+V8_NOINLINE void StackToHeapTest(TestEmbedderHeapTracer* tracer, Operation op,
+                                 TargetHandling target_handling) {
   v8::Isolate* isolate = CcTest::isolate();
   v8::Global<v8::Object> observer;
   T stack_handle;
   T* heap_handle = new T();
+  if (target_handling != TargetHandling::kNonInitialized) {
+    v8::HandleScope scope(isolate);
+    v8::Local<v8::Object> to_object(ConstructTraceableJSApiObject(
+        isolate->GetCurrentContext(), nullptr, nullptr));
+    CHECK(i::Heap::InYoungGeneration(*v8::Utils::OpenHandle(*to_object)));
+    if (target_handling == TargetHandling::kInitializedOldGen) {
+      heap::InvokeScavenge();
+      heap::InvokeScavenge();
+      CHECK(!i::Heap::InYoungGeneration(*v8::Utils::OpenHandle(*to_object)));
+    }
+    heap_handle->Reset(isolate, to_object);
+  }
   {
     v8::HandleScope scope(isolate);
     v8::Local<v8::Object> object(ConstructTraceableJSApiObject(
@@ -982,6 +1001,7 @@ V8_NOINLINE void StackToHeapTest(TestEmbedderHeapTracer* tracer, Operation op) {
     observer.SetWeak();
   }
   CHECK(!observer.IsEmpty());
+  tracer->AddReferenceForTracing(heap_handle);
   heap::InvokeMarkSweep();
   CHECK(!observer.IsEmpty());
   tracer->AddReferenceForTracing(heap_handle);
@@ -994,11 +1014,24 @@ V8_NOINLINE void StackToHeapTest(TestEmbedderHeapTracer* tracer, Operation op) {
 }
 
 template <typename T>
-V8_NOINLINE void HeapToStackTest(TestEmbedderHeapTracer* tracer, Operation op) {
+V8_NOINLINE void HeapToStackTest(TestEmbedderHeapTracer* tracer, Operation op,
+                                 TargetHandling target_handling) {
   v8::Isolate* isolate = CcTest::isolate();
   v8::Global<v8::Object> observer;
   T stack_handle;
   T* heap_handle = new T();
+  if (target_handling != TargetHandling::kNonInitialized) {
+    v8::HandleScope scope(isolate);
+    v8::Local<v8::Object> to_object(ConstructTraceableJSApiObject(
+        isolate->GetCurrentContext(), nullptr, nullptr));
+    CHECK(i::Heap::InYoungGeneration(*v8::Utils::OpenHandle(*to_object)));
+    if (target_handling == TargetHandling::kInitializedOldGen) {
+      heap::InvokeScavenge();
+      heap::InvokeScavenge();
+      CHECK(!i::Heap::InYoungGeneration(*v8::Utils::OpenHandle(*to_object)));
+    }
+    stack_handle.Reset(isolate, to_object);
+  }
   {
     v8::HandleScope scope(isolate);
     v8::Local<v8::Object> object(ConstructTraceableJSApiObject(
@@ -1021,12 +1054,24 @@ V8_NOINLINE void HeapToStackTest(TestEmbedderHeapTracer* tracer, Operation op) {
 }
 
 template <typename T>
-V8_NOINLINE void StackToStackTest(TestEmbedderHeapTracer* tracer,
-                                  Operation op) {
+V8_NOINLINE void StackToStackTest(TestEmbedderHeapTracer* tracer, Operation op,
+                                  TargetHandling target_handling) {
   v8::Isolate* isolate = CcTest::isolate();
   v8::Global<v8::Object> observer;
   T stack_handle1;
   T stack_handle2;
+  if (target_handling != TargetHandling::kNonInitialized) {
+    v8::HandleScope scope(isolate);
+    v8::Local<v8::Object> to_object(ConstructTraceableJSApiObject(
+        isolate->GetCurrentContext(), nullptr, nullptr));
+    CHECK(i::Heap::InYoungGeneration(*v8::Utils::OpenHandle(*to_object)));
+    if (target_handling == TargetHandling::kInitializedOldGen) {
+      heap::InvokeScavenge();
+      heap::InvokeScavenge();
+      CHECK(!i::Heap::InYoungGeneration(*v8::Utils::OpenHandle(*to_object)));
+    }
+    stack_handle2.Reset(isolate, to_object);
+  }
   {
     v8::HandleScope scope(isolate);
     v8::Local<v8::Object> object(ConstructTraceableJSApiObject(
@@ -1134,9 +1179,24 @@ TEST(TracedReferenceMove) {
   heap::TemporaryEmbedderHeapTracerScope tracer_scope(CcTest::isolate(),
                                                       &tracer);
   tracer.SetStackStart(&manual_gc);
-  StackToHeapTest<ReferenceType>(&tracer, Operation::kMove);
-  HeapToStackTest<ReferenceType>(&tracer, Operation::kMove);
-  StackToStackTest<ReferenceType>(&tracer, Operation::kMove);
+  StackToHeapTest<ReferenceType>(&tracer, Operation::kMove,
+                                 TargetHandling::kNonInitialized);
+  StackToHeapTest<ReferenceType>(&tracer, Operation::kMove,
+                                 TargetHandling::kInitializedYoungGen);
+  StackToHeapTest<ReferenceType>(&tracer, Operation::kMove,
+                                 TargetHandling::kInitializedOldGen);
+  HeapToStackTest<ReferenceType>(&tracer, Operation::kMove,
+                                 TargetHandling::kNonInitialized);
+  HeapToStackTest<ReferenceType>(&tracer, Operation::kMove,
+                                 TargetHandling::kInitializedYoungGen);
+  HeapToStackTest<ReferenceType>(&tracer, Operation::kMove,
+                                 TargetHandling::kInitializedOldGen);
+  StackToStackTest<ReferenceType>(&tracer, Operation::kMove,
+                                  TargetHandling::kNonInitialized);
+  StackToStackTest<ReferenceType>(&tracer, Operation::kMove,
+                                  TargetHandling::kInitializedYoungGen);
+  StackToStackTest<ReferenceType>(&tracer, Operation::kMove,
+                                  TargetHandling::kInitializedOldGen);
 }
 
 TEST(TracedReferenceCopy) {
@@ -1147,12 +1207,27 @@ TEST(TracedReferenceCopy) {
   heap::TemporaryEmbedderHeapTracerScope tracer_scope(CcTest::isolate(),
                                                       &tracer);
   tracer.SetStackStart(&manual_gc);
-  StackToHeapTest<ReferenceType>(&tracer, Operation::kCopy);
-  HeapToStackTest<ReferenceType>(&tracer, Operation::kCopy);
-  StackToStackTest<ReferenceType>(&tracer, Operation::kCopy);
+  StackToHeapTest<ReferenceType>(&tracer, Operation::kCopy,
+                                 TargetHandling::kNonInitialized);
+  StackToHeapTest<ReferenceType>(&tracer, Operation::kCopy,
+                                 TargetHandling::kInitializedYoungGen);
+  StackToHeapTest<ReferenceType>(&tracer, Operation::kCopy,
+                                 TargetHandling::kInitializedOldGen);
+  HeapToStackTest<ReferenceType>(&tracer, Operation::kCopy,
+                                 TargetHandling::kNonInitialized);
+  HeapToStackTest<ReferenceType>(&tracer, Operation::kCopy,
+                                 TargetHandling::kInitializedYoungGen);
+  HeapToStackTest<ReferenceType>(&tracer, Operation::kCopy,
+                                 TargetHandling::kInitializedOldGen);
+  StackToStackTest<ReferenceType>(&tracer, Operation::kCopy,
+                                  TargetHandling::kNonInitialized);
+  StackToStackTest<ReferenceType>(&tracer, Operation::kCopy,
+                                  TargetHandling::kInitializedYoungGen);
+  StackToStackTest<ReferenceType>(&tracer, Operation::kCopy,
+                                  TargetHandling::kInitializedOldGen);
 }
 
-TEST(TraceGlobalMove) {
+TEST(TracedGlobalMove) {
   using ReferenceType = v8::TracedGlobal<v8::Value>;
   ManualGCScope manual_gc;
   CcTest::InitializeVM();
@@ -1160,9 +1235,24 @@ TEST(TraceGlobalMove) {
   heap::TemporaryEmbedderHeapTracerScope tracer_scope(CcTest::isolate(),
                                                       &tracer);
   tracer.SetStackStart(&manual_gc);
-  StackToHeapTest<ReferenceType>(&tracer, Operation::kMove);
-  HeapToStackTest<ReferenceType>(&tracer, Operation::kMove);
-  StackToStackTest<ReferenceType>(&tracer, Operation::kMove);
+  StackToHeapTest<ReferenceType>(&tracer, Operation::kMove,
+                                 TargetHandling::kNonInitialized);
+  StackToHeapTest<ReferenceType>(&tracer, Operation::kMove,
+                                 TargetHandling::kInitializedYoungGen);
+  StackToHeapTest<ReferenceType>(&tracer, Operation::kMove,
+                                 TargetHandling::kInitializedOldGen);
+  HeapToStackTest<ReferenceType>(&tracer, Operation::kMove,
+                                 TargetHandling::kNonInitialized);
+  HeapToStackTest<ReferenceType>(&tracer, Operation::kMove,
+                                 TargetHandling::kInitializedYoungGen);
+  HeapToStackTest<ReferenceType>(&tracer, Operation::kMove,
+                                 TargetHandling::kInitializedOldGen);
+  StackToStackTest<ReferenceType>(&tracer, Operation::kMove,
+                                  TargetHandling::kNonInitialized);
+  StackToStackTest<ReferenceType>(&tracer, Operation::kMove,
+                                  TargetHandling::kInitializedYoungGen);
+  StackToStackTest<ReferenceType>(&tracer, Operation::kMove,
+                                  TargetHandling::kInitializedOldGen);
 }
 
 TEST(TracedGlobalCopy) {
@@ -1173,9 +1263,24 @@ TEST(TracedGlobalCopy) {
   heap::TemporaryEmbedderHeapTracerScope tracer_scope(CcTest::isolate(),
                                                       &tracer);
   tracer.SetStackStart(&manual_gc);
-  StackToHeapTest<ReferenceType>(&tracer, Operation::kCopy);
-  HeapToStackTest<ReferenceType>(&tracer, Operation::kCopy);
-  StackToStackTest<ReferenceType>(&tracer, Operation::kCopy);
+  StackToHeapTest<ReferenceType>(&tracer, Operation::kCopy,
+                                 TargetHandling::kNonInitialized);
+  StackToHeapTest<ReferenceType>(&tracer, Operation::kCopy,
+                                 TargetHandling::kInitializedYoungGen);
+  StackToHeapTest<ReferenceType>(&tracer, Operation::kCopy,
+                                 TargetHandling::kInitializedOldGen);
+  HeapToStackTest<ReferenceType>(&tracer, Operation::kCopy,
+                                 TargetHandling::kNonInitialized);
+  HeapToStackTest<ReferenceType>(&tracer, Operation::kCopy,
+                                 TargetHandling::kInitializedYoungGen);
+  HeapToStackTest<ReferenceType>(&tracer, Operation::kCopy,
+                                 TargetHandling::kInitializedOldGen);
+  StackToStackTest<ReferenceType>(&tracer, Operation::kCopy,
+                                  TargetHandling::kNonInitialized);
+  StackToStackTest<ReferenceType>(&tracer, Operation::kCopy,
+                                  TargetHandling::kInitializedYoungGen);
+  StackToStackTest<ReferenceType>(&tracer, Operation::kCopy,
+                                  TargetHandling::kInitializedOldGen);
 }
 
 TEST(TracedGlobalDestructor) {