Attachment 373523 Details for Bug 182434 – Patch

[patch] Patch

bug-182434-20190705222447.patch (text/plain), 19.20 KB, created by Alexey Shvayka on 2019-07-05 12:24:49 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Alexey Shvayka

Created: 2019-07-05 12:24:49 PDT

Size: 19.20 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 247167)
>+++ JSTests/ChangeLog	(working copy)
>@@ -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 NOBODY (OOPS!).
>+
>+        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
>Index: JSTests/stress/array-flatmap.js
>===================================================================
>--- JSTests/stress/array-flatmap.js	(revision 247164)
>+++ JSTests/stress/array-flatmap.js	(working copy)
>@@ -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;
>Index: JSTests/stress/array-flatten.js
>===================================================================
>--- JSTests/stress/array-flatten.js	(revision 247164)
>+++ JSTests/stress/array-flatten.js	(working copy)
>@@ -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;
>Index: JSTests/stress/array-species-create-should-handle-masquerader.js
>===================================================================
>--- JSTests/stress/array-species-create-should-handle-masquerader.js	(revision 247164)
>+++ JSTests/stress/array-species-create-should-handle-masquerader.js	(working copy)
>@@ -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`);
> }
>Index: JSTests/test262/expectations.yaml
>===================================================================
>--- JSTests/test262/expectations.yaml	(revision 247164)
>+++ JSTests/test262/expectations.yaml	(working copy)
>@@ -660,9 +660,6 @@ test/built-ins/Array/prototype/push/thro
> 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/S1
> 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.'
>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 247164)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -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 NOBODY (OOPS!).
>+
>+        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  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r247115.
>Index: Source/JavaScriptCore/builtins/ArrayPrototype.js
>===================================================================
>--- Source/JavaScriptCore/builtins/ArrayPrototype.js	(revision 247164)
>+++ Source/JavaScriptCore/builtins/ArrayPrototype.js	(working copy)
>@@ -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;
>@@ -744,33 +685,6 @@ function copyWithin(target, start /*, en
> }
> 
> @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)
> {
>     "use strict";
>Index: Source/JavaScriptCore/builtins/BuiltinNames.h
>===================================================================
>--- Source/JavaScriptCore/builtins/BuiltinNames.h	(revision 247164)
>+++ Source/JavaScriptCore/builtins/BuiltinNames.h	(working copy)
>@@ -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) \
>Index: Source/JavaScriptCore/runtime/ArrayConstructor.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/ArrayConstructor.cpp	(revision 247164)
>+++ Source/JavaScriptCore/runtime/ArrayConstructor.cpp	(working copy)
>@@ -146,10 +146,4 @@ EncodedJSValue JSC_HOST_CALL arrayConstr
>     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
>Index: Source/JavaScriptCore/runtime/ArrayConstructor.h
>===================================================================
>--- Source/JavaScriptCore/runtime/ArrayConstructor.h	(revision 247164)
>+++ Source/JavaScriptCore/runtime/ArrayConstructor.h	(working copy)
>@@ -58,7 +58,6 @@ private:
> 
> 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);
> 
>Index: Source/JavaScriptCore/runtime/ArrayPrototype.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/ArrayPrototype.cpp	(revision 247164)
>+++ Source/JavaScriptCore/runtime/ArrayPrototype.cpp	(working copy)
>@@ -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<SpeciesCo
>         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<SpeciesCo
>     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);
>Index: Source/JavaScriptCore/runtime/ArrayPrototype.h
>===================================================================
>--- Source/JavaScriptCore/runtime/ArrayPrototype.h	(revision 247164)
>+++ Source/JavaScriptCore/runtime/ArrayPrototype.h	(working copy)
>@@ -50,6 +50,7 @@ protected:
>     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*);
>Index: Source/JavaScriptCore/runtime/JSGlobalObject.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/JSGlobalObject.cpp	(revision 247164)
>+++ Source/JavaScriptCore/runtime/JSGlobalObject.cpp	(working copy)
>@@ -908,7 +908,6 @@ putDirectWithoutTransition(vm, vm.proper
> #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.proper
>         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),
>

Actions: View | Formatted Diff | Diff

Attachments on bug 182434: 332969 | 373352 | 373523