Closed Bug 1733289 Opened 4 months ago Closed 4 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
95 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox-esr91 --- wontfix
firefox92 --- wontfix
firefox93 --- wontfix
firefox94 --- wontfix
firefox95 --- fixed

People

(Reporter: asumu, Assigned: lth)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

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

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.

Assignee: nobody → lhansen
Severity: -- → S2
Status: NEW → ASSIGNED
Priority: -- → P2
Regressions: 1546138
Regressed by: 1546138
No longer regressions: 1546138
Has Regression Range: --- → yes

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.

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.

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.

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.

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 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.

Attachment #9243910 - Attachment is obsolete: true
Attachment #9243910 - Attachment is obsolete: false
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ff04e10cd23
The JS API name is 'anyfunc', not 'funcref'. r=rhunt

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

Flags: needinfo?(lhansen)

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

Flags: needinfo?(lhansen)
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c84ca8ed2360
The JS API name is 'anyfunc', not 'funcref'. r=rhunt
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch

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

You need to log in before you can comment on or make changes to this bug.