heap: MarkingVerifier: Add support for large objects and tests

- Adds support for large objects
- Adds tests for strong and weak backing stores

With these changes, the marking verifier is able to catch any object
resurrections during pre-finalization as well.

Bug: 976703
Change-Id: I75097a0a0d79e136992b07d8881e9136f1a9bd4b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1669932
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#671537}

third_party/blink/renderer/platform/heap/BUILD.gn

@@ -46,6 +46,7 @@ blink_platform_sources("heap") {
     "heap_stats_collector.cc",
     "heap_stats_collector.h",
     "heap_traits.h",
+    "marking_verifier.cc",
     "marking_verifier.h",
     "marking_visitor.cc",
     "marking_visitor.h",

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

@@ -365,7 +365,7 @@ size_t ThreadHeap::ObjectPayloadSizeForTesting() {
 
 void ThreadHeap::ResetAllocationPointForTesting() {
   for (int i = 0; i < BlinkGC::kNumberOfArenas; ++i)
-    arenas_[i]->ResetAllocationPointForTesting();
+    arenas_[i]->ResetAllocationPoint();
 }
 
 BasePage* ThreadHeap::LookupPageForAddress(Address address) {

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

@@ -572,17 +572,24 @@ void NormalPageArena::VerifyObjectStartBitmap() {
 #endif  // DCHECK_IS_ON()
 }
 
-void NormalPageArena::VerifyMarking() {
+void BaseArena::VerifyMarking() {
 #if DCHECK_IS_ON()
   // We cannot rely on other marking phases to clear the allocation area as
   // for incremental marking the application is running between steps and
-  // might set up a new area.
-  SetAllocationPoint(nullptr, 0);
+  // might set up a new area. For large object arenas this is a no-op.
+  ResetAllocationPoint();
+
   DCHECK(swept_unfinalized_pages_.IsEmpty());
   DCHECK(swept_unfinalized_empty_pages_.IsEmpty());
-  // There may be objects on swept_pages_ as pre-finalizers may allocate.
+  // There may be objects on |swept_pages_| as pre-finalizers may allocate.
+  // These objects may point to other object on |swept_pages_| or marked objects
+  // on |unswept_pages_| but may never point to a dead (unmarked) object in
+  // |unswept_pages_|.
+  for (BasePage* page : swept_pages_) {
+    page->VerifyMarking();
+  }
   for (BasePage* page : unswept_pages_) {
-    static_cast<NormalPage*>(page)->VerifyMarking();
+    page->VerifyMarking();
   }
 #endif  // DCHECK_IS_ON()
 }
@@ -1673,6 +1680,11 @@ void NormalPage::VerifyMarking() {
   }
 }
 
+void LargeObjectPage::VerifyMarking() {
+  MarkingVerifier verifier(Arena()->GetThreadState());
+  verifier.VerifyObject(ObjectHeader());
+}
+
 Address ObjectStartBitmap::FindHeader(
     Address address_maybe_pointing_to_the_middle_of_object) {
   size_t object_offset =

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

@@ -762,7 +762,7 @@ class PLATFORM_EXPORT LargeObjectPage final : public BasePage {
 
   bool IsLargeObjectPage() override { return true; }
 
-  void VerifyMarking() override {}
+  void VerifyMarking() override;
 
 #if defined(ADDRESS_SANITIZER)
   void PoisonUnmarkedObjects() override;
@@ -835,9 +835,11 @@ class PLATFORM_EXPORT BaseArena {
 
   Address AllocateLargeObject(size_t allocation_size, size_t gc_info_index);
 
+  // Resets the allocation point if it exists for an arena.
+  virtual void ResetAllocationPoint() {}
+
+  void VerifyMarking();
   virtual void VerifyObjectStartBitmap() {}
-  virtual void VerifyMarking() {}
-  virtual void ResetAllocationPointForTesting() {}
 
  protected:
   bool SweepingCompleted() const { return unswept_pages_.IsEmpty(); }
@@ -908,11 +910,9 @@ class PLATFORM_EXPORT NormalPageArena final : public BaseArena {
 
   void SweepAndCompact();
 
+  void ResetAllocationPoint() override { SetAllocationPoint(nullptr, 0); }
+
   void VerifyObjectStartBitmap() override;
-  void VerifyMarking() override;
-  void ResetAllocationPointForTesting() override {
-    SetAllocationPoint(nullptr, 0);
-  }
 
   Address CurrentAllocationPoint() const { return current_allocation_point_; }
 
@@ -964,6 +964,7 @@ class LargeObjectArena final : public BaseArena {
   LargeObjectArena(ThreadState*, int index);
   Address AllocateLargeObjectPage(size_t, size_t gc_info_index);
   void FreeLargeObjectPage(LargeObjectPage*);
+
 #if DCHECK_IS_ON()
   bool IsConsistentForGC() override { return true; }
 #endif

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

+// Copyright 2019 The Chromium 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 "third_party/blink/renderer/platform/heap/marking_verifier.h"
+
+#include "third_party/blink/renderer/platform/heap/garbage_collected.h"
+#include "third_party/blink/renderer/platform/heap/heap_page.h"
+
+namespace blink {
+
+void MarkingVerifier::VerifyObject(HeapObjectHeader* header) {
+  // Verify only non-free marked objects.
+  if (header->IsFree() || !header->IsMarked())
+    return;
+
+  const GCInfo* info =
+      GCInfoTable::Get().GCInfoFromIndex(header->GcInfoIndex());
+  const bool can_verify =
+      !info->has_v_table || blink::VTableInitialized(header->Payload());
+  if (can_verify) {
+    CHECK(header->IsValid());
+    parent_ = header;
+    info->trace(this, header->Payload());
+  }
+}
+
+void MarkingVerifier::Visit(void* object, TraceDescriptor desc) {
+  VerifyChild(object, desc.base_object_payload);
+}
+
+void MarkingVerifier::VisitWeak(void* object,
+                                void** object_slot,
+                                TraceDescriptor desc,
+                                WeakCallback callback) {
+  // Weak objects should have been cleared at this point. As a consequence, all
+  // objects found through weak references have to point to live objects at this
+  // point.
+  VerifyChild(object, desc.base_object_payload);
+}
+
+void MarkingVerifier::VisitBackingStoreStrongly(const char*,
+                                                void* object,
+                                                void**,
+                                                TraceDescriptor desc) {
+  if (!object)
+    return;
+
+  // Contents of backing stores are verified through page iteration. The
+  // verification here only makes sure that the backing itself is properly
+  // marked.
+  VerifyChild(object, desc.base_object_payload);
+}
+
+void MarkingVerifier::VisitBackingStoreWeakly(const char*,
+                                              void* object,
+                                              void**,
+                                              TraceDescriptor desc,
+                                              WeakCallback,
+                                              void*) {
+  if (!object)
+    return;
+
+  // Contents of weak backing stores are found themselves through page
+  // iteration and are treated strongly that way, similar to how they are
+  // treated strongly when found through stack scanning. The verification
+  // here only makes sure that the backing itself is properly marked. Weak
+  // backing stores found through
+  VerifyChild(object, desc.base_object_payload);
+}
+
+void MarkingVerifier::VerifyChild(void* object, void* base_object_payload) {
+  CHECK(object);
+  // Verification may check objects that are currently under construction and
+  // would require vtable access to figure out their headers. A nullptr in
+  // |base_object_payload| indicates that a mixin object is in construction
+  // and the vtable cannot be used to get to the object header.
+  const HeapObjectHeader* const child_header =
+      (base_object_payload) ? HeapObjectHeader::FromPayload(base_object_payload)
+                            : HeapObjectHeader::FromInnerAddress(object);
+  // These checks ensure that any children reachable from marked parents are
+  // also marked. If you hit these checks then marking is in an inconsistent
+  // state meaning that there are unmarked objects reachable from marked
+  // ones.
+  CHECK(child_header);
+  if (!child_header->IsMarked()) {
+    // Pre-finalizers may allocate. In that case the newly allocated objects
+    // reside on a page that is not scheduled for sweeping.
+    if (PageFromObject(child_header->Payload())->HasBeenSwept())
+      return;
+
+    LOG(FATAL) << "MarkingVerifier: Encountered unmarked object. " << std::endl
+               << std::endl
+               << "Hint (use v8_enable_raw_heap_snapshots for better naming): "
+               << std::endl
+               << parent_->Name() << std::endl
+               << "\\-> " << child_header->Name() << std::endl;
+  }
+}
+
+}  // namespace blink

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

@@ -5,8 +5,6 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_MARKING_VERIFIER_H_
 #define THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_MARKING_VERIFIER_H_
 
-#include "third_party/blink/renderer/platform/heap/garbage_collected.h"
-#include "third_party/blink/renderer/platform/heap/heap_page.h"
 #include "third_party/blink/renderer/platform/heap/visitor.h"
 
 namespace blink {
@@ -15,46 +13,29 @@ namespace blink {
 class MarkingVerifier final : public Visitor {
  public:
   explicit MarkingVerifier(ThreadState* state) : Visitor(state) {}
-  ~MarkingVerifier() override {}
+  ~MarkingVerifier() override = default;
 
-  void VerifyObject(HeapObjectHeader* header) {
-    // Verify only non-free marked objects.
-    if (header->IsFree() || !header->IsMarked())
-      return;
-
-    const GCInfo* info =
-        GCInfoTable::Get().GCInfoFromIndex(header->GcInfoIndex());
-    const bool can_verify =
-        !info->has_v_table || blink::VTableInitialized(header->Payload());
-    if (can_verify) {
-      CHECK(header->IsValid());
-      parent_ = header;
-      info->trace(this, header->Payload());
-    }
-  }
-
-  void Visit(void* object, TraceDescriptor desc) final {
-    VerifyChild(object, desc.base_object_payload);
-  }
+  void VerifyObject(HeapObjectHeader* header);
 
+  void Visit(void* object, TraceDescriptor desc) final;
   void VisitWeak(void* object,
                  void** object_slot,
                  TraceDescriptor desc,
-                 WeakCallback callback) final {
-    VerifyChild(object, desc.base_object_payload);
-  }
+                 WeakCallback callback) final;
 
-  // Unused overrides.
   void VisitBackingStoreStrongly(const char*,
                                  void*,
                                  void**,
-                                 TraceDescriptor) final {}
+                                 TraceDescriptor) final;
+
   void VisitBackingStoreWeakly(const char*,
                                void*,
                                void**,
                                TraceDescriptor,
                                WeakCallback,
-                               void*) final {}
+                               void*) final;
+
+  // Unused overrides.
   void VisitBackingStoreOnly(const char*, void*, void**) final {}
   void RegisterBackingStoreCallback(void**, MovingObjectCallback, void*) final {
   }
@@ -62,36 +43,7 @@ class MarkingVerifier final : public Visitor {
   void Visit(const TraceWrapperV8Reference<v8::Value>&) final {}
 
  private:
-  void VerifyChild(void* object, void* base_object_payload) {
-    CHECK(object);
-    // Verification may check objects that are currently under construction and
-    // would require vtable access to figure out their headers. A nullptr in
-    // |base_object_payload| indicates that a mixin object is in construction
-    // and the vtable cannot be used to get to the object header.
-    const HeapObjectHeader* const child_header =
-        (base_object_payload)
-            ? HeapObjectHeader::FromPayload(base_object_payload)
-            : HeapObjectHeader::FromInnerAddress(object);
-    // These checks ensure that any children reachable from marked parents are
-    // also marked. If you hit these checks then marking is in an inconsistent
-    // state meaning that there are unmarked objects reachable from marked
-    // ones.
-    CHECK(child_header);
-    if (!child_header->IsMarked()) {
-      // Pre-finalizers may allocate. In that case the newly allocated objects
-      // reside on a page that is not scheduled for sweeping.
-      if (PageFromObject(child_header->Payload())->HasBeenSwept())
-        return;
-
-      LOG(FATAL)
-          << "MarkingVerifier: Encountered unmarked object. " << std::endl
-          << std::endl
-          << "Hint (use v8_enable_raw_heap_snapshots for better naming): "
-          << std::endl
-          << parent_->Name() << std::endl
-          << "\\-> " << child_header->Name() << std::endl;
-    }
-  }
+  void VerifyChild(void* object, void* base_object_payload);
 
   HeapObjectHeader* parent_ = nullptr;
 };

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

@@ -18,17 +18,37 @@ class ResurrectingPreFinalizer
   USING_PRE_FINALIZER(ResurrectingPreFinalizer, Dispose);
 
  public:
-  enum TestType { kMember, kWeakMember };
+  enum TestType {
+    kMember,
+    kWeakMember,
+    kHeapVectorMember,
+    kHeapHashSetMember,
+    kHeapHashSetWeakMember
+  };
 
   class GlobalStorage : public GarbageCollected<GlobalStorage> {
    public:
+    GlobalStorage() {
+      // Reserve storage upfront to avoid allocations during pre-finalizer
+      // insertion.
+      vector_member.ReserveCapacity(32);
+      hash_set_member.ReserveCapacityForSize(32);
+      hash_set_weak_member.ReserveCapacityForSize(32);
+    }
+
     void Trace(Visitor* visitor) {
       visitor->Trace(strong);
       visitor->Trace(weak);
+      visitor->Trace(vector_member);
+      visitor->Trace(hash_set_member);
+      visitor->Trace(hash_set_weak_member);
     }
 
     Member<LinkedObject> strong;
     WeakMember<LinkedObject> weak;
+    HeapVector<Member<LinkedObject>> vector_member;
+    HeapHashSet<Member<LinkedObject>> hash_set_member;
+    HeapHashSet<WeakMember<LinkedObject>> hash_set_weak_member;
   };
 
   ResurrectingPreFinalizer(TestType test_type,
@@ -52,6 +72,15 @@ class ResurrectingPreFinalizer
       case TestType::kWeakMember:
         storage_->weak = object_that_dies_;
         break;
+      case TestType::kHeapVectorMember:
+        storage_->vector_member.push_back(object_that_dies_);
+        break;
+      case TestType::kHeapHashSetMember:
+        storage_->hash_set_member.insert(object_that_dies_);
+        break;
+      case TestType::kHeapHashSetWeakMember:
+        storage_->hash_set_weak_member.insert(object_that_dies_);
+        break;
     }
   }
 
@@ -88,4 +117,43 @@ TEST(MarkingVerifierDeathTest, DiesOnResurrectedWeakMember) {
                             "MarkingVerifier: Encountered unmarked object.");
 }
 
+TEST(MarkingVerifierDeathTest, DiesOnResurrectedHeapVectorMember) {
+  if (!ThreadState::Current()->VerifyMarkingEnabled())
+    return;
+
+  Persistent<ResurrectingPreFinalizer::GlobalStorage> storage(
+      MakeGarbageCollected<ResurrectingPreFinalizer::GlobalStorage>());
+  MakeGarbageCollected<ResurrectingPreFinalizer>(
+      ResurrectingPreFinalizer::kHeapVectorMember, storage.Get(),
+      MakeGarbageCollected<LinkedObject>());
+  ASSERT_DEATH_IF_SUPPORTED(PreciselyCollectGarbage(),
+                            "MarkingVerifier: Encountered unmarked object.");
+}
+
+TEST(MarkingVerifierDeathTest, DiesOnResurrectedHeapHashSetMember) {
+  if (!ThreadState::Current()->VerifyMarkingEnabled())
+    return;
+
+  Persistent<ResurrectingPreFinalizer::GlobalStorage> storage(
+      MakeGarbageCollected<ResurrectingPreFinalizer::GlobalStorage>());
+  MakeGarbageCollected<ResurrectingPreFinalizer>(
+      ResurrectingPreFinalizer::kHeapHashSetMember, storage.Get(),
+      MakeGarbageCollected<LinkedObject>());
+  ASSERT_DEATH_IF_SUPPORTED(PreciselyCollectGarbage(),
+                            "MarkingVerifier: Encountered unmarked object.");
+}
+
+TEST(MarkingVerifierDeathTest, DiesOnResurrectedHeapHashSetWeakMember) {
+  if (!ThreadState::Current()->VerifyMarkingEnabled())
+    return;
+
+  Persistent<ResurrectingPreFinalizer::GlobalStorage> storage(
+      MakeGarbageCollected<ResurrectingPreFinalizer::GlobalStorage>());
+  MakeGarbageCollected<ResurrectingPreFinalizer>(
+      ResurrectingPreFinalizer::kHeapHashSetWeakMember, storage.Get(),
+      MakeGarbageCollected<LinkedObject>());
+  ASSERT_DEATH_IF_SUPPORTED(PreciselyCollectGarbage(),
+                            "MarkingVerifier: Encountered unmarked object.");
+}
+
 }  // namespace blink