From ec741dbd7d95d0c37c3542d59d1a7d96a4767a03 Mon Sep 17 00:00:00 2001
From: Michael Lippautz <mlippautz@chromium.org>
Date: Thu, 4 Mar 2021 10:41:56 +0100
Subject: [PATCH] cppgc: Rework Visitor ephemeron handling

Fixes an issue with tracing empty ephemeron values of mixin types.

Bug: chromium:1056170
Change-Id: I0089df29943ba7670ec4bdfa5592a01b0ec6de04
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2732025
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#73185}
---
 include/cppgc/visitor.h                       | 63 +++++++++----------
 .../heap/cppgc/ephemeron-pair-unittest.cc     | 13 ++++
 2 files changed, 43 insertions(+), 33 deletions(-)

diff --git a/include/cppgc/visitor.h b/include/cppgc/visitor.h
index 6ab78428cd..98de9957bd 100644
--- a/include/cppgc/visitor.h
+++ b/include/cppgc/visitor.h
@@ -158,48 +158,52 @@ class V8_EXPORT Visitor {
   }
 
   /**
-   * Trace method for ephemerons. Used for tracing raw ephemeron in which the
-   * key and value are kept separately.
+   * Trace method for a single ephemeron. Used for tracing a raw ephemeron in
+   * which the `key` and `value` are kept separately.
    *
-   * \param key WeakMember reference weakly retaining a key object.
-   * \param value Member reference weakly retaining a value object.
+   * \param weak_member_key WeakMember reference weakly retaining a key object.
+   * \param member_value Member reference with ephemeron semantics.
    */
   template <typename KeyType, typename ValueType>
-  void TraceEphemeron(const WeakMember<KeyType>& key,
-                      const Member<ValueType>* value) {
-    const KeyType* k = key.GetRawAtomic();
-    if (!k) return;
+  void TraceEphemeron(const WeakMember<KeyType>& weak_member_key,
+                      const Member<ValueType>* member_value) {
+    const KeyType* key = weak_member_key.GetRawAtomic();
+    if (!key) return;
 
     // `value` must always be non-null.
-    CPPGC_DCHECK(value);
-    TraceDescriptor value_desc =
-        TraceTrait<Member<ValueType>>::GetTraceDescriptor(value);
-    if (!value_desc.base_object_payload) return;
+    CPPGC_DCHECK(member_value);
+    const ValueType* value = member_value->GetRawAtomic();
+    if (!value) return;
 
-    // KeyType might be a GarbageCollectedMixin.
+    // KeyType and ValueType may refer to GarbageCollectedMixin.
+    TraceDescriptor value_desc =
+        TraceTrait<ValueType>::GetTraceDescriptor(value);
+    CPPGC_DCHECK(value_desc.base_object_payload);
     const void* key_base_object_payload =
-        TraceTrait<KeyType>::GetTraceDescriptor(k).base_object_payload;
+        TraceTrait<KeyType>::GetTraceDescriptor(key).base_object_payload;
     CPPGC_DCHECK(key_base_object_payload);
 
-    // `value_desc.base_object_payload` must also be non-null because empty
-    // values are filtered above.
-    CPPGC_DCHECK(value_desc.base_object_payload);
     VisitEphemeron(key_base_object_payload, value, value_desc);
   }
 
   /**
-   * Trace method for ephemerons. Used for tracing raw ephemeron in which the
-   * key and value are kept separately.
+   * Trace method for a single ephemeron. Used for tracing a raw ephemeron in
+   * which the `key` and `value` are kept separately. Note that this overload
+   * is for non-GarbageCollected `value`s that can be traced though.
    *
-   * \param key WeakMember reference weakly retaining a key object.
-   * \param value Traceable reference weakly retaining a value object.
+   * \param key `WeakMember` reference weakly retaining a key object.
+   * \param value Reference weakly retaining a value object. Note that
+   *   `ValueType` here should not be `Member`. It is expected that
+   *   `TraceTrait<ValueType>::GetTraceDescriptor(value)` returns a
+   *   `TraceDescriptor` with a null base pointer but a valid trace method.
    */
   template <typename KeyType, typename ValueType>
-  void TraceEphemeron(const WeakMember<KeyType>& key, const ValueType* value) {
+  void TraceEphemeron(const WeakMember<KeyType>& weak_member_key,
+                      const ValueType* value) {
     static_assert(!IsGarbageCollectedOrMixinTypeV<ValueType>,
                   "garbage-collected types must use WeakMember and Member");
-    const KeyType* k = key.GetRawAtomic();
-    if (!k) return;
+    const KeyType* key = weak_member_key.GetRawAtomic();
+    if (!key) return;
 
     // `value` must always be non-null.
     CPPGC_DCHECK(value);
@@ -211,8 +215,9 @@ class V8_EXPORT Visitor {
 
     // KeyType might be a GarbageCollectedMixin.
     const void* key_base_object_payload =
-        TraceTrait<KeyType>::GetTraceDescriptor(k).base_object_payload;
+        TraceTrait<KeyType>::GetTraceDescriptor(key).base_object_payload;
     CPPGC_DCHECK(key_base_object_payload);
+
     VisitEphemeron(key_base_object_payload, value, value_desc);
   }
 
@@ -367,14 +372,6 @@ class V8_EXPORT Visitor {
   friend class internal::VisitorBase;
 };
 
-template <typename T>
-struct TraceTrait<Member<T>> {
-  static TraceDescriptor GetTraceDescriptor(const void* self) {
-    return TraceTrait<T>::GetTraceDescriptor(
-        static_cast<const Member<T>*>(self)->GetRawAtomic());
-  }
-};
-
 }  // namespace cppgc
 
 #endif  // INCLUDE_CPPGC_VISITOR_H_
diff --git a/test/unittests/heap/cppgc/ephemeron-pair-unittest.cc b/test/unittests/heap/cppgc/ephemeron-pair-unittest.cc
index ca29fabe37..32a5929fe4 100644
--- a/test/unittests/heap/cppgc/ephemeron-pair-unittest.cc
+++ b/test/unittests/heap/cppgc/ephemeron-pair-unittest.cc
@@ -226,5 +226,18 @@ TEST_F(EphemeronPairTest, EphemeronPairWithMixinKey) {
   EXPECT_TRUE(HeapObjectHeader::FromPayload(value).IsMarked());
 }
 
+TEST_F(EphemeronPairTest, EphemeronPairWithEmptyMixinValue) {
+  GCedWithMixin* key =
+      MakeGarbageCollected<GCedWithMixin>(GetAllocationHandle());
+  Persistent<EphemeronHolderWithMixins> holder =
+      MakeGarbageCollected<EphemeronHolderWithMixins>(GetAllocationHandle(),
+                                                      key, nullptr);
+  EXPECT_NE(static_cast<void*>(key), holder->ephemeron_pair().key.Get());
+  EXPECT_TRUE(HeapObjectHeader::FromPayload(key).TryMarkAtomic());
+  InitializeMarker(*Heap::From(GetHeap()), GetPlatformHandle().get());
+  FinishSteps();
+  FinishMarking();
+}
+
 }  // namespace internal
 }  // namespace cppgc