André Bargull [:anba] Assignee
	Description • 8 years ago

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

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 1 • 7 years ago

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.

	André Bargull [:anba] Assignee
	Comment 2 • 7 years ago

Moves NonStandardToIndex to SIMD.cpp

	André Bargull [:anba] Assignee
	Comment 3 • 7 years ago

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.

	André Bargull [:anba] Assignee
	Comment 4 • 7 years ago

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.

	André Bargull [:anba] Assignee
	Comment 5 • 7 years ago

Adds some spec step references to the TypedArray constructor.

	Tom S [:evilpie]
	Comment 6 • 7 years ago

Did you want to ask for review for this?

	André Bargull [:anba] Assignee
	Comment 7 • 7 years ago

(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. :-)

	Julien Cristau [:jcristau]
	Comment 8 • 7 years ago

Mass wontfix for bugs affecting firefox 52.

	André Bargull [:anba] Assignee
	Comment 10 • 7 years ago

NonStandardToIndex() is only used in SIMD.cpp, so let's move it into that file.

	André Bargull [:anba] Assignee
	Comment 11 • 7 years ago

The comment #3 still applies for this change.

	André Bargull [:anba] Assignee
	Comment 12 • 7 years ago

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.

	André Bargull [:anba] Assignee
	Comment 13 • 7 years ago

Adds some spec step references to the TypedArray constructor methods.

	Tom S [:evilpie]
	Comment 14 • 7 years ago

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)

	Tom S [:evilpie]
	Comment 15 • 7 years ago

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.

	André Bargull [:anba] Assignee
	Comment 16 • 7 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27f3061caa7cc2c908cd282cd77eee795c037fde

	Pulsebot
	Comment 17 • 7 years ago

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

	André Bargull [:anba] Assignee
	Comment 18 • 7 years ago

(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! :-/

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 19 • 7 years ago

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

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 20 • 7 years ago

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)

	André Bargull [:anba] Assignee
	Comment 21 • 7 years ago

(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

	André Bargull [:anba] Assignee
	Comment 22 • 7 years ago

Disable a test from part 2 because it can easily cause OOM errors. Carrying r+.

	André Bargull [:anba] Assignee
	Comment 23 • 7 years ago

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

	André Bargull [:anba] Assignee
	Comment 24 • 7 years ago

Updates the error message to match the new code in TypedArrayObject.cpp.

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 25 • 7 years ago

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.

	André Bargull [:anba] Assignee
	Comment 26 • 7 years ago

Fix hg commit description.

	André Bargull [:anba] Assignee
	Comment 27 • 7 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c56f85a04554726d46f177d7ecfb8d8f981c6caf

	Pulsebot
	Comment 28 • 7 years ago

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

	Carsten Book [:Tomcat]
	Comment 29 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/548e11d30cad
https://hg.mozilla.org/mozilla-central/rev/0a0d65bba22a
https://hg.mozilla.org/mozilla-central/rev/55bb2cb43c1e
https://hg.mozilla.org/mozilla-central/rev/88aa549e686e
https://hg.mozilla.org/mozilla-central/rev/8f25ab890f79
https://hg.mozilla.org/mozilla-central/rev/c66a7dc6df75

	Florian Scholz (Open Web Docs)
	Comment 30 • 7 years ago

Added to Fx 55 developer release notes
https://developer.mozilla.org/en-US/Firefox/Releases/55#JavaScript

Updated reference pages
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Int8Array
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8ClampedArray
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Int16Array
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint16Array
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Int32Array
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint32Array
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Float32Array
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Float64Array