[object] Make the interaction [String::Get()-access guard] explicit

We have recursive calls such ThinStrings where we go String::Get into
ThinString::Get into String::Get again for the internalized string. If
we need to, we would acquire the StringAccessGuard in the first
String::Get and it wouldn't be needed to be re-acquired for the second
String::Get. Trying to re-acquire it would in fact be an error since we
are already holding the lock.

The code, however, didn't know if we acquired it or not. It was working
correctly due to the way the methods were defined and called. By passing
down the access guard through the Get() calls we make this interaction
explicit.

Also add some thin string tests to test the interaction.

Bug: v8:7790
Change-Id: I1181edec1e802cb754c4d1d1ac268577257b92f3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2936598
Commit-Queue: Santiago Aboy Solanes <solanes@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74984}

src/objects/string-inl.h

@@ -620,33 +620,44 @@ Handle<String> String::Flatten(LocalIsolate* isolate, Handle<String> string,
 }
 
 uint16_t String::Get(int index, Isolate* isolate) const {
-  DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(*this));
-  return GetImpl(index);
+  SharedStringAccessGuardIfNeeded scope(isolate);
+  return GetImpl(index, scope);
 }
 
 uint16_t String::Get(int index, LocalIsolate* local_isolate) const {
   SharedStringAccessGuardIfNeeded scope(local_isolate);
-  return GetImpl(index);
+  return GetImpl(index, scope);
+}
+
+uint16_t String::Get(
+    int index, const SharedStringAccessGuardIfNeeded& access_guard) const {
+  return GetImpl(index, access_guard);
 }
 
-uint16_t String::GetImpl(int index) const {
+uint16_t String::GetImpl(
+    int index, const SharedStringAccessGuardIfNeeded& access_guard) const {
   DCHECK(index >= 0 && index < length());
 
   class StringGetDispatcher : public AllStatic {
    public:
 #define DEFINE_METHOD(Type)                                  \
-  static inline uint16_t Handle##Type(Type str, int index) { \
-    return str.Get(index);                                   \
+  static inline uint16_t Handle##Type(                       \
+      Type str, int index,                                   \
+      const SharedStringAccessGuardIfNeeded& access_guard) { \
+    return str.Get(index, access_guard);                     \
   }
     STRING_CLASS_TYPES(DEFINE_METHOD)
 #undef DEFINE_METHOD
-    static inline uint16_t HandleInvalidString(String str, int index) {
+    static inline uint16_t HandleInvalidString(
+        String str, int index,
+        const SharedStringAccessGuardIfNeeded& access_guard) {
       UNREACHABLE();
     }
   };
 
   return StringShape(*this)
-      .DispatchToSpecificType<StringGetDispatcher, uint16_t>(*this, index);
+      .DispatchToSpecificType<StringGetDispatcher, uint16_t>(*this, index,
+                                                             access_guard);
 }
 
 void String::Set(int index, uint16_t value) {
@@ -768,6 +779,13 @@ uint32_t String::ToValidIndex(Object number) {
 }
 
 uint8_t SeqOneByteString::Get(int index) const {
+  DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(*this));
+  return Get(index, SharedStringAccessGuardIfNeeded::NotNeeded());
+}
+
+uint8_t SeqOneByteString::Get(
+    int index, const SharedStringAccessGuardIfNeeded& access_guard) const {
+  USE(access_guard);
   DCHECK(index >= 0 && index < length());
   return ReadField<byte>(kHeaderSize + index * kCharSize);
 }
@@ -814,7 +832,9 @@ uc16* SeqTwoByteString::GetChars(
   return reinterpret_cast<uc16*>(GetCharsAddress());
 }
 
-uint16_t SeqTwoByteString::Get(int index) const {
+uint16_t SeqTwoByteString::Get(
+    int index, const SharedStringAccessGuardIfNeeded& access_guard) const {
+  USE(access_guard);
   DCHECK(index >= 0 && index < length());
   return ReadField<uint16_t>(kHeaderSize + index * kShortSize);
 }
@@ -964,7 +984,9 @@ const uint8_t* ExternalOneByteString::GetChars() const {
   return reinterpret_cast<const uint8_t*>(resource()->data());
 }
 
-uint8_t ExternalOneByteString::Get(int index) const {
+uint8_t ExternalOneByteString::Get(
+    int index, const SharedStringAccessGuardIfNeeded& access_guard) const {
+  USE(access_guard);
   DCHECK(index >= 0 && index < length());
   return GetChars()[index];
 }
@@ -1029,7 +1051,9 @@ const uint16_t* ExternalTwoByteString::GetChars() const {
   return resource()->data();
 }
 
-uint16_t ExternalTwoByteString::Get(int index) const {
+uint16_t ExternalTwoByteString::Get(
+    int index, const SharedStringAccessGuardIfNeeded& access_guard) const {
+  USE(access_guard);
   DCHECK(index >= 0 && index < length());
   return GetChars()[index];
 }

src/objects/string.cc

@@ -1442,7 +1442,8 @@ void SeqTwoByteString::clear_padding() {
          SizeFor(length()) - data_size);
 }
 
-uint16_t ConsString::Get(int index) const {
+uint16_t ConsString::Get(
+    int index, const SharedStringAccessGuardIfNeeded& access_guard) const {
   DCHECK(index >= 0 && index < this->length());
 
   // Check for a flattened cons string
@@ -1464,17 +1465,21 @@ uint16_t ConsString::Get(int index) const {
         string = cons_string.second();
       }
     } else {
-      return string.Get(index);
+      return string.Get(index, access_guard);
     }
   }
 
   UNREACHABLE();
 }
 
-uint16_t ThinString::Get(int index) const { return actual().Get(index); }
+uint16_t ThinString::Get(
+    int index, const SharedStringAccessGuardIfNeeded& access_guard) const {
+  return actual().Get(index, access_guard);
+}
 
-uint16_t SlicedString::Get(int index) const {
-  return parent().Get(offset() + index);
+uint16_t SlicedString::Get(
+    int index, const SharedStringAccessGuardIfNeeded& access_guard) const {
+  return parent().Get(offset() + index, access_guard);
 }
 
 int ExternalString::ExternalPayloadSize() const {

src/objects/string.h

@@ -220,6 +220,11 @@ class String : public TorqueGeneratedString<String, Name> {
   // be used.
   V8_INLINE uint16_t Get(int index, Isolate* isolate = nullptr) const;
   V8_INLINE uint16_t Get(int index, LocalIsolate* local_isolate) const;
+  // Method to pass down the access_guard. Useful for recursive calls such as
+  // ThinStrings where we go String::Get into ThinString::Get into String::Get
+  // again for the internalized string.
+  V8_INLINE uint16_t
+  Get(int index, const SharedStringAccessGuardIfNeeded& access_guard) const;
 
   // ES6 section 7.1.3.1 ToNumber Applied to the String Type
   static Handle<Object> ToNumber(Isolate* isolate, Handle<String> subject);
@@ -542,7 +547,8 @@ class String : public TorqueGeneratedString<String, Name> {
   friend class InternalizedStringKey;
 
   // Implementation of the Get() public methods. Do not use directly.
-  V8_INLINE uint16_t GetImpl(int index) const;
+  V8_INLINE uint16_t
+  GetImpl(int index, const SharedStringAccessGuardIfNeeded& access_guard) const;
 
   // Implementation of the IsEqualTo() public methods. Do not use directly.
   template <EqualityType kEqType, typename Char>
@@ -633,8 +639,12 @@ class SeqOneByteString
   static const bool kHasOneByteEncoding = true;
   using Char = uint8_t;
 
-  // Dispatched behavior.
+  // Dispatched behavior. The non SharedStringAccessGuardIfNeeded method is also
+  // defined for convenience and it will check that the access guard is not
+  // needed.
   inline uint8_t Get(int index) const;
+  inline uint8_t Get(int index,
+                     const SharedStringAccessGuardIfNeeded& access_guard) const;
   inline void SeqOneByteStringSet(int index, uint16_t value);
 
   // Get the address of the characters in this string.
@@ -680,7 +690,8 @@ class SeqTwoByteString
   using Char = uint16_t;
 
   // Dispatched behavior.
-  inline uint16_t Get(int index) const;
+  inline uint16_t Get(
+      int index, const SharedStringAccessGuardIfNeeded& access_guard) const;
   inline void SeqTwoByteStringSet(int index, uint16_t value);
 
   // Get the address of the characters in this string.
@@ -737,7 +748,8 @@ class ConsString : public TorqueGeneratedConsString<ConsString, String> {
   inline Object unchecked_second();
 
   // Dispatched behavior.
-  V8_EXPORT_PRIVATE uint16_t Get(int index) const;
+  V8_EXPORT_PRIVATE uint16_t
+  Get(int index, const SharedStringAccessGuardIfNeeded& access_guard) const;
 
   // Minimum length for a cons string.
   static const int kMinLength = 13;
@@ -760,7 +772,8 @@ class ThinString : public TorqueGeneratedThinString<ThinString, String> {
  public:
   DECL_GETTER(unchecked_actual, HeapObject)
 
-  V8_EXPORT_PRIVATE uint16_t Get(int index) const;
+  V8_EXPORT_PRIVATE uint16_t
+  Get(int index, const SharedStringAccessGuardIfNeeded& access_guard) const;
 
   DECL_VERIFIER(ThinString)
 
@@ -786,7 +799,8 @@ class SlicedString : public TorqueGeneratedSlicedString<SlicedString, String> {
   inline void set_parent(String parent,
                          WriteBarrierMode mode = UPDATE_WRITE_BARRIER);
   // Dispatched behavior.
-  V8_EXPORT_PRIVATE uint16_t Get(int index) const;
+  V8_EXPORT_PRIVATE uint16_t
+  Get(int index, const SharedStringAccessGuardIfNeeded& access_guard) const;
 
   // Minimum length for a sliced string.
   static const int kMinLength = 13;
@@ -868,7 +882,8 @@ class ExternalOneByteString : public ExternalString {
   inline const uint8_t* GetChars() const;
 
   // Dispatched behavior.
-  inline uint8_t Get(int index) const;
+  inline uint8_t Get(int index,
+                     const SharedStringAccessGuardIfNeeded& access_guard) const;
 
   DECL_CAST(ExternalOneByteString)
 
@@ -914,7 +929,8 @@ class ExternalTwoByteString : public ExternalString {
   inline const uint16_t* GetChars() const;
 
   // Dispatched behavior.
-  inline uint16_t Get(int index) const;
+  inline uint16_t Get(
+      int index, const SharedStringAccessGuardIfNeeded& access_guard) const;
 
   // For regexp code.
   inline const uint16_t* ExternalTwoByteStringGetData(unsigned start);

test/cctest/test-concurrent-string.cc

@@ -236,6 +236,191 @@ TEST(InspectTwoByteExternalizing) {
   thread->Join();
 }
 
+// Inspect a one byte string, while the main thread externalizes it. Same as
+// InspectOneByteExternalizing, but using thin strings.
+TEST(InspectOneByteExternalizing_ThinString) {
+  if (!FLAG_thin_strings) return;
+
+  CcTest::InitializeVM();
+  Isolate* isolate = CcTest::i_isolate();
+
+  std::unique_ptr<PersistentHandles> ph = isolate->NewPersistentHandles();
+
+  auto factory = isolate->factory();
+  HandleScope handle_scope(isolate);
+
+  // Create a string.
+  const char* raw_string = STRING_VALUE;
+  Handle<String> thin_string = factory->NewStringFromAsciiChecked(raw_string);
+  CHECK(thin_string->IsOneByteRepresentation());
+  CHECK(!thin_string->IsExternalString());
+  CHECK(!thin_string->IsInternalizedString());
+
+  // Crate an internalized one-byte version of that string string.
+  Handle<String> internalized_string = factory->InternalizeString(thin_string);
+  CHECK(internalized_string->IsOneByteRepresentation());
+  CHECK(!internalized_string->IsExternalString());
+  CHECK(internalized_string->IsInternalizedString());
+
+  // We now should have an internalized string, and a thin string pointing to
+  // it.
+  CHECK(thin_string->IsThinString());
+  CHECK_NE(*thin_string, *internalized_string);
+
+  Handle<String> persistent_string = ph->NewHandle(thin_string);
+
+  std::vector<uint16_t> chars;
+  for (int i = 0; i < thin_string->length(); ++i) {
+    chars.push_back(thin_string->Get(i));
+  }
+
+  base::Semaphore sema_started(0);
+
+  std::unique_ptr<ConcurrentStringThread> thread(new ConcurrentStringThread(
+      isolate, persistent_string, std::move(ph), &sema_started, chars));
+  CHECK(thread->Start());
+
+  sema_started.Wait();
+
+  // Externalize it to a one-byte external string.
+  // We need to use StrDup in this case since the TestOneByteResource will get
+  // ownership of raw_string otherwise.
+  CHECK(internalized_string->MakeExternal(
+      new TestOneByteResource(i::StrDup(raw_string))));
+  CHECK(internalized_string->IsExternalOneByteString());
+  CHECK(internalized_string->IsInternalizedString());
+
+  // Check that the thin string is unmodified.
+  CHECK(!thin_string->IsExternalString());
+  CHECK(!thin_string->IsInternalizedString());
+  CHECK(thin_string->IsThinString());
+
+  thread->Join();
+}
+
+// Inspect a one byte string, while the main thread externalizes it into a two
+// bytes string. Same as InspectOneIntoTwoByteExternalizing, but using thin
+// strings.
+TEST(InspectOneIntoTwoByteExternalizing_ThinString) {
+  if (!FLAG_thin_strings) return;
+  CcTest::InitializeVM();
+  Isolate* isolate = CcTest::i_isolate();
+
+  std::unique_ptr<PersistentHandles> ph = isolate->NewPersistentHandles();
+
+  auto factory = isolate->factory();
+  HandleScope handle_scope(isolate);
+
+  // Create a string.
+  const char* raw_string = STRING_VALUE;
+  Handle<String> thin_string = factory->NewStringFromAsciiChecked(raw_string);
+  CHECK(thin_string->IsOneByteRepresentation());
+  CHECK(!thin_string->IsExternalString());
+  CHECK(!thin_string->IsInternalizedString());
+
+  // Crate an internalized one-byte version of that string string.
+  Handle<String> internalized_string = factory->InternalizeString(thin_string);
+  CHECK(internalized_string->IsOneByteRepresentation());
+  CHECK(!internalized_string->IsExternalString());
+  CHECK(internalized_string->IsInternalizedString());
+
+  // We now should have an internalized string, and a thin string pointing to
+  // it.
+  CHECK(thin_string->IsThinString());
+  CHECK_NE(*thin_string, *internalized_string);
+
+  Handle<String> persistent_string = ph->NewHandle(thin_string);
+  std::vector<uint16_t> chars;
+  for (int i = 0; i < thin_string->length(); ++i) {
+    chars.push_back(thin_string->Get(i));
+  }
+
+  base::Semaphore sema_started(0);
+
+  std::unique_ptr<ConcurrentStringThread> thread(new ConcurrentStringThread(
+      isolate, persistent_string, std::move(ph), &sema_started, chars));
+  CHECK(thread->Start());
+
+  sema_started.Wait();
+
+  // Externalize it to a two-bytes external string. AsciiToTwoByteString does
+  // the string duplication for us.
+  CHECK(internalized_string->MakeExternal(
+      new TestTwoByteResource(AsciiToTwoByteString(raw_string))));
+  CHECK(internalized_string->IsExternalTwoByteString());
+  CHECK(internalized_string->IsInternalizedString());
+
+  // Check that the thin string is unmodified.
+  CHECK(!thin_string->IsExternalString());
+  CHECK(!thin_string->IsInternalizedString());
+  CHECK(thin_string->IsThinString());
+  // Even its representation is still one byte, even when the internalized
+  // string moved to two bytes.
+  CHECK(thin_string->IsOneByteRepresentation());
+
+  thread->Join();
+}
+
+// Inspect a two byte string, while the main thread externalizes it. Same as
+// InspectTwoByteExternalizing, but using thin strings.
+TEST(InspectTwoByteExternalizing_ThinString) {
+  if (!FLAG_thin_strings) return;
+
+  CcTest::InitializeVM();
+  Isolate* isolate = CcTest::i_isolate();
+
+  std::unique_ptr<PersistentHandles> ph = isolate->NewPersistentHandles();
+
+  auto factory = isolate->factory();
+  HandleScope handle_scope(isolate);
+
+  // Crate an internalized two-byte string.
+  // TODO(solanes): Can we have only one raw string?
+  const char* raw_string = STRING_VALUE;
+  // TODO(solanes): Is this the best way to create a two byte string from chars?
+  const int kLength = 12;
+  const uint16_t two_byte_array[kLength] = ARRAY_VALUE;
+  Handle<String> thin_string;
+  {
+    Handle<SeqTwoByteString> raw =
+        factory->NewRawTwoByteString(kLength).ToHandleChecked();
+    DisallowGarbageCollection no_gc;
+    CopyChars(raw->GetChars(no_gc), two_byte_array, kLength);
+    thin_string = raw;
+  }
+
+  Handle<String> internalized_string = factory->InternalizeString(thin_string);
+  CHECK(internalized_string->IsTwoByteRepresentation());
+  CHECK(!internalized_string->IsExternalString());
+  CHECK(internalized_string->IsInternalizedString());
+
+  Handle<String> persistent_string = ph->NewHandle(thin_string);
+  std::vector<uint16_t> chars;
+  for (int i = 0; i < thin_string->length(); ++i) {
+    chars.push_back(thin_string->Get(i));
+  }
+  base::Semaphore sema_started(0);
+
+  std::unique_ptr<ConcurrentStringThread> thread(new ConcurrentStringThread(
+      isolate, persistent_string, std::move(ph), &sema_started, chars));
+  CHECK(thread->Start());
+
+  sema_started.Wait();
+
+  // Externalize it to a two-bytes external string.
+  CHECK(internalized_string->MakeExternal(
+      new TestTwoByteResource(AsciiToTwoByteString(raw_string))));
+  CHECK(internalized_string->IsExternalTwoByteString());
+  CHECK(internalized_string->IsInternalizedString());
+
+  // Check that the thin string is unmodified.
+  CHECK(!thin_string->IsExternalString());
+  CHECK(!thin_string->IsInternalizedString());
+  CHECK(thin_string->IsThinString());
+
+  thread->Join();
+}
+
 }  // anonymous namespace
 
 }  // namespace internal