[MERGE #5251 @msdimitrov] Convert JavascriptString::EntryLocaleCompare to use LogicalStringCompare

Merge pull request #5251 from msdimitrov:entrylocalecompare

Resolves #4841
Resolves #4843

I didn't change the error message, since I'm not sure if a new error message should be added to `rterros.h` or throw the error from `GetLastError()` .

Author: jackhorton

bin/ChakraCore/TestHooks.cpp

@@ -14,7 +14,7 @@ namespace Internal
 {
 // this is in place of including PlatformAgnostic/UnicodeTextInternal.h, which has template
 // instantiations that upset Clang on macOS and Linux
-int LogicalStringCompareImpl(const char16* p1, const char16* p2);
+int LogicalStringCompareImpl(const char16* p1, int p1size, const char16* p2, int p2size);
 }
 }
 }

bin/ChakraCore/TestHooks.h

@@ -22,7 +22,7 @@ struct TestHooks
     typedef HRESULT(TESTHOOK_CALL *SetAssertToConsoleFlagPtr)(bool flag);
     typedef HRESULT(TESTHOOK_CALL *SetEnableCheckMemoryLeakOutputPtr)(bool flag);
     typedef void(TESTHOOK_CALL * NotifyUnhandledExceptionPtr)(PEXCEPTION_POINTERS exceptionInfo);
-    typedef int(TESTHOOK_CALL *LogicalStringCompareImpl)(const char16* p1, const char16* p2);
+    typedef int(TESTHOOK_CALL *LogicalStringCompareImpl)(const char16* p1, int p1size, const char16* p2, int p2size);
 
     SetConfigFlagsPtr pfSetConfigFlags;
     SetConfigFilePtr  pfSetConfigFile;

bin/NativeTests/UnicodeTextTests.cpp

@@ -17,11 +17,29 @@ namespace UnicodeTextTest
         REQUIRE(g_testHooksLoaded);
 
         // there are two tests, one to validate the expected value and validate the result of the call
-        int compareStringResult = CompareStringW(LOCALE_USER_DEFAULT, NORM_IGNORECASE | SORT_DIGITSASNUMBERS, str1, -1, str2, -1);
+        int compareStringResult = CompareStringEx(LOCALE_NAME_USER_DEFAULT, NORM_IGNORECASE | SORT_DIGITSASNUMBERS, str1, -1, str2, -1, NULL, NULL, 0);
         CHECK(compareStringResult != 0);
         compareStringResult = compareStringResult - CSTR_EQUAL;
 
-        int res = g_testHooks.pfLogicalCompareStringImpl(str1, str2);
+        int res = g_testHooks.pfLogicalCompareStringImpl(str1, static_cast<int>(wcslen(str1)), str2, static_cast<int>(wcslen(str2)));
+
+        //test to check that expected value passed is correct
+        REQUIRE(compareStringResult == expected);
+
+        //test to check that the result from call to LogicalStringCompareImpl is the expected value
+        REQUIRE(res == expected);
+    }
+
+    void TestNullCharacters(const WCHAR* str1, int str1size, const WCHAR* str2, int str2size, int expected)
+    {
+        REQUIRE(g_testHooksLoaded);
+
+        // there are two tests, one to validate the expected value and validate the result of the call
+        int compareStringResult = CompareStringEx(LOCALE_NAME_USER_DEFAULT, NORM_IGNORECASE | SORT_DIGITSASNUMBERS, str1, str1size, str2, str2size, NULL, NULL, 0);
+        CHECK(compareStringResult != 0);
+        compareStringResult = compareStringResult - CSTR_EQUAL;
+
+        int res = g_testHooks.pfLogicalCompareStringImpl(str1, str1size, str2, str2size);
 
         //test to check that expected value passed is correct
         REQUIRE(compareStringResult == expected);
@@ -88,4 +106,10 @@ namespace UnicodeTextTest
         Test(_u("20sTRing"), _u("st2ring"), -1);
         Test(_u("st3rING"), _u("st2riNG"), 1);
     }
+
+    TEST_CASE("LogicalCompareString_EmbeddedNullCharacters", "[UnicodeText]")
+    {
+        TestNullCharacters(_u("\0\0ab\0\0c123def\0\0"), 15, _u("abc123def"), 9, 0);
+    }
+
 }

lib/Runtime/Library/JavascriptString.cpp

@@ -1,4 +1,4 @@
-//-------------------------------------------------------------------------------------------------------
+//-------------------------------------------------------------------------------------------------------
 // Copyright (C) Microsoft. All rights reserved.
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
@@ -1296,6 +1296,8 @@ namespace Js
 
     Var JavascriptString::EntryLocaleCompare(RecyclableObject* function, CallInfo callInfo, ...)
     {
+        using namespace PlatformAgnostic;
+
         PROBE_STACK(function->GetScriptContext(), Js::Constants::MinStackDefault);
 
         ARGUMENTS(args, callInfo);
@@ -1371,25 +1373,18 @@ namespace Js
         const char16* pThatStr = pThat->GetString();
         int thatStrCount = pThat->GetLength();
 
-        // xplat-todo: doing a locale-insensitive compare here
-        // but need to move locale-specific string comparison to
-        // platform agnostic interface
-#ifdef ENABLE_GLOBALIZATION
-        LCID lcid = GetUserDefaultLCID();
-        int result = CompareStringW(lcid, NULL, pThisStr, thisStrCount, pThatStr, thatStrCount );
-        if (result == 0)
+        int result = UnicodeText::LogicalStringCompare(pThisStr, thisStrCount, pThatStr, thatStrCount);
+
+        // LogicalStringCompare will return -2 if CompareStringEx fails.
+        if (result == -2)
         {
             // TODO there is no spec on the error thrown here.
             // When the support for HR errors is implemented replace this with the same error reported by v5.8
             JavascriptError::ThrowRangeError(function->GetScriptContext(),
                 VBSERR_InternalError /* TODO-ERROR: _u("Failed compare operation")*/ );
         }
-        return JavascriptNumber::ToVar(result-2, scriptContext);
-#else // !ENABLE_GLOBALIZATION
-        // no ICU / or external support for localization. Use c-lib
-        const int result = wcscmp(pThisStr, pThatStr);
-        return JavascriptNumber::ToVar(result > 0 ? 1 : result == 0 ? 0 : -1, scriptContext);
-#endif
+
+        return JavascriptNumber::ToVar(result, scriptContext);
     }
 
 

lib/Runtime/PlatformAgnostic/Platform/Common/UnicodeText.Common.cpp

@@ -72,6 +72,12 @@ namespace PlatformAgnostic
                 return num;
             }
 
+            template <typename CharType>
+            bool isNull(__in CharType c)
+            {
+                return c == '\0';
+            }
+
             ///
             /// Implementation of CompareString(NORM_IGNORECASE | SORT_DIGITSASNUMBERS)
             /// This code is in the common library so that we can unit test it on Windows too
@@ -88,14 +94,18 @@ namespace PlatformAgnostic
             ///     Otherwise, they're the same to continue scanning
             ///  else they're both not numbers:
             ///     Compare lexically till we find a number
+            ///  if either of the strings current character is a null character continue scanning
             /// The algorithm treats the characters in a case-insensitive manner
             ///
-            int LogicalStringCompareImpl(const char16* p1, const char16* p2)
+            int LogicalStringCompareImpl(const char16* p1, int p1size, const char16* p2, int p2size)
             {
                 Assert(p1 != nullptr);
                 Assert(p2 != nullptr);
 
-                while (*p1 && *p2)
+                const char16* str1End = p1 + p1size;
+                const char16* str2End = p2 + p2size;
+
+                while (p1 < str1End && p2 < str2End)
                 {
                     bool isDigit1 = isDigit(*p1);
                     bool isDigit2 = isDigit(*p2);
@@ -170,6 +180,16 @@ namespace PlatformAgnostic
                             p1++; p2++;
                         }
                     }
+
+                    while (isNull(*p1) && p1 < str1End)
+                    {
+                        p1++;
+                    }
+
+                    while (isNull(*p2) && p2 < str2End)
+                    {
+                        p2++;
+                    }
                 }
 
                 if (*p1 == *p2) return 0;

lib/Runtime/PlatformAgnostic/Platform/Common/UnicodeText.ICU.cpp

@@ -266,10 +266,12 @@ namespace PlatformAgnostic
             return u_hasBinaryProperty(ch, UCHAR_ID_CONTINUE);
         }
 
-        int LogicalStringCompare(const char16* string1, const char16* string2)
+#ifndef _WIN32
+        int LogicalStringCompare(const char16* string1, int str1size, const char16* string2, int str2size)
         {
-            return PlatformAgnostic::UnicodeText::Internal::LogicalStringCompareImpl(string1, string2);
+            return PlatformAgnostic::UnicodeText::Internal::LogicalStringCompareImpl(string1, str1size, string2, str2size);
         }
+#endif
 
         bool IsExternalUnicodeLibraryAvailable()
         {

lib/Runtime/PlatformAgnostic/Platform/POSIX/UnicodeText.cpp

@@ -123,9 +123,9 @@ namespace PlatformAgnostic
             }
         }
 
-        int LogicalStringCompare(const char16* string1, const char16* string2)
+        int LogicalStringCompare(const char16* string1, int str1size, const char16* string2, int str2size)
         {
-            return PlatformAgnostic::UnicodeText::Internal::LogicalStringCompareImpl(string1, string2);
+            return PlatformAgnostic::UnicodeText::Internal::LogicalStringCompareImpl(string1, str1size, string2, str2size);
         }
 
         bool IsExternalUnicodeLibraryAvailable()

lib/Runtime/PlatformAgnostic/Platform/Windows/UnicodeText.cpp

@@ -164,6 +164,15 @@ namespace PlatformAgnostic
             return CharacterClassificationType::Invalid;
         }
 
+        int LogicalStringCompare(const char16* string1, int str1size, const char16* string2, int str2size)
+        {
+            // CompareStringEx called with these flags is equivalent to calling StrCmpLogicalW
+            // and we have the added advantage of not having to link with shlwapi.lib just for one function
+            int i = CompareStringEx(LOCALE_NAME_USER_DEFAULT, NORM_IGNORECASE | SORT_DIGITSASNUMBERS, string1, str1size, string2, str2size, NULL, NULL, 0);
+
+            return i - CSTR_EQUAL;
+        }
+
 // Everything below this has a preferred ICU implementation in Platform\Common\UnicodeText.ICU.cpp
 #ifndef HAS_ICU
         template <typename TRet, typename Fn>
@@ -426,15 +435,6 @@ namespace PlatformAgnostic
 #endif
             }, false);
         }
-
-        int LogicalStringCompare(const char16* string1, const char16* string2)
-        {
-            // CompareStringW called with these flags is equivalent to calling StrCmpLogicalW
-            // and we have the added advantage of not having to link with shlwapi.lib just for one function
-            int i = CompareStringW(LOCALE_USER_DEFAULT, NORM_IGNORECASE | SORT_DIGITSASNUMBERS, string1, -1, string2, -1);
-
-            return i - CSTR_EQUAL;
-        }
 #endif // HAS_ICU
     };
 };

lib/Runtime/PlatformAgnostic/UnicodeText.h

@@ -231,6 +231,6 @@ namespace PlatformAgnostic
         //     -1 - string1 is greater than string2
         //     +1 - string1 is lesser than string2
         //
-        int LogicalStringCompare(const char16* string1, const char16* string2);
+        int LogicalStringCompare(const char16* string1, int str1size, const char16* string2, int str2size);
     };
 };

lib/Runtime/PlatformAgnostic/UnicodeTextInternal.h

@@ -18,7 +18,7 @@ template charcount_t ChangeStringLinguisticCase<false, false>(const char16* sour
 namespace Internal
 {
 
-int LogicalStringCompareImpl(const char16* p1, const char16* p2);
+int LogicalStringCompareImpl(const char16* p1, int p1size, const char16* p2, int p2size);
 
 }; // namespace Internal
 }; // namespace UnicodeText