1733289 - WebAssembly.Global doesn't accept "anyfunc" value type

Status: RESOLVED FIXED

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

Description

[Asumu Takikawa]

The following JS shell interaction should succeed, but it currently errors:

js> new WebAssembly.Global({ value: "anyfunc" }, null);
typein:1:1 TypeError: bad value type
Stack:
  @typein:1:1

Providing "funcref" instead works. The Wasm JS API spec uses "anyfunc" only: https://webassembly.github.io/spec/js-api/index.html#globals

There's an upstream update in WPT that tests this behavior: https://github.com/web-platform-tests/wpt/commit/0286e8069d9d592a69ba8aaac7a90cd8afd2115f#diff-631970eae3abaebb8e90fc901c0725f3eea12e792f2c83b45b2b96b647a56dcb

Comment 1

[Lars T Hansen [:lth]]

This is fallout from changing ToValType to accept "funcref" instead of "anyfunc", as the code for WA.Global descriptor parsing simply calls ToValType and has not changed in a long time. We have a couple test cases in wasm/ref-types/funcref.js that use 'funcref' here, those test cases came in with that change.

Comment 2

[Lars T Hansen [:lth]]

Paper trail:

In (this PR)[https://github.com/WebAssembly/reference-types/pull/85] the committee discusses whether to rename "anyfunc" to "funcref" in the js api and (so far) decides against it, the PR is still open.

(This issue)[https://github.com/WebAssembly/js-types/issues/25] is the tracker for that problem, and it's still open, so the current name is indeed "anyfunc".

The author of the WPT PR cited in comment 0 is the same as the author of the above PR (and works for Google). I think everything is probably self-consistent at this point and we should make this change.

Comment 3

[Lars T Hansen [:lth]]

Quoting myself on the PR mentioned in comment 2:

FWIW, through an oversight Firefox has been accepting 'funcref' exclusively for the last two years, while Chrome has been accepting 'anyfunc' for at least the last three years, judging from git blame. I have received zero bug reports about the incompatibility until Asumu just pointed me to a bug about WPT adding some tests for this (web-platform-tests/wpt@0286e80#diff-631970eae3abaebb8e90fc901c0725f3eea12e792f2c83b45b2b96b647a56dcb)

Since Chrome accepts anyfunc only I will fix this in Firefox, but I think the compat concerns are probably a little overblow. (Of course there could be workarounds for the discrepancy out there, but again, I've received no bug reports.)

In addition to fixing the code, we have to fix test cases, and we have to fix our spec test importer too, because it generates 'funcref' in some places, see eg the ref_null test.

Comment 4

[Lars T Hansen [:lth]]

Attachment: Bug 1733289 - The JS API name is 'anyfunc', not 'funcref'. r?rhunt

Make ToValType recognize anyfunc instead of funcref.

Make the test case converter generate anyfunc instead of funcref.

Change all test cases that use funcref to use anyfunc, in the JS API.

The main issue remaining here is whether to keep accepting funcref as an
alias for anyfunc, given that we've been accepting funcref for the last
two years. There could be workarounds for this Chrome/Firefox discrepancy
on the web, and it would be conservative to allow both, at least for
a time.

Comment 5

[Lars T Hansen [:lth]]

FWIW, for table descriptors we seem to use a different code path, where we recognize either anyfunc or funcref. We should probably be consistent here and do the same for globals. Will update the patch, but will also consider other problems, such as js-types.

Comment 6

[Lars T Hansen [:lth]]

Of course, js-types should produce "anyref" not "funcref", and when I change that I get zero test failures, so we're not testing comprehensively...

Comment 7

[Phabricator Automation]

Comment on attachment 9243910 [details]
Bug 1733289 - The JS API name is 'anyfunc', not 'funcref'. r?rhunt

Revision D127248 was moved to bug 1733288. Setting attachment 9243910 [details] to obsolete.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ff04e10cd23
The JS API name is 'anyfunc', not 'funcref'. r=rhunt

Comment 9

[Cristian Tuns]

Backed out for causing Spidermonkey bustages and web-platform-tests failures.

• Backout link
• Push with failures
• Failure Log SM
• Failure Log WPT

Comment 10

[Lars T Hansen [:lth]]

Some tests that were failing are now passing and expectations need to be updated.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c84ca8ed2360
The JS API name is 'anyfunc', not 'funcref'. r=rhunt

Comment 12

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/c84ca8ed2360

Comment 13

[Marco Castelluccio [:marco]]

Set release status flags based on info from the regressing bug 1546138