Reland "[api, global-handles] Fix moving weak Global<T>"

v8::Global may be used as a weak reference. In the case this reference is a
simple phantom reference, we need to update the internal state to be able to
clear the right slot once the object referred to is dead.

This reverts commit 18f32ca8.

Bug: chromium:924220
Change-Id: I3caec77448b0c5fcb461c8f8b5015de2978b3931
Reviewed-on: https://chromium-review.googlesource.com/c/1430015Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59055}

include/v8.h

@@ -758,6 +758,7 @@ class Global : public PersistentBase<T> {
    * A Global with no storage cell.
    */
   V8_INLINE Global() : PersistentBase<T>(nullptr) {}
+
   /**
    * Construct a Global from a Local.
    * When the Local is non-empty, a new storage cell is created
@@ -768,6 +769,7 @@ class Global : public PersistentBase<T> {
       : PersistentBase<T>(PersistentBase<T>::New(isolate, *that)) {
     TYPE_CHECK(T, S);
   }
+
   /**
    * Construct a Global from a PersistentBase.
    * When the Persistent is non-empty, a new storage cell is created
@@ -778,26 +780,20 @@ class Global : public PersistentBase<T> {
       : PersistentBase<T>(PersistentBase<T>::New(isolate, that.val_)) {
     TYPE_CHECK(T, S);
   }
+
   /**
    * Move constructor.
    */
-  V8_INLINE Global(Global&& other) : PersistentBase<T>(other.val_) {
-    other.val_ = nullptr;
-  }
+  V8_INLINE Global(Global&& other);
+
   V8_INLINE ~Global() { this->Reset(); }
+
   /**
    * Move via assignment.
    */
   template <class S>
-  V8_INLINE Global& operator=(Global<S>&& rhs) {  // NOLINT
-    TYPE_CHECK(T, S);
-    if (this != &rhs) {
-      this->Reset();
-      this->val_ = rhs.val_;
-      rhs.val_ = nullptr;
-    }
-    return *this;
-  }
+  V8_INLINE Global& operator=(Global<S>&& rhs);
+
   /**
    * Pass allows returning uniques from functions, etc.
    */
@@ -822,7 +818,6 @@ class Global : public PersistentBase<T> {
 template <class T>
 using UniquePersistent = Global<T>;
 
-
  /**
  * A stack-allocated class that governs a number of local handles.
  * After a handle scope has been created, all local handles will be
@@ -8565,7 +8560,9 @@ class V8_EXPORT V8 {
 
   static internal::Address* GlobalizeReference(internal::Isolate* isolate,
                                                internal::Address* handle);
-  static internal::Address* CopyPersistent(internal::Address* handle);
+  static void MoveGlobalReference(internal::Address** from,
+                                  internal::Address** to);
+  static internal::Address* CopyGlobalReference(internal::Address* from);
   static void DisposeGlobal(internal::Address* global_handle);
   static void MakeWeak(internal::Address* location, void* data,
                        WeakCallbackInfo<void>::Callback weak_callback,
@@ -8585,6 +8582,8 @@ class V8_EXPORT V8 {
   static void FromJustIsNothing();
   static void ToLocalEmpty();
   static void InternalFieldOutOfBounds(int index);
+  template <class T>
+  friend class Global;
   template <class T> friend class Local;
   template <class T>
   friend class MaybeLocal;
@@ -9512,7 +9511,7 @@ void Persistent<T, M>::Copy(const Persistent<S, M2>& that) {
   this->Reset();
   if (that.IsEmpty()) return;
   internal::Address* p = reinterpret_cast<internal::Address*>(that.val_);
-  this->val_ = reinterpret_cast<T*>(V8::CopyPersistent(p));
+  this->val_ = reinterpret_cast<T*>(V8::CopyGlobalReference(p));
   M::Copy(that, this);
 }
 
@@ -9645,6 +9644,32 @@ uint16_t PersistentBase<T>::WrapperClassId() const {
   return *reinterpret_cast<uint16_t*>(addr);
 }
 
+template <class T>
+Global<T>::Global(Global&& other) : PersistentBase<T>(other.val_) {
+  if (other.val_ != nullptr) {
+    V8::MoveGlobalReference(reinterpret_cast<internal::Address**>(&other.val_),
+                            reinterpret_cast<internal::Address**>(&this->val_));
+    other.val_ = nullptr;
+  }
+}
+
+template <class T>
+template <class S>
+Global<T>& Global<T>::operator=(Global<S>&& rhs) {
+  TYPE_CHECK(T, S);
+  if (this != &rhs) {
+    this->Reset();
+    if (rhs.val_ != nullptr) {
+      this->val_ = rhs.val_;
+      V8::MoveGlobalReference(
+          reinterpret_cast<internal::Address**>(&rhs.val_),
+          reinterpret_cast<internal::Address**>(&this->val_));
+      rhs.val_ = nullptr;
+    }
+  }
+  return *this;
+}
+
 template <typename T>
 ReturnValue<T>::ReturnValue(internal::Address* slot) : value_(slot) {}
 

src/api.cc

@@ -1012,11 +1012,15 @@ i::Address* V8::GlobalizeReference(i::Isolate* isolate, i::Address* obj) {
   return result.location();
 }
 
-i::Address* V8::CopyPersistent(i::Address* obj) {
-  i::Handle<i::Object> result = i::GlobalHandles::CopyGlobal(obj);
+i::Address* V8::CopyGlobalReference(i::Address* from) {
+  i::Handle<i::Object> result = i::GlobalHandles::CopyGlobal(from);
   return result.location();
 }
 
+void V8::MoveGlobalReference(internal::Address** from, internal::Address** to) {
+  i::GlobalHandles::MoveGlobal(from, to);
+}
+
 void V8::RegisterExternallyReferencedObject(i::Address* location,
                                             i::Isolate* isolate) {
   isolate->heap()->RegisterExternallyReferencedObject(location);

src/global-handles.cc

@@ -625,6 +625,20 @@ Handle<Object> GlobalHandles::CopyGlobal(Address* location) {
   return global_handles->Create(*location);
 }
 
+void GlobalHandles::MoveGlobal(Address** from, Address** to) {
+  DCHECK_NOT_NULL(*from);
+  DCHECK_NOT_NULL(*to);
+  DCHECK_EQ(*from, *to);
+  Node* node = Node::FromLocation(*from);
+  if (node->IsWeak() && node->IsPhantomResetHandle()) {
+    node->set_parameter(to);
+  }
+
+  // - Strong handles do not require fixups.
+  // - Weak handles with finalizers and callbacks are too general to fix up. For
+  //   those the callers need to ensure consistency.
+}
+
 void GlobalHandles::Destroy(Address* location) {
   if (location != nullptr) {
     NodeSpace<Node>::Release(Node::FromLocation(location));

src/global-handles.h

@@ -40,7 +40,10 @@ enum WeaknessType {
 // callbacks and finalizers attached to them.
 class GlobalHandles final {
  public:
-  // Copy a global handle
+  // Move a global handle.
+  static void MoveGlobal(Address** from, Address** to);
+
+  // Copy a global handle.
   static Handle<Object> CopyGlobal(Address* location);
 
   // Destroy a global handle.

test/cctest/test-global-handles.cc

@@ -69,6 +69,14 @@ void ConstructJSObject(v8::Isolate* isolate, v8::Local<v8::Context> context,
   CHECK(!flag_and_persistent->handle.IsEmpty());
 }
 
+void ConstructJSObject(v8::Isolate* isolate, v8::Global<v8::Object>* global) {
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::Object> object(v8::Object::New(isolate));
+  CHECK(!object.IsEmpty());
+  *global = v8::Global<v8::Object>(isolate, object);
+  CHECK(!global->IsEmpty());
+}
+
 void ConstructJSApiObject(v8::Isolate* isolate, v8::Local<v8::Context> context,
                           FlagAndPersistent* flag_and_persistent) {
   v8::HandleScope handle_scope(isolate);
@@ -532,5 +540,32 @@ TEST(SecondPassPhantomCallbacks) {
   CHECK(fp.flag);
 }
 
+TEST(MoveStrongGlobal) {
+  CcTest::InitializeVM();
+  v8::Isolate* isolate = CcTest::isolate();
+  v8::HandleScope scope(isolate);
+
+  v8::Global<v8::Object>* global = new Global<v8::Object>();
+  ConstructJSObject(isolate, global);
+  InvokeMarkSweep();
+  v8::Global<v8::Object> global2(std::move(*global));
+  delete global;
+  InvokeMarkSweep();
+}
+
+TEST(MoveWeakGlobal) {
+  CcTest::InitializeVM();
+  v8::Isolate* isolate = CcTest::isolate();
+  v8::HandleScope scope(isolate);
+
+  v8::Global<v8::Object>* global = new Global<v8::Object>();
+  ConstructJSObject(isolate, global);
+  InvokeMarkSweep();
+  global->SetWeak();
+  v8::Global<v8::Object> global2(std::move(*global));
+  delete global;
+  InvokeMarkSweep();
+}
+
 }  // namespace internal
 }  // namespace v8