[MERGE #5670 @kfarnung] Remove invalid optimization from xplat qsort

Merge pull request #5670 from kfarnung:xplatsort

If the pivot ends up being the highest number, this optimization
concludes that the array is already sorted. Since this algorithm is
only used for arrays of length 513 and higher, the likelihood of
hitting is basically 100%.

Introduced: #2382
Fixes: #5667

Author: kfarnung

lib/Common/DataStructures/QuickSort.h

@@ -244,24 +244,32 @@ namespace JsUtil
             const size_t pivot = (nmemb - 1) * size;
             // make last element the median(pivot)
             CCQ_SWAP(base + pivot, base + ((nmemb / 2) * size), size);
+
             // standard qsort pt. below
-            for (size_t i = 0; i < pivot; i+= size)
+            size_t i = 0;
+            for (; i < nmemb / 2 * size; i+= size)
             {
+                // During the first half, count equal values as below the pivot
                 if (comparer(context, base + i, base + pivot) <= 0)
                 {
                     CCQ_SWAP(base + i, base + (pos * size), size);
                     pos++;
                 }
             }
 
-            // issue the last change
-            CCQ_SWAP(base + (pos * size), base + pivot, size);
-
-            if (pos >= nmemb - 1)
+            for (; i < pivot; i+= size)
             {
-                return; // looks like it was either all sorted OR nothing to sort
+                // During the second half, count equal values as above the pivot
+                if (comparer(context, base + i, base + pivot) < 0)
+                {
+                    CCQ_SWAP(base + i, base + (pos * size), size);
+                    pos++;
+                }
             }
 
+            // issue the last change
+            CCQ_SWAP(base + (pos * size), base + pivot, size);
+
             Sort(base, pos++, size, comparer, context);
             Sort(base + (pos * size), nmemb - pos, size, comparer, context);
         }

test/Array/array_qsortr_random.js

@@ -0,0 +1,50 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+function getRandomArray(size)
+{
+    const arr = [];
+
+    for (let i = 0; i < size; ++i)
+    {
+        arr[i] = Math.random() * 100 | 0;
+    }
+
+    return arr;
+}
+
+function testRandomSort(size)
+{
+    const unsorted = getRandomArray(size);
+
+    // Copy the array and sort it
+    const sorted = unsorted.slice();
+    sorted.sort(function (a, b){ return a - b;});
+
+    // Verify that the array is sorted
+    for (let i = 1; i < size; ++i)
+    {
+        // Sort has not completed correctly
+        if (sorted[i] < sorted[i - 1])
+        {
+            WScript.Echo(`Unsorted: ${unsorted}`);
+            WScript.Echo(`Sorted: ${sorted}`);
+            throw new Error(`Array is not sorted correctly at index '${i}'`);
+        }
+    }
+}
+
+function stressTestSort(iterations, size = 1000)
+{
+    for (let i = 0; i < iterations; ++i)
+    {
+        testRandomSort(size);
+    }
+}
+
+// Test 1000 random arrays of 1000 elements, print out the failures.
+stressTestSort(1000, 1000);
+
+WScript.Echo("PASS");

test/Array/bug_gh5667.js

@@ -0,0 +1,34 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+// Test a bug fix for the xplat qsort implementation
+// https://github.com/Microsoft/ChakraCore/issues/5667
+function testArray(size)
+{
+    // Create an array with all the same value
+    const arr = new Array(size);
+    arr.fill(100);
+
+    // Change the second to last value to be smaller
+    arr[arr.length - 2] = 99;
+
+    // Sort the array
+    arr.sort((a, b) => a - b);
+
+    // Verify that the array is sorted
+    for (let i = 1; i < arr.length; ++i)
+    {
+        if (arr[i] < arr[i - 1])
+        {
+            // Sort has not completed correctly
+            throw new Error (`Array is not sorted correctly at index '${i}'`);
+        }
+    }
+}
+
+testArray(512);
+testArray(513);
+
+WScript.Echo("PASS");

test/Array/rlexe.xml

@@ -775,4 +775,16 @@
       <files>bug16717501.js</files>
     </default>
   </test>
+  <test>
+    <default>
+      <files>bug_gh5667.js</files>
+      <tags>exclude_windows</tags>
+    </default>
+  </test>
+  <test>
+    <default>
+      <files>test_qsortr_random.js</files>
+      <tags>exclude_windows</tags>
+    </default>
+  </test>
 </regress-exe>

test/es6/ES6TypedArrayExtensions.js

@@ -1486,7 +1486,7 @@ var tests = [
             if (WScript.Platform.OS == "win32") { // Windows
                 assert.areEqual([9,8,7,2,10,5,4,3,1,6], getTypedArray(10).sort(sortCallbackMalformed), "%TypedArrayPrototype%.sort basic behavior with a sort callback which returns random values");
             } else { // xplat
-                assert.areEqual([2,9,8,7,10,4,1,3,5,6], getTypedArray(10).sort(sortCallbackMalformed), "%TypedArrayPrototype%.sort basic behavior with a sort callback which returns random values");
+                assert.areEqual([2,9,10,8,7,5,4,3,6,1], getTypedArray(10).sort(sortCallbackMalformed), "%TypedArrayPrototype%.sort basic behavior with a sort callback which returns random values");
             }
 
             assert.throws(function() { sort.call(); }, TypeError, "Calling %TypedArrayPrototype%.sort with no this throws TypeError", "'this' is not a typed array object");