-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[js-api] add tests for global imports with GC types #498
base: main
Are you sure you want to change the base?
Conversation
Adds JS API tests for the "read the imports" part of the spec for globals.
Update: I ran the tests in Firefox/SM as well and they all pass if I change |
The issue here (IIUC) is that 'read the imports' generally tries to throw LinkError, but it can delegate to Digging a little deeper, it seems like we have a pattern in 'read the imports' of catching every case that could cause |
https://bugs.webkit.org/show_bug.cgi?id=264655 Reviewed by Justin Michaud. The GC proposal JS API was recently updated (WebAssembly/gc#467) to allow direct import of reftype globals in more cases. This patch also includes a version of the pending WPT tests for this case (tracked upstream here: WebAssembly/gc#498) which should be updated later if there are any changes in the accepted upstream version. * JSTests/wasm/gc/js-api.js: (testImport.): (testImport): * LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/gc/global-import.tentative.any.js: Added. (setup.doLink): (setup): (test): * Source/JavaScriptCore/wasm/js/JSWebAssemblyHelpers.h: (JSC::fromJSValue): * Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp: (JSC::WebAssemblyModuleRecord::initializeImports): Canonical link: https://commits.webkit.org/272367@main
Sorry, I had somehow missed this PR before the holidays. I agree that it looks like the original intention was that all failures in linking throw LinkError. Perhaps the most modular fix to the spec is to catch a TypeError from ToWebAssemblyValue and rethrow it as LinkError? |
This PR adds tests for the update to the JS API made in #467 to enable global imports of various reftype values.
These tests pass in V8 and there's a pending patch for JSC. I'm looking into the status for Spidermonkey.
There is one open question with these tests, which is that all the error cases are assumed to throw a
WebAssembly.LinkError
. However, I believe the current spec actually mandates throwingTypeError
instead. Which behavior would we prefer here?(note: V8 & Spidermonkey currently disagree on the error type to throw)