[compiler] Add MRSW lock for String externalization

Since we have synchronized versions of map and length we can use those.
For reading the contents, however, we have to add a lock around
externalizing a string.

Bug: v8:7790
Change-Id: Iedcb6d9b865d80fbe6d8aec5dd677943ab9ac1d0
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2497179Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Commit-Queue: Santiago Aboy Solanes <solanes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71010}

src/execution/isolate.h

@@ -626,6 +626,14 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
   // Mutex for serializing access to break control structures.
   base::RecursiveMutex* break_access() { return &break_access_; }
 
+  // Shared mutex for allowing concurrent read/writes to FeedbackVectors.
+  base::SharedMutex* feedback_vector_access() {
+    return &feedback_vector_access_;
+  }
+
+  // Shared mutex for allowing concurrent read/writes to Strings.
+  base::SharedMutex* string_access() { return &string_access_; }
+
   // Shared mutex for allowing concurrent read/writes to TransitionArrays.
   base::SharedMutex* transition_array_access() {
     return &transition_array_access_;
@@ -634,11 +642,6 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
   // The isolate's string table.
   StringTable* string_table() { return string_table_.get(); }
 
-  // Shared mutex for allowing concurrent read/writes to FeedbackVectors.
-  base::SharedMutex* feedback_vector_access() {
-    return &feedback_vector_access_;
-  }
-
   Address get_address_from_id(IsolateAddressId id);
 
   // Access to top context (where the current function object was created).
@@ -1752,8 +1755,9 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
   CompilationCache* compilation_cache_ = nullptr;
   std::shared_ptr<Counters> async_counters_;
   base::RecursiveMutex break_access_;
-  base::SharedMutex transition_array_access_;
   base::SharedMutex feedback_vector_access_;
+  base::SharedMutex string_access_;
+  base::SharedMutex transition_array_access_;
   Logger* logger_ = nullptr;
   StubCache* load_stub_cache_ = nullptr;
   StubCache* store_stub_cache_ = nullptr;

src/objects/string-inl.h

@@ -26,6 +26,24 @@ namespace internal {
 
 #include "torque-generated/src/objects/string-tq-inl.inc"
 
+// Creates a SharedMutexGuard<kShared> for the string access if:
+// A) {str} is not a read only string, and
+// B) We are on a background thread.
+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());
+  }
+
+ private:
+  base::Optional<base::SharedMutexGuard<base::kShared>> mutex_guard;
+};
+
 int String::synchronized_length() const {
   return base::AsAtomic32::Acquire_Load(
       reinterpret_cast<const int32_t*>(field_address(kLengthOffset)));
@@ -52,7 +70,8 @@ CAST_ACCESSOR(ExternalOneByteString)
 CAST_ACCESSOR(ExternalString)
 CAST_ACCESSOR(ExternalTwoByteString)
 
-StringShape::StringShape(const String str) : type_(str.map().instance_type()) {
+StringShape::StringShape(const String str)
+    : type_(str.synchronized_map().instance_type()) {
   set_valid();
   DCHECK_EQ(type_ & kIsNotStringMask, kStringTag);
 }
@@ -421,6 +440,8 @@ Handle<String> String::Flatten(LocalIsolate* isolate, Handle<String> string,
 uint16_t String::Get(int index) {
   DCHECK(index >= 0 && index < length());
 
+  SharedStringAccessGuardIfNeeded scope(*this);
+
   class StringGetDispatcher : public AllStatic {
    public:
 #define DEFINE_METHOD(Type)                                  \

src/objects/string.cc

@@ -168,6 +168,11 @@ bool String::MakeExternal(v8::String::ExternalStringResource* resource) {
     isolate->heap()->NotifyObjectLayoutChange(*this, no_allocation,
                                               InvalidateRecordedSlots::kYes);
   }
+
+  // Disallow garbage collection to avoid possible GC vs string access deadlock.
+  DisallowGarbageCollection no_gc;
+  base::SharedMutexGuard<base::kExclusive> shared_mutex_guard(
+      isolate->string_access());
   // Morph the string to an external string by replacing the map and
   // reinitializing the fields.  This won't work if the space the existing
   // string occupies is too small for a regular external string.  Instead, we
@@ -240,6 +245,11 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
     isolate->heap()->NotifyObjectLayoutChange(*this, no_allocation,
                                               InvalidateRecordedSlots::kYes);
   }
+
+  // Disallow garbage collection to avoid possible GC vs string access deadlock.
+  DisallowGarbageCollection no_gc;
+  base::SharedMutexGuard<base::kExclusive> shared_mutex_guard(
+      isolate->string_access());
   // Morph the string to an external string by replacing the map and
   // reinitializing the fields.  This won't work if the space the existing
   // string occupies is too small for a regular external string.  Instead, we