-
Notifications
You must be signed in to change notification settings - Fork 36
[js-api] Throw RangeError when getArg's index is out of bounds #196
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Do you want to update the "Getting out-of-range argument" subtest in getArg.tentative.any.js
as well?
@thibaudmichaud Is this change OK in the V8 implementation of JS API? |
Yes, this change should be straightforward. |
@thibaudmichaud Has this been implemented? I ran this on https://github.com/web-platform-tests/wpt and it is failing: Running 3 tests in web-platform-tests
▶ Unexpected subtest result in /wasm/jsapi/exception/getArg.tentative.any.worker.html:
│ FAIL [expected PASS] Getting out-of-range argument
│ → assert_throws_js: function "() => exn.getArg(tag, value)" threw object "TypeError: WebAssembly.Exception.getArg(): Index must be convertible to a valid number" ("TypeError") expected instance of function "function RangeError() { [native code] }" ("RangeError")
│
│ at Test.<anonymous> (http://web-platform.test:8000/wasm/jsapi/exception/getArg.tentative.any.js:46:5)
│ at Test.step (http://web-platform.test:8000/resources/testharness.js:2595:25)
│ at test (http://web-platform.test:8000/resources/testharness.js:628:30)
└ at http://web-platform.test:8000/wasm/jsapi/exception/getArg.tentative.any.js:26:1
▶ Unexpected subtest result in /wasm/jsapi/exception/getArg.tentative.any.html:
│ FAIL [expected PASS] Getting out-of-range argument
│ → assert_throws_js: function "() => exn.getArg(tag, value)" threw object "TypeError: WebAssembly.Exception.getArg(): Index must be convertible to a valid number" ("TypeError") expected instance of function "function RangeError() { [native code] }" ("RangeError")
│
│ at Test.<anonymous> (http://web-platform.test:8000/wasm/jsapi/exception/getArg.tentative.any.js:46:5)
│ at Test.step (http://web-platform.test:8000/resources/testharness.js:2595:25)
│ at test (http://web-platform.test:8000/resources/testharness.js:628:30)
└ at http://web-platform.test:8000/wasm/jsapi/exception/getArg.tentative.any.js:26:1
Ran 3 tests finished in 8.8 seconds.
• 1 ran as expected. 1 tests skipped.
• 2 tests had unexpected subtest results |
It was implemented, but if the argument cannot be converted to a uint32, we throw a TypeError rather than a RangeError. This seems very similar to table.grow, which also throws a TypeError in this case: https://github.com/WebAssembly/spec/blob/main/test/js-api/table/grow.any.js. Wdyt about changing the getArg spec test for consistency? I am not sure why we didn't catch that earlier though, we are supposed to include these tests in our test suite, I'll look into it. |
I didn't see that the test was only added to wpt a few hours ago, so that answers my last concern. |
Doh, my mistake in reviewing the test change. It seems like the "Index out of bounds" test above the changed one already expected the new behavior. |
Closes #184.