[static-roots] Remove unused Isolate::InitWithReadOnlySnapshot

In the end we managed to have static root builds without a two stage
isolate setup. Thus, the mode for creating isolates with an existing
read only page is unused. Also, no other usecase for this mode emerged.

Bug: v8:13598
Bug: v8:13466
Change-Id: I0a8174ba9383db7364b6e4545702aafc6f48170c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4178814
Commit-Queue: Olivier Flückiger <olivf@chromium.org>
Reviewed-by: Jakob Linke <jgruber@chromium.org>
Auto-Submit: Olivier Flückiger <olivf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#85396}

src/execution/isolate.cc

@@ -4038,13 +4038,6 @@ bool Isolate::InitWithoutSnapshot() {
   return Init(nullptr, nullptr, nullptr, false);
 }
 
-bool Isolate::InitWithReadOnlySnapshot(SnapshotData* read_only_snapshot_data) {
-  DCHECK_NOT_NULL(read_only_snapshot_data);
-  // Without external code space builtin code objects are allocated in ro space.
-  DCHECK(V8_EXTERNAL_CODE_SPACE_BOOL);
-  return Init(nullptr, read_only_snapshot_data, nullptr, false);
-}
-
 bool Isolate::InitWithSnapshot(SnapshotData* startup_snapshot_data,
                                SnapshotData* read_only_snapshot_data,
                                SnapshotData* shared_heap_snapshot_data,
@@ -4198,12 +4191,9 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
 #endif  // V8_COMPRESS_POINTERS_IN_SHARED_CAGE
 
   const bool create_heap_objects = (shared_heap_snapshot_data == nullptr);
-  const bool setup_up_existing_read_only_roots =
-      create_heap_objects && (read_only_snapshot_data != nullptr);
   // We either have both or none.
   DCHECK_EQ(create_heap_objects, startup_snapshot_data == nullptr);
-  DCHECK_IMPLIES(setup_up_existing_read_only_roots,
+  DCHECK_EQ(create_heap_objects, read_only_snapshot_data == nullptr);
-                 startup_snapshot_data == nullptr);
 
   // Code space setup requires the permissions to be set to default state.
   RwxMemoryWriteScope::SetDefaultPermissionsForNewThread();
@@ -4356,28 +4346,6 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
     string_forwarding_table_ = shared_heap_isolate()->string_forwarding_table_;
   }
 
-  // If we create an isolate from scratch, but based on existing read only
-  // roots, then we need to populate the string cache with its strings.
-  if (setup_up_existing_read_only_roots) {
-    HandleScope scope(this);
-    ReadOnlyHeapObjectIterator iterator(read_only_heap());
-    for (HeapObject object = iterator.Next(); !object.is_null();
-         object = iterator.Next()) {
-      if (object.IsInternalizedString()) {
-        auto s = String::cast(object);
-        Handle<String> str(s, this);
-        StringTableInsertionKey key(
-            this, str, DeserializingUserCodeOption::kNotDeserializingUserCode);
-        auto result = string_table()->LookupKey(this, &key);
-        // Since this is startup, there should be no duplicate entries in the
-        // string table, and the lookup should unconditionally add the given
-        // string.
-        DCHECK_EQ(*result, *str);
-        USE(result);
-      }
-    }
-  }
-
   if (V8_SHORT_BUILTIN_CALLS_BOOL && v8_flags.short_builtin_calls) {
 #if defined(V8_OS_ANDROID)
     // On Android, the check is not operative to detect memory, and re-embedded
@@ -4522,9 +4490,7 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
     CodePageCollectionMemoryModificationScope modification_scope(heap());
 
     if (create_heap_objects) {
-      if (!setup_up_existing_read_only_roots) {
+      read_only_heap_->OnCreateHeapObjectsComplete(this);
-        read_only_heap_->OnCreateHeapObjectsComplete(this);
-      }
     } else {
       SharedHeapDeserializer shared_heap_deserializer(
           this, shared_heap_snapshot_data, can_rehash);

src/execution/isolate.h

@@ -662,7 +662,6 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
   bool InitializeCounters();  // Returns false if already initialized.
 
   bool InitWithoutSnapshot();
-  bool InitWithReadOnlySnapshot(SnapshotData* read_only_snapshot_data);
   bool InitWithSnapshot(SnapshotData* startup_snapshot_data,
                         SnapshotData* read_only_snapshot_data,
                         SnapshotData* shared_heap_snapshot_data,

test/cctest/cctest.status

@@ -100,9 +100,6 @@
   'test-strings/StringOOM*': [PASS, ['mode == debug', SKIP]],
   'test-serialize/CustomSnapshotDataBlobImmortalImmovableRoots': [PASS, ['mode == debug', SKIP]],
 
-  # Slow, and fails flakily on Mac debug: https://crbug.com/v8/13598.
-  'test-serialize/CreateIsolateFromReadOnlySnapshot': [PASS, SLOW, ['system == macos and mode == debug', SKIP]],
-
   # Tests that need to run sequentially (e.g. due to memory consumption).
   'test-accessors/HandleScopePop': [PASS, HEAVY],
   'test-api/FastApiCalls': [PASS, HEAVY],
@@ -305,8 +302,6 @@
   'test-serialize/StartupSerializerTwiceRunScript': [SKIP],
   'test-serialize/StartupSerializerTwice': [SKIP],
 
-  # Test too slow with ASan
-  'test-serialize/CreateIsolateFromReadOnlySnapshot': [SKIP],
 }],  # 'system == windows and arch == x64 and mode == debug'
 
 ##############################################################################

test/cctest/test-serialize.cc

@@ -125,17 +125,6 @@ class TestSerializer {
     return v8_isolate;
   }
 
-  static v8::Isolate* NewIsolateFromReadOnlySnapshot(
-      SnapshotData* read_only_snapshot) {
-    const bool kEnableSerializer = false;
-    const bool kIsShared = false;
-    v8::Isolate* v8_isolate = NewIsolate(kEnableSerializer, kIsShared);
-    v8::Isolate::Scope isolate_scope(v8_isolate);
-    i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
-    isolate->InitWithReadOnlySnapshot(read_only_snapshot);
-    return v8_isolate;
-  }
-
   static void InitializeProcessWideSharedIsolateFromBlob(
       const StartupBlobs& blobs) {
     base::MutexGuard guard(
@@ -5309,52 +5298,5 @@ UNINITIALIZED_TEST(BreakPointAccessorContextSnapshot) {
   FreeCurrentEmbeddedBlob();
 }
 
-#if V8_EXTERNAL_CODE_SPACE_BOOL
-UNINITIALIZED_TEST(CreateIsolateFromReadOnlySnapshot) {
-  auto working_builtins = [](v8::Isolate* isolate) {
-    v8::Isolate::Scope i_scope(isolate);
-    v8::HandleScope h_scope(isolate);
-    v8::Local<v8::Context> context = v8::Context::New(isolate);
-    v8::Context::Scope c_scope(context);
-    v8::Maybe<int32_t> result = CompileRun("(function(x){return +x+40})(\"2\")")
-                                    ->Int32Value(isolate->GetCurrentContext());
-    CHECK_EQ(42, result.FromJust());
-  };
-
-  v8::Isolate* isolate1 = TestSerializer::NewIsolateInitialized();
-  StartupBlobs blobs1 = Serialize(isolate1);
-  auto ro1 = SnapshotData(blobs1.read_only);
-  isolate1->Dispose();
-
-  v8::Isolate* isolate2 = TestSerializer::NewIsolateFromReadOnlySnapshot(&ro1);
-  auto blobs2 = Serialize(isolate2);
-  auto ro2 = SnapshotData(blobs2.read_only);
-  isolate2->Dispose();
-
-  v8::Isolate* isolate3 = TestSerializer::NewIsolateFromReadOnlySnapshot(&ro2);
-  auto blobs3 = Serialize(isolate3);
-  auto ro3 = SnapshotData(blobs3.read_only);
-  isolate3->Dispose();
-
-  v8::Isolate* isolate4 = TestSerializer::NewIsolateFromBlob(blobs2);
-  working_builtins(isolate4);
-  isolate4->Dispose();
-  v8::Isolate* isolate5 = TestSerializer::NewIsolateFromBlob(blobs3);
-  working_builtins(isolate5);
-  isolate5->Dispose();
-
-  // The first snapshot is not stable since the serializer does not preserve the
-  // order of objects in the read only heap.
-  CHECK_EQ(blobs2.startup, blobs3.startup);
-  CHECK_EQ(blobs2.shared_space, blobs3.shared_space);
-  CHECK_EQ(blobs2.read_only, blobs3.read_only);
-
-  blobs1.Dispose();
-  blobs2.Dispose();
-  blobs3.Dispose();
-  FreeCurrentEmbeddedBlob();
-}
-#endif  // V8_EXTERNAL_CODE_SPACE_BOOL
-
 }  // namespace internal
 }  // namespace v8