1490274 - Check for missing arguments in WebAssembly APIs

Status: RESOLVED FIXED

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

Description

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

Spec: First five steps of https://heycam.github.io/webidl/#dfn-overload-resolution-algorithm

Tested in:

wasm/jsapi/global/value-set.any.js (setter of the value attribute)
wasm/jsapi/memory/grow.any.js
wasm/jsapi/module/customSections.any.js
wasm/jsapi/table/get-set.any.js
wasm/jsapi/table/grow.any.js

(which runs in jstests and wpt).

Probably some other methods are missing the checks too, but pass because converting undefined to the argument type throws a TypeError; it might be worth seeing if it'd be helpful to change the checks to improve error messages.

Comment 1

[Lars T Hansen [:lth]]

In implementation terms this means that we check that the number of actual arguments matches the number of formal parameters, taking into count optional parameters (in such a case as the "set" operation on Table, where the second parameter is optional).  We have no type overloads.

Comment 2

[Lars T Hansen [:lth]]

Nit: One of the table.set tests seem to be in error; the second argument to set is optional and defaults to null (which is also a valid argument).

Also, so far as I can tell, WebIDL requires that not too many arguments be passed; if I read that correctly then it would be useful to expand the test programs with cases that pass too many arguments.

Comment 3

[Lars T Hansen [:lth]]

BTW, I've started using a distinguished error message for the argument count checking, as suggested; this should indeed allow for more sophisticated testing.

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1490274-wasm-jsapi-arg-count-checks.patch

Benjamin for the code, Ms2ger for the test case fix.

Comment 5

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

Comment on attachment 9009127 [details] [diff] [review]
bug1490274-wasm-jsapi-arg-count-checks.patch

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

> There's also a bug fix here; the second argument to WebAssembly.Table::set is optional, defaulting to null.  It was previously required.

I don't understand this comment.

::: js/src/wasm/WasmJS.cpp
@@ +614,2 @@
>  {
> +    if (args.length() != numargs) {

This is incorrect; you're always allowed to pass *more* arguments than required. ("Initialize argcount to be min(maxarg, n).") Now I need to write tests for that :)

(args.requireAtLeast() will be the idiomatic check.)

Comment 6

[Lars T Hansen [:lth]]

(In reply to :Ms2ger (he/him; ⌚ UTC+1/+2) from comment #5)
> Comment on attachment 9009127 [details] [diff] [review]
> bug1490274-wasm-jsapi-arg-count-checks.patch
> 
> Review of attachment 9009127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > There's also a bug fix here; the second argument to WebAssembly.Table::set is optional, defaulting to null.  It was previously required.
> 
> I don't understand this comment.

This pertains to the change to setImpl in this patch.  In our previous implementation of this API, the second argument to set() was required and we would throw if it was not present, but the WebIDL API makes it an optional argument whose default value is null.

> ::: js/src/wasm/WasmJS.cpp
> @@ +614,2 @@
> >  {
> > +    if (args.length() != numargs) {
> 
> This is incorrect; you're always allowed to pass *more* arguments than
> required. ("Initialize argcount to be min(maxarg, n).") Now I need to write
> tests for that :)

You're right, I misread the algorithm, misreading 'argcount' as 'n' in step 4 that prunes the set S.

Comment 7

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

(In reply to Lars T Hansen [:lth] from comment #6)
> (In reply to :Ms2ger (he/him; ⌚ UTC+1/+2) from comment #5)
> > Comment on attachment 9009127 [details] [diff] [review]
> > bug1490274-wasm-jsapi-arg-count-checks.patch
> > 
> > Review of attachment 9009127 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > > There's also a bug fix here; the second argument to WebAssembly.Table::set is optional, defaulting to null.  It was previously required.
> > 
> > I don't understand this comment.
> 
> This pertains to the change to setImpl in this patch.  In our previous
> implementation of this API, the second argument to set() was required and we
> would throw if it was not present, but the WebIDL API makes it an optional
> argument whose default value is null.

It's not optional? The spec I'm looking at has

interface Table {
  void set([EnforceRange] unsigned long index, Function? value);
};

What you're saying would be

interface Table {
  void set([EnforceRange] unsigned long index, optional Function? value = null);
};


> > ::: js/src/wasm/WasmJS.cpp
> > @@ +614,2 @@
> > >  {
> > > +    if (args.length() != numargs) {
> > 
> > This is incorrect; you're always allowed to pass *more* arguments than
> > required. ("Initialize argcount to be min(maxarg, n).") Now I need to write
> > tests for that :)
> 
> You're right, I misread the algorithm, misreading 'argcount' as 'n' in step
> 4 that prunes the set S.

Yeah, it's probably one of the least readable parts of WebIDL :/

Comment 8

[Lars T Hansen [:lth]]

Attachment: bug1490274-wasm-jsapi-arg-count-checks-v2.patch

Much simpler than the previous one.  Note the change to the get-set test case, one argument should be allowed here, it's just zero arguments that should be prohibited.

Comment 9

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

Comment on attachment 9009609 [details] [diff] [review]
bug1490274-wasm-jsapi-arg-count-checks-v2.patch

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

::: testing/web-platform/tests/wasm/jsapi/table/get-set.any.js
@@ -61,4 @@
>    const argument = { "element": "anyfunc", "initial": 5 };
>    const table = new WebAssembly.Table(argument);
>    assert_throws(new TypeError(), () => table.set());
> -  assert_throws(new TypeError(), () => table.set(0));

Still not seeing how this is correct.

Comment 10

[Lars T Hansen [:lth]]

(In reply to :Ms2ger (he/him; ⌚ UTC+1/+2) from comment #9)
> Comment on attachment 9009609 [details] [diff] [review]
> bug1490274-wasm-jsapi-arg-count-checks-v2.patch
> 
> Review of attachment 9009609 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/web-platform/tests/wasm/jsapi/table/get-set.any.js
> @@ -61,4 @@
> >    const argument = { "element": "anyfunc", "initial": 5 };
> >    const table = new WebAssembly.Table(argument);
> >    assert_throws(new TypeError(), () => table.set());
> > -  assert_throws(new TypeError(), () => table.set(0));
> 
> Still not seeing how this is correct.

Here's my reasoning: according to the spec, Table.set looks like this:

  void set([EnforceRange] unsigned long index, Function? value);

Thus at least one argument is required but the second argument is optional, defaulting to null (which would also be a valid argument, were it provided).  The value 0 for the first argument is valid and should not itself cause a TypeError, so any error here must be caused by the second argument, but I don't see how it could be.

Comment 11

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

(In reply to Lars T Hansen [:lth] from comment #10)
> (In reply to :Ms2ger (he/him; ⌚ UTC+1/+2) from comment #9)
> > Comment on attachment 9009609 [details] [diff] [review]
> > bug1490274-wasm-jsapi-arg-count-checks-v2.patch
> > 
> > Review of attachment 9009609 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: testing/web-platform/tests/wasm/jsapi/table/get-set.any.js
> > @@ -61,4 @@
> > >    const argument = { "element": "anyfunc", "initial": 5 };
> > >    const table = new WebAssembly.Table(argument);
> > >    assert_throws(new TypeError(), () => table.set());
> > > -  assert_throws(new TypeError(), () => table.set(0));
> > 
> > Still not seeing how this is correct.
> 
> Here's my reasoning: according to the spec, Table.set looks like this:
> 
>   void set([EnforceRange] unsigned long index, Function? value);
> 
> Thus at least one argument is required but the second argument is optional,
> defaulting to null (which would also be a valid argument, were it provided).
> The value 0 for the first argument is valid and should not itself cause a
> TypeError, so any error here must be caused by the second argument, but I
> don't see how it could be.

There's no "optional" keyword, so the second argument isn't optional. The "?" only means it accepts null.

Comment 12

[Lars T Hansen [:lth]]

> > 
> > Here's my reasoning: according to the spec, Table.set looks like this:
> > 
> >   void set([EnforceRange] unsigned long index, Function? value);
> > 
> > Thus at least one argument is required but the second argument is optional,
> > defaulting to null (which would also be a valid argument, were it provided).
> > The value 0 for the first argument is valid and should not itself cause a
> > TypeError, so any error here must be caused by the second argument, but I
> > don't see how it could be.
> 
> There's no "optional" keyword, so the second argument isn't optional. The
> "?" only means it accepts null.

Ouch, the notation conspiracy strikes again.  I will fix it.

Comment 13

[Lars T Hansen [:lth]]

Attachment: bug1490274-wasm-jsapi-arg-count-checks-v3.patch

This only required an update to the test case, relative to the previous patch.  (So in fact, the code in the previous patch was incorrect relative to what I thought it should be, too. :-P )

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9009878 [details] [diff] [review]
bug1490274-wasm-jsapi-arg-count-checks-v3.patch

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

I guess excessive arguments are fine. Thanks, lgtm.

::: js/src/wasm/WasmJS.cpp
@@ +610,4 @@
>  }
>  
>  static bool
> +GetModuleArg(JSContext* cx, CallArgs args, uint32_t required, const char* name, Module** module)

Maybe rename required => numRequired instead?

Comment 15

[Lars T Hansen [:lth]]

Comment on attachment 9009878 [details] [diff] [review]
bug1490274-wasm-jsapi-arg-count-checks-v3.patch

Don't actually need review from ms2ger since no changes to wpt.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/434c466717cf
wasm JS api argument count checking. r=bbouvier

Comment 17

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/434c466717cf