[string] Ensure ThinString's don't have a forwarding index

With the flag --always-use-forwarding-table we could end up turning a
String into a ThinString that had a forwarding index set.
This could happen when a String with a forwarding index is externalized.

Bug: chromium:1337469
Change-Id: Iea05586f61e2b78d83d04d5d2e94c4dca2892c1f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3735164Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Commit-Queue: Patrick Thier <pthier@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81660}

src/objects/string-table.cc

@@ -435,14 +435,17 @@ void SetInternalizedReference(Isolate* isolate, String string,
   // TODO(v8:12007): Support external strings.
   DCHECK(!string.IsThinString());
   DCHECK(internalized.IsInternalizedString());
+  DCHECK(!internalized.HasForwardingIndex());
   if ((string.IsShared() || FLAG_always_use_string_forwarding_table) &&
       !string.IsExternalString()) {
-    DCHECK(!internalized.HasForwardingIndex());
     uint32_t field = string.raw_hash_field();
     // Don't use the forwarding table for strings that have an integer index.
     // Using the hash field for the integer index is more beneficial than
     // using it to store the forwarding index to the internalized string.
     if (Name::IsIntegerIndex(field)) return;
+    // Check one last time if we already have a forwarding index to prevent
+    // too many copies of the string in the forwarding table.
+    if (Name::IsForwardingIndex(field)) return;
 
     const int forwarding_index =
         isolate->string_forwarding_table()->Add(isolate, string, internalized);
@@ -451,6 +454,14 @@ void SetInternalizedReference(Isolate* isolate, String string,
                                      String::HashFieldType::kForwardingIndex),
         kReleaseStore);
   } else {
+    if (V8_UNLIKELY(FLAG_always_use_string_forwarding_table)) {
+      // It is possible that the string has a forwarding index (the string was
+      // externalized after it had its forwarding index set). Overwrite the
+      // hash field to avoid having a ThinString with a forwarding index.
+      DCHECK(string.IsExternalString());
+      string.set_raw_hash_field(internalized.raw_hash_field());
+    }
+    DCHECK(!string.HasForwardingIndex());
     string.MakeThin(isolate, internalized);
   }
 }

test/mjsunit/string-forwarding-table.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --always-use-string-forwarding-table
+// Flags: --always-use-string-forwarding-table --expose-externalize-string
 
 // The main purpose of this test is to make ClusterFuzz aware of the flag and
 // provide some interesting input.
@@ -42,4 +42,18 @@ const integer_index = long_key.substring(3,8);
   assertEquals('substr_value', obj[substr_key]);
   assertEquals('long_key_value', obj[consstr_key]);
   assertEquals('integer_index', obj[integer_index]);
+
+  // Externalize keys.
+  // Externalization might fail in various stress modes (the strings might be
+  // externalized already).
+  try {
+    externalizeString(long_key);
+    externalizeString(substr_key);
+    externalizeString(consstr_key);
+    externalizeString(integer_index);
+  } catch {}
+  assertEquals('long_key_value', obj[long_key]);
+  assertEquals('substr_value', obj[substr_key]);
+  assertEquals('long_key_value', obj[consstr_key]);
+  assertEquals('integer_index', obj[integer_index]);
 }