heap: Merge atomic loads of member types

Tracing a Member or a WeakMember first checks whether the member is
deleted. If the check passes, the member is traced. The check and
the trace both use atomic loads, but not the same load. This can
result in time-of-check vs time-of-use data races. This CL merges
both loads to a single load to avoid such issues.

Furthermore, members in hash tables now might be deleted while the
relevant entry is deleted. Since the is deleted check is performed
on a copy, member types need to bail out on deleted values (used to
be a DCHECK before).

Bug: 986235
Change-Id: I4307361af94081b62d159af67cdbde5666378550
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2012285
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735356}

third_party/blink/renderer/platform/heap/member.h

@@ -195,8 +195,12 @@ class MemberBase {
     return result;
   }
 
+  static bool IsMemberHashTableDeletedValue(T* t) {
+    return t == reinterpret_cast<T*>(kHashTableDeletedRawValue);
+  }
+
   bool IsHashTableDeletedValue() const {
-    return GetRaw() == reinterpret_cast<T*>(kHashTableDeletedRawValue);
+    return IsMemberHashTableDeletedValue(GetRaw());
   }
 
  protected:
@@ -237,11 +241,6 @@ class MemberBase {
     return WTF::AsAtomicPtr(&raw_)->load(std::memory_order_relaxed);
   }
 
-  // Thread safe version of IsHashTableDeletedValue for use while tracing.
-  bool IsHashTableDeletedValueSafe() const {
-    return GetSafe() == reinterpret_cast<T*>(kHashTableDeletedRawValue);
-  }
-
   T* raw_;
 #if DCHECK_IS_ON()
   MemberPointerVerifier<T, tracenessConfiguration> pointer_verifier_;

third_party/blink/renderer/platform/heap/trace_traits.h

@@ -358,11 +358,28 @@ struct TraceInCollectionTrait<kNoWeakHandling, T, Traits> {
   }
 };
 
+template <typename T, typename Traits>
+struct TraceInCollectionTrait<kNoWeakHandling, blink::Member<T>, Traits> {
+  static bool IsAlive(blink::Member<T>& t) { return true; }
+  static bool Trace(blink::Visitor* visitor, blink::Member<T>& t) {
+    visitor->TraceMaybeDeleted(t);
+    return false;
+  }
+};
+
+template <typename T, typename Traits>
+struct TraceInCollectionTrait<kWeakHandling, blink::Member<T>, Traits> {
+  static bool IsAlive(blink::Member<T>& t) { return true; }
+  static bool Trace(blink::Visitor* visitor, blink::Member<T>& t) {
+    visitor->TraceMaybeDeleted(t);
+    return false;
+  }
+};
+
 template <typename T, typename Traits>
 struct TraceInCollectionTrait<kNoWeakHandling, blink::WeakMember<T>, Traits> {
   static bool Trace(blink::Visitor* visitor, blink::WeakMember<T>& t) {
-    // Extract raw pointer to avoid using the WeakMember<> overload in Visitor.
-    visitor->Trace(t.Get());
+    visitor->TraceMaybeDeleted(static_cast<blink::Member<T>>(t));
     return false;
   }
 };

third_party/blink/renderer/platform/heap/visitor.h

@@ -108,10 +108,20 @@ class PLATFORM_EXPORT Visitor {
     VisitRoot(const_cast<T*>(t), TraceDescriptorFor(t), location);
   }
 
-  template <typename T>
+  template <typename T, bool maybe_deleted = false>
   void Trace(const Member<T>& t) {
-    DCHECK(!t.IsHashTableDeletedValueSafe());
-    Trace(t.GetSafe());
+    T* value = t.GetSafe();
+
+    if (maybe_deleted && Member<T>::IsMemberHashTableDeletedValue(value))
+      return;
+    DCHECK(!Member<T>::IsMemberHashTableDeletedValue(value));
+
+    Trace(value);
+  }
+
+  template <typename T>
+  ALWAYS_INLINE void TraceMaybeDeleted(const Member<T>& t) {
+    Trace<T, true>(t);
   }
 
   // Fallback methods used only when we need to trace raw pointers of T. This is
@@ -187,7 +197,7 @@ class PLATFORM_EXPORT Visitor {
     if (!value)
       return;
 
-    DCHECK(!weak_member.IsHashTableDeletedValueSafe());
+    DCHECK(!WeakMember<T>::IsMemberHashTableDeletedValue(value));
     VisitWeak(value, &weak_member, TraceDescriptorFor(value),
               &HandleWeakCell<T>);
   }