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

This reverts commit 2b24cd03.

Reason for revert: Causes layout test failures
https://ci.chromium.org/p/chromium/builders/try/linux-chromeos-rel/275121
and https://ci.chromium.org/p/chromium/builders/try/win7-rel/86354

Original change's description:
> [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}

TBR=ulan@chromium.org,delphick@chromium.org,goszczycki@google.com

Change-Id: I13cc09dd44a10bad854fa861b6e43149babb1b5e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:7464
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1601498Reviewed-by: Maya Lekova <mslekova@chromium.org>
Commit-Queue: Maya Lekova <mslekova@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61363}

src/heap/combined-heap.h

@@ -30,10 +30,6 @@ 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,7 +24,6 @@
 #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"
@@ -3600,6 +3599,13 @@ 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;
   }
@@ -5661,7 +5667,7 @@ void VerifyPointersVisitor::VisitRootPointers(Root root,
 }
 
 void VerifyPointersVisitor::VerifyHeapObjectImpl(HeapObject heap_object) {
-  CHECK(IsValidHeapObject(heap_, heap_object));
+  CHECK(heap_->Contains(heap_object));
   CHECK(heap_object->map()->IsMap());
 }
 

src/heap/spaces.cc

@@ -12,7 +12,6 @@
 #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"
@@ -3739,7 +3738,7 @@ void LargeObjectSpace::Verify(Isolate* isolate) {
         Object element = array->get(j);
         if (element->IsHeapObject()) {
           HeapObject element_object = HeapObject::cast(element);
-          CHECK(IsValidHeapObject(heap(), element_object));
+          CHECK(heap()->Contains(element_object));
           CHECK(element_object->map()->IsMap());
         }
       }

src/objects-debug.cc

@@ -12,7 +12,6 @@
 #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"
@@ -476,7 +475,8 @@ void HeapObject::HeapObjectVerify(Isolate* isolate) {
 // static
 void HeapObject::VerifyHeapPointer(Isolate* isolate, Object p) {
   CHECK(p->IsHeapObject());
-  CHECK(IsValidHeapObject(isolate->heap(), HeapObject::cast(p)));
+  HeapObject ho = HeapObject::cast(p);
+  CHECK(isolate->heap()->Contains(ho));
 }
 
 void Symbol::SymbolVerify(Isolate* isolate) {

src/objects/string.cc

@@ -8,7 +8,6 @@
 #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"
@@ -397,7 +396,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 (ReadOnlyHeap::Contains(*this)) return true;
+  if (chunk->owner()->identity() == RO_SPACE) return true;
   if (chunk->heap() == nullptr) return false;
   return chunk->heap()->Contains(*this);
 }

src/snapshot/partial-serializer.cc

@@ -6,7 +6,6 @@
 #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"
@@ -172,7 +171,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(IsValidHeapObject(isolate()->heap(), HeapObject::cast(object)));
+      DCHECK(isolate()->heap()->Contains(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,7 +38,6 @@
 #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"
@@ -75,7 +74,9 @@ static const int kPretenureCreationCount =
 
 static void CheckMap(Map map, int type, int instance_size) {
   CHECK(map->IsHeapObject());
-  DCHECK(IsValidHeapObject(CcTest::heap(), map));
+#ifdef DEBUG
+  CHECK(CcTest::heap()->Contains(map));
+#endif
   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,6 +51,7 @@ 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());
@@ -60,7 +61,6 @@ 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,7 +75,6 @@ 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);
@@ -90,7 +89,7 @@ TEST(CombinedHeapIterator) {
 
   for (HeapObject obj = iterator.next(); !obj.is_null();
        obj = iterator.next()) {
-    CHECK(IsValidHeapObject(CcTest::heap(), obj));
+    CHECK(CcTest::heap()->Contains(obj));
     if (sample_object == obj) seen_sample_object = true;
   }
   CHECK(seen_sample_object);