[object] Add string reader lock to StringCharacterStream

It acquires the string lock to avoid race conditions. It does so in a
slow way (by getting the isolate from the string) to avoid piping the
Isolate through.

Bug: v8:7790, chromium:1166095
Change-Id: I8b769b4e96ee780314359d1d15d712012aade88a
Fix: chromium:1166095
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2637861Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Commit-Queue: Santiago Aboy Solanes <solanes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72236}

src/objects/string-inl.h

@@ -42,23 +42,19 @@ class V8_NODISCARD SharedStringAccessGuardIfNeeded {
     }
   }
 
+  // Slow version which gets the isolate from the String.
+  explicit SharedStringAccessGuardIfNeeded(String str) {
+    Isolate* isolate = GetIsolateIfNeeded(str);
+    if (isolate != nullptr) mutex_guard.emplace(isolate->string_access());
+  }
+
   static SharedStringAccessGuardIfNeeded NotNeeded() {
     return SharedStringAccessGuardIfNeeded();
   }
 
 #ifdef DEBUG
   static bool IsNeeded(String str) {
-    LocalHeap* local_heap = LocalHeap::Current();
-    // Don't acquire the lock for the main thread.
-    if (!local_heap || local_heap->is_main_thread()) return false;
-
-    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;
-    }
-    return true;
+    return GetIsolateIfNeeded(str) != nullptr;
   }
 #endif
 
@@ -76,6 +72,21 @@ class V8_NODISCARD SharedStringAccessGuardIfNeeded {
     DCHECK(!mutex_guard.has_value());
   }
 
+  // Returns the Isolate from the String if we need it for the lock.
+  static Isolate* GetIsolateIfNeeded(String str) {
+    LocalHeap* local_heap = LocalHeap::Current();
+    // Don't acquire the lock for the main thread.
+    if (!local_heap || local_heap->is_main_thread()) return 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 nullptr;
+    }
+    return isolate;
+  }
+
   base::Optional<base::SharedMutexGuard<base::kShared>> mutex_guard;
 };
 
@@ -628,6 +639,15 @@ String String::GetUnderlying() {
 template <class Visitor>
 ConsString String::VisitFlat(Visitor* visitor, String string,
                              const int offset) {
+  DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(string));
+  return VisitFlat(visitor, string, offset,
+                   SharedStringAccessGuardIfNeeded::NotNeeded());
+}
+
+template <class Visitor>
+ConsString String::VisitFlat(
+    Visitor* visitor, String string, const int offset,
+    const SharedStringAccessGuardIfNeeded& access_guard) {
   DisallowGarbageCollection no_gc;
   int slice_offset = offset;
   const int length = string.length();
@@ -637,13 +657,15 @@ ConsString String::VisitFlat(Visitor* visitor, String string,
     switch (type & (kStringRepresentationMask | kStringEncodingMask)) {
       case kSeqStringTag | kOneByteStringTag:
         visitor->VisitOneByteString(
-            SeqOneByteString::cast(string).GetChars(no_gc) + slice_offset,
+            SeqOneByteString::cast(string).GetChars(no_gc, access_guard) +
+                slice_offset,
             length - offset);
         return ConsString();
 
       case kSeqStringTag | kTwoByteStringTag:
         visitor->VisitTwoByteString(
-            SeqTwoByteString::cast(string).GetChars(no_gc) + slice_offset,
+            SeqTwoByteString::cast(string).GetChars(no_gc, access_guard) +
+                slice_offset,
             length - offset);
         return ConsString();
 
@@ -946,6 +968,28 @@ void ConsStringIterator::Pop() {
   depth_--;
 }
 
+class StringCharacterStream {
+ public:
+  inline explicit StringCharacterStream(String string, int offset = 0);
+  StringCharacterStream(const StringCharacterStream&) = delete;
+  StringCharacterStream& operator=(const StringCharacterStream&) = delete;
+  inline uint16_t GetNext();
+  inline bool HasMore();
+  inline void Reset(String string, int offset = 0);
+  inline void VisitOneByteString(const uint8_t* chars, int length);
+  inline void VisitTwoByteString(const uint16_t* chars, int length);
+
+ private:
+  ConsStringIterator iter_;
+  bool is_one_byte_;
+  union {
+    const uint8_t* buffer8_;
+    const uint16_t* buffer16_;
+  };
+  const uint8_t* end_;
+  SharedStringAccessGuardIfNeeded access_guard_;
+};
+
 uint16_t StringCharacterStream::GetNext() {
   DCHECK(buffer8_ != nullptr && end_ != nullptr);
   // Advance cursor if needed.
@@ -954,19 +998,25 @@ uint16_t StringCharacterStream::GetNext() {
   return is_one_byte_ ? *buffer8_++ : *buffer16_++;
 }
 
+// TODO(solanes, v8:7790, chromium:1166095): Assess if we need to use
+// Isolate/LocalIsolate and pipe them through, instead of using the slow
+// version of the SharedStringAccessGuardIfNeeded.
 StringCharacterStream::StringCharacterStream(String string, int offset)
-    : is_one_byte_(false) {
+    : is_one_byte_(false), access_guard_(string) {
   Reset(string, offset);
 }
 
 void StringCharacterStream::Reset(String string, int offset) {
   buffer8_ = nullptr;
   end_ = nullptr;
-  ConsString cons_string = String::VisitFlat(this, string, offset);
+
+  ConsString cons_string =
+      String::VisitFlat(this, string, offset, access_guard_);
   iter_.Reset(cons_string, offset);
   if (!cons_string.is_null()) {
     string = iter_.Next(&offset);
-    if (!string.is_null()) String::VisitFlat(this, string, offset);
+    if (!string.is_null())
+      String::VisitFlat(this, string, offset, access_guard_);
   }
 }
 
@@ -976,7 +1026,7 @@ bool StringCharacterStream::HasMore() {
   String string = iter_.Next(&offset);
   DCHECK_EQ(offset, 0);
   if (string.is_null()) return false;
-  String::VisitFlat(this, string);
+  String::VisitFlat(this, string, 0, access_guard_);
   DCHECK(buffer8_ != end_);
   return true;
 }

src/objects/string.h

@@ -512,10 +512,17 @@ class String : public TorqueGeneratedString<String, Name> {
     return NonOneByteStart(chars, length) >= length;
   }
 
+  // May only be called when a SharedStringAccessGuard is not needed (i.e. on
+  // the main thread or on read-only strings).
   template <class Visitor>
   static inline ConsString VisitFlat(Visitor* visitor, String string,
                                      int offset = 0);
 
+  template <class Visitor>
+  static inline ConsString VisitFlat(
+      Visitor* visitor, String string, int offset,
+      const SharedStringAccessGuardIfNeeded& access_guard);
+
   template <typename LocalIsolate>
   static Handle<FixedArray> CalculateLineEnds(LocalIsolate* isolate,
                                               Handle<String> string,
@@ -975,26 +982,7 @@ class ConsStringIterator {
   int consumed_;
 };
 
-class StringCharacterStream {
- public:
-  inline explicit StringCharacterStream(String string, int offset = 0);
-  StringCharacterStream(const StringCharacterStream&) = delete;
-  StringCharacterStream& operator=(const StringCharacterStream&) = delete;
-  inline uint16_t GetNext();
-  inline bool HasMore();
-  inline void Reset(String string, int offset = 0);
-  inline void VisitOneByteString(const uint8_t* chars, int length);
-  inline void VisitTwoByteString(const uint16_t* chars, int length);
-
- private:
-  ConsStringIterator iter_;
-  bool is_one_byte_;
-  union {
-    const uint8_t* buffer8_;
-    const uint16_t* buffer16_;
-  };
-  const uint8_t* end_;
-};
+class StringCharacterStream;
 
 template <typename Char>
 struct CharTraits;

test/mjsunit/regress/regress-crbug-1166095.js

+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --trace-turbo-reduction
+
+function foo() {
+  const v11 = new Int8Array(150);
+  Object(v11,...v11,v11);
+}
+
+for (i = 0; i < 100; i++)
+  foo();