[offthread] Add string access lock to GetChars

Add a requirement to String::GetChars that we either have a string
access lock, or a string access lock is not needed. This prevents us
from reading strings during internalization that may be in the middle
of being made external.

To avoid taking the lock too often when known to be unnecessary (e.g.
for strings that were only just created), there's now a static
SharedStringAccessGuardIfNeeded::NotNeeded(). This is hopefully ugly
enough that it's used sparingly.

One fix required for this is to enter the Isolate when tearing down
IsolateData in inspector tests -- this is so that the V8Inspector
instance being torn down will see the current Isolate and be able to
verify its thread id against the current thread.

Bug: chromium:1011762, chromium:1148680
Change-Id: Ic5d29c1b066ebae5a351c7b4bb116b9b1bf61889
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2536465
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Santiago Aboy Solanes <solanes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71197}

src/ast/ast-value-factory.cc

@@ -34,6 +34,7 @@
 #include "src/heap/local-factory-inl.h"
 #include "src/objects/objects-inl.h"
 #include "src/objects/objects.h"
+#include "src/objects/string.h"
 #include "src/strings/char-predicates-inl.h"
 #include "src/strings/string-hasher.h"
 #include "src/utils/utils-inl.h"
@@ -231,14 +232,17 @@ Handle<String> AstConsString::AllocateFlat(LocalIsolate* isolate) const {
             ->NewRawOneByteString(result_length, AllocationType::kOld)
             .ToHandleChecked();
     DisallowHeapAllocation no_gc;
-    uint8_t* dest = result->GetChars(no_gc) + result_length;
+    uint8_t* dest =
+        result->GetChars(no_gc, SharedStringAccessGuardIfNeeded::NotNeeded()) +
+        result_length;
     for (const AstConsString::Segment* current = &segment_; current != nullptr;
          current = current->next) {
       int length = current->string->length();
       dest -= length;
       CopyChars(dest, current->string->raw_data(), length);
     }
-    DCHECK_EQ(dest, result->GetChars(no_gc));
+    DCHECK_EQ(dest, result->GetChars(
+                        no_gc, SharedStringAccessGuardIfNeeded::NotNeeded()));
     return result;
   }
 
@@ -247,7 +251,9 @@ Handle<String> AstConsString::AllocateFlat(LocalIsolate* isolate) const {
           ->NewRawTwoByteString(result_length, AllocationType::kOld)
           .ToHandleChecked();
   DisallowHeapAllocation no_gc;
-  uint16_t* dest = result->GetChars(no_gc) + result_length;
+  uint16_t* dest =
+      result->GetChars(no_gc, SharedStringAccessGuardIfNeeded::NotNeeded()) +
+      result_length;
   for (const AstConsString::Segment* current = &segment_; current != nullptr;
        current = current->next) {
     int length = current->string->length();
@@ -260,7 +266,8 @@ Handle<String> AstConsString::AllocateFlat(LocalIsolate* isolate) const {
                 length);
     }
   }
-  DCHECK_EQ(dest, result->GetChars(no_gc));
+  DCHECK_EQ(dest, result->GetChars(
+                      no_gc, SharedStringAccessGuardIfNeeded::NotNeeded()));
   return result;
 }
 template EXPORT_TEMPLATE_DEFINE(V8_EXPORT_PRIVATE)

src/heap/factory-base.cc

@@ -21,6 +21,7 @@
 #include "src/objects/shared-function-info-inl.h"
 #include "src/objects/source-text-module.h"
 #include "src/objects/string-inl.h"
+#include "src/objects/string.h"
 #include "src/objects/template-objects-inl.h"
 
 namespace v8 {
@@ -508,7 +509,8 @@ Handle<SeqOneByteString> FactoryBase<Impl>::NewOneByteInternalizedString(
   Handle<SeqOneByteString> result =
       AllocateRawOneByteInternalizedString(str.length(), raw_hash_field);
   DisallowHeapAllocation no_gc;
-  MemCopy(result->GetChars(no_gc), str.begin(), str.length());
+  MemCopy(result->GetChars(no_gc, SharedStringAccessGuardIfNeeded::NotNeeded()),
+          str.begin(), str.length());
   return result;
 }
 
@@ -518,7 +520,8 @@ Handle<SeqTwoByteString> FactoryBase<Impl>::NewTwoByteInternalizedString(
   Handle<SeqTwoByteString> result =
       AllocateRawTwoByteInternalizedString(str.length(), raw_hash_field);
   DisallowHeapAllocation no_gc;
-  MemCopy(result->GetChars(no_gc), str.begin(), str.length() * kUC16Size);
+  MemCopy(result->GetChars(no_gc, SharedStringAccessGuardIfNeeded::NotNeeded()),
+          str.begin(), str.length() * kUC16Size);
   return result;
 }
 
@@ -606,21 +609,31 @@ MaybeHandle<String> FactoryBase<Impl>::NewConsString(
       Handle<SeqOneByteString> result =
           NewRawOneByteString(length, allocation).ToHandleChecked();
       DisallowHeapAllocation no_gc;
-      uint8_t* dest = result->GetChars(no_gc);
+      uint8_t* dest =
+          result->GetChars(no_gc, SharedStringAccessGuardIfNeeded::NotNeeded());
       // Copy left part.
-      const uint8_t* src = left->template GetChars<uint8_t>(no_gc);
+      {
+        SharedStringAccessGuardIfNeeded access_guard(*left);
+        const uint8_t* src =
+            left->template GetChars<uint8_t>(no_gc, access_guard);
         CopyChars(dest, src, left_length);
+      }
       // Copy right part.
-      src = right->template GetChars<uint8_t>(no_gc);
+      {
+        SharedStringAccessGuardIfNeeded access_guard(*right);
+        const uint8_t* src =
+            right->template GetChars<uint8_t>(no_gc, access_guard);
         CopyChars(dest + left_length, src, right_length);
+      }
       return result;
     }
 
     Handle<SeqTwoByteString> result =
         NewRawTwoByteString(length, allocation).ToHandleChecked();
 
-    DisallowHeapAllocation pointer_stays_valid;
-    uc16* sink = result->GetChars(pointer_stays_valid);
+    DisallowHeapAllocation no_gc;
+    uc16* sink =
+        result->GetChars(no_gc, SharedStringAccessGuardIfNeeded::NotNeeded());
     String::WriteToFlat(*left, sink, 0, left->length());
     String::WriteToFlat(*right, sink + left->length(), 0, right->length());
     return result;

src/objects/string-inl.h

@@ -32,15 +32,34 @@ namespace internal {
 class SharedStringAccessGuardIfNeeded {
  public:
   explicit SharedStringAccessGuardIfNeeded(String str) {
-    // If we can get the isolate from the String, it means it is not a read only
-    // string.
     Isolate* isolate;
-    if (GetIsolateFromHeapObject(str, &isolate) &&
-        ThreadId::Current() != isolate->thread_id())
-      mutex_guard.emplace(isolate->string_access());
+    if (IsNeeded(str, &isolate)) mutex_guard.emplace(isolate->string_access());
+  }
+
+  static SharedStringAccessGuardIfNeeded NotNeeded() {
+    return SharedStringAccessGuardIfNeeded();
+  }
+
+  static bool IsNeeded(String str, Isolate** out_isolate = nullptr) {
+    Isolate* isolate;
+    if (!GetIsolateFromHeapObject(str, &isolate)) {
+      // If we can't get the isolate from the String, it must be read-only.
+      DCHECK(ReadOnlyHeap::Contains(str));
+      return false;
+    }
+    if (out_isolate) *out_isolate = isolate;
+    return ThreadId::Current() != isolate->thread_id();
   }
 
  private:
+  // Default constructor and move constructor required for the NotNeeded()
+  // static constructor.
+  constexpr SharedStringAccessGuardIfNeeded() = default;
+  constexpr SharedStringAccessGuardIfNeeded(SharedStringAccessGuardIfNeeded&&)
+      V8_NOEXCEPT {
+    DCHECK(!mutex_guard.has_value());
+  }
+
   base::Optional<base::SharedMutexGuard<base::kShared>> mutex_guard;
 };
 
@@ -282,12 +301,13 @@ class SequentialStringKey final : public StringTableKey {
         convert_(convert) {}
 
   bool IsMatch(String s) override {
+    SharedStringAccessGuardIfNeeded access_guard(s);
     DisallowHeapAllocation no_gc;
     if (s.IsOneByteRepresentation()) {
-      const uint8_t* chars = s.GetChars<uint8_t>(no_gc);
+      const uint8_t* chars = s.GetChars<uint8_t>(no_gc, access_guard);
       return CompareChars(chars, chars_.begin(), chars_.length()) == 0;
     }
-    const uint16_t* chars = s.GetChars<uint16_t>(no_gc);
+    const uint16_t* chars = s.GetChars<uint16_t>(no_gc, access_guard);
     return CompareChars(chars, chars_.begin(), chars_.length()) == 0;
   }
 
@@ -413,6 +433,16 @@ const Char* String::GetChars(const DisallowHeapAllocation& no_gc) {
              : CharTraits<Char>::String::cast(*this).GetChars(no_gc);
 }
 
+template <typename Char>
+const Char* String::GetChars(
+    const DisallowHeapAllocation& no_gc,
+    const SharedStringAccessGuardIfNeeded& access_guard) {
+  return StringShape(*this).IsExternal()
+             ? CharTraits<Char>::ExternalString::cast(*this).GetChars()
+             : CharTraits<Char>::String::cast(*this).GetChars(no_gc,
+                                                              access_guard);
+}
+
 Handle<String> String::Flatten(Isolate* isolate, Handle<String> string,
                                AllocationType allocation) {
   if (string->IsConsString()) {
@@ -582,6 +612,15 @@ Address SeqOneByteString::GetCharsAddress() {
 
 uint8_t* SeqOneByteString::GetChars(const DisallowHeapAllocation& no_gc) {
   USE(no_gc);
+  DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(*this));
+  return reinterpret_cast<uint8_t*>(GetCharsAddress());
+}
+
+uint8_t* SeqOneByteString::GetChars(
+    const DisallowHeapAllocation& no_gc,
+    const SharedStringAccessGuardIfNeeded& access_guard) {
+  USE(no_gc);
+  USE(access_guard);
   return reinterpret_cast<uint8_t*>(GetCharsAddress());
 }
 
@@ -591,6 +630,15 @@ Address SeqTwoByteString::GetCharsAddress() {
 
 uc16* SeqTwoByteString::GetChars(const DisallowHeapAllocation& no_gc) {
   USE(no_gc);
+  DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(*this));
+  return reinterpret_cast<uc16*>(GetCharsAddress());
+}
+
+uc16* SeqTwoByteString::GetChars(
+    const DisallowHeapAllocation& no_gc,
+    const SharedStringAccessGuardIfNeeded& access_guard) {
+  USE(no_gc);
+  USE(access_guard);
   return reinterpret_cast<uc16*>(GetCharsAddress());
 }
 

src/objects/string-table.cc

@@ -358,7 +358,10 @@ class InternalizedStringKey final : public StringTableKey {
     set_raw_hash_field(string->raw_hash_field());
   }
 
-  bool IsMatch(String string) override { return string_->SlowEquals(string); }
+  bool IsMatch(String string) override {
+    DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(string));
+    return string_->SlowEquals(string);
+  }
 
   Handle<String> AsHandle(Isolate* isolate) {
     // Internalize the string if possible.

src/objects/string.cc

@@ -640,11 +640,9 @@ std::unique_ptr<char[]> String::ToCString(AllowNullsFlag allow_nulls,
 }
 
 template <typename sinkchar>
-void String::WriteToFlat(String src, sinkchar* sink, int f, int t) {
+void String::WriteToFlat(String source, sinkchar* sink, int from, int to) {
   DisallowHeapAllocation no_gc;
-  String source = src;
-  int from = f;
-  int to = t;
+  SharedStringAccessGuardIfNeeded access_guard(source);
   while (from < to) {
     DCHECK_LE(0, from);
     DCHECK_LE(to, source.length());
@@ -660,12 +658,16 @@ void String::WriteToFlat(String src, sinkchar* sink, int f, int t) {
         return;
       }
       case kOneByteStringTag | kSeqStringTag: {
-        CopyChars(sink, SeqOneByteString::cast(source).GetChars(no_gc) + from,
+        CopyChars(
+            sink,
+            SeqOneByteString::cast(source).GetChars(no_gc, access_guard) + from,
             to - from);
         return;
       }
       case kTwoByteStringTag | kSeqStringTag: {
-        CopyChars(sink, SeqTwoByteString::cast(source).GetChars(no_gc) + from,
+        CopyChars(
+            sink,
+            SeqTwoByteString::cast(source).GetChars(no_gc, access_guard) + from,
             to - from);
         return;
       }
@@ -699,8 +701,9 @@ void String::WriteToFlat(String src, sinkchar* sink, int f, int t) {
             if (to - boundary == 1) {
               sink[boundary - from] = static_cast<sinkchar>(second.Get(0));
             } else if (second.IsSeqOneByteString()) {
-              CopyChars(sink + boundary - from,
-                        SeqOneByteString::cast(second).GetChars(no_gc),
+              CopyChars(
+                  sink + boundary - from,
+                  SeqOneByteString::cast(second).GetChars(no_gc, access_guard),
                   to - boundary);
             } else {
               WriteToFlat(second, sink + boundary - from, 0, to - boundary);

src/objects/string.h

@@ -21,6 +21,8 @@
 namespace v8 {
 namespace internal {
 
+class SharedStringAccessGuardIfNeeded;
+
 enum InstanceType : uint16_t;
 
 enum AllowNullsFlag { ALLOW_NULLS, DISALLOW_NULLS };
@@ -174,10 +176,18 @@ class String : public TorqueGeneratedString<String, Name> {
   V8_INLINE Vector<const Char> GetCharVector(
       const DisallowHeapAllocation& no_gc);
 
-  // Get chars from sequential or external strings.
+  // Get chars from sequential or external strings. May only be called when a
+  // SharedStringAccessGuard is not needed (i.e. on the main thread or on
+  // read-only strings).
   template <typename Char>
   inline const Char* GetChars(const DisallowHeapAllocation& no_gc);
 
+  // Get chars from sequential or external strings.
+  template <typename Char>
+  inline const Char* GetChars(
+      const DisallowHeapAllocation& no_gc,
+      const SharedStringAccessGuardIfNeeded& access_guard);
+
   // Returns the address of the character at an offset into this string.
   // Requires: this->IsFlat()
   const byte* AddressOfCharacterAt(int start_index,
@@ -575,8 +585,15 @@ class SeqOneByteString
   // Get the address of the characters in this string.
   inline Address GetCharsAddress();
 
+  // Get a pointer to the characters of the string. May only be called when a
+  // SharedStringAccessGuard is not needed (i.e. on the main thread or on
+  // read-only strings).
   inline uint8_t* GetChars(const DisallowHeapAllocation& no_gc);
 
+  // Get a pointer to the characters of the string.
+  inline uint8_t* GetChars(const DisallowHeapAllocation& no_gc,
+                           const SharedStringAccessGuardIfNeeded& access_guard);
+
   // Clear uninitialized padding space. This ensures that the snapshot content
   // is deterministic.
   void clear_padding();
@@ -613,8 +630,15 @@ class SeqTwoByteString
   // Get the address of the characters in this string.
   inline Address GetCharsAddress();
 
+  // Get a pointer to the characters of the string. May only be called when a
+  // SharedStringAccessGuard is not needed (i.e. on the main thread or on
+  // read-only strings).
   inline uc16* GetChars(const DisallowHeapAllocation& no_gc);
 
+  // Get a pointer to the characters of the string.
+  inline uc16* GetChars(const DisallowHeapAllocation& no_gc,
+                        const SharedStringAccessGuardIfNeeded& access_guard);
+
   // Clear uninitialized padding space. This ensures that the snapshot content
   // is deterministic.
   void clear_padding();

test/inspector/isolate-data.h

@@ -38,6 +38,13 @@ class IsolateData : public v8_inspector::V8InspectorClient {
               v8::StartupData* startup_data, WithInspector with_inspector);
   static IsolateData* FromContext(v8::Local<v8::Context> context);
 
+  ~IsolateData() override {
+    // Enter the isolate before destructing this IsolateData, so that
+    // destructors that run before the Isolate's destructor still see it as
+    // entered.
+    isolate()->Enter();
+  }
+
   v8::Isolate* isolate() const { return isolate_.get(); }
   TaskRunner* task_runner() const { return task_runner_; }
 
@@ -128,7 +135,11 @@ class IsolateData : public v8_inspector::V8InspectorClient {
   // call {Dispose}. We have to use the unique_ptr so that the isolate get
   // disposed in the right order, relative to other member variables.
   struct IsolateDeleter {
-    void operator()(v8::Isolate* isolate) const { isolate->Dispose(); }
+    void operator()(v8::Isolate* isolate) const {
+      // Exit the isolate after it was entered by ~IsolateData.
+      isolate->Exit();
+      isolate->Dispose();
+    }
   };
 
   TaskRunner* task_runner_;