[MERGE #3608 @Penguinwizzard] Add a flag to use a stronger array sort…

… for bug analysis.

Merge pull request #3608 from Penguinwizzard:strongArraySort

The default JS sort often results in an implementation-defined order.
This is not generally a problem, and helps improve our perf. However,
there are cases where our ordering depends on the internal structure,
not the user-visible structure, of the array. This change adds a flag
to make this ordering more consistent, which should help identify our
bugs where the issue is just our sort orders being inconsistent.

lib/Common/ConfigFlagsList.h

@@ -432,6 +432,7 @@ PHASE(All)
 #define DEFAULT_CONFIG_ForceSerialized      (false)
 #define DEFAULT_CONFIG_ForceES5Array        (false)
 #define DEFAULT_CONFIG_ForceAsmJsLinkFail   (false)
+#define DEFAULT_CONFIG_StrongArraySort      (false)
 #define DEFAULT_CONFIG_DumpCommentsFromReferencedFiles (false)
 #define DEFAULT_CONFIG_ExtendedErrorStackForTestHost (false)
 #define DEFAULT_CONFIG_ForceSplitScope      (false)
@@ -1094,6 +1095,7 @@ FLAGNR(Boolean, ForceLegacyEngine     , "Force a jscrip9 dll load", false)
 FLAGNR(Phases,  Force                 , "Force certain phase to run ignoring heuristics", )
 FLAGNR(Phases,  Stress                , "Stress certain phases by making them kick in even if they normally would not.", )
 FLAGNR(Boolean, ForceArrayBTree       , "Force enable creation of BTree for Arrays", false)
+FLAGNR(Boolean, StrongArraySort       , "Add secondary comparisons to the default array sort comparator to disambiguate sorts of equal-toString'd objects.", DEFAULT_CONFIG_StrongArraySort)
 FLAGNR(Boolean, ForceCleanPropertyOnCollect, "Force cleaning of property on collection", DEFAULT_CONFIG_ForceCleanPropertyOnCollect)
 FLAGNR(Boolean, ForceCleanCacheOnCollect, "Force cleaning of dynamic caches on collection", DEFAULT_CONFIG_ForceCleanCacheOnCollect)
 FLAGNR(Boolean, ForceGCAfterJSONParse, "Force GC to happen after JSON parsing", DEFAULT_CONFIG_ForceGCAfterJSONParse)

lib/Runtime/Library/JavascriptArray.cpp

@@ -6533,7 +6533,13 @@ namespace Js
         TryFinally([&]()
         {
             //The array is a continuous array if there is only one segment
-            if (startSeg->next == nullptr) // Single segment fast path
+            if (startSeg->next == nullptr
+                // If this flag is specified, we want to improve the consistency of our array sorts
+                // by removing missing values from all kinds of arrays before sorting (done here by
+                // using the copy-to-one-segment path for array sorts) and by using a stronger sort
+                // comparer than the spec requires (done in CompareElements).
+                && !CONFIG_FLAG(StrongArraySort)
+                ) // Single segment fast path
             {
                 if (compFn != nullptr)
                 {
@@ -6702,7 +6708,38 @@ namespace Js
         Assert(element1 != NULL);
         Assert(element2 != NULL);
 
-        return JavascriptString::strcmp(element1->StringValue, element2->StringValue);
+        if (!CONFIG_FLAG(StrongArraySort))
+        {
+            return JavascriptString::strcmp(element1->StringValue, element2->StringValue);
+        }
+        else
+        {
+            int str_cmp = JavascriptString::strcmp(element1->StringValue, element2->StringValue);
+            if (str_cmp != 0)
+            {
+                return str_cmp;
+            }
+            // If they are equal, we get to a slightly more complex problem. We want to make a very
+            // predictable sort here, regardless of the structure of the array. To achieve this, we
+            // need to get an order for every pair of non-identical elements, else there will be an
+            // identifiable difference between sparse and dense array sorts in some cases.
+
+            // Handle a common set of equivalent nodes first for speed/convenience
+            if (element1->Value == element2->Value)
+            {
+                return 0;
+            }
+
+            // Easy way to do most remaining cases is to just compare the type ids if they differ.
+            if (JavascriptOperators::GetTypeId(element1->Value) != JavascriptOperators::GetTypeId(element2->Value))
+            {
+                return JavascriptOperators::GetTypeId(element1->Value) - JavascriptOperators::GetTypeId(element2->Value);
+            }
+
+            // Further comparisons are possible, but get increasingly complex, and aren't necessary
+            // for the cases on hand.
+            return 0;
+        }
     }
 
     void JavascriptArray::SortElements(Element* elements, uint32 left, uint32 right)
@@ -9799,11 +9836,13 @@ namespace Js
                 }
 
                 JS_REENTRANT(jsReentLock,
-                    accumulator = CALL_FUNCTION(scriptContext->GetThreadContext(), callBackFn, CallInfo(flags, 5), undefinedValue,
+                    accumulator = CALL_FUNCTION(scriptContext->GetThreadContext(), callBackFn, CallInfo(flags, 5),
+                        undefinedValue,
                         accumulator,
                         element,
                         JavascriptNumber::ToVar(k, scriptContext),
-                        pArr));
+                        pArr
+                ));
 
                 // Side-effects in the callback function may have changed the source array into an ES5Array. If this happens
                 // we will process the rest of the array elements like an ES5Array.