Add verifier for retaining paths in heap snapshots

The web app owner who notified me about bugs v8:12112 and v8:12126 asked
me a reasonable question: "how am I ever supposed to trust the retaining
paths in the devtools, if the heap snapshot is generated by a different
component than the actual marking code?". This change is my attempt to
answer that question. If verification is enabled, the heap snapshot
generator will visit each heap object with a realistic marking visitor
to find all references from that object. It will then check that those
references match the HeapGraphEdges in the snapshot.

I also considered the idea that we could collect retaining information
during the last GC cycle before taking the heap snapshot, or during an
extra GC cycle immediately after. However, running the full GC provides
the embedder with the opportunity to run arbitrary code (including JS)
both before and after PerformGarbageCollection, so there is no clear
guarantee that the heap state during the snapshot actually matches the
heap state during marking.

Bug: v8:12112, v8:12126
Change-Id: Id29e75ecf9eee19e35daedbdb4a3e1df64785380
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3299590Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#78952}

BUILD.gn

@@ -342,6 +342,9 @@ declare_args() {
   # When it's disabled, the --turbo-allocation-folding runtime flag will be ignored.
   v8_enable_allocation_folding = true
 
+  # Enable runtime verification of heap snapshots produced for devtools.
+  v8_enable_heap_snapshot_verify = ""
+
   # Enable global allocation site tracking.
   v8_allocation_site_tracking = true
 
@@ -385,6 +388,10 @@ if (v8_enable_test_features == "") {
 if (v8_enable_v8_checks == "") {
   v8_enable_v8_checks = v8_enable_debugging_features
 }
+if (v8_enable_heap_snapshot_verify == "") {
+  v8_enable_heap_snapshot_verify =
+      v8_enable_debugging_features || dcheck_always_on
+}
 if (v8_enable_snapshot_code_comments) {
   assert(v8_code_comments == true || v8_code_comments == "",
          "v8_enable_snapshot_code_comments conflicts with v8_code_comments.")
@@ -891,6 +898,9 @@ config("features") {
   if (v8_enable_debug_code) {
     defines += [ "V8_ENABLE_DEBUG_CODE" ]
   }
+  if (v8_enable_heap_snapshot_verify) {
+    defines += [ "V8_ENABLE_HEAP_SNAPSHOT_VERIFY" ]
+  }
   if (v8_enable_snapshot_native_code_counters) {
     defines += [ "V8_SNAPSHOT_NATIVE_CODE_COUNTERS" ]
   }
@@ -3572,6 +3582,10 @@ v8_header_set("v8_internal_headers") {
     ]
   }
 
+  if (v8_enable_heap_snapshot_verify) {
+    sources += [ "src/heap/reference-summarizer.h" ]
+  }
+
   if (v8_current_cpu == "x86") {
     sources += [  ### gcmole(arch:ia32) ###
       "src/baseline/ia32/baseline-assembler-ia32-inl.h",
@@ -4513,6 +4527,10 @@ v8_source_set("v8_base_without_compiler") {
     ]
   }
 
+  if (v8_enable_heap_snapshot_verify) {
+    sources += [ "src/heap/reference-summarizer.cc" ]
+  }
+
   if (v8_current_cpu == "x86") {
     sources += [  ### gcmole(arch:ia32) ###
       "src/codegen/ia32/assembler-ia32.cc",

src/DEPS

@@ -36,6 +36,7 @@ include_rules = [
   "+src/heap/parked-scope.h",
   "+src/heap/read-only-heap-inl.h",
   "+src/heap/read-only-heap.h",
+  "+src/heap/reference-summarizer.h",
   "+src/heap/safepoint.h",
   "+src/heap/base/stack.h",
   "+src/heap/conservative-stack-visitor.h",

src/flags/flag-definitions.h

@@ -1618,6 +1618,11 @@ DEFINE_INT(heap_snapshot_string_limit, 1024,
            "truncate strings to this length in the heap snapshot")
 DEFINE_BOOL(heap_profiler_show_hidden_objects, false,
             "use 'native' rather than 'hidden' node type in snapshot")
+#ifdef V8_ENABLE_HEAP_SNAPSHOT_VERIFY
+DEFINE_BOOL(heap_snapshot_verify, false,
+            "verify that heap snapshot matches marking visitor behavior")
+DEFINE_IMPLICATION(enable_slow_asserts, heap_snapshot_verify)
+#endif
 
 // sampling-heap-profiler.cc
 DEFINE_BOOL(sampling_heap_profiler_suppress_randomness, false,

src/heap/marking-visitor-inl.h

@@ -26,6 +26,7 @@ void MarkingVisitorBase<ConcreteVisitor, MarkingState>::MarkObject(
     HeapObject host, HeapObject object) {
   DCHECK(ReadOnlyHeap::Contains(object) || heap_->Contains(object));
   concrete_visitor()->SynchronizePageAccess(object);
+  AddStrongReferenceForReferenceSummarizer(host, object);
   if (concrete_visitor()->marking_state()->WhiteToGrey(object)) {
     local_marking_worklists_->Push(object);
     if (V8_UNLIKELY(concrete_visitor()->retaining_path_mode() ==
@@ -65,6 +66,7 @@ void MarkingVisitorBase<ConcreteVisitor, MarkingState>::ProcessWeakHeapObject(
     // the reference when we know the liveness of the whole transitive
     // closure.
     local_weak_objects_->weak_references_local.Push(std::make_pair(host, slot));
+    AddWeakReferenceForReferenceSummarizer(host, heap_object);
   }
 }
 
@@ -117,6 +119,7 @@ void MarkingVisitorBase<ConcreteVisitor, MarkingState>::VisitEmbeddedPointer(
     if (host.IsWeakObject(object)) {
       local_weak_objects_->weak_objects_in_code_local.Push(
           std::make_pair(object, host));
+      AddWeakReferenceForReferenceSummarizer(host, object);
     } else {
       MarkObject(host, object);
     }
@@ -244,7 +247,7 @@ int MarkingVisitorBase<ConcreteVisitor, MarkingState>::VisitFixedArray(
   // in the large object space.
   ProgressBar& progress_bar =
       MemoryChunk::FromHeapObject(object)->ProgressBar();
-  return progress_bar.IsEnabled()
+  return CanUpdateValuesInHeap() && progress_bar.IsEnabled()
              ? VisitFixedArrayWithProgressBar(map, object, progress_bar)
              : concrete_visitor()->VisitLeftTrimmableArray(map, object);
 }
@@ -344,6 +347,7 @@ int MarkingVisitorBase<ConcreteVisitor, MarkingState>::VisitEphemeronHashTable(
 
     concrete_visitor()->SynchronizePageAccess(key);
     concrete_visitor()->RecordSlot(table, key_slot, key);
+    AddWeakReferenceForReferenceSummarizer(table, key);
 
     ObjectSlot value_slot =
         table.RawFieldOfElementAt(EphemeronHashTable::EntryToValueIndex(i));
@@ -357,6 +361,7 @@ int MarkingVisitorBase<ConcreteVisitor, MarkingState>::VisitEphemeronHashTable(
         HeapObject value = HeapObject::cast(value_obj);
         concrete_visitor()->SynchronizePageAccess(value);
         concrete_visitor()->RecordSlot(table, value_slot, value);
+        AddWeakReferenceForReferenceSummarizer(table, value);
 
         // Revisit ephemerons with both key and value unreachable at end
         // of concurrent marking cycle.
@@ -387,6 +392,7 @@ int MarkingVisitorBase<ConcreteVisitor, MarkingState>::VisitJSWeakRef(
       // JSWeakRef points to a potentially dead object. We have to process
       // them when we know the liveness of the whole transitive closure.
       local_weak_objects_->js_weak_refs_local.Push(weak_ref);
+      AddWeakReferenceForReferenceSummarizer(weak_ref, target);
     }
   }
   return size;
@@ -417,6 +423,8 @@ int MarkingVisitorBase<ConcreteVisitor, MarkingState>::VisitWeakCell(
     // token. We have to process them when we know the liveness of the whole
     // transitive closure.
     local_weak_objects_->weak_cells_local.Push(weak_cell);
+    AddWeakReferenceForReferenceSummarizer(weak_cell, target);
+    AddWeakReferenceForReferenceSummarizer(weak_cell, unregister_token);
   }
   return size;
 }
@@ -443,8 +451,11 @@ template <typename ConcreteVisitor, typename MarkingState>
 void MarkingVisitorBase<ConcreteVisitor, MarkingState>::VisitDescriptors(
     DescriptorArray descriptor_array, int number_of_own_descriptors) {
   int16_t new_marked = static_cast<int16_t>(number_of_own_descriptors);
-  int16_t old_marked = descriptor_array.UpdateNumberOfMarkedDescriptors(
-      mark_compact_epoch_, new_marked);
+  int16_t old_marked = 0;
+  if (CanUpdateValuesInHeap()) {
+    old_marked = descriptor_array.UpdateNumberOfMarkedDescriptors(
+        mark_compact_epoch_, new_marked);
+  }
   if (old_marked < new_marked) {
     VisitPointers(
         descriptor_array,

src/heap/marking-visitor.h

@@ -25,6 +25,11 @@ struct EphemeronMarking {
 template <typename ConcreteState, AccessMode access_mode>
 class MarkingStateBase {
  public:
+  // Declares that this marking state is not collecting retainers, so the
+  // marking visitor may update the heap state to store information about
+  // progress, and may avoid fully visiting an object if it is safe to do so.
+  static constexpr bool kCollectRetainers = false;
+
   explicit MarkingStateBase(PtrComprCageBase cage_base)
 #if V8_COMPRESS_POINTERS
       : cage_base_(cage_base)
@@ -102,6 +107,15 @@ class MarkingStateBase {
     static_cast<ConcreteState*>(this)->SetLiveBytes(chunk, 0);
   }
 
+  void AddStrongReferenceForReferenceSummarizer(HeapObject host,
+                                                HeapObject obj) {
+    // This is not a reference summarizer, so there is nothing to do here.
+  }
+
+  void AddWeakReferenceForReferenceSummarizer(HeapObject host, HeapObject obj) {
+    // This is not a reference summarizer, so there is nothing to do here.
+  }
+
  private:
 #if V8_COMPRESS_POINTERS
   const PtrComprCageBase cage_base_;
@@ -258,6 +272,23 @@ class MarkingVisitorBase : public HeapVisitor<int, ConcreteVisitor> {
   // Marks the object grey and pushes it on the marking work list.
   V8_INLINE void MarkObject(HeapObject host, HeapObject obj);
 
+  V8_INLINE void AddStrongReferenceForReferenceSummarizer(HeapObject host,
+                                                          HeapObject obj) {
+    concrete_visitor()
+        ->marking_state()
+        ->AddStrongReferenceForReferenceSummarizer(host, obj);
+  }
+
+  V8_INLINE void AddWeakReferenceForReferenceSummarizer(HeapObject host,
+                                                        HeapObject obj) {
+    concrete_visitor()->marking_state()->AddWeakReferenceForReferenceSummarizer(
+        host, obj);
+  }
+
+  constexpr bool CanUpdateValuesInHeap() {
+    return !MarkingState::kCollectRetainers;
+  }
+
   MarkingWorklists::Local* const local_marking_worklists_;
   WeakObjects::Local* const local_weak_objects_;
   Heap* const heap_;

src/heap/reference-summarizer.cc

+// Copyright 2022 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/heap/reference-summarizer.h"
+
+#include "src/heap/mark-compact-inl.h"
+#include "src/heap/marking-visitor-inl.h"
+#include "src/objects/embedder-data-array-inl.h"
+#include "src/objects/js-array-buffer-inl.h"
+
+namespace v8 {
+namespace internal {
+
+namespace {
+
+// A class which acts as a MarkingState but does not actually update any marking
+// bits. It reports all objects as white and all transitions as successful. It
+// also tracks which objects are retained by the primary object according to the
+// marking visitor.
+class ReferenceSummarizerMarkingState final {
+ public:
+  // Declares that this marking state is collecting retainers, so the marking
+  // visitor must fully visit each object and can't update on-heap state.
+  static constexpr bool kCollectRetainers = true;
+
+  explicit ReferenceSummarizerMarkingState(HeapObject object)
+      : primary_object_(object),
+        local_marking_worklists_(&marking_worklists_),
+        local_weak_objects_(&weak_objects_) {}
+
+  ~ReferenceSummarizerMarkingState() {
+    // Clean up temporary state.
+    local_weak_objects_.Publish();
+    weak_objects_.Clear();
+    local_marking_worklists_.Publish();
+    marking_worklists_.Clear();
+  }
+
+  // Retrieves the references that were collected by this marker. This operation
+  // transfers ownership of the set, so calling it again would yield an empty
+  // result.
+  ReferenceSummary DestructivelyRetrieveReferences() {
+    ReferenceSummary tmp = std::move(references_);
+    references_.Clear();
+    return tmp;
+  }
+
+  // Standard marking visitor functions:
+
+  bool IsWhite(HeapObject obj) const { return true; }
+
+  bool IsBlackOrGrey(HeapObject obj) const { return false; }
+
+  bool WhiteToGrey(HeapObject obj) { return true; }
+
+  bool GreyToBlack(HeapObject obj) { return true; }
+
+  // Adds a retaining relationship found by the marking visitor.
+  void AddStrongReferenceForReferenceSummarizer(HeapObject host,
+                                                HeapObject obj) {
+    AddReference(host, obj, references_.strong_references());
+  }
+
+  // Adds a non-retaining weak reference found by the marking visitor. The value
+  // in an ephemeron hash table entry is also included here, since it is not
+  // known to be strong without further information about the key.
+  void AddWeakReferenceForReferenceSummarizer(HeapObject host, HeapObject obj) {
+    AddReference(host, obj, references_.weak_references());
+  }
+
+  // Other member functions, not part of the marking visitor contract:
+
+  MarkingWorklists::Local* local_marking_worklists() {
+    return &local_marking_worklists_;
+  }
+  WeakObjects::Local* local_weak_objects() { return &local_weak_objects_; }
+
+ private:
+  void AddReference(
+      HeapObject host, HeapObject obj,
+      std::unordered_set<HeapObject, Object::Hasher>& references) {
+    // It's possible that the marking visitor handles multiple objects at once,
+    // such as a Map and its DescriptorArray, but we're only interested in
+    // references from the primary object.
+    if (host == primary_object_) {
+      references.insert(obj);
+    }
+  }
+
+  ReferenceSummary references_;
+  HeapObject primary_object_;
+  MarkingWorklists marking_worklists_;
+  MarkingWorklists::Local local_marking_worklists_;
+  WeakObjects weak_objects_;
+  WeakObjects::Local local_weak_objects_;
+};
+
+}  // namespace
+
+ReferenceSummary ReferenceSummary::SummarizeReferencesFrom(Heap* heap,
+                                                           HeapObject obj) {
+  ReferenceSummarizerMarkingState marking_state(obj);
+
+  MainMarkingVisitor<ReferenceSummarizerMarkingState> visitor(
+      &marking_state, marking_state.local_marking_worklists(),
+      marking_state.local_weak_objects(), heap, 0 /*mark_compact_epoch*/,
+      {} /*code_flush_mode*/, false /*embedder_tracing_enabled*/,
+      true /*should_keep_ages_unchanged*/);
+  visitor.Visit(obj.map(heap->isolate()), obj);
+
+  return marking_state.DestructivelyRetrieveReferences();
+}
+
+}  // namespace internal
+}  // namespace v8

src/heap/reference-summarizer.h

+// Copyright 2022 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef V8_HEAP_REFERENCE_SUMMARIZER_H_
+#define V8_HEAP_REFERENCE_SUMMARIZER_H_
+
+#include <unordered_set>
+
+#include "src/objects/heap-object.h"
+
+namespace v8 {
+namespace internal {
+
+class Heap;
+
+class ReferenceSummary {
+ public:
+  ReferenceSummary() = default;
+  ReferenceSummary(ReferenceSummary&& other) V8_NOEXCEPT
+      : strong_references_(std::move(other.strong_references_)),
+        weak_references_(std::move(other.weak_references_)) {}
+
+  // Produces a set of objects referred to by the object. This function uses a
+  // realistic marking visitor, so its results are likely to match real GC
+  // behavior. Intended only for verification.
+  static ReferenceSummary SummarizeReferencesFrom(Heap* heap, HeapObject obj);
+
+  // All objects which the chosen object has strong pointers to.
+  std::unordered_set<HeapObject, Object::Hasher>& strong_references() {
+    return strong_references_;
+  }
+
+  // All objects which the chosen object has weak pointers to. The values in
+  // ephemeron hash tables are also included here, even though they aren't
+  // normal weak pointers.
+  std::unordered_set<HeapObject, Object::Hasher>& weak_references() {
+    return weak_references_;
+  }
+
+  void Clear() {
+    strong_references_.clear();
+    weak_references_.clear();
+  }
+
+ private:
+  std::unordered_set<HeapObject, Object::Hasher> strong_references_;
+  std::unordered_set<HeapObject, Object::Hasher> weak_references_;
+  DISALLOW_GARBAGE_COLLECTION(no_gc)
+};
+
+}  // namespace internal
+}  // namespace v8
+
+#endif  // V8_HEAP_REFERENCE_SUMMARIZER_H_

src/profiler/heap-snapshot-generator.h

@@ -24,6 +24,10 @@
 #include "src/profiler/strings-storage.h"
 #include "src/strings/string-hasher.h"
 
+#ifdef V8_ENABLE_HEAP_SNAPSHOT_VERIFY
+#include "src/heap/reference-summarizer.h"
+#endif
+
 namespace v8 {
 namespace internal {
 
@@ -141,17 +145,40 @@ class HeapEntry {
   }
   uint8_t detachedness() const { return detachedness_; }
 
-  void SetIndexedReference(
-      HeapGraphEdge::Type type, int index, HeapEntry* entry);
-  void SetNamedReference(
-      HeapGraphEdge::Type type, const char* name, HeapEntry* entry);
-  void SetIndexedAutoIndexReference(HeapGraphEdge::Type type,
-                                    HeapEntry* child) {
-    SetIndexedReference(type, children_count_ + 1, child);
+  enum ReferenceVerification {
+    // Verify that the reference can be found via marking, if verification is
+    // enabled.
+    kVerify,
+
+    // Skip verifying that the reference can be found via marking, for any of
+    // the following reasons:
+
+    kEphemeron,
+    kOffHeapPointer,
+    kCustomWeakPointer,
+  };
+
+  void VerifyReference(HeapGraphEdge::Type type, HeapEntry* entry,
+                       HeapSnapshotGenerator* generator,
+                       ReferenceVerification verification);
+  void SetIndexedReference(HeapGraphEdge::Type type, int index,
+                           HeapEntry* entry, HeapSnapshotGenerator* generator,
+                           ReferenceVerification verification = kVerify);
+  void SetNamedReference(HeapGraphEdge::Type type, const char* name,
+                         HeapEntry* entry, HeapSnapshotGenerator* generator,
+                         ReferenceVerification verification = kVerify);
+  void SetIndexedAutoIndexReference(
+      HeapGraphEdge::Type type, HeapEntry* child,
+      HeapSnapshotGenerator* generator,
+      ReferenceVerification verification = kVerify) {
+    SetIndexedReference(type, children_count_ + 1, child, generator,
+                        verification);
   }
   void SetNamedAutoIndexReference(HeapGraphEdge::Type type,
                                   const char* description, HeapEntry* child,
-                                  StringsStorage* strings);
+                                  StringsStorage* strings,
+                                  HeapSnapshotGenerator* generator,
+                                  ReferenceVerification verification = kVerify);
 
   V8_EXPORT_PRIVATE void Print(const char* prefix, const char* edge_name,
                                int max_depth, int indent) const;
@@ -438,8 +465,10 @@ class V8_EXPORT_PRIVATE V8HeapExplorer : public HeapEntriesAllocator {
                             int field_offset = -1);
   void SetHiddenReference(HeapObject parent_obj, HeapEntry* parent_entry,
                           int index, Object child, int field_offset);
-  void SetWeakReference(HeapEntry* parent_entry, const char* reference_name,
-                        Object child_obj, int field_offset);
+  void SetWeakReference(
+      HeapEntry* parent_entry, const char* reference_name, Object child_obj,
+      int field_offset,
+      HeapEntry::ReferenceVerification verification = HeapEntry::kVerify);
   void SetWeakReference(HeapEntry* parent_entry, int index, Object child_obj,
                         base::Optional<int> field_offset);
   void SetPropertyReference(HeapEntry* parent_entry, Name reference_name,
@@ -511,6 +540,8 @@ class NativeObjectsExplorer {
   friend class GlobalHandlesExtractor;
 };
 
+class HeapEntryVerifier;
+
 class HeapSnapshotGenerator : public SnapshottingProgressReportingInterface {
  public:
   // The HeapEntriesMap instance is used to track a mapping between
@@ -539,10 +570,33 @@ class HeapSnapshotGenerator : public SnapshottingProgressReportingInterface {
   }
 
   HeapEntry* AddEntry(HeapThing ptr, HeapEntriesAllocator* allocator) {
-    return entries_map_.emplace(ptr, allocator->AllocateEntry(ptr))
-        .first->second;
+    HeapEntry* result =
+        entries_map_.emplace(ptr, allocator->AllocateEntry(ptr)).first->second;
+#ifdef V8_ENABLE_HEAP_SNAPSHOT_VERIFY
+    if (FLAG_heap_snapshot_verify) {
+      reverse_entries_map_.emplace(result, ptr);
+    }
+#endif
+    return result;
   }
 
+#ifdef V8_ENABLE_HEAP_SNAPSHOT_VERIFY
+  HeapThing FindHeapThingForHeapEntry(HeapEntry* entry) {
+    // The reverse lookup map is only populated if the verification flag is
+    // enabled.
+    DCHECK(FLAG_heap_snapshot_verify);
+
+    auto it = reverse_entries_map_.find(entry);
+    return it == reverse_entries_map_.end() ? nullptr : it->second;
+  }
+
+  HeapEntryVerifier* verifier() const { return verifier_; }
+  void set_verifier(HeapEntryVerifier* verifier) {
+    DCHECK_IMPLIES(verifier_, !verifier);
+    verifier_ = verifier;
+  }
+#endif
+
   HeapEntry* AddEntry(Smi smi, HeapEntriesAllocator* allocator) {
     return smis_map_.emplace(smi.value(), allocator->AllocateEntry(smi))
         .first->second;
@@ -558,6 +612,8 @@ class HeapSnapshotGenerator : public SnapshottingProgressReportingInterface {
     return entry != nullptr ? entry : AddEntry(smi, allocator);
   }
 
+  Heap* heap() const { return heap_; }
+
  private:
   bool FillReferences();
   void ProgressStep() override;
@@ -575,6 +631,11 @@ class HeapSnapshotGenerator : public SnapshottingProgressReportingInterface {
   uint32_t progress_counter_;
   uint32_t progress_total_;
   Heap* heap_;
+
+#ifdef V8_ENABLE_HEAP_SNAPSHOT_VERIFY
+  std::unordered_map<HeapEntry*, HeapThing> reverse_entries_map_;
+  HeapEntryVerifier* verifier_ = nullptr;
+#endif
 };
 
 class OutputStreamWriter;