1486000 - Extend wasm js-api tests

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect)

Description

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

Attachment: Patch v1

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9003801 [details] [diff] [review]
Patch v1

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

Thanks for doing this! Your patch revealed many spec issues (or that the current spec website is out of date), so it needs to be fixed first before we can merge tests here.

::: testing/web-platform/tests/wasm/jsapi/global/constructor.any.js
@@ +101,5 @@
> +    assert_Global(global, 0);
> +  }, `Default value for type ${type}`);
> +
> +  const valueArguments = [
> +    [undefined, 0],

That's incorrect, our impl returns NaN for f32/f64, which is ToNumber(undefined), which is what the spec dictates.

::: testing/web-platform/tests/wasm/jsapi/global/value-set.any.js
@@ +1,4 @@
> +// META: global=jsshell
> +
> +test(() => {
> +  const argument = { "value": "i32" };

nit: unused

@@ +90,5 @@
> +
> +  const setter = desc.set;
> +  assert_equals(typeof setter, "function");
> +
> +  assert_throws(new TypeError(), () => setter.call(global));

The spec might be unclear here, but it says that the setter just calls ToWebAssembly on the value, which passes down to ToInt32, which would return 0 and not throw here. Am I missing something?

::: testing/web-platform/tests/wasm/jsapi/instance/constructor.any.js
@@ +28,5 @@
>      assert_true(property.enumerable, `${key}: enumerable`);
>      assert_false(property.configurable, `${key}: configurable`);
>      const actual = property.value;
> +    assert_true(Object.isExtensible(actual), `${key}: extensible`);
> +    

nit: remove blank line+trailing space

::: testing/web-platform/tests/wasm/jsapi/memory/grow.any.js
@@ +25,5 @@
> +
> +test(() => {
> +  const argument = { "initial": 0 };
> +  const memory = new WebAssembly.Memory(argument);
> +  assert_throws(new TypeError(), () => memory.grow());

The spec is unclear: http://webassembly.github.io/spec/js-api/index.html#memories links to https://webassembly.github.io/spec/core/appendix/embedding.html#embed-grow-mem which just asserts that the *provided integer* is not negative, implying that the provided argument has been converted to an integer. We apply ToNumber to the argument in our impl, but that would probably need to be properly defined on the spec side first.

@@ +164,5 @@
> +for (const value of outOfRangeValues) {
> +  test(() => {
> +    const argument = { "initial": 0 };
> +    const memory = new WebAssembly.Memory(argument);
> +    assert_throws(new TypeError(), () => memory.grow(value));

Again, this is unclear since there's no coercion mentioned in the spec but the wasm spec talks about an integer.

::: testing/web-platform/tests/wasm/jsapi/table/constructor.any.js
@@ +24,5 @@
>    assert_throws(new TypeError(), () => new WebAssembly.Table());
>  }, "No arguments");
>  
>  test(() => {
> +  const argument = { "element": "anyfunc", "initial": 0 };

good catch.

@@ +124,5 @@
> +    },
> +    "initial": 1,
> +  });
> +  assert_Table(table, { "length": 1 });
> +}, "Type conversion for descriptor.element");

This behavior should be spec'd, I think. At the moment, element: anyfunc isn't even checked in the ctor's spec... http://webassembly.github.io/spec/js-api/index.html#tables

@@ +163,5 @@
> +  });
> +
> +  assert_array_equals(order, [
> +    "element",
> +    "element toString",

ditto

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

This is unclear in the spec, which is invoking `>=`, thus probably implicitly coercing with ToNumber (as we do). Should be refined in the spec first.

@@ +205,5 @@
> +  test(() => {
> +    const argument = { "element": "anyfunc", "initial": 1 };
> +    const table = new WebAssembly.Table(argument);
> +    assert_throws(new TypeError(), () => table.get(value));
> +  }, `Getting out-of-range argument: ${format_value(value)}`);

ditto

@@ +211,5 @@
> +  test(() => {
> +    const argument = { "element": "anyfunc", "initial": 1 };
> +    const table = new WebAssembly.Table(argument);
> +    assert_throws(new TypeError(), () => table.set(value, null));
> +  }, `Setting out-of-range argument: ${format_value(value)}`);

ditto

::: testing/web-platform/tests/wasm/jsapi/table/grow.any.js
@@ +1,3 @@
> +// META: global=jsshell
> +
> +function assert_equal_to_array(table, expected, message) {

Is there a way to share code in a directory? This function is also implemented in get-set.any.js at least.

@@ +92,5 @@
> +  test(() => {
> +    const argument = { "element": "anyfunc", "initial": 1 };
> +    const table = new WebAssembly.Table(argument);
> +    assert_throws(new TypeError(), () => table.grow(value));
> +  }, `Out-of-range argument: ${format_value(value)}`);

Again, the spec doesn't mention any coercion here, so it needs to be fixed first.

Comment 2

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

Comment on attachment 9003801 [details] [diff] [review]
Patch v1

(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> Comment on attachment 9003801 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 9003801 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for doing this! Your patch revealed many spec issues (or that the
> current spec website is out of date), so it needs to be fixed first before
> we can merge tests here.

Note that the spec uses WebIDL, so it should be read in conjunction with that.
 
> ::: testing/web-platform/tests/wasm/jsapi/global/constructor.any.js
> @@ +101,5 @@
> > +    assert_Global(global, 0);
> > +  }, `Default value for type ${type}`);
> > +
> > +  const valueArguments = [
> > +    [undefined, 0],
> 
> That's incorrect, our impl returns NaN for f32/f64, which is
> ToNumber(undefined), which is what the spec dictates.

The spec:

If v is undefined,
    let value be DefaultValue(valuetype).

DefaultValue() returns the appropriate representation of zero.


> ::: testing/web-platform/tests/wasm/jsapi/global/value-set.any.js
> @@ +90,5 @@
> > +
> > +  const setter = desc.set;
> > +  assert_equals(typeof setter, "function");
> > +
> > +  assert_throws(new TypeError(), () => setter.call(global));
> 
> The spec might be unclear here, but it says that the setter just calls
> ToWebAssembly on the value, which passes down to ToInt32, which would return
> 0 and not throw here. Am I missing something?

The behaviour of the setter function is defined here: <https://heycam.github.io/webidl/#dfn-attribute-setter>.

Step 3.1: "If no arguments were passed, then throw a TypeError."


> ::: testing/web-platform/tests/wasm/jsapi/memory/grow.any.js
> @@ +25,5 @@
> > +
> > +test(() => {
> > +  const argument = { "initial": 0 };
> > +  const memory = new WebAssembly.Memory(argument);
> > +  assert_throws(new TypeError(), () => memory.grow());
> 
> The spec is unclear:
> http://webassembly.github.io/spec/js-api/index.html#memories links to
> https://webassembly.github.io/spec/core/appendix/embedding.html#embed-grow-
> mem which just asserts that the *provided integer* is not negative, implying
> that the provided argument has been converted to an integer. We apply
> ToNumber to the argument in our impl, but that would probably need to be
> properly defined on the spec side first.

If you're using "implies" when interpreting a spec, something's gone wrong. The IDL is:

  unsigned long grow([EnforceRange] unsigned long delta);

The definition of the function is here: <https://heycam.github.io/webidl/#dfn-create-operation-function>.

Step 2.1.3: n = 0.
Step 2.1.4: S is the set containing a single tuple (grow, « [EnforceRange] unsigned long », « required »).
Step 2.1.5: Calls the overload resolution algorithm: (1) maxarg = 1; (2) n = 0; (3) argcount = 0; (4) S becomes empty; (5) TypeError is thrown.

so we never get to step 2.1.8.1 where we'd call <https://webassembly.github.io/spec/js-api/#dom-memory-grow>.

[A]


> @@ +164,5 @@
> > +for (const value of outOfRangeValues) {
> > +  test(() => {
> > +    const argument = { "initial": 0 };
> > +    const memory = new WebAssembly.Memory(argument);
> > +    assert_throws(new TypeError(), () => memory.grow(value));
> 
> Again, this is unclear since there's no coercion mentioned in the spec but
> the wasm spec talks about an integer.

A coercion is mentioned here:

  unsigned long grow([EnforceRange] unsigned long delta);

Conversion to [EnforceRange] unsigned long is defined here: <https://heycam.github.io/webidl/#es-unsigned-long> and here: <https://heycam.github.io/webidl/#abstract-opdef-converttoint>, which throws an exception in step 6.

[B]


> ::: testing/web-platform/tests/wasm/jsapi/table/constructor.any.js
> @@ +124,5 @@
> > +    },
> > +    "initial": 1,
> > +  });
> > +  assert_Table(table, { "length": 1 });
> > +}, "Type conversion for descriptor.element");
> 
> This behavior should be spec'd, I think. At the moment, element: anyfunc
> isn't even checked in the ctor's spec...
> http://webassembly.github.io/spec/js-api/index.html#tables

Indeed it is. Here's the IDL:

enum TableKind {
  "anyfunc",
};

dictionary TableDescriptor {
  required TableKind element;
};

[Constructor(TableDescriptor descriptor)]
interface Table {};

Conversion to dictionary types <https://heycam.github.io/webidl/#es-dictionary> forwards to the conversion to enumeration types: <https://heycam.github.io/webidl/#es-enumeration>, which calls ToString() first, and then checks if the result is in the single-element list « "anyfunc" ».

> @@ +163,5 @@
> > +  });
> > +
> > +  assert_array_equals(order, [
> > +    "element",
> > +    "element toString",
> 
> ditto

Conversion to dictionary types <https://heycam.github.io/webidl/#es-dictionary> defines the order, and the conversion operations it forwards to define the toString/valueOf calls.

> ::: testing/web-platform/tests/wasm/jsapi/table/get-set.any.js
> @@ +39,5 @@
> > +
> > +test(() => {
> > +  const argument = { "element": "anyfunc", "initial": 5 };
> > +  const table = new WebAssembly.Table(argument);
> > +  assert_throws(new TypeError(), () => table.get());
> 
> This is unclear in the spec, which is invoking `>=`, thus probably
> implicitly coercing with ToNumber (as we do). Should be refined in the spec
> first.

Ditto as the explanation for `grow` marked [A].

> @@ +205,5 @@
> > +  test(() => {
> > +    const argument = { "element": "anyfunc", "initial": 1 };
> > +    const table = new WebAssembly.Table(argument);
> > +    assert_throws(new TypeError(), () => table.get(value));
> > +  }, `Getting out-of-range argument: ${format_value(value)}`);
> 
> ditto

Ditto as the explanation for `grow` marked [B].
 
> @@ +211,5 @@
> > +  test(() => {
> > +    const argument = { "element": "anyfunc", "initial": 1 };
> > +    const table = new WebAssembly.Table(argument);
> > +    assert_throws(new TypeError(), () => table.set(value, null));
> > +  }, `Setting out-of-range argument: ${format_value(value)}`);
> 
> ditto

Ditto [B].

> ::: testing/web-platform/tests/wasm/jsapi/table/grow.any.js
> @@ +92,5 @@
> > +  test(() => {
> > +    const argument = { "element": "anyfunc", "initial": 1 };
> > +    const table = new WebAssembly.Table(argument);
> > +    assert_throws(new TypeError(), () => table.grow(value));
> > +  }, `Out-of-range argument: ${format_value(value)}`);
> 
> Again, the spec doesn't mention any coercion here, so it needs to be fixed
> first.

Ditto [B].


Fixed your other comments.

Comment 3

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

Comment on attachment 9003801 [details] [diff] [review]
Patch v1

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

::: testing/web-platform/tests/wasm/jsapi/global/constructor.any.js
@@ +54,5 @@
> +  assert_array_equals(order, [
> +    "descriptor mutable",
> +    "descriptor value",
> +    "descriptor value toString",
> +    "value valueOf",

(Fixed missing parentheses here.)

Comment 4

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

Attachment: Failures on Chrome

Failures in Chrome attached; they're mostly similar to SpiderMonkey's. (Just over half the failures are RangeErrors that should be TypeErrors.)

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9003801 [details] [diff] [review]
Patch v1

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

Alright, I totally missed WebIDL indeed, looks good then (and we have many things to fix!). Thanks.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18c0aff587fa
Extend wasm js-api tests; r=bbouvier

Comment 7

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

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

Comment 8

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

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/18c0aff587fa

Comment 10

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

Upstream PR merged