Reland "Avoiding re-externalization of strings."

This is a reland of 2c4c2ad6

Original change's description:
> Avoiding re-externalization of strings.
>
> Bug: chromium:845409
> Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
> Change-Id: I75bddcf0e8879d2161486f24d1cd4e46d8fe008d
> Reviewed-on: https://chromium-review.googlesource.com/1139056
> Commit-Queue: Rodrigo Bruno <rfbpb@google.com>
> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#54599}

Bug: chromium:866208
Change-Id: I7714bfc695ebeaf55b9ccbbc6b11368416ce7fec
Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/1146583Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Rodrigo Bruno <rfbpb@google.com>
Cr-Commit-Position: refs/heads/master@{#54637}

include/v8.h

@@ -3048,6 +3048,12 @@ class V8_EXPORT String : public Name {
   void VerifyExternalStringResourceBase(ExternalStringResourceBase* v,
                                         Encoding encoding) const;
   void VerifyExternalStringResource(ExternalStringResource* val) const;
+  ExternalStringResource* GetExternalStringResourceSlow() const;
+  ExternalStringResourceBase* GetExternalStringResourceBaseSlow(
+      String::Encoding* encoding_out) const;
+  const ExternalOneByteStringResource* GetExternalOneByteStringResourceSlow()
+      const;
+
   static void CheckCast(v8::Value* obj);
 };
 
@@ -10182,12 +10188,13 @@ String::ExternalStringResource* String::GetExternalStringResource() const {
   typedef internal::Object O;
   typedef internal::Internals I;
   O* obj = *reinterpret_cast<O* const*>(this);
-  String::ExternalStringResource* result;
+
+  ExternalStringResource* result;
   if (I::IsExternalTwoByteString(I::GetInstanceType(obj))) {
     void* value = I::ReadField<void*>(obj, I::kStringResourceOffset);
     result = reinterpret_cast<String::ExternalStringResource*>(value);
   } else {
-    result = NULL;
+    result = GetExternalStringResourceSlow();
   }
 #ifdef V8_ENABLE_CHECKS
   VerifyExternalStringResource(result);
@@ -10203,14 +10210,16 @@ String::ExternalStringResourceBase* String::GetExternalStringResourceBase(
   O* obj = *reinterpret_cast<O* const*>(this);
   int type = I::GetInstanceType(obj) & I::kFullStringRepresentationMask;
   *encoding_out = static_cast<Encoding>(type & I::kStringEncodingMask);
-  ExternalStringResourceBase* resource = NULL;
+  ExternalStringResourceBase* resource;
   if (type == I::kExternalOneByteRepresentationTag ||
       type == I::kExternalTwoByteRepresentationTag) {
     void* value = I::ReadField<void*>(obj, I::kStringResourceOffset);
     resource = static_cast<ExternalStringResourceBase*>(value);
+  } else {
+    resource = GetExternalStringResourceBaseSlow(encoding_out);
   }
 #ifdef V8_ENABLE_CHECKS
-    VerifyExternalStringResourceBase(resource, *encoding_out);
+  VerifyExternalStringResourceBase(resource, *encoding_out);
 #endif
   return resource;
 }

src/api.cc

@@ -5930,6 +5930,15 @@ void v8::String::VerifyExternalStringResource(
     v8::String::ExternalStringResource* value) const {
   i::Handle<i::String> str = Utils::OpenHandle(this);
   const v8::String::ExternalStringResource* expected;
+  i::Isolate* isolate = nullptr;
+
+  i::Isolate::FromWritableHeapObject(*str, &isolate);
+  DCHECK(isolate != nullptr);
+
+  if (str->IsThinString()) {
+    str = i::Handle<i::String>(i::ThinString::cast(*str)->actual(), isolate);
+  }
+
   if (i::StringShape(*str).IsExternalTwoByte()) {
     const void* resource =
         i::Handle<i::ExternalTwoByteString>::cast(str)->resource();
@@ -5945,6 +5954,15 @@ void v8::String::VerifyExternalStringResourceBase(
   i::Handle<i::String> str = Utils::OpenHandle(this);
   const v8::String::ExternalStringResourceBase* expected;
   Encoding expectedEncoding;
+  i::Isolate* isolate = nullptr;
+
+  i::Isolate::FromWritableHeapObject(*str, &isolate);
+  DCHECK(isolate != nullptr);
+
+  if (str->IsThinString()) {
+    str = i::Handle<i::String>(i::ThinString::cast(*str)->actual(), isolate);
+  }
+
   if (i::StringShape(*str).IsExternalOneByte()) {
     const void* resource =
         i::Handle<i::ExternalOneByteString>::cast(str)->resource();
@@ -5964,15 +5982,74 @@ void v8::String::VerifyExternalStringResourceBase(
   CHECK_EQ(expectedEncoding, encoding);
 }
 
-const v8::String::ExternalOneByteStringResource*
-v8::String::GetExternalOneByteStringResource() const {
+String::ExternalStringResource* String::GetExternalStringResourceSlow() const {
+  typedef internal::Internals I;
   i::Handle<i::String> str = Utils::OpenHandle(this);
+  ExternalStringResource* result = nullptr;
+  internal::Object* obj;
+
+  if (str->IsThinString()) {
+    obj = i::Handle<i::ThinString>::cast(str)->actual();
+  } else {
+    obj = *str;
+  }
+
+  if (I::IsExternalTwoByteString(I::GetInstanceType(obj))) {
+    void* value = I::ReadField<void*>(obj, I::kStringResourceOffset);
+    result = reinterpret_cast<String::ExternalStringResource*>(value);
+  }
+  return result;
+}
+
+String::ExternalStringResourceBase* String::GetExternalStringResourceBaseSlow(
+    String::Encoding* encoding_out) const {
+  typedef internal::Internals I;
+  i::Handle<i::String> str = Utils::OpenHandle(this);
+  ExternalStringResourceBase* resource = nullptr;
+  internal::Object* obj;
+
+  if (str->IsThinString()) {
+    obj = i::Handle<i::ThinString>::cast(str)->actual();
+  } else {
+    obj = *str;
+  }
+
+  int type = I::GetInstanceType(obj) & I::kFullStringRepresentationMask;
+  *encoding_out = static_cast<Encoding>(type & I::kStringEncodingMask);
+
+  if (type == I::kExternalOneByteRepresentationTag ||
+      type == I::kExternalTwoByteRepresentationTag) {
+    void* value = I::ReadField<void*>(obj, I::kStringResourceOffset);
+    resource = static_cast<ExternalStringResourceBase*>(value);
+  }
+  return resource;
+}
+
+const String::ExternalOneByteStringResource*
+String::GetExternalOneByteStringResourceSlow() const {
+  i::Handle<i::String> str = Utils::OpenHandle(this);
+  if (str->IsThinString()) {
+    i::MemoryChunk* chunk = i::MemoryChunk::FromHeapObject(*str);
+    str = i::Handle<i::String>(i::Handle<i::ThinString>::cast(str)->actual(),
+                               chunk->heap()->isolate());
+  }
   if (i::StringShape(*str).IsExternalOneByte()) {
     const void* resource =
         i::Handle<i::ExternalOneByteString>::cast(str)->resource();
     return reinterpret_cast<const ExternalOneByteStringResource*>(resource);
+  }
+  return nullptr;
+}
+
+const v8::String::ExternalOneByteStringResource*
+v8::String::GetExternalOneByteStringResource() const {
+  i::Handle<i::String> str = Utils::OpenHandle(this);
+  if (i::StringShape(*str).IsExternalOneByte()) {
+    const void* value =
+        i::Handle<i::ExternalOneByteString>::cast(str)->resource();
+    return reinterpret_cast<const ExternalOneByteStringResource*>(value);
   } else {
-    return nullptr;
+    return GetExternalOneByteStringResourceSlow();
   }
 }
 
@@ -6860,28 +6937,26 @@ Local<String> v8::String::NewExternal(
 
 bool v8::String::MakeExternal(v8::String::ExternalStringResource* resource) {
   i::Handle<i::String> obj = Utils::OpenHandle(this);
+  i::Isolate* isolate = nullptr;
 
-  i::Isolate* isolate;
-  if (!i::Isolate::FromWritableHeapObject(*obj, &isolate)) {
-    // RO_SPACE strings cannot be externalized.
-    return false;
-  }
+  i::Isolate::FromWritableHeapObject(*obj, &isolate);
+  DCHECK(isolate != nullptr);
 
-  if (i::StringShape(*obj).IsExternal()) {
-    return false;  // Already an external string.
+  if (obj->IsThinString()) {
+    obj = i::Handle<i::String>(i::ThinString::cast(*obj)->actual(), isolate);
   }
-  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
-  if (isolate->heap()->IsInGCPostProcessing()) {
+
+  if (!obj->SupportsExternalization()) {
     return false;
   }
+
+  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
+
   CHECK(resource && resource->data());
 
   bool result = obj->MakeExternal(resource);
-  // Assert that if CanMakeExternal(), then externalizing actually succeeds.
-  DCHECK(!CanMakeExternal() || result);
-  if (result) {
-    DCHECK(obj->IsExternalString());
-  }
+  DCHECK(result);
+  DCHECK(obj->IsExternalString());
   return result;
 }
 
@@ -6889,41 +6964,45 @@ bool v8::String::MakeExternal(v8::String::ExternalStringResource* resource) {
 bool v8::String::MakeExternal(
     v8::String::ExternalOneByteStringResource* resource) {
   i::Handle<i::String> obj = Utils::OpenHandle(this);
+  i::Isolate* isolate = nullptr;
 
-  i::Isolate* isolate;
-  if (!i::Isolate::FromWritableHeapObject(*obj, &isolate)) {
-    // RO_SPACE strings cannot be externalized.
-    return false;
-  }
+  i::Isolate::FromWritableHeapObject(*obj, &isolate);
+  DCHECK(isolate != nullptr);
 
-  if (i::StringShape(*obj).IsExternal()) {
-    return false;  // Already an external string.
+  if (obj->IsThinString()) {
+    obj = i::Handle<i::String>(i::ThinString::cast(*obj)->actual(), isolate);
   }
-  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
-  if (isolate->heap()->IsInGCPostProcessing()) {
+
+  if (!obj->SupportsExternalization()) {
     return false;
   }
+
+  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
+
   CHECK(resource && resource->data());
 
   bool result = obj->MakeExternal(resource);
-  // Assert that if CanMakeExternal(), then externalizing actually succeeds.
-  DCHECK(!CanMakeExternal() || result);
-  if (result) {
-    DCHECK(obj->IsExternalString());
-  }
+  DCHECK(result);
+  DCHECK(obj->IsExternalString());
   return result;
 }
 
 
 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.
+  if (obj->IsThinString()) {
+    i::Isolate* isolate = nullptr;
+    i::Isolate::FromWritableHeapObject(*obj, &isolate);
+    DCHECK(isolate != nullptr);
+    obj = i::Handle<i::String>(i::ThinString::cast(*obj)->actual(), isolate);
+    return Utils::ToLocal(obj)->CanMakeExternal();
+  }
+
+  if (!obj->SupportsExternalization()) {
     return false;
   }
+
   // Only old space strings should be externalized.
   return !i::Heap::InNewSpace(*obj);
 }

src/extensions/externalize-string-extension.cc

@@ -84,12 +84,14 @@ void ExternalizeStringExtension::Externalize(
     }
   }
   bool result = false;
-  Handle<String> string = Utils::OpenHandle(*args[0].As<v8::String>());
-  if (string->IsExternalString()) {
+  v8::String* v8_string = *args[0].As<v8::String>();
+  Handle<String> string = Utils::OpenHandle(v8_string);
+  if (!string->SupportsExternalization()) {
     args.GetIsolate()->ThrowException(
         v8::String::NewFromUtf8(args.GetIsolate(),
-                                "externalizeString() can't externalize twice.",
-                                NewStringType::kNormal).ToLocalChecked());
+                                "string does not support externalization.",
+                                NewStringType::kNormal)
+            .ToLocalChecked());
     return;
   }
   if (string->IsOneByteRepresentation() && !force_two_byte) {
@@ -97,14 +99,14 @@ void ExternalizeStringExtension::Externalize(
     String::WriteToFlat(*string, data, 0, string->length());
     SimpleOneByteStringResource* resource = new SimpleOneByteStringResource(
         reinterpret_cast<char*>(data), string->length());
-    result = string->MakeExternal(resource);
+    result = v8_string->MakeExternal(resource);
     if (!result) delete resource;
   } else {
     uc16* data = new uc16[string->length()];
     String::WriteToFlat(*string, data, 0, string->length());
     SimpleTwoByteStringResource* resource = new SimpleTwoByteStringResource(
         data, string->length());
-    result = string->MakeExternal(resource);
+    result = v8_string->MakeExternal(resource);
     if (!result) delete resource;
   }
   if (!result) {

src/heap/heap-inl.h

@@ -530,6 +530,8 @@ Isolate* Heap::isolate() {
 
 void Heap::ExternalStringTable::AddString(String* string) {
   DCHECK(string->IsExternalString());
+  DCHECK(!Contains(string));
+
   if (InNewSpace(string)) {
     new_space_strings_.push_back(string);
   } else {

src/heap/heap.cc

@@ -2296,6 +2296,16 @@ void Heap::ProtectUnprotectedMemoryChunks() {
   unprotected_memory_chunks_.clear();
 }
 
+bool Heap::ExternalStringTable::Contains(HeapObject* obj) {
+  for (size_t i = 0; i < new_space_strings_.size(); ++i) {
+    if (new_space_strings_[i] == obj) return true;
+  }
+  for (size_t i = 0; i < old_space_strings_.size(); ++i) {
+    if (old_space_strings_[i] == obj) return true;
+  }
+  return false;
+}
+
 String* Heap::UpdateNewSpaceReferenceInExternalStringTableEntry(Heap* heap,
                                                                 Object** p) {
   MapWord first_word = HeapObject::cast(*p)->map_word();

src/heap/heap.h

@@ -1481,6 +1481,7 @@ class Heap {
 
     // Registers an external string.
     inline void AddString(String* string);
+    bool Contains(HeapObject* obj);
 
     void IterateAll(RootVisitor* v);
     void IterateNewSpaceStrings(RootVisitor* v);

src/objects.cc

@@ -2573,7 +2573,7 @@ bool String::MakeExternal(v8::String::ExternalStringResource* resource) {
   DisallowHeapAllocation no_allocation;
   // Externalizing twice leaks the external resource, so it's
   // prohibited by the API.
-  DCHECK(!this->IsExternalString());
+  DCHECK(this->SupportsExternalization());
   DCHECK(!resource->IsCompressible());
 #ifdef ENABLE_SLOW_DCHECKS
   if (FLAG_enable_slow_asserts) {
@@ -2653,7 +2653,7 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
   DisallowHeapAllocation no_allocation;
   // Externalizing twice leaks the external resource, so it's
   // prohibited by the API.
-  DCHECK(!this->IsExternalString());
+  DCHECK(this->SupportsExternalization());
   DCHECK(!resource->IsCompressible());
 #ifdef ENABLE_SLOW_DCHECKS
   if (FLAG_enable_slow_asserts) {
@@ -2722,6 +2722,26 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
   return true;
 }
 
+bool String::SupportsExternalization() {
+  Isolate* isolate;
+
+  if (this->IsThinString()) {
+    return i::ThinString::cast(this)->actual()->SupportsExternalization();
+  }
+
+  // RO_SPACE strings cannot be externalized.
+  if (!Isolate::FromWritableHeapObject(this, &isolate)) {
+    return false;
+  }
+
+  // Already an external string.
+  if (StringShape(this).IsExternal()) {
+    return false;
+  }
+
+  return !isolate->heap()->IsInGCPostProcessing();
+}
+
 void String::StringShortPrint(StringStream* accumulator, bool show_details) {
   int len = length();
   if (len > kMaxShortPrintLength) {

src/objects/string.h

@@ -311,6 +311,7 @@ class String : public Name {
   // Externalization.
   bool MakeExternal(v8::String::ExternalStringResource* resource);
   bool MakeExternal(v8::String::ExternalOneByteStringResource* resource);
+  bool SupportsExternalization();
 
   // Conversion.
   inline bool AsArrayIndex(uint32_t* index);