[heap] Skip read-only space in Heap::Contains

Bug: v8:7464
Change-Id: I27e82cdf0f8cc56ff68dcfaecab9644fe74916c7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1559861
Commit-Queue: Maciej Goszczycki <goszczycki@google.com>
Reviewed-by: Dan Elphick <delphick@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61350}

src/heap/combined-heap.h

@@ -30,6 +30,10 @@ class V8_EXPORT_PRIVATE CombinedHeapIterator final {
   ReadOnlyHeapIterator ro_heap_iterator_;
 };
 
+inline bool IsValidHeapObject(Heap* heap, HeapObject object) {
+  return ReadOnlyHeap::Contains(object) || heap->Contains(object);
+}
+
 }  // namespace internal
 }  // namespace v8
 

src/heap/heap.cc

@@ -24,6 +24,7 @@
 #include "src/heap/array-buffer-tracker-inl.h"
 #include "src/heap/barrier.h"
 #include "src/heap/code-stats.h"
+#include "src/heap/combined-heap.h"
 #include "src/heap/concurrent-marking.h"
 #include "src/heap/embedder-tracing.h"
 #include "src/heap/gc-idle-time-handler.h"
@@ -3601,13 +3602,6 @@ const char* Heap::GarbageCollectionReasonToString(
 }
 
 bool Heap::Contains(HeapObject value) {
-  // Check RO_SPACE first because IsOutsideAllocatedSpace cannot account for a
-  // shared RO_SPACE.
-  // TODO(v8:7464): Exclude read-only space. Use ReadOnlyHeap::Contains where
-  // appropriate.
-  if (read_only_space_ != nullptr && read_only_space_->Contains(value)) {
-    return true;
-  }
   if (memory_allocator()->IsOutsideAllocatedSpace(value->address())) {
     return false;
   }
@@ -5669,7 +5663,7 @@ void VerifyPointersVisitor::VisitRootPointers(Root root,
 }
 
 void VerifyPointersVisitor::VerifyHeapObjectImpl(HeapObject heap_object) {
-  CHECK(heap_->Contains(heap_object));
+  CHECK(IsValidHeapObject(heap_, heap_object));
   CHECK(heap_object->map()->IsMap());
 }
 

src/heap/spaces.cc

@@ -12,6 +12,7 @@
 #include "src/base/template-utils.h"
 #include "src/counters.h"
 #include "src/heap/array-buffer-tracker.h"
+#include "src/heap/combined-heap.h"
 #include "src/heap/concurrent-marking.h"
 #include "src/heap/gc-tracer.h"
 #include "src/heap/heap-controller.h"
@@ -3734,7 +3735,7 @@ void LargeObjectSpace::Verify(Isolate* isolate) {
         Object element = array->get(j);
         if (element->IsHeapObject()) {
           HeapObject element_object = HeapObject::cast(element);
-          CHECK(heap()->Contains(element_object));
+          CHECK(IsValidHeapObject(heap(), element_object));
           CHECK(element_object->map()->IsMap());
         }
       }

src/objects-debug.cc

@@ -12,6 +12,7 @@
 #include "src/disassembler.h"
 #include "src/elements.h"
 #include "src/field-type.h"
+#include "src/heap/combined-heap.h"
 #include "src/heap/heap-write-barrier-inl.h"
 #include "src/ic/handler-configuration-inl.h"
 #include "src/layout-descriptor.h"
@@ -475,8 +476,7 @@ void HeapObject::HeapObjectVerify(Isolate* isolate) {
 // static
 void HeapObject::VerifyHeapPointer(Isolate* isolate, Object p) {
   CHECK(p->IsHeapObject());
-  HeapObject ho = HeapObject::cast(p);
-  CHECK(isolate->heap()->Contains(ho));
+  CHECK(IsValidHeapObject(isolate->heap(), HeapObject::cast(p)));
 }
 
 void Symbol::SymbolVerify(Isolate* isolate) {

src/objects/string.cc

@@ -8,6 +8,7 @@
 #include "src/conversions.h"
 #include "src/handles-inl.h"
 #include "src/heap/heap-inl.h"  // For LooksValid implementation.
+#include "src/heap/read-only-heap.h"
 #include "src/objects/map.h"
 #include "src/objects/oddball.h"
 #include "src/objects/string-comparator.h"
@@ -396,7 +397,7 @@ bool String::LooksValid() {
   // basically the same logic as the way we access the heap in the first place.
   MemoryChunk* chunk = MemoryChunk::FromHeapObject(*this);
   // RO_SPACE objects should always be valid.
-  if (chunk->owner()->identity() == RO_SPACE) return true;
+  if (ReadOnlyHeap::Contains(*this)) return true;
   if (chunk->heap() == nullptr) return false;
   return chunk->heap()->Contains(*this);
 }

src/snapshot/partial-serializer.cc

@@ -6,6 +6,7 @@
 #include "src/snapshot/startup-serializer.h"
 
 #include "src/api-inl.h"
+#include "src/heap/combined-heap.h"
 #include "src/math-random.h"
 #include "src/microtask-queue.h"
 #include "src/objects-inl.h"
@@ -171,7 +172,7 @@ bool PartialSerializer::SerializeJSObjectWithEmbedderFields(Object obj) {
     original_embedder_values.emplace_back(embedder_data_slot.load_raw(no_gc));
     Object object = embedder_data_slot.load_tagged();
     if (object->IsHeapObject()) {
-      DCHECK(isolate()->heap()->Contains(HeapObject::cast(object)));
+      DCHECK(IsValidHeapObject(isolate()->heap(), HeapObject::cast(object)));
       serialized_data.push_back({nullptr, 0});
     } else {
       // If no serializer is provided and the field was empty, we serialize it

test/cctest/heap/test-heap.cc

@@ -38,6 +38,7 @@
 #include "src/field-type.h"
 #include "src/global-handles.h"
 #include "src/hash-seed-inl.h"
+#include "src/heap/combined-heap.h"
 #include "src/heap/factory.h"
 #include "src/heap/gc-tracer.h"
 #include "src/heap/heap-inl.h"
@@ -74,9 +75,7 @@ static const int kPretenureCreationCount =
 
 static void CheckMap(Map map, int type, int instance_size) {
   CHECK(map->IsHeapObject());
-#ifdef DEBUG
-  CHECK(CcTest::heap()->Contains(map));
-#endif
+  DCHECK(IsValidHeapObject(CcTest::heap(), map));
   CHECK_EQ(ReadOnlyRoots(CcTest::heap()).meta_map(), map->map());
   CHECK_EQ(type, map->instance_type());
   CHECK_EQ(instance_size, map->instance_size());

test/cctest/heap/test-iterators.cc

@@ -51,7 +51,6 @@ Object CreateWritableObject() {
 }
 }  // namespace
 
-// TODO(v8:7464): Add more CHECKs once Contains doesn't include read-only space.
 TEST(ReadOnlyHeapIterator) {
   CcTest::InitializeVM();
   HandleScope handle_scope(CcTest::i_isolate());
@@ -61,6 +60,7 @@ TEST(ReadOnlyHeapIterator) {
   for (HeapObject obj = iterator.next(); !obj.is_null();
        obj = iterator.next()) {
     CHECK(ReadOnlyHeap::Contains(obj));
+    CHECK(!CcTest::heap()->Contains(obj));
     CHECK_NE(sample_object, obj);
   }
 }
@@ -75,6 +75,7 @@ TEST(HeapIterator) {
   for (HeapObject obj = iterator.next(); !obj.is_null();
        obj = iterator.next()) {
     CHECK(!ReadOnlyHeap::Contains(obj));
+    CHECK(CcTest::heap()->Contains(obj));
     if (sample_object == obj) seen_sample_object = true;
   }
   CHECK(seen_sample_object);
@@ -89,7 +90,7 @@ TEST(CombinedHeapIterator) {
 
   for (HeapObject obj = iterator.next(); !obj.is_null();
        obj = iterator.next()) {
-    CHECK(CcTest::heap()->Contains(obj));
+    CHECK(IsValidHeapObject(CcTest::heap(), obj));
     if (sample_object == obj) seen_sample_object = true;
   }
   CHECK(seen_sample_object);