[intl] Fix two issues in the localeCompare fast path

1. The default locale may be reset / is not immutable as we thought.
2. A suffix of ignorable code points after the common length affects
   the comparison result.

Bug: v8:12196
Fixed: v8:12398
Change-Id: I6f60f56352956779df801c43de6ebac8cd9c592d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3291314
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Auto-Submit: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78053}

src/objects/intl-objects.cc

@@ -912,10 +912,10 @@ MaybeHandle<String> Intl::StringLocaleConvertCase(Isolate* isolate,
 }
 
 // static
+template <class IsolateT>
 Intl::CompareStringsOptions Intl::CompareStringsOptionsFor(
-    LocalIsolate* local_isolate, Handle<Object> locales,
-    Handle<Object> options) {
-  if (!options->IsUndefined(local_isolate)) {
+    IsolateT* isolate, Handle<Object> locales, Handle<Object> options) {
+  if (!options->IsUndefined(isolate)) {
     return CompareStringsOptions::kNone;
   }
 
@@ -933,31 +933,22 @@ Intl::CompareStringsOptions Intl::CompareStringsOptionsFor(
       "sl",    "sv", "sw", "vi",    "en-DE", "en-GB",
   };
 
-  if (locales->IsUndefined(local_isolate)) {
-    static bool default_is_fast = false;
-
-    // The default locale is immutable after initialization.
-    static base::OnceType once = V8_ONCE_INIT;
-    base::CallOnce(&once, [&]() {
-      const std::string& default_locale = local_isolate->DefaultLocale();
-      for (const char* fast_locale : kFastLocales) {
-        if (strcmp(fast_locale, default_locale.c_str()) == 0) {
-          default_is_fast = true;
-          return;
-        }
+  if (locales->IsUndefined(isolate)) {
+    const std::string& default_locale = isolate->DefaultLocale();
+    for (const char* fast_locale : kFastLocales) {
+      if (strcmp(fast_locale, default_locale.c_str()) == 0) {
+        return CompareStringsOptions::kTryFastPath;
       }
-    });
+    }
 
-    return default_is_fast ? CompareStringsOptions::kTryFastPath
-                           : CompareStringsOptions::kNone;
+    return CompareStringsOptions::kNone;
   }
 
   if (!locales->IsString()) return CompareStringsOptions::kNone;
 
   Handle<String> locales_string = Handle<String>::cast(locales);
   for (const char* fast_locale : kFastLocales) {
-    if (locales_string->IsEqualTo(base::CStrVector(fast_locale),
-                                  local_isolate)) {
+    if (locales_string->IsEqualTo(base::CStrVector(fast_locale), isolate)) {
       return CompareStringsOptions::kTryFastPath;
     }
   }
@@ -965,6 +956,12 @@ Intl::CompareStringsOptions Intl::CompareStringsOptionsFor(
   return CompareStringsOptions::kNone;
 }
 
+// Instantiations.
+template Intl::CompareStringsOptions Intl::CompareStringsOptionsFor(
+    Isolate*, Handle<Object>, Handle<Object>);
+template Intl::CompareStringsOptions Intl::CompareStringsOptionsFor(
+    LocalIsolate*, Handle<Object>, Handle<Object>);
+
 base::Optional<int> Intl::StringLocaleCompare(
     Isolate* isolate, Handle<String> string1, Handle<String> string2,
     Handle<Object> locales, Handle<Object> options, const char* method_name) {
@@ -977,7 +974,7 @@ base::Optional<int> Intl::StringLocaleCompare(
   // We may be able to take the fast path, depending on the `locales` and
   // `options` arguments.
   const CompareStringsOptions compare_strings_options =
-      CompareStringsOptionsFor(isolate->AsLocalIsolate(), locales, options);
+      CompareStringsOptionsFor(isolate, locales, options);
   if (can_cache) {
     // Both locales and options are undefined, check the cache.
     icu::Collator* cached_icu_collator =
@@ -1179,6 +1176,12 @@ bool CharIsAsciiOrOutOfBounds(const String::FlatContent& string,
   return index >= string_length || isascii(string.Get(index));
 }
 
+bool CharCanFastCompareOrOutOfBounds(const String::FlatContent& string,
+                                     int string_length, int index) {
+  DCHECK_EQ(string.length(), string_length);
+  return index >= string_length || CanFastCompare(string.Get(index));
+}
+
 #ifdef DEBUG
 bool USetContainsAllAsciiItem(USet* set) {
   static constexpr int kBufferSize = 64;
@@ -1375,7 +1378,15 @@ base::Optional<UCollationResult> TryFastCompareStrings(
 
   // Strings are L1-equal up to their common length, length differences win.
   UCollationResult length_result = ToUCollationResult(length1 - length2);
-  if (length_result != UCollationResult::UCOL_EQUAL) return length_result;
+  if (length_result != UCollationResult::UCOL_EQUAL) {
+    // Strings of different lengths may still compare as equal if the longer
+    // string has a fully ignored suffix, e.g. "a" vs. "a\u{1}".
+    if (!CharCanFastCompareOrOutOfBounds(flat1, length1, common_length) ||
+        !CharCanFastCompareOrOutOfBounds(flat2, length2, common_length)) {
+      return d.FastCompareFailed(processed_until_out);
+    }
+    return length_result;
+  }
 
   // L1-equal and same length, the L3 result wins.
   return d.l3_result;

src/objects/intl-objects.h

@@ -100,9 +100,9 @@ class Intl {
     kNone,
     kTryFastPath,
   };
+  template <class IsolateT>
   V8_EXPORT_PRIVATE static CompareStringsOptions CompareStringsOptionsFor(
-      LocalIsolate* local_isolate, Handle<Object> locales,
-      Handle<Object> options);
+      IsolateT* isolate, Handle<Object> locales, Handle<Object> options);
   V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static int CompareStrings(
       Isolate* isolate, const icu::Collator& collator, Handle<String> s1,
       Handle<String> s2,

test/intl/string-localecompare.js

@@ -11,8 +11,7 @@ function check() {
     assertTrue("asdff".localeCompare("asdf") > 0);
 
     // Completely Ignorable.
-    // TODO (jgruber): This case is currently wrong in the C++ fast-path.
-    // assertEquals("asdf".localeCompare("asdf\01"), 0);
+    assertEquals("asdf".localeCompare("asdf\01"), 0);
     assertEquals("asdf".localeCompare("as\01df"), 0);
     assertEquals("asdf".localeCompare("\01asdf"), 0);