[1.6>1.7] [MERGE #3415 @akroshg] slice helper should check for the ES5array or Proxy in the protototype

Merge pull request #3415 from akroshg:nosideeffect

This way we can identify if the array's content can be affected. If there is sideeffect expected we can take the slower path
to finish the logic.

Author: akroshg

lib/Runtime/Library/JavascriptArray.cpp

@@ -5318,6 +5318,58 @@ namespace Js
         JS_REENTRANT_UNLOCK(jsReentLock, return JavascriptArray::ReverseHelper(pArr, nullptr, obj, length.GetBigIndex(), scriptContext));
     }
 
+    bool JavascriptArray::HasAnyES5ArrayInPrototypeChain(JavascriptArray *arr, bool forceCheckProtoChain)
+    {
+        Assert(arr != nullptr);
+
+#if ENABLE_COPYONACCESS_ARRAY
+        JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(arr);
+#endif
+
+        bool hasAnyES5Array = false;
+
+        // If there is no gap (unless forced) we are not filling from the prototype - so no point checking for ES5Array.
+        if (forceCheckProtoChain || arr->IsFillFromPrototypes())
+        {
+            RecyclableObject* prototype = arr->GetPrototype();
+
+            while (JavascriptOperators::GetTypeId(prototype) != TypeIds_Null)
+            {
+                RecyclableObject* protoObj = prototype;
+
+                if (!(DynamicObject::IsAnyArray(protoObj) || JavascriptOperators::IsObject(protoObj)) 
+                    || JavascriptProxy::Is(protoObj)
+                    || protoObj->IsExternal())
+                {
+                    hasAnyES5Array = true;
+                    break;
+                }
+
+                if (DynamicObject::IsAnyArray(protoObj))
+                {
+                    if (ES5Array::Is(protoObj))
+                    {
+                        hasAnyES5Array = true;
+                        break;
+                    }
+                }
+                else if (DynamicType::Is(protoObj->GetTypeId()))
+                {
+                    DynamicObject* dynobj = DynamicObject::FromVar(protoObj);
+                    ArrayObject* objectArray = dynobj->GetObjectArray();
+                    if (objectArray != nullptr && ES5Array::Is(objectArray))
+                    {
+                        hasAnyES5Array = true;
+                        break;
+                    }
+                }
+
+                prototype = prototype->GetPrototype();
+            }
+        }
+        return hasAnyES5Array;
+    }
+
     // Array.prototype.reverse as described in ES6.0 (draft 22) Section 22.1.3.20
     template <typename T>
     Var JavascriptArray::ReverseHelper(JavascriptArray* pArr, Js::TypedArrayBase* typedArrayBase, RecyclableObject* obj, T length, ScriptContext* scriptContext)
@@ -5347,7 +5399,9 @@ namespace Js
 
         ThrowTypeErrorOnFailureHelper h(scriptContext, methodName);
 
-        if (pArr)
+        bool useNoSideEffectReverse = pArr != nullptr && !HasAnyES5ArrayInPrototypeChain(pArr);
+
+        if (useNoSideEffectReverse)
         {
             Recycler * recycler = scriptContext->GetRecycler();
 
@@ -5373,6 +5427,12 @@ namespace Js
                 }
             }
 
+            // As we have already established that the FillFromPrototype should not change the bound of the array.
+            if (length != (T)pArr->length)
+            {
+                Js::Throw::FatalInternalError();
+            }
+
             if (pArr->HasNoMissingValues() && pArr->head && pArr->head->next)
             {
                 // This function currently does not track missing values in the head segment if there are multiple segments
@@ -5679,23 +5739,33 @@ namespace Js
         {
             return res;
         }
-        if (JavascriptArray::Is(args[0]))
+
+        bool useNoSideEffectShift = JavascriptArray::Is(args[0])
+            && !JavascriptArray::FromVar(args[0])->IsCrossSiteObject()
+            && !HasAnyES5ArrayInPrototypeChain(JavascriptArray::FromVar(args[0]));
+
+        if (useNoSideEffectShift)
         {
             JavascriptArray * pArr = JavascriptArray::FromVar(args[0]);
-#if ENABLE_COPYONACCESS_ARRAY
-            JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(pArr);
-#endif
 
             if (pArr->length == 0)
             {
                 return res;
             }
 
+            uint32 length = pArr->length;
+
             if(pArr->IsFillFromPrototypes())
             {
                 JS_REENTRANT(jsReentLock, pArr->FillFromPrototypes(0, pArr->length)); // We need find all missing value from [[proto]] object
             }
 
+            // As we have already established that the FillFromPrototype should not change the bound of the array.
+            if (length != pArr->length)
+            {
+                Js::Throw::FatalInternalError();
+            }
+
             if(pArr->HasNoMissingValues() && pArr->head && pArr->head->next)
             {
                 // This function currently does not track missing values in the head segment if there are multiple segments
@@ -6137,6 +6207,11 @@ namespace Js
 
         if (pArr)
         {
+            if (HasAnyES5ArrayInPrototypeChain(pArr))
+            {
+                JS_REENTRANT_UNLOCK(jsReentLock, return JavascriptArray::SliceObjectHelper(obj, start, 0u, newArr, newObj, newLen, scriptContext));
+            }
+
             // If we constructed a new Array object, we have some nice helpers here
             if (newArr && isBuiltinArrayCtor)
             {
@@ -6662,24 +6737,32 @@ namespace Js
             }
         }
 
-        if (JavascriptArray::Is(args[0]))
-        {
-#if ENABLE_COPYONACCESS_ARRAY
-            JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(args[0]);
-#endif
+        bool useNoSideEffectSort = JavascriptArray::Is(args[0])
+            && !JavascriptArray::FromVar(args[0])->IsCrossSiteObject()
+            && !HasAnyES5ArrayInPrototypeChain(JavascriptArray::FromVar(args[0]));
 
+        if (useNoSideEffectSort)
+        {
             JavascriptArray *arr = JavascriptArray::FromVar(args[0]);
 
             if (arr->length <= 1)
             {
                 return args[0];
             }
 
+            uint32 length = arr->length;
+
             if(arr->IsFillFromPrototypes())
             {
                 JS_REENTRANT(jsReentLock, arr->FillFromPrototypes(0, arr->length)); // We need find all missing value from [[proto]] object
             }
 
+            // As we have already established that the FillFromPrototype should not change the bound of the array.
+            if (length != arr->length)
+            {
+                Js::Throw::FatalInternalError();
+            }
+
             // Maintain nativity of the array only for the following cases (To favor inplace conversions - keeps the conversion cost less):
             // -    int cases for X86 and
             // -    FloatArray for AMD64
@@ -7677,22 +7760,38 @@ namespace Js
         {
            return res;
         }
-        if (JavascriptArray::Is(args[0]) && !JavascriptArray::FromVar(args[0])->IsCrossSiteObject())
+
+        JavascriptArray * pArr = nullptr;
+        if (JavascriptArray::Is(args[0])
+            && !JavascriptArray::FromVar(args[0])->IsCrossSiteObject())
         {
 #if ENABLE_COPYONACCESS_ARRAY
             JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(args[0]);
 #endif
-            JavascriptArray * pArr = JavascriptArray::FromVar(args[0]);
+            pArr = JavascriptArray::FromVar(args[0]);
+        }
 
             uint32 unshiftElements = args.Info.Count - 1;
 
+        // forceCheckProtoChain - since the array expand to accommodate new items thus we have to check if we have accessor on the proto chain.
+        bool useNoSideEffectUnshift = pArr != nullptr && (unshiftElements == 0 || !HasAnyES5ArrayInPrototypeChain(pArr, true /*forceCheckProtoChain*/));
+
+        if (useNoSideEffectUnshift)
+        {
             if (unshiftElements > 0)
             {
+                uint32 length = pArr->length;
                 if (pArr->IsFillFromPrototypes())
                 {
                     JS_REENTRANT(jsReentLock, pArr->FillFromPrototypes(0, pArr->length)); // We need find all missing value from [[proto]] object
                 }
 
+                // As we have already established that the FillFromPrototype should not change the bound of the array.
+                if (length != pArr->length)
+                {
+                    Js::Throw::FatalInternalError();
+                }
+
                 // Pre-process: truncate overflowing elements to properties
                 bool newLenOverflowed = false;
                 uint32 maxLen = MaxArrayLength - unshiftElements;
@@ -7795,7 +7894,6 @@ namespace Js
             }
 
             JS_REENTRANT(jsReentLock, BigIndex length = OP_GetLength(dynamicObject, scriptContext));
-            uint32 unshiftElements = args.Info.Count - 1;
             if (unshiftElements > 0)
             {
                 uint32 MaxSpaceUint32 = MaxArrayLength - unshiftElements;

lib/Runtime/Library/JavascriptArray.h

@@ -625,6 +625,10 @@ namespace Js
         template <typename Fn>
         static void ForEachOwnMissingArrayIndexOfObject(JavascriptArray *baseArr, JavascriptArray *destArray, RecyclableObject* obj, uint32 startIndex, uint32 limitIndex, uint32 destIndex, Fn fn);
 
+        // This helper function is mainly used as a precheck before going to the FillFromPrototype code path.
+        // Proxy and CustomExternalObject in the prototype chain will be returned as if ES5Array is there.
+        static bool HasAnyES5ArrayInPrototypeChain(JavascriptArray *arr, bool forceCheckProtoChain = false);
+
         // NativeArrays may change it's content type, but not others
         template <typename T> static bool MayChangeType() { return false; }