Reland "cppgc: Fix CTP for destruction""

This avoids a benign race in setting the raw pointer inside CTP
destructor by not emitting the write at all. The handle is destructed
which means that we only need to destroy any backing node but may
leave the handle untouched.

Drive-by:
- Add a few more docs.
- Make Clear() thread-safe.
- Make assignment of a sentinel pointer thread-safe.
- Make assignment of a nullptr thread-safe.

Depends on the Blink change: https://crrev.com/c/3116259

Bug: chromium:1242795, chromium:1243350
Change-Id: I8d76da30893c165e3946322b6d02f6ea2c8e529e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3114064
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76493}

BUILD.gn

@@ -5174,6 +5174,7 @@ v8_header_set("cppgc_headers") {
   sources = [
     "include/cppgc/allocation.h",
     "include/cppgc/common.h",
+    "include/cppgc/cross-thread-persistent.h",
     "include/cppgc/custom-space.h",
     "include/cppgc/default-platform.h",
     "include/cppgc/ephemeron-pair.h",

include/cppgc/cross-thread-persistent.h

@@ -75,7 +75,30 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
   using typename WeaknessPolicy::IsStrongPersistent;
   using PointeeType = T;
 
-  ~BasicCrossThreadPersistent() { Clear(); }
+  ~BasicCrossThreadPersistent() {
+    // Simplified version of `AssignUnsafe()` to allow calling without a
+    // complete type `T`. Also performs a thread-safe check for a handle that is
+    // not valid. This implements fast path for destroying empty/sentinel
+    // handles.
+    if (GetNodeSafe()) {
+      PersistentRegionLock guard;
+      const void* old_value = GetValue();
+      // The fast path check (GetNodeSafe()) does not acquire the lock. Recheck
+      // validity while holding the lock to ensure the reference has not been
+      // cleared.
+      if (IsValid(old_value)) {
+        CrossThreadPersistentRegion& region =
+            this->GetPersistentRegion(old_value);
+        region.FreeNode(GetNode());
+        SetNode(nullptr);
+      } else {
+        CPPGC_DCHECK(!GetNode());
+      }
+    }
+    // No need to call SetValue() as the handle is not used anymore. This can
+    // leave behind stale sentinel values but will always destroy the underlying
+    // node.
+  }
 
   BasicCrossThreadPersistent(
       const SourceLocation& loc = SourceLocation::Current())
@@ -162,7 +185,7 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
   BasicCrossThreadPersistent& operator=(
       const BasicCrossThreadPersistent& other) {
     PersistentRegionLock guard;
-    AssignUnsafe(other.Get());
+    AssignSafe(guard, other.Get());
     return *this;
   }
 
@@ -174,7 +197,7 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
                                        OtherLocationPolicy,
                                        OtherCheckingPolicy>& other) {
     PersistentRegionLock guard;
-    AssignUnsafe(other.Get());
+    AssignSafe(guard, other.Get());
     return *this;
   }
 
@@ -192,8 +215,13 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
     return *this;
   }
 
+  /**
+   * Assigns a raw pointer.
+   *
+   * Note: **Not thread-safe.**
+   */
   BasicCrossThreadPersistent& operator=(T* other) {
-    Assign(other);
+    AssignUnsafe(other);
     return *this;
   }
 
@@ -208,13 +236,24 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
     return operator=(member.Get());
   }
 
+  /**
+   * Assigns a nullptr.
+   *
+   * \returns the handle.
+   */
   BasicCrossThreadPersistent& operator=(std::nullptr_t) {
     Clear();
     return *this;
   }
 
+  /**
+   * Assigns the sentinel pointer.
+   *
+   * \returns the handle.
+   */
   BasicCrossThreadPersistent& operator=(SentinelPointer s) {
-    Assign(s);
+    PersistentRegionLock guard;
+    AssignSafe(guard, s);
     return *this;
   }
 
@@ -236,23 +275,8 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
    * Clears the stored object.
    */
   void Clear() {
-    // Simplified version of `Assign()` to allow calling without a complete type
-    // `T`. Also performs a thread-safe check for a handle that is not valid.
-    if (GetNodeSafe()) {
-      PersistentRegionLock guard;
-      const void* old_value = GetValue();
-      // The fast path check (IsValid()) does not acquire the lock. Reload
-      // the value to ensure the reference has not been cleared.
-      if (IsValid(old_value)) {
-        CrossThreadPersistentRegion& region =
-            this->GetPersistentRegion(old_value);
-        region.FreeNode(GetNode());
-        SetNode(nullptr);
-      } else {
-        CPPGC_DCHECK(!GetNode());
-      }
-    }
-    SetValue(nullptr);
+    PersistentRegionLock guard;
+    AssignSafe(guard, nullptr);
   }
 
   /**
@@ -328,7 +352,7 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
     v->TraceRoot(*handle, handle->Location());
   }
 
-  void Assign(T* ptr) {
+  void AssignUnsafe(T* ptr) {
     const void* old_value = GetValue();
     if (IsValid(old_value)) {
       PersistentRegionLock guard;
@@ -356,7 +380,7 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
     this->CheckPointer(ptr);
   }
 
-  void AssignUnsafe(T* ptr) {
+  void AssignSafe(PersistentRegionLock&, T* ptr) {
     PersistentRegionLock::AssertLocked();
     const void* old_value = GetValue();
     if (IsValid(old_value)) {