bindings: Handle null ephemeron keys in heap snapshot

Heap snapshot filters ephemeron entries with null keys.

Also, because heap snapshot doesn't do stack scannig, we are not guaranteed
to trace all ephemeron entries by the end of the snapshot.
Replacing ephemeron/worklist handling with a fixed-point computation to
guarantee we cover as many objects as we can.

Followup CL will filter untraceable ephemeron values.

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

third_party/blink/renderer/bindings/core/v8/v8_embedder_graph_builder.cc

@@ -3,7 +3,6 @@
 // found in the LICENSE file.
 
 #include "third_party/blink/renderer/bindings/core/v8/v8_embedder_graph_builder.h"
-
 #include "third_party/blink/renderer/bindings/core/v8/active_script_wrappable.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_gc_controller.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_node.h"
@@ -354,7 +353,10 @@ class GC_PLUGIN_IGNORE(
         TraceKeysScope scope(builder, &key);
         key_tracing_callback_(builder, const_cast<void*>(key_));
       }
-      DCHECK(key);
+      if (!key) {
+        // Don't trace the value if the key is nullptr.
+        return true;
+      }
       if (!builder->StateExists(key))
         return false;
       {
@@ -562,7 +564,12 @@ void V8EmbedderGraphBuilder::BuildEmbedderGraph() {
   DCHECK(worklist_.empty());
   DCHECK(detached_worklist_.empty());
   DCHECK(unknown_worklist_.empty());
-  DCHECK(ephemeron_worklist_.empty());
+  // ephemeron_worklist_ might not be empty. We might have an ephemeron whose
+  // key is alive but was never observed by the snapshot (e.g. objects pointed
+  // to by the stack). Such entries will remain in the worklist.
+  //
+  // TODO(omerkatz): add DCHECK(ephemeron_worklist_.empty()) when heap snapshot
+  // covers all live objects.
 }
 
 void V8EmbedderGraphBuilder::VisitPersistentHandleInternal(
@@ -783,7 +790,7 @@ void V8EmbedderGraphBuilder::VisitTransitiveClosure() {
   // where key's do not have yet a state associated with them which prohibits
   // them from being processed. Such ephemerons are stashed for later
   // processing.
-  Deque<std::unique_ptr<EphemeronItem>> unprocessed_ephemerons_;
+  bool processed_ephemerons;
   do {
     // Step 1: Go through all items in the worklist using depth-first search.
     while (!worklist_.empty()) {
@@ -793,12 +800,8 @@ void V8EmbedderGraphBuilder::VisitTransitiveClosure() {
     }
 
     // Step 2: Go through ephemeron items.
-
-    // Re-add unprocessed ephemerons from last loop iteration.
-    while (!unprocessed_ephemerons_.empty()) {
-      ephemeron_worklist_.push_back(unprocessed_ephemerons_.TakeFirst());
-    }
-
+    processed_ephemerons = false;
+    Deque<std::unique_ptr<EphemeronItem>> unprocessed_ephemerons_;
     //  Only process an ephemeron item if its key was already observed.
     while (!ephemeron_worklist_.empty()) {
       std::unique_ptr<EphemeronItem> item =
@@ -806,15 +809,12 @@ void V8EmbedderGraphBuilder::VisitTransitiveClosure() {
       ephemeron_worklist_.pop_front();
       if (!item->Process(this)) {
         unprocessed_ephemerons_.push_back(std::move(item));
+      } else {
+        processed_ephemerons = true;
       }
     }
-  } while (!worklist_.empty());
-
-  // Re-add unprocessed ephemerons. A later invocation of VisitTransitiveClosure
-  // must process them.
-  while (!unprocessed_ephemerons_.empty()) {
-    ephemeron_worklist_.push_back(unprocessed_ephemerons_.TakeFirst());
-  }
+    ephemeron_worklist_.Swap(unprocessed_ephemerons_);
+  } while (!worklist_.empty() || processed_ephemerons);
 }
 
 }  // namespace

third_party/blink/renderer/platform/heap/weakness_marking_test.cc

@@ -52,7 +52,7 @@ void TestMapImpl(ObjectLiveness expected_key_liveness,
 }
 
 TEST_F(WeaknessMarkingTest, WeakToWeakMap) {
-  typedef HeapHashMap<WeakMember<IntegerObject>, WeakMember<IntegerObject>> Map;
+  using Map = HeapHashMap<WeakMember<IntegerObject>, WeakMember<IntegerObject>>;
   TestMapImpl<Map, Persistent, Persistent>(ObjectLiveness::Alive,
                                            ObjectLiveness::Alive);
   TestMapImpl<Map, WeakPersistent, Persistent>(ObjectLiveness::Dead,
@@ -64,7 +64,7 @@ TEST_F(WeaknessMarkingTest, WeakToWeakMap) {
 }
 
 TEST_F(WeaknessMarkingTest, WeakToStrongMap) {
-  typedef HeapHashMap<WeakMember<IntegerObject>, Member<IntegerObject>> Map;
+  using Map = HeapHashMap<WeakMember<IntegerObject>, Member<IntegerObject>>;
   TestMapImpl<Map, Persistent, Persistent>(ObjectLiveness::Alive,
                                            ObjectLiveness::Alive);
   TestMapImpl<Map, WeakPersistent, Persistent>(ObjectLiveness::Dead,
@@ -76,7 +76,7 @@ TEST_F(WeaknessMarkingTest, WeakToStrongMap) {
 }
 
 TEST_F(WeaknessMarkingTest, StrongToWeakMap) {
-  typedef HeapHashMap<Member<IntegerObject>, WeakMember<IntegerObject>> Map;
+  using Map = HeapHashMap<Member<IntegerObject>, WeakMember<IntegerObject>>;
   TestMapImpl<Map, Persistent, Persistent>(ObjectLiveness::Alive,
                                            ObjectLiveness::Alive);
   TestMapImpl<Map, WeakPersistent, Persistent>(ObjectLiveness::Alive,
@@ -88,7 +88,7 @@ TEST_F(WeaknessMarkingTest, StrongToWeakMap) {
 }
 
 TEST_F(WeaknessMarkingTest, StrongToStrongMap) {
-  typedef HeapHashMap<Member<IntegerObject>, Member<IntegerObject>> Map;
+  using Map = HeapHashMap<Member<IntegerObject>, Member<IntegerObject>>;
   TestMapImpl<Map, Persistent, Persistent>(ObjectLiveness::Alive,
                                            ObjectLiveness::Alive);
   TestMapImpl<Map, WeakPersistent, Persistent>(ObjectLiveness::Alive,
@@ -114,15 +114,37 @@ void TestSetImpl(ObjectLiveness object_liveness) {
 }
 
 TEST_F(WeaknessMarkingTest, WeakSet) {
-  typedef HeapHashSet<WeakMember<IntegerObject>> Set;
+  using Set = HeapHashSet<WeakMember<IntegerObject>>;
   TestSetImpl<Set, Persistent>(ObjectLiveness::Alive);
   TestSetImpl<Set, WeakPersistent>(ObjectLiveness::Dead);
 }
 
 TEST_F(WeaknessMarkingTest, StrongSet) {
-  typedef HeapHashSet<Member<IntegerObject>> Set;
+  using Set = HeapHashSet<Member<IntegerObject>>;
   TestSetImpl<Set, Persistent>(ObjectLiveness::Alive);
   TestSetImpl<Set, WeakPersistent>(ObjectLiveness::Alive);
 }
 
+TEST_F(WeaknessMarkingTest, DeadValueInReverseEphemeron) {
+  using Map = HeapHashMap<Member<IntegerObject>, WeakMember<IntegerObject>>;
+  Persistent<Map> map = MakeGarbageCollected<Map>();
+  Persistent<IntegerObject> key = MakeGarbageCollected<IntegerObject>(1);
+  map->insert(key.Get(), MakeGarbageCollected<IntegerObject>(2));
+  EXPECT_EQ(1u, map->size());
+  TestSupportingGC::PreciselyCollectGarbage();
+  // Entries with dead values are removed.
+  EXPECT_EQ(0u, map->size());
+}
+
+TEST_F(WeaknessMarkingTest, NullValueInReverseEphemeron) {
+  using Map = HeapHashMap<Member<IntegerObject>, WeakMember<IntegerObject>>;
+  Persistent<Map> map = MakeGarbageCollected<Map>();
+  Persistent<IntegerObject> key = MakeGarbageCollected<IntegerObject>(1);
+  map->insert(key.Get(), nullptr);
+  EXPECT_EQ(1u, map->size());
+  TestSupportingGC::PreciselyCollectGarbage();
+  // Entries with null values are kept.
+  EXPECT_EQ(1u, map->size());
+}
+
 }  // namespace blink