Bug 217854 – Ensure %TypedArray% essential internal methods adhere to spec

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

Ensure %TypedArray% essential internal methods adhere to spec

[Ross Kirsling]

*** Bug 184749 has been marked as a duplicate of this bug. ***

[Ross Kirsling]

Created attachment 411629 [details]
Patch

[Ross Kirsling]

Created attachment 411639 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 411639 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411639&action=review

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:343
> +    if (Optional<uint32_t> index = parseIndex(propertyName))
> +        return getOwnPropertySlotByIndex(thisObject, globalObject, index.value(), slot);

Let's insert `static_assert(std::is_final_v<JSGenericTypedArrayView<Adaptor>>, "getOwnPropertySlotByIndex must not be overridden")` to ensure that no derived classes are overriding getOwnPropertySlotByIndex.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:359
> +        return putByIndex(thisObject, globalObject, index.value(), value, slot.isStrictMode());

Ditto.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:420
> +        return deletePropertyByIndex(thisObject, globalObject, index.value());

Ditto.

> JSTests/test262/expectations.yaml:1266
> +  strict mode: 'Test262Error: Expected a TypeError but got a Test262Error (Testing with Float64Array.)'

What is the reason of getting these new errors?

[Yusuke Suzuki]

Comment on attachment 411639 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411639&action=review

r=me with LayoutTests updates.
And can you check the correct semantics is right in LLInt, JIT, etc.'s TypedArray fast paths?

>> JSTests/test262/expectations.yaml:1266
>> +  strict mode: 'Test262Error: Expected a TypeError but got a Test262Error (Testing with Float64Array.)'
> 
> What is the reason of getting these new errors?

Discussed with Ross offline. They are test262's bugs and tracked right now https://github.com/tc39/test262/pull/2833.

[Ross Kirsling]

Created attachment 411658 [details]
Patch for landing

[Ross Kirsling]

Created attachment 411661 [details]
Patch for landing

[EWS]

Committed r268640: <https://trac.webkit.org/changeset/268640>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411661 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70405764>

[Ross Kirsling]

Created attachment 411683 [details]
Unreviewed fix, for EWS

[Ross Kirsling]

Whoops, it seems I made a late adjustment that broke a test262 test. Opened bug 217883 to have EWS verify that my fix doesn't affect layout tests.

[Alexey Shvayka]

Comment on attachment 411661 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=411661&action=review

> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:-197
> -    function mergeSort(array, valueCount, comparator)

nit: When implementing r267514, I've measured a 10-15% regression when using outer scope variables instead of parameters.
We might also want to make a change similar to r267827, possibly sharing a few functions?

[Ross Kirsling]

(In reply to Alexey Shvayka from comment #12)
> Comment on attachment 411661 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411661&action=review
> 
> > Source/JavaScriptCore/builtins/TypedArrayPrototype.js:-197
> > -    function mergeSort(array, valueCount, comparator)
> 
> nit: When implementing r267514, I've measured a 10-15% regression when using
> outer scope variables instead of parameters.
> We might also want to make a change similar to r267827, possibly sharing a
> few functions?

Thanks, that's a great idea! I can look into this tomorrow.