[JSC] Clean up ArraySpeciesCreate

https://bugs.webkit.org/show_bug.cgi?id=182434

Patch by Alexey Shvayka <shvaikalesh@gmail.com> on 2019-07-05
Reviewed by Yusuke Suzuki.

JSTests:

Adjusts error message expectations in stress tests.

* stress/array-flatmap.js:
* stress/array-flatten.js:
* stress/array-species-create-should-handle-masquerader.js:
* test262/expectations.yaml: Mark 4 test cases as passing.

Source/JavaScriptCore:

We have duplicate code in arraySpeciesCreate, filter, map, concatSlowPath of ArrayPrototype.js
and speciesConstructArray of ArrayPrototype.cpp. This patch fixes cross-realm Array constructor
detection in native speciesConstructArray, upgrades `length` type to correctly handle large integers,
and exposes it as @arraySpeciesCreate. Also removes now unused @isArrayConstructor private function.
Native speciesConstructArray is preferred because it has fast path via speciesWatchpointIsValid.

Thoroughly benchmarked: this change progresses ARES-6 by 0-1%.

* builtins/ArrayPrototype.js:
(filter):
(map):
(globalPrivate.concatSlowPath):
(globalPrivate.arraySpeciesCreate): Deleted.
* builtins/BuiltinNames.h:
* runtime/ArrayConstructor.cpp:
(JSC::arrayConstructorPrivateFuncIsArrayConstructor): Deleted.
* runtime/ArrayConstructor.h:
* runtime/ArrayPrototype.cpp:
(JSC::arrayProtoFuncSpeciesCreate):
* runtime/ArrayPrototype.h:
* runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::init):

Canonical link: https://commits.webkit.org/213436@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247173 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog

@@ -1,3 +1,17 @@
+2019-07-05  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        [JSC] Clean up ArraySpeciesCreate
+        https://bugs.webkit.org/show_bug.cgi?id=182434
+
+        Reviewed by Yusuke Suzuki.
+
+        Adjusts error message expectations in stress tests.
+
+        * stress/array-flatmap.js:
+        * stress/array-flatten.js:
+        * stress/array-species-create-should-handle-masquerader.js:
+        * test262/expectations.yaml: Mark 4 test cases as passing.
+
 2019-07-02  Michael Saboff  <msaboff@apple.com>
 
         Exception from For..of loop assignment eliminates TDZ checks in subsequent code

JSTests/stress/array-flatmap.js

@@ -66,7 +66,7 @@ array2.constructor = 0;
 
 shouldThrow(() => {
     flatMap.call(array2, () => {});
-}, `TypeError: 0 is not a constructor`);
+}, `TypeError: Species construction did not get a valid constructor`);
 
 var array2 = new realm.Array;
 array2.constructor = undefined;

JSTests/stress/array-flatten.js

@@ -77,7 +77,7 @@ array2.constructor = 0;
 
 shouldThrow(() => {
     flat.call(array2);
-}, `TypeError: 0 is not a constructor`);
+}, `TypeError: Species construction did not get a valid constructor`);
 
 var array2 = new realm.Array;
 array2.constructor = undefined;

JSTests/stress/array-species-create-should-handle-masquerader.js

@@ -17,5 +17,5 @@ noInline(shouldThrow);
 for (var i = 0; i < 1e5; ++i) {
     shouldThrow(() => {
         new (class extends Array { static get [Symbol.species]() { return makeMasquerader(); } })(1, 2, 3).flat().constructor
-    }, `TypeError: Masquerader is not a constructor`);
+    }, `TypeError: Species construction did not get a valid constructor`);
 }

JSTests/test262/expectations.yaml

@@ -660,9 +660,6 @@ test/built-ins/Array/prototype/push/throws-if-integer-limit-exceeded.js:
 test/built-ins/Array/prototype/reverse/length-exceeding-integer-limit-with-proxy.js:
   default: 'Test262Error: Expected a StopReverse but got a Test262Error'
   strict mode: 'Test262Error: Expected a StopReverse but got a Test262Error'
-test/built-ins/Array/prototype/slice/create-proto-from-ctor-realm-non-array.js:
-  default: 'Test262Error: Expected SameValue(«», «[object Object]») to be true'
-  strict mode: 'Test262Error: Expected SameValue(«», «[object Object]») to be true'
 test/built-ins/Array/prototype/slice/length-exceeding-integer-limit-proxied-array.js:
   default: 'Test262Error: Expected [] and [9007199254740989, 9007199254740990] to have the same contents. slice(9007199254740989)'
   strict mode: 'Test262Error: Expected [] and [9007199254740989, 9007199254740990] to have the same contents. slice(9007199254740989)'
@@ -678,9 +675,6 @@ test/built-ins/Array/prototype/splice/S15.4.4.12_A3_T1.js:
 test/built-ins/Array/prototype/splice/clamps-length-to-integer-limit.js:
   default: 'Test262Error: Length is 2**53 - 1 Expected SameValue(«4294967295», «9007199254740991») to be true'
   strict mode: 'Test262Error: Length is 2**53 - 1 Expected SameValue(«4294967295», «9007199254740991») to be true'
-test/built-ins/Array/prototype/splice/create-proto-from-ctor-realm-non-array.js:
-  default: 'Test262Error: Expected SameValue(«», «[object Object]») to be true'
-  strict mode: 'Test262Error: Expected SameValue(«», «[object Object]») to be true'
 test/built-ins/Array/prototype/splice/create-proxy.js:
   default: 'TypeError: Attempted to assign to readonly property.'
   strict mode: 'TypeError: Attempted to assign to readonly property.'

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,33 @@
+2019-07-05  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        [JSC] Clean up ArraySpeciesCreate
+        https://bugs.webkit.org/show_bug.cgi?id=182434
+
+        Reviewed by Yusuke Suzuki.
+
+        We have duplicate code in arraySpeciesCreate, filter, map, concatSlowPath of ArrayPrototype.js
+        and speciesConstructArray of ArrayPrototype.cpp. This patch fixes cross-realm Array constructor
+        detection in native speciesConstructArray, upgrades `length` type to correctly handle large integers,
+        and exposes it as @arraySpeciesCreate. Also removes now unused @isArrayConstructor private function.
+        Native speciesConstructArray is preferred because it has fast path via speciesWatchpointIsValid.
+
+        Thoroughly benchmarked: this change progresses ARES-6 by 0-1%.
+
+        * builtins/ArrayPrototype.js:
+        (filter):
+        (map):
+        (globalPrivate.concatSlowPath):
+        (globalPrivate.arraySpeciesCreate): Deleted.
+        * builtins/BuiltinNames.h:
+        * runtime/ArrayConstructor.cpp:
+        (JSC::arrayConstructorPrivateFuncIsArrayConstructor): Deleted.
+        * runtime/ArrayConstructor.h:
+        * runtime/ArrayPrototype.cpp:
+        (JSC::arrayProtoFuncSpeciesCreate):
+        * runtime/ArrayPrototype.h:
+        * runtime/JSGlobalObject.cpp:
+        (JSC::JSGlobalObject::init):
+
 2019-07-05  Tadeu Zagallo  <tzagallo@apple.com>
 
         Unreviewed, change the value used to scribble Heap::m_worldState

Source/JavaScriptCore/builtins/ArrayPrototype.js

@@ -175,27 +175,7 @@ function filter(callback /*, thisArg */)
         @throwTypeError("Array.prototype.filter callback must be a function");
 
     var thisArg = @argument(1);
-
-    // Do 9.4.2.3 ArraySpeciesCreate
-    var result;
-    var constructor;
-    if (@isArray(array)) {
-        constructor = array.constructor;
-        // We have this check so that if some array from a different global object
-        // calls this map they don't get an array with the Array.prototype of the
-        // other global object.
-        if (@Array !== constructor && @isArrayConstructor(constructor))
-            constructor = @undefined;
-        if (@isObject(constructor)) {
-            constructor = constructor.@speciesSymbol;
-            if (constructor === null)
-                constructor = @undefined;
-        }
-    }
-    if (constructor === @Array || constructor === @undefined)
-        result = @newArrayWithSize(0);
-    else
-        result = new constructor(0);
+    var result = @arraySpeciesCreate(array, 0);
 
     var nextIndex = 0;
     for (var i = 0; i < length; i++) {
@@ -221,27 +201,7 @@ function map(callback /*, thisArg */)
         @throwTypeError("Array.prototype.map callback must be a function");
 
     var thisArg = @argument(1);
-
-    // Do 9.4.2.3 ArraySpeciesCreate
-    var result;
-    var constructor;
-    if (@isArray(array)) {
-        constructor = array.constructor;
-        // We have this check so that if some array from a different global object
-        // calls this map they don't get an array with the Array.prototype of the
-        // other global object.
-        if (@Array !== constructor && @isArrayConstructor(constructor))
-            constructor = @undefined;
-        if (@isObject(constructor)) {
-            constructor = constructor.@speciesSymbol;
-            if (constructor === null)
-                constructor = @undefined;
-        }
-    }
-    if (constructor === @Array || constructor === @undefined)
-        result = @newArrayWithSize(length);
-    else
-        result = new constructor(length);
+    var result = @arraySpeciesCreate(array, length);
 
     for (var i = 0; i < length; i++) {
         if (!(i in array))
@@ -620,28 +580,9 @@ function concatSlowPath()
     "use strict";
 
     var currentElement = @toObject(this, "Array.prototype.concat requires that |this| not be null or undefined");
-
-    var constructor;
-    if (@isArray(currentElement)) {
-        constructor = currentElement.constructor;
-        // We have this check so that if some array from a different global object
-        // calls this map they don't get an array with the Array.prototype of the
-        // other global object.
-        if (@Array !== constructor && @isArrayConstructor(constructor))
-            constructor = @undefined;
-        else if (@isObject(constructor)) {
-            constructor = constructor.@speciesSymbol;
-            if (constructor === null)
-                constructor = @Array;
-        }
-    }
-
     var argCount = arguments.length;
-    var result;
-    if (constructor === @Array || constructor === @undefined)
-        result = @newArrayWithSize(0);
-    else
-        result = new constructor(0);
+
+    var result = @arraySpeciesCreate(currentElement, 0);
     var resultIsArray = @isJSArray(result);
 
     var resultIndex = 0;
@@ -743,33 +684,6 @@ function copyWithin(target, start /*, end */)
     return array;
 }
 
-@globalPrivate
-function arraySpeciesCreate(array, length)
-{
-    "use strict";
-
-    if (!@isArray(array))
-        return @newArrayWithSize(length);
-
-    var constructor = array.constructor;
-    var arrayConstructorInRealm = @Array;
-    // We have this check so that if some array from a different global object
-    // calls this map they don't get an array with the Array.prototype of the
-    // other global object.
-    if (arrayConstructorInRealm !== constructor && @isArrayConstructor(constructor))
-        return @newArrayWithSize(length);
-
-    if (@isObject(constructor)) {
-        constructor = constructor.@speciesSymbol;
-        if (@isUndefinedOrNull(constructor))
-            return @newArrayWithSize(length);
-    }
-
-    if (constructor === arrayConstructorInRealm || constructor === @undefined)
-        return @newArrayWithSize(length);
-    return new constructor(length);
-}
-
 @globalPrivate
 function flatIntoArray(target, source, sourceLength, targetIndex, depth)
 {

Source/JavaScriptCore/builtins/BuiltinNames.h

@@ -50,6 +50,7 @@ namespace JSC {
     macro(arrayIteratorNext) \
     macro(arrayIteratorIsDone) \
     macro(arrayIteratorKind) \
+    macro(arraySpeciesCreate) \
     macro(assert) \
     macro(charCodeAt) \
     macro(executor) \
@@ -132,7 +133,6 @@ namespace JSC {
     macro(hasInstanceBoundFunction) \
     macro(instanceOf) \
     macro(isArraySlow) \
-    macro(isArrayConstructor) \
     macro(isConstructor) \
     macro(concatMemcpy) \
     macro(appendMemcpy) \

Source/JavaScriptCore/runtime/ArrayConstructor.cpp

@@ -146,10 +146,4 @@ EncodedJSValue JSC_HOST_CALL arrayConstructorPrivateFuncIsArraySlow(ExecState* e
     return JSValue::encode(jsBoolean(isArraySlowInline(exec, jsCast<ProxyObject*>(exec->uncheckedArgument(0)))));
 }
 
-EncodedJSValue JSC_HOST_CALL arrayConstructorPrivateFuncIsArrayConstructor(ExecState* exec)
-{
-    VM& vm = exec->vm();
-    return JSValue::encode(jsBoolean(jsDynamicCast<ArrayConstructor*>(vm, exec->uncheckedArgument(0))));
-}
-
 } // namespace JSC

Source/JavaScriptCore/runtime/ArrayConstructor.h

@@ -58,7 +58,6 @@ class ArrayConstructor final : public InternalFunction {
 
 JSArray* constructArrayWithSizeQuirk(ExecState*, ArrayAllocationProfile*, JSGlobalObject*, JSValue length, JSValue prototype = JSValue());
 
-EncodedJSValue JSC_HOST_CALL arrayConstructorPrivateFuncIsArrayConstructor(ExecState*);
 EncodedJSValue JSC_HOST_CALL arrayConstructorPrivateFuncIsArraySlow(ExecState*);
 bool isArraySlow(ExecState*, ProxyObject* argument);
 

Source/JavaScriptCore/runtime/ArrayPrototype.cpp

@@ -213,7 +213,7 @@ enum class SpeciesConstructResult {
     CreatedObject
 };
 
-static ALWAYS_INLINE std::pair<SpeciesConstructResult, JSObject*> speciesConstructArray(ExecState* exec, JSObject* thisObject, unsigned length)
+static ALWAYS_INLINE std::pair<SpeciesConstructResult, JSObject*> speciesConstructArray(ExecState* exec, JSObject* thisObject, uint64_t length)
 {
     VM& vm = exec->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
@@ -238,14 +238,16 @@ static ALWAYS_INLINE std::pair<SpeciesConstructResult, JSObject*> speciesConstru
         RETURN_IF_EXCEPTION(scope, exceptionResult());
         if (constructor.isConstructor(vm)) {
             JSObject* constructorObject = jsCast<JSObject*>(constructor);
-            if (exec->lexicalGlobalObject() != constructorObject->globalObject(vm))
-                return std::make_pair(SpeciesConstructResult::FastPath, nullptr);;
+            bool isArrayConstructorFromAnotherRealm = exec->lexicalGlobalObject() != constructorObject->globalObject(vm)
+                && constructorObject->inherits<ArrayConstructor>(vm);
+            if (isArrayConstructorFromAnotherRealm)
+                return std::make_pair(SpeciesConstructResult::FastPath, nullptr);
         }
         if (constructor.isObject()) {
             constructor = constructor.get(exec, vm.propertyNames->speciesSymbol);
             RETURN_IF_EXCEPTION(scope, exceptionResult());
             if (constructor.isNull())
-                return std::make_pair(SpeciesConstructResult::FastPath, nullptr);;
+                return std::make_pair(SpeciesConstructResult::FastPath, nullptr);
         }
     } else {
         // If isArray is false, return ? ArrayCreate(length).
@@ -263,6 +265,28 @@ static ALWAYS_INLINE std::pair<SpeciesConstructResult, JSObject*> speciesConstru
     return std::make_pair(SpeciesConstructResult::CreatedObject, newObject);
 }
 
+EncodedJSValue JSC_HOST_CALL arrayProtoFuncSpeciesCreate(ExecState* exec)
+{
+    VM& vm = exec->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+    JSObject* object = asObject(exec->uncheckedArgument(0));
+    uint64_t length = static_cast<uint64_t>(exec->uncheckedArgument(1).asNumber());
+
+    std::pair<SpeciesConstructResult, JSObject*> speciesResult = speciesConstructArray(exec, object, length);
+    EXCEPTION_ASSERT(!!scope.exception() == (speciesResult.first == SpeciesConstructResult::Exception));
+    if (UNLIKELY(speciesResult.first == SpeciesConstructResult::Exception))
+        return { };
+    if (speciesResult.first == SpeciesConstructResult::CreatedObject)
+        return JSValue::encode(speciesResult.second);
+
+    if (length > std::numeric_limits<unsigned>::max()) {
+        throwRangeError(exec, scope, "Array size is not a small enough positive integer."_s);
+        return { };
+    }
+
+    RELEASE_AND_RETURN(scope, JSValue::encode(constructEmptyArray(exec, nullptr, length)));
+}
+
 static inline unsigned argumentClampedIndexFromStartOrEnd(ExecState* exec, int argument, unsigned length, unsigned undefinedValue = 0)
 {
     JSValue value = exec->argument(argument);

Source/JavaScriptCore/runtime/ArrayPrototype.h

@@ -50,6 +50,7 @@ class ArrayPrototype final : public JSArray {
     void finishCreation(VM&, JSGlobalObject*);
 };
 
+EncodedJSValue JSC_HOST_CALL arrayProtoFuncSpeciesCreate(ExecState*);
 EncodedJSValue JSC_HOST_CALL arrayProtoFuncToString(ExecState*);
 EncodedJSValue JSC_HOST_CALL arrayProtoFuncValues(ExecState*);
 EncodedJSValue JSC_HOST_CALL arrayProtoPrivateFuncConcatMemcpy(ExecState*);

Source/JavaScriptCore/runtime/JSGlobalObject.cpp

@@ -908,7 +908,6 @@ putDirectWithoutTransition(vm, vm.propertyNames-> jsName, lowerName ## Construct
 #if ENABLE(INTL)
     JSFunction* privateFuncDateTimeFormat = JSFunction::create(vm, this, 0, String(), globalFuncDateTimeFormat);
 #endif
-    JSFunction* privateFuncIsArrayConstructor = JSFunction::create(vm, this, 0, String(), arrayConstructorPrivateFuncIsArrayConstructor);
     JSFunction* privateFuncIsArraySlow = JSFunction::create(vm, this, 0, String(), arrayConstructorPrivateFuncIsArraySlow);
     JSFunction* privateFuncConcatMemcpy = JSFunction::create(vm, this, 0, String(), arrayProtoPrivateFuncConcatMemcpy);
     JSFunction* privateFuncAppendMemcpy = JSFunction::create(vm, this, 0, String(), arrayProtoPrivateFuncAppendMemcpy);
@@ -988,9 +987,9 @@ putDirectWithoutTransition(vm, vm.propertyNames-> jsName, lowerName ## Construct
         GlobalPropertyInfo(vm.propertyNames->builtinNames().InternalPromisePrivateName(), internalPromiseConstructor, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
 
         GlobalPropertyInfo(vm.propertyNames->builtinNames().repeatCharacterPrivateName(), JSFunction::create(vm, this, 2, String(), stringProtoFuncRepeatCharacter), PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
+        GlobalPropertyInfo(vm.propertyNames->builtinNames().arraySpeciesCreatePrivateName(), JSFunction::create(vm, this, 2, String(), arrayProtoFuncSpeciesCreate), PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
         GlobalPropertyInfo(vm.propertyNames->builtinNames().isArrayPrivateName(), arrayConstructor->getDirect(vm, vm.propertyNames->isArray), PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
         GlobalPropertyInfo(vm.propertyNames->builtinNames().isArraySlowPrivateName(), privateFuncIsArraySlow, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
-        GlobalPropertyInfo(vm.propertyNames->builtinNames().isArrayConstructorPrivateName(), privateFuncIsArrayConstructor, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
         GlobalPropertyInfo(vm.propertyNames->builtinNames().concatMemcpyPrivateName(), privateFuncConcatMemcpy, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
         GlobalPropertyInfo(vm.propertyNames->builtinNames().appendMemcpyPrivateName(), privateFuncAppendMemcpy, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),