1488473 - Wasm js-api should use TypeError for out-of-range inputs

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P3)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

As defined in https://heycam.github.io/webidl/#abstract-opdef-converttoint

Tested in:

wasm/jsapi/memory/constructor.any.js
wasm/jsapi/memory/grow.any.js
wasm/jsapi/table/constructor.any.js
wasm/jsapi/table/get-set.any.js
wasm/jsapi/table/grow.any.js

(which run in jstests and wpt).

Comment 1

[Lars T Hansen [:lth]]

Fascinating, I can find no spec language anywhere restricting the memory's maximum value to be at least as large as its minimum value, or any language about what should happen if that is not true.  For tables there is such language, and it requires a RangeError.  The reason for the present bug is that we reuse some of the error signaling for the int clamping.

Comment 2

[Lars T Hansen [:lth]]

There's a bug in some of the table tests here.  assert_equal_to_array performs a t.get(-1) and expects a RangeError.  Not so: the WebIDL argument is "[EnforceRange] unsigned long" which means the range check on entry happens first, and that throws TypeError (exactly what this bug is about).  A reference above the table limit should throw a RangeError though...

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1488473-throw-appropriate-error.patch

Benjamin for the code (trivial though the changes may be); Ms2ger for the updated test cases.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 9009158 [details] [diff] [review]
bug1488473-throw-appropriate-error.patch

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

::: testing/web-platform/tests/wasm/jsapi/table/assertions.js
@@ +1,5 @@
>  function assert_equal_to_array(table, expected, message) {
>    assert_equals(table.length, expected.length, `${message}: length`);
> +  // The argument check in get() happens before the range check, and negative numbers
> +  // are illegal, hence will throw TypeError per spec.
> +  assert_throws(new TypeError(), () => table.get(-1), `${message}: table.get(-1)`);

Indeed, thank you.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9009158 [details] [diff] [review]
bug1488473-throw-appropriate-error.patch

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

For consistency with other WebIDL code, we could use the same errors as in https://searchfox.org/mozilla-central/source/dom/bindings/PrimitiveConversions.h#228, but that would probably mean moving them in js.msg and be a bit messy. Oh well.

Comment 6

[Lars T Hansen [:lth]]

There's another problem here, described in https://github.com/WebAssembly/spec/issues/879 - do we throw TypeError or RangeError for a value above the implementation max?  (Above 2^32-1 we must throw TypeError per WebIDL spec.)  Right now, as a fallout from other fixes, we end up throwing TypeError; this is sane if you assume that (a) WebIDL made the right choice here and (b) we simply use the impl max as the upper limit of the allowable range instead of 2^32-1.  I happen to think (a) is false but I can't do anything about that.

Comment 7

[Lars T Hansen [:lth]]

Attachment: bug1488473-throw-appropriate-error-v2.patch

Incorporates fixes to our own test cases too.  Note there are open spec bugs linked from this bug, if they can be resolved quickly I will hold this bug until they are.

Carrying r+ from bbouvier and ms2ger.

Did not do anything about trying to harmonize with Firefox's error message for similar case.

Comment 8

[Lars T Hansen [:lth]]

Spec #876 is tending toward (is virtually resolved as) throwing RangeError for Memory maximum < initial, as we do for Table.

Spec #879 is tending toward throwing RangeError when the argument is in bounds for unsigned long but larger than the implementation maximum.  The present patch throws TypeError here (and there are many changes to our own test cases to reflect that); I'll update the code and undo the change to the test cases, since I think RangeError is the better behavior, it just requires a little more code.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54e9cd8211b3
throw the appropriate exception object for each error.  r=bbouvier, r=ms2ger
https://hg.mozilla.org/integration/mozilla-inbound/rev/03d7d05c12a2
Adjust WPT copy of mozilla jsapi tests. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a79ebf58e4d
Remove some expected-fail clauses because errors thrown are now the right ones. r=me

Comment 10

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13151 for changes under testing/web-platform/tests

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/54e9cd8211b3
https://hg.mozilla.org/mozilla-central/rev/03d7d05c12a2
https://hg.mozilla.org/mozilla-central/rev/0a79ebf58e4d

Comment 12

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/13151
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/HYzmnZEWRbu5fjoIKSQb3Q)

Comment 13

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged