Changeset 267037 in webkit


Ignore:
Timestamp:
Sep 14, 2020 1:48:38 PM (4 years ago)
Author:
Alexey Shvayka
Message:

ArraySetLength should coerce Value? before descriptor validation
https://bugs.webkit.org/show_bug.cgi?id=158791

Reviewed by Darin Adler.

JSTests:

  • test262/expectations.yaml: Mark 4 test cases as passing.

Source/JavaScriptCore:

This patch:

  1. Moves Value? coercion before descriptor validation as per spec [1], which fixes ASSERT() failure and aligns JSC with V8 & SpiderMonkey.
  1. Prevents JSArray::setLengthWithArrayStorage() from throwing if the length is unchanged, even if it's read-only [2].
  1. Refactors JSArray::defineOwnProperty() leveraging #2 to always perform setLength(), which greatly reduces the number of checks, branches, and setLengthWritable() calls.

Following the ArraySetLength spec steps precisely [1] would result in
more difficult-to-follow code because descriptor validation [2] is inlined
and Delete? failures are handled in setLength().

This change is performance-neutral as it doesn't affect JSArray::put(),
which was vetted to be spec-correct and is covered by test262 suite.

[1]: https://tc39.es/ecma262/#sec-arraysetlength (steps 3-4)
[2]: https://tc39.es/ecma262/#sec-validateandapplypropertydescriptor (step 7.a.ii)

  • runtime/JSArray.cpp:

(JSC::JSArray::defineOwnProperty):
(JSC::JSArray::setLengthWithArrayStorage):

Location:
trunk
Files:
4 edited

Legend:

Unmodified
Added
Removed
  • trunk/JSTests/ChangeLog

    r267032 r267037  
     12020-09-14  Alexey Shvayka  <shvaikalesh@gmail.com>
     2
     3        ArraySetLength should coerce [[Value]] before descriptor validation
     4        https://bugs.webkit.org/show_bug.cgi?id=158791
     5
     6        Reviewed by Darin Adler.
     7
     8        * test262/expectations.yaml: Mark 4 test cases as passing.
     9
    1102020-09-14  Saam Barati  <sbarati@apple.com>
    211
  • trunk/JSTests/test262/expectations.yaml

    r267029 r267037  
    604604test/annexB/language/global-code/switch-dflt-global-skip-early-err.js:
    605605  default: "SyntaxError: Cannot declare a function that shadows a let/const/class/function variable 'f' in strict mode."
    606 test/built-ins/Array/length/define-own-prop-length-coercion-order.js:
    607   default: 'Bad exit code: 11'
    608   strict mode: 'Bad exit code: 11'
    609 test/built-ins/Array/length/define-own-prop-length-overflow-order.js:
    610   default: 'Test262Error: Expected a RangeError but got a TypeError'
    611   strict mode: 'Test262Error: Expected a RangeError but got a TypeError'
    612606test/built-ins/ArrayBuffer/prototype/byteLength/detached-buffer.js:
    613607  default: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
  • trunk/Source/JavaScriptCore/ChangeLog

    r267032 r267037  
     12020-09-14  Alexey Shvayka  <shvaikalesh@gmail.com>
     2
     3        ArraySetLength should coerce [[Value]] before descriptor validation
     4        https://bugs.webkit.org/show_bug.cgi?id=158791
     5
     6        Reviewed by Darin Adler.
     7
     8        This patch:
     9
     10        1. Moves [[Value]] coercion before descriptor validation as per spec [1],
     11           which fixes ASSERT() failure and aligns JSC with V8 & SpiderMonkey.
     12
     13        2. Prevents JSArray::setLengthWithArrayStorage() from throwing if the length
     14           is unchanged, even if it's read-only [2].
     15
     16        3. Refactors JSArray::defineOwnProperty() leveraging #2 to always perform
     17           setLength(), which greatly reduces the number of checks, branches,
     18           and setLengthWritable() calls.
     19
     20        Following the ArraySetLength spec steps precisely [1] would result in
     21        more difficult-to-follow code because descriptor validation [2] is inlined
     22        and [[Delete]] failures are handled in setLength().
     23
     24        This change is performance-neutral as it doesn't affect JSArray::put(),
     25        which was vetted to be spec-correct and is covered by test262 suite.
     26
     27        [1]: https://tc39.es/ecma262/#sec-arraysetlength (steps 3-4)
     28        [2]: https://tc39.es/ecma262/#sec-validateandapplypropertydescriptor (step 7.a.ii)
     29
     30        * runtime/JSArray.cpp:
     31        (JSC::JSArray::defineOwnProperty):
     32        (JSC::JSArray::setLengthWithArrayStorage):
     33
    1342020-09-14  Saam Barati  <sbarati@apple.com>
    235
  • trunk/Source/JavaScriptCore/runtime/JSArray.cpp

    r266907 r267037  
    142142}
    143143
    144 // Defined in ES5.1 15.4.5.1
     144// https://tc39.es/ecma262/#sec-array-exotic-objects-defineownproperty-p-desc
    145145bool JSArray::defineOwnProperty(JSObject* object, JSGlobalObject* globalObject, PropertyName propertyName, const PropertyDescriptor& descriptor, bool throwException)
    146146{
     
    150150    JSArray* array = jsCast<JSArray*>(object);
    151151
    152     // 3. If P is "length", then
     152    // 2. If P is "length", then
     153    // https://tc39.es/ecma262/#sec-arraysetlength
    153154    if (propertyName == vm.propertyNames->length) {
    154         // All paths through length definition call the default [[DefineOwnProperty]], hence:
    155         // from ES5.1 8.12.9 7.a.
     155        // FIXME: Nothing prevents this from being called on a RuntimeArray, and the length function will always return 0 in that case.
     156        unsigned newLength = array->length();
     157        if (descriptor.value()) {
     158            newLength = descriptor.value().toUInt32(globalObject);
     159            RETURN_IF_EXCEPTION(scope, false);
     160            double valueAsNumber = descriptor.value().toNumber(globalObject);
     161            RETURN_IF_EXCEPTION(scope, false);
     162            if (valueAsNumber != static_cast<double>(newLength)) {
     163                throwRangeError(globalObject, scope, "Invalid array length"_s);
     164                return false;
     165            }
     166        }
     167
     168        // OrdinaryDefineOwnProperty (https://tc39.es/ecma262/#sec-validateandapplypropertydescriptor) at steps 1.a, 11.a, and 15 is now performed:
     169        // 4. If current.[[Configurable]] is false, then
     170        // 4.a. If Desc.[[Configurable]] is present and its value is true, return false.
    156171        if (descriptor.configurablePresent() && descriptor.configurable())
    157172            return typeError(globalObject, scope, throwException, UnconfigurablePropertyChangeConfigurabilityError);
    158         // from ES5.1 8.12.9 7.b.
     173        // 4.b. If Desc.[[Enumerable]] is present and SameValue(Desc.[[Enumerable]], current.[[Enumerable]]) is false, return false.
    159174        if (descriptor.enumerablePresent() && descriptor.enumerable())
    160175            return typeError(globalObject, scope, throwException, UnconfigurablePropertyChangeEnumerabilityError);
    161 
    162         // a. If the [[Value]] field of Desc is absent, then
    163         // a.i. Return the result of calling the default [[DefineOwnProperty]] internal method (8.12.9) on A passing "length", Desc, and Throw as arguments.
     176        // 6. Else if SameValue(IsDataDescriptor(current), IsDataDescriptor(Desc)) is false, then
     177        // 6.a. If current.[[Configurable]] is false, return false.
    164178        if (descriptor.isAccessorDescriptor())
    165179            return typeError(globalObject, scope, throwException, UnconfigurablePropertyChangeAccessMechanismError);
    166         // from ES5.1 8.12.9 10.a.
    167         if (!array->isLengthWritable() && descriptor.writablePresent() && descriptor.writable())
    168             return typeError(globalObject, scope, throwException, UnconfigurablePropertyChangeWritabilityError);
    169         // This descriptor is either just making length read-only, or changing nothing!
    170         if (!descriptor.value()) {
    171             if (descriptor.writablePresent())
    172                 array->setLengthWritable(globalObject, descriptor.writable());
    173             return true;
    174         }
    175        
    176         // b. Let newLenDesc be a copy of Desc.
    177         // c. Let newLen be ToUint32(Desc.[[Value]]).
    178         unsigned newLen = descriptor.value().toUInt32(globalObject);
    179         RETURN_IF_EXCEPTION(scope, false);
    180         // d. If newLen is not equal to ToNumber( Desc.[[Value]]), throw a RangeError exception.
    181         double valueAsNumber = descriptor.value().toNumber(globalObject);
    182         RETURN_IF_EXCEPTION(scope, false);
    183         if (newLen != valueAsNumber) {
    184             JSC::throwException(globalObject, scope, createRangeError(globalObject, "Invalid array length"_s));
    185             return false;
    186         }
    187 
    188         // Based on SameValue check in 8.12.9, this is always okay.
    189         // FIXME: Nothing prevents this from being called on a RuntimeArray, and the length function will always return 0 in that case.
    190         if (newLen == array->length()) {
    191             if (descriptor.writablePresent())
    192                 array->setLengthWritable(globalObject, descriptor.writable());
    193             return true;
    194         }
    195 
    196         // e. Set newLenDesc.[[Value] to newLen.
    197         // f. If newLen >= oldLen, then
    198         // f.i. Return the result of calling the default [[DefineOwnProperty]] internal method (8.12.9) on A passing "length", newLenDesc, and Throw as arguments.
    199         // g. Reject if oldLenDesc.[[Writable]] is false.
    200         if (!array->isLengthWritable())
    201             return typeError(globalObject, scope, throwException, ReadonlyPropertyChangeError);
    202        
    203         // h. If newLenDesc.[[Writable]] is absent or has the value true, let newWritable be true.
    204         // i. Else,
    205         // i.i. Need to defer setting the [[Writable]] attribute to false in case any elements cannot be deleted.
    206         // i.ii. Let newWritable be false.
    207         // i.iii. Set newLenDesc.[[Writable] to true.
    208         // j. Let succeeded be the result of calling the default [[DefineOwnProperty]] internal method (8.12.9) on A passing "length", newLenDesc, and Throw as arguments.
    209         // k. If succeeded is false, return false.
    210         // l. While newLen < oldLen repeat,
    211         // l.i. Set oldLen to oldLen – 1.
    212         // l.ii. Let deleteSucceeded be the result of calling the [[Delete]] internal method of A passing ToString(oldLen) and false as arguments.
    213         // l.iii. If deleteSucceeded is false, then
    214         bool success = array->setLength(globalObject, newLen, throwException);
     180        // 7. Else if IsDataDescriptor(current) and IsDataDescriptor(Desc) are both true, then
     181        // 7.a. If current.[[Configurable]] is false and current.[[Writable]] is false, then
     182        if (!array->isLengthWritable()) {
     183            // 7.a.i. If Desc.[[Writable]] is present and Desc.[[Writable]] is true, return false.
     184            // This check is unaffected by steps 13-14 of ArraySetLength as they change non-writable descriptors only.
     185            if (descriptor.writablePresent() && descriptor.writable())
     186                return typeError(globalObject, scope, throwException, UnconfigurablePropertyChangeWritabilityError);
     187            // 7.a.ii. If Desc.[[Value]] is present and SameValue(Desc.[[Value]], current.[[Value]]) is false, return false.
     188            // This check also covers step 12 of ArraySetLength, which is only reachable if newLen < oldLen.
     189            if (newLength != array->length())
     190                return typeError(globalObject, scope, throwException, ReadonlyPropertyChangeError);
     191        }
     192
     193        // setLength() clears indices >= newLength and sets correct "length" value if [[Delete]] fails (step 17.b.i)
     194        bool success = array->setLength(globalObject, newLength, throwException);
    215195        EXCEPTION_ASSERT(!scope.exception() || !success);
    216         if (!success) {
    217             // 1. Set newLenDesc.[[Value] to oldLen+1.
    218             // 2. If newWritable is false, set newLenDesc.[[Writable] to false.
    219             // 3. Call the default [[DefineOwnProperty]] internal method (8.12.9) on A passing "length", newLenDesc, and false as arguments.
    220             // 4. Reject.
    221             if (descriptor.writablePresent())
    222                 array->setLengthWritable(globalObject, descriptor.writable());
    223             return false;
    224         }
    225 
    226         // m. If newWritable is false, then
    227         // i. Call the default [[DefineOwnProperty]] internal method (8.12.9) on A passing "length",
    228         //    Property Descriptor{[[Writable]]: false}, and false as arguments. This call will always
    229         //    return true.
    230196        if (descriptor.writablePresent())
    231197            array->setLengthWritable(globalObject, descriptor.writable());
    232         // n. Return true.
    233         return true;
     198        return success;
    234199    }
    235200
     
    267232}
    268233
    269 // ECMA 15.4.5.1
     234// https://tc39.es/ecma262/#sec-array-exotic-objects-defineownproperty-p-desc
    270235bool JSArray::put(JSCell* cell, JSGlobalObject* globalObject, PropertyName propertyName, JSValue value, PutPropertySlot& slot)
    271236{
     
    456421
    457422    unsigned length = storage->length();
     423    if (newLength == length)
     424        return true;
    458425   
    459426    // If the length is read only then we enter sparse mode, so should enter the following 'if'.
Note: See TracChangeset for help on using the changeset viewer.