[heap] Do not allocate external strings in shared heap

ExternalStrings in the shared heap currently conflicts with the sandbox
project. We would need concurrent concurrent allocation in the external
pointer table but also require different accessors for them.

Since the shared string table doesn't really need ExternalStrings in
the shared heap for now, simply keep ExternalStrings in the client
heaps.

Bug: v8:11708, v8:12617
Change-Id: I272e40eaec4b7f368ce44f42f7f69bf27d53f9c7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3451717Reviewed-by: Shu-yu Guo <syg@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79039}

src/heap/factory-base.cc

@@ -1073,8 +1073,7 @@ FactoryBase<Impl>::RefineAllocationTypeForInPlaceInternalizableString(
 #ifdef DEBUG
   InstanceType instance_type = string_map.instance_type();
   DCHECK(InstanceTypeChecker::IsInternalizedString(instance_type) ||
-         String::IsInPlaceInternalizable(instance_type) ||
-         InstanceTypeChecker::IsExternalString(instance_type));
+         String::IsInPlaceInternalizable(instance_type));
 #endif
   if (FLAG_single_generation && allocation == AllocationType::kYoung) {
     allocation = AllocationType::kOld;

src/heap/factory.cc

@@ -1056,20 +1056,15 @@ MaybeHandle<String> Factory::NewExternalStringFromOneByte(
   Handle<Map> map = resource->IsCacheable()
                         ? external_one_byte_string_map()
                         : uncached_external_one_byte_string_map();
-  AllocationType type = RefineAllocationTypeForInPlaceInternalizableString(
-      AllocationType::kOld, *map);
   ExternalOneByteString external_string =
-      ExternalOneByteString::cast(New(map, type));
+      ExternalOneByteString::cast(New(map, AllocationType::kOld));
   DisallowGarbageCollection no_gc;
   external_string.AllocateExternalPointerEntries(isolate());
   external_string.set_length(static_cast<int>(length));
   external_string.set_raw_hash_field(String::kEmptyHashField);
   external_string.SetResource(isolate(), resource);
 
-  Heap* owner_heap = isolate()->OwnsStringTable()
-                         ? isolate()->heap()
-                         : isolate()->shared_isolate()->heap();
-  owner_heap->RegisterExternalString(external_string);
+  isolate()->heap()->RegisterExternalString(external_string);
 
   return Handle<String>(external_string, isolate());
 }
@@ -1084,19 +1079,15 @@ MaybeHandle<String> Factory::NewExternalStringFromTwoByte(
 
   Handle<Map> map = resource->IsCacheable() ? external_string_map()
                                             : uncached_external_string_map();
-  AllocationType type = RefineAllocationTypeForInPlaceInternalizableString(
-      AllocationType::kOld, *map);
-  ExternalTwoByteString string = ExternalTwoByteString::cast(New(map, type));
+  ExternalTwoByteString string =
+      ExternalTwoByteString::cast(New(map, AllocationType::kOld));
   DisallowGarbageCollection no_gc;
   string.AllocateExternalPointerEntries(isolate());
   string.set_length(static_cast<int>(length));
   string.set_raw_hash_field(String::kEmptyHashField);
   string.SetResource(isolate(), resource);
 
-  Heap* owner_heap = isolate()->OwnsStringTable()
-                         ? isolate()->heap()
-                         : isolate()->shared_isolate()->heap();
-  owner_heap->RegisterExternalString(string);
+  isolate()->heap()->RegisterExternalString(string);
 
   return Handle<ExternalTwoByteString>(string, isolate());
 }

src/heap/heap.cc

@@ -4446,6 +4446,7 @@ bool Heap::ShouldBeInSharedOldSpace(HeapObject value) {
   if (isolate()->OwnsStringTable()) return false;
   if (ReadOnlyHeap::Contains(value)) return false;
   if (Heap::InYoungGeneration(value)) return false;
+  if (value.IsExternalString()) return false;
   if (value.IsString()) {
     return value.IsInternalizedString() ||
            String::IsInPlaceInternalizable(String::cast(value));

src/heap/mark-compact.cc

@@ -51,6 +51,7 @@
 #include "src/objects/embedder-data-array-inl.h"
 #include "src/objects/foreign.h"
 #include "src/objects/hash-table-inl.h"
+#include "src/objects/instance-type.h"
 #include "src/objects/js-array-buffer-inl.h"
 #include "src/objects/js-objects-inl.h"
 #include "src/objects/maybe-object.h"
@@ -1631,7 +1632,8 @@ class EvacuateVisitorBase : public HeapObjectVisitor {
 
   inline bool ShouldPromoteIntoSharedHeap(Map map) {
     if (shared_string_table_) {
-      return String::IsInPlaceInternalizable(map.instance_type());
+      return String::IsInPlaceInternalizableExcludingExternal(
+          map.instance_type());
     }
     return false;
   }
@@ -2445,12 +2447,12 @@ void MarkCompactCollector::ClearNonLiveReferences() {
     string_table->DropOldData();
     string_table->IterateElements(&internalized_visitor);
     string_table->NotifyElementsRemoved(internalized_visitor.PointersRemoved());
-
-    ExternalStringTableCleaner external_visitor(heap());
-    heap()->external_string_table_.IterateAll(&external_visitor);
-    heap()->external_string_table_.CleanUpAll();
   }
 
+  ExternalStringTableCleaner external_visitor(heap());
+  heap()->external_string_table_.IterateAll(&external_visitor);
+  heap()->external_string_table_.CleanUpAll();
+
   {
     TRACE_GC(heap()->tracer(), GCTracer::Scope::MC_CLEAR_FLUSHABLE_BYTECODE);
     // ProcessFlusheBaselineCandidates should be called after clearing bytecode
@@ -3386,7 +3388,8 @@ class ClientHeapVerifier final : public ObjectVisitorWithCageBases {
 
 static String UpdateReferenceInExternalStringTableEntry(Heap* heap,
                                                         FullObjectSlot p) {
-  MapWord map_word = HeapObject::cast(*p).map_word(kRelaxedLoad);
+  HeapObject old_string = HeapObject::cast(*p);
+  MapWord map_word = old_string.map_word(kRelaxedLoad);
 
   if (map_word.IsForwardingAddress()) {
     String new_string = String::cast(map_word.ToForwardingAddress());

src/heap/scavenger-inl.h

@@ -381,7 +381,8 @@ SlotCallbackResult Scavenger::EvacuateObject(THeapObjectSlot slot, Map map,
           map, slot, String::unchecked_cast(source), size,
           ObjectFields::kMaybePointers);
     case kVisitDataObject:  // External strings have kVisitDataObject.
-      if (String::IsInPlaceInternalizable(map.instance_type())) {
+      if (String::IsInPlaceInternalizableExcludingExternal(
+              map.instance_type())) {
         return EvacuateInPlaceInternalizableString(
             map, slot, String::unchecked_cast(source), size,
             ObjectFields::kDataOnly);

src/objects/string-inl.h

@@ -1449,6 +1449,13 @@ bool String::IsInPlaceInternalizable(InstanceType instance_type) {
   }
 }
 
+// static
+bool String::IsInPlaceInternalizableExcludingExternal(
+    InstanceType instance_type) {
+  return IsInPlaceInternalizable(instance_type) &&
+         !InstanceTypeChecker::IsExternalString(instance_type);
+}
+
 }  // namespace internal
 }  // namespace v8
 

src/objects/string.cc

@@ -541,6 +541,10 @@ bool String::SupportsExternalization() {
     return false;
   }
 
+  // External strings in the shared heap conflicts with the heap sandbox at the
+  // moment. Disable it until supported.
+  if (InSharedHeap()) return false;
+
 #ifdef V8_COMPRESS_POINTERS
   // Small strings may not be in-place externalizable.
   if (this->Size() < ExternalString::kUncachedSize) return false;

src/objects/string.h

@@ -577,6 +577,9 @@ class String : public TorqueGeneratedString<String, Name> {
   static inline bool IsInPlaceInternalizable(String string);
   static inline bool IsInPlaceInternalizable(InstanceType instance_type);
 
+  static inline bool IsInPlaceInternalizableExcludingExternal(
+      InstanceType instance_type);
+
  private:
   friend class Name;
   friend class StringTableInsertionKey;