Closed Bug 1317383 Opened 3 years ago Closed 3 years ago

TypedArray constructors use ToIndex operation in ES2017

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- wontfix
firefox55 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 6 obsolete files)

6.72 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
15.29 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
4.08 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
25.72 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
1.21 KB, patch
mossop
: review+
Details | Diff | Splinter Review
16.98 KB, patch
anba
: review+
Details | Diff | Splinter Review
For example `new Int8Array(undefined)` is now expected to return a zero length typed array.


ES2017 spec:
https://tc39.github.io/ecma262/#sec-typedarray-constructors
https://tc39.github.io/ecma262/#sec-toindex


The following test262 tests expect the new ES2017 semantics:
built-ins/TypedArrays/buffer-arg-bufferbyteoffset-throws-from-modulo-element-size.js
built-ins/TypedArrays/buffer-arg-byteoffset-is-negative-throws.js
built-ins/TypedArrays/buffer-arg-defined-negative-length.js
built-ins/TypedArrays/length-arg-is-infinity-throws-rangeerror.js
built-ins/TypedArrays/length-arg-is-negative-integer-throws-rangeerror.js
built-ins/TypedArrays/length-arg-toindex-length.js
built-ins/TypedArrays/object-arg-length-excessive-throws.js
Depends on: 1255128
For what it's worth, it's causing an error in a Web Platform test case (/testing/web-platform/mozilla/tests/wasm/jsapi.js) because there a test tries to do `new Uint8Array('hi')` and this fails at the WPT harness level. So suspect an unexpected pass there when this gets fixed.
Depends on: 1345115
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Moves NonStandardToIndex to SIMD.cpp
Attached patch bug1317383-part2-length.patch (obsolete) — Splinter Review
Use ToIndex() when the typed array constructor is called with a non-Object argument. Also made sure we always throw a RangeError instead of TypeError or InternalError when the length argument is too large. And added an |errorNumber| parameter to js::ToIndex so we can generate better error messages when ToIndex is called from the TypedArray constructor.

This is mostly covered by test262, I've only added some extra tests which are specific to the current typed array length limits.
Use ToIndex for the byteOffset and length arguments when the TypedArray constructor is called with an ArrayBuffer.

Also covered by test262, the extra tests are specific to our internal representation for the byteOffset (uint32_t) and length (int32_t) parameters.
Adds some spec step references to the TypedArray constructor.
Did you want to ask for review for this?
(In reply to Tom Schuster [:evilpie] from comment #6)
> Did you want to ask for review for this?

I'll request review after bug 1345115 has been reviewed, because this patch depends on 1345115 and 1345115 probably won't be applied in its current form, so the patches here may need to be updated again. :-)
Mass wontfix for bugs affecting firefox 52.
Duplicate of this bug: 1354195
NonStandardToIndex() is only used in SIMD.cpp, so let's move it into that file.
Attachment #8845315 - Attachment is obsolete: true
Attachment #8861925 - Flags: review?(evilpies)
Attached patch bug1317383-part2-length.patch (obsolete) — Splinter Review
The comment #3 still applies for this change.
Attachment #8845316 - Attachment is obsolete: true
Attachment #8861926 - Flags: review?(evilpies)
Use ToIndex for the |byteOffset| and |length| arguments when the TypedArray constructor is called with an ArrayBuffer.

We can't directly throw an error after the ToIndex calls if either byteOffset or length exceeds UINT32_MAX, because we need to ensure step 9 (throws a TypeError) is executed before the various bound checking steps are executed (throw RangeErrors). So I needed to change the parameter types of computeAndCheckLength() to uint64_t. On the plus side, using uint64_t allows us to change the implementation to follow the spec more verbatim, because we no longer need to care about integer overflows.
Attachment #8845317 - Attachment is obsolete: true
Attachment #8861930 - Flags: review?(evilpies)
Adds some spec step references to the TypedArray constructor methods.
Attachment #8845318 - Attachment is obsolete: true
Attachment #8861931 - Flags: review?(evilpies)
Comment on attachment 8861925 [details] [diff] [review]
bug1317383-part1-nonstandardtoindex.patch

Review of attachment 8861925 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. (Weirdly enough I thought we had already done this)
Attachment #8861925 - Flags: review?(evilpies) → review+
Attachment #8861926 - Flags: review?(evilpies) → review+
Comment on attachment 8861930 [details] [diff] [review]
bug1317383-part3-byteoffset.patch

Review of attachment 8861930 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/TypedArrayObject.cpp
@@ +799,4 @@
>          uint32_t len;
> +        if (lengthIndex == UINT64_MAX) {
> +            // Steps 11.a, 11.c.
> +            if (bufferByteLength % sizeof(NativeType) != 0 || byteOffset > bufferByteLength) {

Nice, much easier to understand and closer to the spec.

@@ +926,1 @@
>      fromBuffer(JSContext* cx, HandleObject bufobj, uint32_t byteOffset, int32_t lengthInt)

Ideally this wouldn't even be signed.
Attachment #8861930 - Flags: review?(evilpies) → review+
Attachment #8861931 - Flags: review?(evilpies) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/884baa5893d6
Part 1: Move NonStandardToIndex to SIMD.cpp. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb52543a6c6
Part 2: Use ToIndex when constructing TypedArray with length argument (ES2017). r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bec9b3fa558
Part 3: Use ToIndex when TypedArray constructor is called with ArrayBuffer (ES2017). r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a5552662b2
Part 4: Add step references to TypedArray constructor methods. r=evilpie
Keywords: checkin-needed
(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> For what it's worth, it's causing an error in a Web Platform test case
> (/testing/web-platform/mozilla/tests/wasm/jsapi.js) because there a test
> tries to do `new Uint8Array('hi')` and this fails at the WPT harness level.
> So suspect an unexpected pass there when this gets fixed.

Who forgot about Benjamin's heads-up? I did! :-/
Backed out bug 1358246, bug 1354974 and bug 1317383 for failing various tests in different test suites:

Bug 1317383
https://hg.mozilla.org/integration/mozilla-inbound/rev/edaf81997a7bd28d3fd6c1955ef52b837b183ff5
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc2539cd72c91ae71633c8aea460577e1120647c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cb0cfe2b32ef6c3ad3096b2cbfca879d375e5da
https://hg.mozilla.org/integration/mozilla-inbound/rev/12a29d618d6eb7ed27c0e8f3b1ce551fa8ef0880

Bug 1354974
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc19ec159be24735c18f3afdee00c6b9c881b39f

Bug 1358246
https://hg.mozilla.org/integration/mozilla-inbound/rev/2baf4e5a516aa6a8eb7ca7080dc7d8c8f41e5c25

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=999318b53733c5ccef06fcf87025cf00d515b0af&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Most of the failures look related to bug 1317383, but not sure about these failures in wpt's /_mozilla/wasm/jsapi.js.html:

https://treeherder.mozilla.org/logviewer.html#?job_id=95206554&repo=mozilla-inbound

[task 2017-04-28T13:53:40.253238Z] 13:53:40     INFO - TEST-PASS | /_mozilla/wasm/jsapi.js.html | unexpected success in assertInstantiateError 
[task 2017-04-28T13:53:40.253621Z] 13:53:40     INFO - TEST-PASS | /_mozilla/wasm/jsapi.js.html | unexpected success in assertInstantiateError 
[task 2017-04-28T13:53:40.253707Z] 13:53:40     INFO - TEST-UNEXPECTED-FAIL | /_mozilla/wasm/jsapi.js.html | unexpected success in assertInstantiateError - assert_equals: expected true but got false
[task 2017-04-28T13:53:40.254089Z] 13:53:40     INFO - assertInstantiateError/</<@http://web-platform.test:8000/_mozilla/wasm/js/jsapi.js:725:21
[task 2017-04-28T13:53:40.254159Z] 13:53:40     INFO - Async*assertInstantiateError/<@http://web-platform.test:8000/_mozilla/wasm/js/jsapi.js:719:20
[task 2017-04-28T13:53:40.254558Z] 13:53:40     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1409:20
[task 2017-04-28T13:53:40.254628Z] 13:53:40     INFO - promise callback*promise_test@http://web-platform.test:8000/resources/testharness.js:529:31
[task 2017-04-28T13:53:40.254728Z] 13:53:40     INFO - assertInstantiateError@http://web-platform.test:8000/_mozilla/wasm/js/jsapi.js:718:9
[task 2017-04-28T13:53:40.254790Z] 13:53:40     INFO - @http://web-platform.test:8000/_mozilla/wasm/js/jsapi.js:744:5
[task 2017-04-28T13:53:40.255032Z] 13:53:40     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1409:20
[task 2017-04-28T13:53:40.255709Z] 13:53:40     INFO - test@http://web-platform.test:8000/resources/testharness.js:501:9
[task 2017-04-28T13:53:40.255779Z] 13:53:40     INFO - testJSAPI@http://web-platform.test:8000/_mozilla/wasm/js/jsapi.js:711:1
[task 2017-04-28T13:53:40.255880Z] 13:53:40     INFO - @http://web-platform.test:8000/_mozilla/wasm/js/jsapi.js:17:11
[task 2017-04-28T13:53:40.256403Z] 13:53:40     INFO - 

Looks like the stack changed: https://dxr.mozilla.org/mozilla-central/rev/2cca333f546f38860f84940d4c72d7470a3410f4/testing/web-platform/mozilla/tests/wasm/js/jsapi.js#725
Flags: needinfo?(andrebargull)
For what it's worth, the wasm outage can be dealt with by just removing the /testing/web-platform/mozilla/meta/wasm/jsapi.js.html.ini file (that would need a confirmation by trial, of course).

Can you also, in /js/src/jit-tests/tests/wasm/spec/jsapi.js, look for the TODOs, remove them and re-enable the tests below, please? (they're just commented out at the moment)
(In reply to Benjamin Bouvier [:bbouvier] from comment #20)
> For what it's worth, the wasm outage can be dealt with by just removing the
> /testing/web-platform/mozilla/meta/wasm/jsapi.js.html.ini file (that would
> need a confirmation by trial, of course).
> 
> Can you also, in /js/src/jit-tests/tests/wasm/spec/jsapi.js, look for the
> TODOs, remove them and re-enable the tests below, please? (they're just
> commented out at the moment)

Well, it's a bit worse, because the error stack is empty for repeated instantiation calls to a wasm-module: https://hg.mozilla.org/try/rev/a3e57b28db4f6c912734b88677515b956db753d2#l5.1

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6c55d13fa587965ef2ffd240c6694164c134ba5
Attached patch bug1317383-part2-length.patch (obsolete) — Splinter Review
Disable a test from part 2 because it can easily cause OOM errors. Carrying r+.
Attachment #8861926 - Attachment is obsolete: true
Attachment #8863904 - Flags: review+
Updates wasm/spec/jsapi.js because we now properly handle `new Uint8Array("hi!")`. There are new failures for the "assertInstantiateError" tests, because the `error.stack` is empty for some cases, see the updated testing/web-platform/mozilla/meta/wasm/jsapi.js.html.ini exclusions. I didn't investigate why this happens. 

dom/imptests/failures/html/typedarrays/test_constructors.html.json was updated because it was also using non-integer arguments for TypedArray constructors calls: http://searchfox.org/mozilla-central/source/dom/imptests/html/typedarrays/test_constructors.html#11

dom/canvas/test/webgl-conf/mochitest-errata.ini was updated because it was using floating point numbers in TypedArray constructor calls: http://searchfox.org/mozilla-central/source/dom/canvas/test/webgl-conf/checkout/conformance2/rendering/instanced-rendering-bug.html#146-149
Flags: needinfo?(andrebargull)
Attachment #8863908 - Flags: review?(bbouvier)
Updates the error message to match the new code in TypedArrayObject.cpp.
Attachment #8863909 - Flags: review?(dtownsend)
Attachment #8863909 - Flags: review?(dtownsend) → review+
Comment on attachment 8863908 [details] [diff] [review]
bug1317383-part5-update-wpt-tests.patch

Review of attachment 8863908 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you! I'll try to investigate the wasm web platform test failures soon.
Attachment #8863908 - Flags: review?(bbouvier) → review+
Fix hg commit description.
Attachment #8863904 - Attachment is obsolete: true
Attachment #8864422 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/548e11d30cad
Part 1: Move NonStandardToIndex to SIMD.cpp. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a0d65bba22a
Part 2: Use ToIndex when constructing TypedArray with length argument (ES2017). r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/55bb2cb43c1e
Part 3: Use ToIndex when TypedArray constructor is called with ArrayBuffer (ES2017). r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/88aa549e686e
Part 4: Add step references to TypedArray constructor methods. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f25ab890f79
Part 5: Update expected results for wpt typedarray and wasm tests. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/c66a7dc6df75
Part 6: Update error message matching for addon sdk. r=mossop
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.