[cleanup] Avoid accessing MemoryChunk directly to get Isolate

Adds an Isolate::FromWritableHeapObject method, with a bool return value
and Isolate* out parameter, and replace most accesses to Isolate via
MemoryChunk (which handle objectsin ROSpace rather than just failing) to
use that instead.

Bug: v8:7754
Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
Change-Id: Idb472a3d6037deed92e6fa8c8a7a1a14293e2462
Reviewed-on: https://chromium-review.googlesource.com/1144933
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54579}

src/api.cc

@@ -219,6 +219,8 @@ Local<Context> ContextFromNeverReadOnlySpaceObject(
 // it are removed.
 // DO NOT USE THIS IN NEW CODE!
 i::Isolate* UnsafeIsolateFromHeapObject(i::Handle<i::HeapObject> obj) {
+  // Use MemoryChunk directly instead of Isolate::FromWritableHeapObject to
+  // temporarily allow isolate access from read-only space objects.
   i::MemoryChunk* chunk = i::MemoryChunk::FromHeapObject(*obj);
   return chunk->heap()->isolate();
 }
@@ -227,6 +229,8 @@ i::Isolate* UnsafeIsolateFromHeapObject(i::Handle<i::HeapObject> obj) {
 // it are removed.
 // DO NOT USE THIS IN NEW CODE!
 Local<Context> UnsafeContextFromHeapObject(i::Handle<i::Object> obj) {
+  // Use MemoryChunk directly instead of Isolate::FromWritableHeapObject to
+  // temporarily allow isolate access from read-only space objects.
   i::MemoryChunk* chunk =
       i::MemoryChunk::FromHeapObject(i::HeapObject::cast(*obj));
   return reinterpret_cast<Isolate*>(chunk->heap()->isolate())
@@ -5973,17 +5977,18 @@ v8::String::GetExternalOneByteStringResource() const {
 Local<Value> Symbol::Name() const {
   i::Handle<i::Symbol> sym = Utils::OpenHandle(this);
 
-  i::MemoryChunk* chunk = i::MemoryChunk::FromHeapObject(*sym);
-  // If the Symbol is in RO_SPACE, then its name must be too. Since RO_SPACE
-  // objects are immovable we can use the Handle(T**) constructor with the
-  // address of the name field in the Symbol object without needing an isolate.
-  if (chunk->owner()->identity() == i::RO_SPACE) {
+  i::Isolate* isolate;
+  if (!i::Isolate::FromWritableHeapObject(*sym, &isolate)) {
+    // If the Symbol is in RO_SPACE, then its name must be too. Since RO_SPACE
+    // objects are immovable we can use the Handle(T**) constructor with the
+    // address of the name field in the Symbol object without needing an
+    // isolate.
     i::Handle<i::HeapObject> ro_name(reinterpret_cast<i::HeapObject**>(
         sym->GetFieldAddress(i::Symbol::kNameOffset)));
     return Utils::ToLocal(ro_name);
   }
 
-  i::Handle<i::Object> name(sym->name(), chunk->heap()->isolate());
+  i::Handle<i::Object> name(sym->name(), isolate);
 
   return Utils::ToLocal(name);
 }
@@ -6852,13 +6857,13 @@ Local<String> v8::String::NewExternal(
 
 bool v8::String::MakeExternal(v8::String::ExternalStringResource* resource) {
   i::Handle<i::String> obj = Utils::OpenHandle(this);
-  // RO_SPACE strings cannot be externalized.
-  i::MemoryChunk* chunk = i::MemoryChunk::FromHeapObject(*obj);
-  if (chunk->owner()->identity() == i::RO_SPACE) {
+
+  i::Isolate* isolate;
+  if (!i::Isolate::FromWritableHeapObject(*obj, &isolate)) {
+    // RO_SPACE strings cannot be externalized.
     return false;
   }
 
-  i::Isolate* isolate = chunk->heap()->isolate();
   if (i::StringShape(*obj).IsExternal()) {
     return false;  // Already an external string.
   }
@@ -6882,13 +6887,12 @@ bool v8::String::MakeExternal(
     v8::String::ExternalOneByteStringResource* resource) {
   i::Handle<i::String> obj = Utils::OpenHandle(this);
 
-  // RO_SPACE strings cannot be externalized.
-  i::MemoryChunk* chunk = i::MemoryChunk::FromHeapObject(*obj);
-  if (chunk->owner()->identity() == i::RO_SPACE) {
+  i::Isolate* isolate;
+  if (!i::Isolate::FromWritableHeapObject(*obj, &isolate)) {
+    // RO_SPACE strings cannot be externalized.
     return false;
   }
 
-  i::Isolate* isolate = chunk->heap()->isolate();
   if (i::StringShape(*obj).IsExternal()) {
     return false;  // Already an external string.
   }
@@ -6912,10 +6916,13 @@ bool v8::String::CanMakeExternal() {
   i::Handle<i::String> obj = Utils::OpenHandle(this);
   if (obj->IsExternalString()) return false;
 
+  i::Isolate* isolate;
+  if (!i::Isolate::FromWritableHeapObject(*obj, &isolate)) {
+    // RO_SPACE strings cannot be externalized.
+    return false;
+  }
   // Only old space strings should be externalized.
-  i::MemoryChunk* chunk = i::MemoryChunk::FromHeapObject(*obj);
-  i::AllocationSpace space = chunk->owner()->identity();
-  return space != i::NEW_SPACE && space != i::RO_SPACE;
+  return !i::Heap::InNewSpace(*obj);
 }
 
 

src/handles.cc

@@ -26,9 +26,9 @@ bool HandleBase::IsDereferenceAllowed(DereferenceCheckMode mode) const {
   Object* object = *location_;
   if (object->IsSmi()) return true;
   HeapObject* heap_object = HeapObject::cast(object);
-  MemoryChunk* chunk = MemoryChunk::FromHeapObject(heap_object);
-  if (chunk->owner()->identity() == RO_SPACE) return true;
-  Heap* heap = chunk->heap();
+  Isolate* isolate;
+  if (!Isolate::FromWritableHeapObject(heap_object, &isolate)) return true;
+  Heap* heap = isolate->heap();
   Object** roots_array_start = heap->roots_array_start();
   if (roots_array_start <= location_ &&
       location_ < roots_array_start + Heap::kStrongRootListLength &&
@@ -43,7 +43,7 @@ bool HandleBase::IsDereferenceAllowed(DereferenceCheckMode mode) const {
     if (heap_object->IsCell()) return true;
     if (heap_object->IsMap()) return true;
     if (heap_object->IsInternalizedString()) return true;
-    return !heap->isolate()->IsDeferredHandle(location_);
+    return !isolate->IsDeferredHandle(location_);
   }
   return true;
 }

src/isolate-inl.h

@@ -11,6 +11,15 @@
 namespace v8 {
 namespace internal {
 
+bool Isolate::FromWritableHeapObject(HeapObject* obj, Isolate** isolate) {
+  i::MemoryChunk* chunk = i::MemoryChunk::FromHeapObject(obj);
+  if (chunk->owner()->identity() == i::RO_SPACE) {
+    *isolate = nullptr;
+    return false;
+  }
+  *isolate = chunk->heap()->isolate();
+  return true;
+}
 
 void Isolate::set_context(Context* context) {
   DCHECK(context == nullptr || context->IsContext());

src/isolate.h

@@ -556,6 +556,11 @@ class Isolate : private HiddenFactory {
     return isolate;
   }
 
+  // Get the isolate that the given HeapObject lives in, returning true on
+  // success. If the object is not writable (i.e. lives in read-only space),
+  // return false.
+  inline static bool FromWritableHeapObject(HeapObject* obj, Isolate** isolate);
+
   // Usually called by Init(), but can be called early e.g. to allow
   // testing components that require logging but not the whole
   // isolate.

src/objects-printer.cc

@@ -803,12 +803,12 @@ void Map::MapPrint(std::ostream& os) {  // NOLINT
     layout_descriptor()->ShortPrint(os);
   }
 
-  MemoryChunk* chunk = MemoryChunk::FromHeapObject(this);
+  Isolate* isolate;
   // Read-only maps can't have transitions, which is fortunate because we need
   // the isolate to iterate over the transitions.
-  if (chunk->owner()->identity() != RO_SPACE) {
+  if (Isolate::FromWritableHeapObject(this, &isolate)) {
     DisallowHeapAllocation no_gc;
-    TransitionsAccessor transitions(chunk->heap()->isolate(), this, &no_gc);
+    TransitionsAccessor transitions(isolate, this, &no_gc);
     int nof_transitions = transitions.NumberOfTransitions();
     if (nof_transitions > 0) {
       os << "\n - transitions #" << nof_transitions << ": ";

src/objects.cc

@@ -2588,11 +2588,11 @@ bool String::MakeExternal(v8::String::ExternalStringResource* resource) {
   int size = this->Size();  // Byte size of the original string.
   // Abort if size does not allow in-place conversion.
   if (size < ExternalString::kShortSize) return false;
-  MemoryChunk* chunk = MemoryChunk::FromHeapObject(this);
+  Isolate* isolate;
   // Read-only strings cannot be made external, since that would mutate the
   // string.
-  if (chunk->owner()->identity() == RO_SPACE) return false;
-  Heap* heap = chunk->heap();
+  if (!Isolate::FromWritableHeapObject(this, &isolate)) return false;
+  Heap* heap = isolate->heap();
   bool is_one_byte = this->IsOneByteRepresentation();
   bool is_internalized = this->IsInternalizedString();
   bool has_pointers = StringShape(this).IsIndirect();
@@ -2673,11 +2673,11 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
   int size = this->Size();  // Byte size of the original string.
   // Abort if size does not allow in-place conversion.
   if (size < ExternalString::kShortSize) return false;
-  MemoryChunk* chunk = MemoryChunk::FromHeapObject(this);
+  Isolate* isolate;
   // Read-only strings cannot be made external, since that would mutate the
   // string.
-  if (chunk->owner()->identity() == RO_SPACE) return false;
-  Heap* heap = chunk->heap();
+  if (!Isolate::FromWritableHeapObject(this, &isolate)) return false;
+  Heap* heap = isolate->heap();
   bool is_internalized = this->IsInternalizedString();
   bool has_pointers = StringShape(this).IsIndirect();