Reland^2 "Avoiding re-externalization of strings"

Previously landed as 2c4c2ad694 / #54599 and
f34158c9d2 / #54637

Previously reviewed at https://chromium-review.googlesource.com/1139056 and
https://chromium-review.googlesource.com/1146583

Bug: chromium:845409, chromium:866208
Change-Id: Idb1b6d1b29499f66bf8cd704977c40b027f99dbd
Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/1148281
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Dan Elphick <delphick@chromium.org>
Commit-Queue: Rodrigo Bruno <rfbpb@google.com>
Cr-Commit-Position: refs/heads/master@{#54703}

include/v8.h

@@ -3053,6 +3053,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);
 };
 
@@ -10187,12 +10193,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);
@@ -10208,14 +10215,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

@@ -5932,11 +5932,16 @@ bool v8::String::IsExternalOneByte() const {
 
 void v8::String::VerifyExternalStringResource(
     v8::String::ExternalStringResource* value) const {
-  i::Handle<i::String> str = Utils::OpenHandle(this);
+  i::DisallowHeapAllocation no_allocation;
+  i::String* str = *Utils::OpenHandle(this);
   const v8::String::ExternalStringResource* expected;
-  if (i::StringShape(*str).IsExternalTwoByte()) {
-    const void* resource =
-        i::Handle<i::ExternalTwoByteString>::cast(str)->resource();
+
+  if (str->IsThinString()) {
+    str = i::ThinString::cast(str)->actual();
+  }
+
+  if (i::StringShape(str).IsExternalTwoByte()) {
+    const void* resource = i::ExternalTwoByteString::cast(str)->resource();
     expected = reinterpret_cast<const ExternalStringResource*>(resource);
   } else {
     expected = nullptr;
@@ -5946,17 +5951,21 @@ void v8::String::VerifyExternalStringResource(
 
 void v8::String::VerifyExternalStringResourceBase(
     v8::String::ExternalStringResourceBase* value, Encoding encoding) const {
-  i::Handle<i::String> str = Utils::OpenHandle(this);
+  i::DisallowHeapAllocation no_allocation;
+  i::String* str = *Utils::OpenHandle(this);
   const v8::String::ExternalStringResourceBase* expected;
   Encoding expectedEncoding;
-  if (i::StringShape(*str).IsExternalOneByte()) {
-    const void* resource =
-        i::Handle<i::ExternalOneByteString>::cast(str)->resource();
+
+  if (str->IsThinString()) {
+    str = i::ThinString::cast(str)->actual();
+  }
+
+  if (i::StringShape(str).IsExternalOneByte()) {
+    const void* resource = i::ExternalOneByteString::cast(str)->resource();
     expected = reinterpret_cast<const ExternalStringResourceBase*>(resource);
     expectedEncoding = ONE_BYTE_ENCODING;
-  } else if (i::StringShape(*str).IsExternalTwoByte()) {
-    const void* resource =
-        i::Handle<i::ExternalTwoByteString>::cast(str)->resource();
+  } else if (i::StringShape(str).IsExternalTwoByte()) {
+    const void* resource = i::ExternalTwoByteString::cast(str)->resource();
     expected = reinterpret_cast<const ExternalStringResourceBase*>(resource);
     expectedEncoding = TWO_BYTE_ENCODING;
   } else {
@@ -5968,15 +5977,69 @@ void v8::String::VerifyExternalStringResourceBase(
   CHECK_EQ(expectedEncoding, encoding);
 }
 
+String::ExternalStringResource* String::GetExternalStringResourceSlow() const {
+  i::DisallowHeapAllocation no_allocation;
+  typedef internal::Internals I;
+  ExternalStringResource* result = nullptr;
+  i::String* str = *Utils::OpenHandle(this);
+
+  if (str->IsThinString()) {
+    str = i::ThinString::cast(str)->actual();
+  }
+
+  if (i::StringShape(str).IsExternalTwoByte()) {
+    void* value = I::ReadField<void*>(str, I::kStringResourceOffset);
+    result = reinterpret_cast<String::ExternalStringResource*>(value);
+  }
+  return result;
+}
+
+String::ExternalStringResourceBase* String::GetExternalStringResourceBaseSlow(
+    String::Encoding* encoding_out) const {
+  i::DisallowHeapAllocation no_allocation;
+  typedef internal::Internals I;
+  ExternalStringResourceBase* resource = nullptr;
+  i::String* str = *Utils::OpenHandle(this);
+
+  if (str->IsThinString()) {
+    str = i::ThinString::cast(str)->actual();
+  }
+
+  int type = I::GetInstanceType(str) & I::kFullStringRepresentationMask;
+  *encoding_out = static_cast<Encoding>(type & I::kStringEncodingMask);
+  if (i::StringShape(str).IsExternalOneByte() ||
+      i::StringShape(str).IsExternalTwoByte()) {
+    void* value = I::ReadField<void*>(str, I::kStringResourceOffset);
+    resource = static_cast<ExternalStringResourceBase*>(value);
+  }
+  return resource;
+}
+
+const String::ExternalOneByteStringResource*
+String::GetExternalOneByteStringResourceSlow() const {
+  i::DisallowHeapAllocation no_allocation;
+  i::String* str = *Utils::OpenHandle(this);
+
+  if (str->IsThinString()) {
+    str = i::ThinString::cast(str)->actual();
+  }
+
+  if (i::StringShape(str).IsExternalOneByte()) {
+    const void* resource = 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* resource =
-        i::Handle<i::ExternalOneByteString>::cast(str)->resource();
+  i::DisallowHeapAllocation no_allocation;
+  i::String* str = *Utils::OpenHandle(this);
+  if (i::StringShape(str).IsExternalOneByte()) {
+    const void* resource = i::ExternalOneByteString::cast(str)->resource();
     return reinterpret_cast<const ExternalOneByteStringResource*>(resource);
   } else {
-    return nullptr;
+    return GetExternalOneByteStringResourceSlow();
   }
 }
 
@@ -6863,73 +6926,76 @@ Local<String> v8::String::NewExternal(
 
 
 bool v8::String::MakeExternal(v8::String::ExternalStringResource* resource) {
-  i::Handle<i::String> obj = Utils::OpenHandle(this);
+  i::DisallowHeapAllocation no_allocation;
 
+  i::String* obj = *Utils::OpenHandle(this);
+
+  if (obj->IsThinString()) {
+    obj = i::ThinString::cast(obj)->actual();
+  }
+
+  if (!obj->SupportsExternalization()) {
+    return false;
+  }
+
+  // It is safe to call FromWritable because SupportsExternalization already
+  // checked that the object is writable.
   i::Isolate* isolate;
-  if (!i::Isolate::FromWritableHeapObject(*obj, &isolate)) {
-    // RO_SPACE strings cannot be externalized.
-    return false;
-  }
-
-  if (i::StringShape(*obj).IsExternal()) {
-    return false;  // Already an external string.
-  }
+  i::Isolate::FromWritableHeapObject(obj, &isolate);
   ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
-  if (isolate->heap()->IsInGCPostProcessing()) {
-    return false;
-  }
+
   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::MakeExternal(
     v8::String::ExternalOneByteStringResource* resource) {
-  i::Handle<i::String> obj = Utils::OpenHandle(this);
+  i::DisallowHeapAllocation no_allocation;
 
+  i::String* obj = *Utils::OpenHandle(this);
+
+  if (obj->IsThinString()) {
+    obj = i::ThinString::cast(obj)->actual();
+  }
+
+  if (!obj->SupportsExternalization()) {
+    return false;
+  }
+
+  // It is safe to call FromWritable because SupportsExternalization already
+  // checked that the object is writable.
   i::Isolate* isolate;
-  if (!i::Isolate::FromWritableHeapObject(*obj, &isolate)) {
-    // RO_SPACE strings cannot be externalized.
-    return false;
-  }
-
-  if (i::StringShape(*obj).IsExternal()) {
-    return false;  // Already an external string.
-  }
+  i::Isolate::FromWritableHeapObject(obj, &isolate);
   ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
-  if (isolate->heap()->IsInGCPostProcessing()) {
-    return false;
-  }
+
   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::DisallowHeapAllocation no_allocation;
+  i::String* obj = *Utils::OpenHandle(this);
 
-  i::Isolate* isolate;
-  if (!i::Isolate::FromWritableHeapObject(*obj, &isolate)) {
-    // RO_SPACE strings cannot be externalized.
+  if (obj->IsThinString()) {
+    obj = i::ThinString::cast(obj)->actual();
+  }
+
+  if (!obj->SupportsExternalization()) {
     return false;
   }
+
   // Only old space strings should be externalized.
-  return !i::Heap::InNewSpace(*obj);
+  return !i::Heap::InNewSpace(obj);
 }
 
 

src/extensions/externalize-string-extension.cc

@@ -85,11 +85,12 @@ void ExternalizeStringExtension::Externalize(
   }
   bool result = false;
   Handle<String> string = Utils::OpenHandle(*args[0].As<v8::String>());
-  if (string->IsExternalString()) {
+  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 +98,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 = Utils::ToLocal(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 = Utils::ToLocal(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

@@ -2275,6 +2275,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

@@ -2576,7 +2576,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) {
@@ -2656,7 +2656,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) {
@@ -2725,6 +2725,25 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
   return true;
 }
 
+bool String::SupportsExternalization() {
+  if (this->IsThinString()) {
+    return i::ThinString::cast(this)->actual()->SupportsExternalization();
+  }
+
+  Isolate* isolate;
+  // 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);