[string] Don't overwrite original string in InternalizedStringKey

When internalizing external strings, a new internalized external string object is allocated if the string is not in-place internalizable. This newly allocated strings external resource is set to null (the actual resource will be transferred by MakeThin to ensure unique ownership of the resource).

We need to preserve the original string in the InternalizedStringKey for
the second lookup (inside the critical section), as we need to access
the external resource in case of hash collisions to check for equality.

Bug: chromium:1402187
Change-Id: I62b637859b06f05d1b34cb26495f08ec44d2f2db
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4128089
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Patrick Thier <pthier@chromium.org>
Cr-Commit-Position: refs/heads/main@{#85057}

src/objects/string-table.cc

@@ -375,6 +375,7 @@ class InternalizedStringKey final : public StringTableKey {
         // We can see already internalized strings here only when sharing the
         // string table and allowing concurrent internalization.
         DCHECK(v8_flags.shared_string_table);
+        internalized_string_ = string_;
         return;
     }
 
@@ -396,7 +397,7 @@ class InternalizedStringKey final : public StringTableKey {
       // original string is not transitioned to a ThinString (setting the
       // resource) immediately.
       DCHECK(!shape.IsShared());
-      string_ =
+      internalized_string_ =
           isolate->factory()->InternalizeExternalString<ExternalOneByteString>(
               string_);
     } else if (can_avoid_copy && shape.IsExternalTwoByte()) {
@@ -406,13 +407,13 @@ class InternalizedStringKey final : public StringTableKey {
       // original string is not transitioned to a ThinString (setting the
       // resource) immediately.
       DCHECK(!shape.IsShared());
-      string_ =
+      internalized_string_ =
           isolate->factory()->InternalizeExternalString<ExternalTwoByteString>(
               string_);
     } else {
       // Otherwise allocate a new internalized string.
-      string_ = isolate->factory()->NewInternalizedStringImpl(string_, length(),
-                                                              raw_hash_field());
+      internalized_string_ = isolate->factory()->NewInternalizedStringImpl(
+          string_, length(), raw_hash_field());
     }
   }
 
@@ -435,11 +436,17 @@ class InternalizedStringKey final : public StringTableKey {
     // in-place migrate the original string instead of internalizing the copy
     // and migrating the original string to a ThinString. This scenario doesn't
     // seem to be common enough to justify re-computing the strategy here.
-    return string_;
+    return internalized_string_.ToHandleChecked();
   }
 
  private:
   Handle<String> string_;
+  // Copy of the string to be internalized (only set if the string is not
+  // in-place internalizable). We can't override the original string, as
+  // internalized external strings don't set the resource directly (deferred to
+  // MakeThin to ensure unique ownership of the resource), and thus would break
+  // equality checks in case of hash collisions.
+  MaybeHandle<String> internalized_string_;
   MaybeHandle<Map> maybe_internalized_map_;
 };
 

test/cctest/test-strings.cc

@@ -1393,6 +1393,47 @@ TEST(InternalizeExternal) {
   CcTest::CollectGarbage(i::OLD_SPACE);
 }
 
+TEST(Regress1402187) {
+  CcTest::InitializeVM();
+  i::Isolate* isolate = CcTest::i_isolate();
+  Factory* factory = isolate->factory();
+  // This won't leak; the external string mechanism will call Dispose() on it.
+  const char ext_string_content[] = "prop-1234567";
+  OneByteVectorResource* resource = new OneByteVectorResource(
+      v8::base::Vector<const char>(ext_string_content, 12));
+  const uint32_t fake_hash =
+      String::CreateHashFieldValue(4711, String::HashFieldType::kHash);
+  {
+    v8::HandleScope scope(CcTest::isolate());
+    // Internalize a string with the same hash to ensure collision.
+    Handle<String> intern = isolate->factory()->NewStringFromAsciiChecked(
+        "internalized", AllocationType::kOld);
+    intern->set_raw_hash_field(fake_hash);
+    factory->InternalizeName(intern);
+    CHECK(intern->IsInternalizedString());
+
+    v8::Local<v8::String> ext_string =
+        v8::String::NewFromUtf8Literal(CcTest::isolate(), ext_string_content);
+    ext_string->MakeExternal(resource);
+    Handle<String> string = v8::Utils::OpenHandle(*ext_string);
+    string->set_raw_hash_field(fake_hash);
+    CHECK(string->IsExternalString());
+    CHECK(!string->IsInternalizedString());
+    CHECK(!String::Equals(isolate, string, intern));
+    CHECK_EQ(string->hash(), intern->hash());
+    CHECK_EQ(string->length(), intern->length());
+
+    CHECK_EQ(isolate->string_table()->TryStringToIndexOrLookupExisting(
+                 isolate, string->ptr()),
+             Smi::FromInt(ResultSentinel::kNotFound).ptr());
+    string = factory->InternalizeString(string);
+    CHECK(string->IsExternalString());
+    CHECK(string->IsInternalizedString());
+  }
+  CcTest::CollectGarbage(i::OLD_SPACE);
+  CcTest::CollectGarbage(i::OLD_SPACE);
+}
+
 TEST(SliceFromExternal) {
   if (!v8_flags.string_slices) return;
   CcTest::InitializeVM();