Skip Array pop fast path if prototype unsafe (#6838)

The Array.prototype.pop fast path does not walk the prototype chain.
That’s a problem, if a sparse array has a getter defined in it’s prototype like in issue #6824.

There are 4 variants of the fast path:

1. JavascriptNativeArray::PopWithNoDst
2. JavascriptNativeIntArray::Pop
3. JavascriptNativeFloatArray::Pop
4. JavascriptArray::Pop.
Behavior:

The issue only occurs in the first case, if the result of .pop() is discarded, because in that case only the length of the array is reduced.
If the result of a JavascriptNativeArray is stored in a variable (2 and 3), the jit method will bailout and hand over to JavascriptArray::EntryPop which will walk the prototype chain (no issue).
The last case explicitly walks the prototype chain (no issue).

(Related to #6598)
Fix #6824

Author: ShortDevelopment

ContributionAgreement.md

@@ -43,3 +43,4 @@ This agreement has been signed by:
 |Kevin Cadieux|kevcadieux|
 |Aidan Bickford| BickfordA|
 |Ryoichi Kaida| camcam-lemon|
+|Lukas Kurz| ShortDevelopment|

lib/Backend/JnHelperMethodList.h

@@ -486,7 +486,7 @@ HELPERCALLCHK(Array_VarPush, Js::JavascriptArray::Push, 0)
 HELPERCALLCHK(Array_NativeIntPush, Js::JavascriptNativeIntArray::Push, 0)
 HELPERCALLCHK(Array_NativeFloatPush, Js::JavascriptNativeFloatArray::Push, 0)
 HELPERCALLCHK(Array_VarPop, Js::JavascriptArray::Pop, 0)
-HELPERCALLCHK(Array_NativePopWithNoDst, Js::JavascriptNativeArray::PopWithNoDst, AttrCanNotBeReentrant)
+HELPERCALLCHK(Array_NativePopWithNoDst, Js::JavascriptNativeArray::PopWithNoDst, 0)
 HELPERCALLCHK(Array_NativeIntPop, Js::JavascriptNativeIntArray::Pop, AttrCanNotBeReentrant)
 HELPERCALLCHK(Array_NativeFloatPop, Js::JavascriptNativeFloatArray::Pop, AttrCanNotBeReentrant)
 HELPERCALLCHK(Array_Reverse, Js::JavascriptArray::EntryReverse, 0)

lib/Backend/Lower.cpp

@@ -12359,6 +12359,7 @@ Lowerer::GenerateHelperToArrayPopFastPath(IR::Instr * instr, IR::LabelInstr * do
     IR::JnHelperMethod helperMethod;
 
     //Decide the helperMethod based on dst availability and nativity of the array.
+    // ToDo: Maybe ignore fast path if `JavascriptArray::HasAnyES5ArrayInPrototypeChain`. See #6582 and #6824.
     if(arrayValueType.IsLikelyNativeArray() && !instr->GetDst())
     {
         helperMethod = IR::HelperArray_NativePopWithNoDst;
@@ -12378,11 +12379,7 @@ Lowerer::GenerateHelperToArrayPopFastPath(IR::Instr * instr, IR::LabelInstr * do
 
     m_lowererMD.LoadHelperArgument(instr, arrayHelperOpnd);
 
-    //We do not need scriptContext for HelperArray_NativePopWithNoDst call.
-    if(helperMethod != IR::HelperArray_NativePopWithNoDst)
-    {
-        LoadScriptContext(instr);
-    }
+    LoadScriptContext(instr);
 
     IR::Instr * retInstr = m_lowererMD.ChangeToHelperCall(instr, helperMethod, bailOutLabelHelper);
 

lib/Runtime/Library/JavascriptArray.cpp

@@ -4968,15 +4968,22 @@ using namespace Js;
     *   PopWithNoDst
     *   - For pop calls that do not return a value, we only need to decrement the length of the array.
     */
-    void JavascriptNativeArray::PopWithNoDst(Var nativeArray)
+    void JavascriptNativeArray::PopWithNoDst(ScriptContext* scriptContext, Var nativeArray)
     {
-        JIT_HELPER_NOT_REENTRANT_NOLOCK_HEADER(Array_NativePopWithNoDst);
+        JIT_HELPER_REENTRANT_HEADER(Array_NativePopWithNoDst);
         Assert(VarIs<JavascriptNativeArray>(nativeArray));
         JavascriptArray * arr = VarTo<JavascriptArray>(nativeArray);
 
         // we will bailout on length 0
         Assert(arr->GetLength() != 0);
 
+        // Check for SparseArray and also has array in prototype chain
+        if (JavascriptArray::HasAnyES5ArrayInPrototypeChain(arr, false)) {
+            // Pop (walk chain) and discard the result
+            EntryPopJavascriptArray(scriptContext, arr);
+            return;
+        }
+
         uint32 index = arr->GetLength() - 1;
         arr->SetLength(index);
         JIT_HELPER_END(Array_NativePopWithNoDst);
@@ -5007,6 +5014,7 @@ using namespace Js;
         {
             arr->SetLength(index);
         }
+        
         return element;
         JIT_HELPER_END(Array_NativeIntPop);
     }

lib/Runtime/Library/JavascriptArray.h

@@ -1033,7 +1033,7 @@ namespace Js
         Var FindMinOrMax(Js::ScriptContext * scriptContext, bool findMax);
         template<typename T, bool checkNaNAndNegZero> Var FindMinOrMax(Js::ScriptContext * scriptContext, bool findMax); // NativeInt arrays can't have NaNs or -0
 
-        static void PopWithNoDst(Var nativeArray);
+        static void PopWithNoDst(ScriptContext* scriptContext, Var nativeArray);
     };
 
     template <> inline bool VarIsImpl<JavascriptNativeArray>(RecyclableObject* obj)

test/Array/pop4.js

@@ -0,0 +1,34 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Copyright (c) 2022 ChakraCore Project Contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+const array = [1, 2, 3];
+
+const testRuns = 10;
+let counter = 0;
+
+let proto = [];
+Object.defineProperty(proto, 3, {
+    get: function () {
+        counter++;
+        return 42;
+    }
+});
+array.__proto__ = proto;
+
+function hotFunction() {
+    // Discard return value
+    array.pop();
+}
+
+for (let i = 0; i < testRuns; i++) {
+    array.length = 4;
+    hotFunction();
+}
+
+if (counter < testRuns) {
+    throw new Error(`Expected ${testRuns} calls to getter, got ${counter}`);
+}
+print("pass");

test/Array/pop5.js

@@ -0,0 +1,35 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Copyright (c) 2022 ChakraCore Project Contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+const array = [1, 2, 3];
+
+const testRuns = 10;
+let counter = 0;
+
+let proto = [];
+Object.defineProperty(proto, 3, {
+    get: function () {
+        counter++;
+        return 42;
+    }
+});
+array.__proto__ = proto;
+
+let globalCache = null;
+function hotFunction() {
+    // Store return value
+    globalCache = array.pop();
+}
+
+for (let i = 0; i < testRuns; i++) {
+    array.length = 4;
+    hotFunction();
+}
+
+if (counter < testRuns) {
+    throw new Error(`Expected ${testRuns} calls to getter, got ${counter}`);
+}
+print("pass");

test/Array/rlexe.xml

@@ -239,6 +239,16 @@
       <baseline>pop3.baseline</baseline>
     </default>
   </test>
+  <test>
+    <default>
+      <files>pop4.js</files>
+    </default>
+  </test>
+  <test>
+    <default>
+      <files>pop5.js</files>
+    </default>
+  </test>
   <test>
     <default>
       <files>push1.js</files>