Skip to content
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

Latest WebAssembly ESM Integration rebase #10380

Merged
merged 26 commits into from
Sep 4, 2024

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented May 30, 2024

WebAssembly ESM Integration including Source Phase Imports

This represents the latest rebase of the ESM Integration spec, replacing the original PR in #4372. Major changes since then include source phase imports support and instantiation being updated to be synchronous.

There is an early validation error algorithm that is applied when modules are created, which needed to be updated to support the source phase imports proposal. Specifically, constructed modules may not have their dependencies loaded when in the source phase. This validation logic is now located in HostLoadImportedModule, and performed against all referrer dependencies on the first call to HostLoadImportedModule for a given referrer, detected by the first argument matching the first module of the Cyclic Module Record (indicating the module is loading its dependencies).

Since the ESM Integration specification is still in Phase 3, its specification is referenced directly here. As soon as the proposal is upstreamed we can reference the main WebAssembly JS API specification.

(See WHATWG Working Mode: Changes for more details.)


/acknowledgements.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/references.html ( diff )
/webappapis.html ( diff )

@guybedford
Copy link
Contributor Author

guybedford commented May 30, 2024

For the question of why Wasm and JS are treated as equal capabilities in the module system, the reasoning here is that unlike other resource imports which are implemented as synthetic modules, Wasm modules are implemented as full cyclic module records that can in turn import any other JS modules. Therefore while Wasm itself is usually a securely sandboxed language, as soon as you have arbitrary imports to JS, you have equal capability since any Wasm module imported this way can import and therefore execute arbitrary JS in turn.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@guybedford
Copy link
Contributor Author

Thanks for the review, I've addressed the changes in f816de8.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolo-ribaudo could you have a look at this please? Looks editorially okay to me.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️

@guybedford
Copy link
Contributor Author

I've integrated the latest changes here so we should be good to go.

Just let me know if I can do anything to help get the participation check passing.

@domenic
Copy link
Member

domenic commented Jul 14, 2024

Just let me know if I can do anything to help get the participation check passing.

It looks like you signed up as an individual at some point, but you work for Fastly, which has signed up as an entity. However, Fastly has only signed up to contribute to Fetch.

So this is a bit complicated. The ideal steps needed are:

@guybedford
Copy link
Contributor Author

The participation check is now passing. @domenic thanks for the clear help with this.

@domenic
Copy link
Member

domenic commented Jul 17, 2024

This looks ready to merge to me, but I'm not 100% sure that we have multiple browsers interested in implementing exactly this shape of the proposal. We have zero browsers passing the tests, for example: https://wpt.fyi/results/wasm/webapi/esm-integration?label=experimental&label=master&aligned

@annevk
Copy link
Member

annevk commented Jul 17, 2024

Although the WebKit team at Apple was initially somewhat skeptical about bundling JS and Wasm as a single destination, we're okay with that now under the expectation that the privileges and capabilities of JS and Wasm won't diverge.

Who can speak for Chromium, maybe @syg?

And Gecko would be @codehag or @mgaudet perhaps?

@codehag
Copy link

codehag commented Jul 17, 2024

This looks ok from our side.

@guybedford
Copy link
Contributor Author

@ajklein may also be able to speak to the current progress on the V8 implementation here.

@ajklein
Copy link
Contributor

ajklein commented Aug 5, 2024

Speaking to implementation progress: in V8/Chromium, we are currently working on supporting source phase imports for loading WebAssembly modules as source (tracked in https://issues.chromium.org/issues/42204365). We are not currently prioritizing work on full ESM/Wasm integration. Essentially, we're working on item (1) of the Wasm ESM Integration proposal's Progressive Implementation Support section.

@syg
Copy link
Contributor

syg commented Aug 6, 2024

IIUC, this PR adds Wasm support for both source phase imports (e.g. import source foo from 'bar.wasm') and evaluation phase imports (e.g. import 'bar.wasm'). As @ajklein says in #10380 (comment), V8/Chromium has planned work on source phase imports only.

It's also my understanding that the WPT tests linked above test both source phase imports and evaluation phase imports.

@annevk Is WebKit planning to implement and ship Wasm evaluation phase imports as well as source phase imports?

@codehag What about Gecko?

If the answer to both questions above is yes, then seems fine. Otherwise I feel that we should separate the source phase support from the evaluation phase support, as we shouldn't merge something that won't (fully) reflect reality for a while.

@codehag
Copy link

codehag commented Aug 8, 2024

Thanks for the ping @syg. My understanding is there are two parts to your question:

  1. Will Gecko Support both source phase imports and evaluation phase imports for wasm?

If they are both standardized, our plan is to support both. The work is not yet scheduled.

  1. Given the changes to the spec in recent years, source phase imports make more sense for wasm. Should we support both source phase imports and evaluation phase imports right now, or should we wait on evaluation phase imports to see if they have a use case?

This PR was initially specifying evaluation phase imports, before source phase imports were discussed in TC39. Evaluation phase imports have been supported by webkit behind a flag for some time IIUC. However, most people importing wasm today are doing so as "source" first, and then instantiating it themselves. This was a major motivation for source phase imports in TC39 to begin with. This begs the question: If evaluation phase imports are specified, will they actually be used? Or are we supporting them here purely out of spec inertia? I don't actually know the answer here myself.

Consistency between JS and WASM importing would be nice, especially since they will be using the same syntax. I could see some users experiencing confusion if imports between WASM and JS are not equivalent. Maybe @guybedford could speak to whether evaluation phase is important for users?

We (Gecko) could go either way. It may make sense to wait just because source phase is more useful. But, if the answer to "will evaluation phase imports be used" is that "yes, evaluation phase imports are important and we won't learn anything new if we wait", then we might as well spec this now and gradually adopt as per the plan in the Progressive module support document.

cc @eqrion @dminor for visibility and any further comments.

@syg
Copy link
Contributor

syg commented Aug 8, 2024

Thanks, @codehag. The "we will support if standardized" is a bit entangled so let me try to disentangle that AFAIU:

  • Evaluation phase Wasm ESM integration is not yet standardized / ready to implement. Whether evaluation phase Wasm ESM integration is standardized is being done in Wasm CG, not WhatWG.
  • If Wasm CG standardizes evaluation phase imports, we need HTML support.
  • Source phase Wasm ESM integration is already Stage 3 in TC39.
  • We need HTML support for source phase imports.

IOW, my understanding of this HTML PR is purely to support proposals standardized elsewhere. We don't really need to get into the actual motivations for the other proposals here, there're other venues for that already. But because this PR is purely about support, I want it to reflect the most likely interoperable future.

Best I can tell, source phase imports is definitely in that future. I don't know if evaluation phase imports is in that future yet. Chrome isn't working on it right now, thus the question to Mozilla and Apple if you have a different opinion.

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Aug 8, 2024

Yes, this PR adds HTML support for wasm imports regardless of their phase (source vs evaluation). Which phases are supported is entirely controlled by the Wasm proposal, which defines the WebAsssembly Module Record.

In general, import phases have been designed to be opaque to HTML's loader, and only "visible" to ECMA-262 and whoever provides custom Module Record types (it's this obviously visible to HTML if it for CSS modules, since they are defined here).

@guybedford
Copy link
Contributor Author

While the motivation for source phase imports remains that it is considered the more useful route for the instantiation of WebAssembly modules, this does not negate the need for or usefulness of the instance / evaluation phase even if those are considered a much smaller use case at this point. There are many packages on npm today that completely work with standards-compatible ESM evaluation phase integration.

There are no plans to deprecate or remove the evaluation phase. The standardization of the ESM Integration spec at the Wasm CG into Phase 3 included both phases, with the original spec mechanics and WPT tests for the evaluation phase remaining throughout the source phase process per the Webkit implementation, with just a couple of small changes (mainly moving to synchronous instantiation). Per the README note linked, the weakening of the implementation constraint to allow shipping one without the other specifically does not indicate that either should not ever be implemented - the standard includes both phases and that is what is expected to be implemented.

Node.js ships the instance phase under the --experimental-wasm-modules flag already. For Node.js, our plan once V8 source phase syntax is shipped in Node.js is to then unflag with both the source and evaluation phase supported.

As @syg points out, the conformance decision here is entirely made in the ESM Integration spec though. We could possibly add some further clarifications on the HTML side to note both phase are specified if that would be useful.

@syg
Copy link
Contributor

syg commented Aug 8, 2024

Yes, this PR adds HTML support for wasm imports regardless of their phase (source vs evaluation). Which phases are supported is entirely controlled by the Wasm proposal, which defines the WebAsssembly Module Record.

This was part of my question: is it exactly the same? That is, if you were to limit this proposal to just supporting source phase imports, can you remove stuff from this PR? E.g. this PR talks about module requests for WebAssembly Module Scripts, which isn't needed for source phase imports, right?

@nicolo-ribaudo
Copy link
Contributor

That step there also affects import source. i.e. this will result in an error (assuming that there is no import map mapping "not a valid URL" to something):

import source foo from "./x.wasm"
(module
  (import "not a valid URL" "num" i32)
)

That however raises indeed a question: is this intentional? @guybedford

@nicolo-ribaudo
Copy link
Contributor

I talked a bit with Guy about that, and no that is definitely not intentional. Doing import source x from "foo" should not validate "foo"'s imports (regardless of whether foo is a WASM source, of a JS source if/when we'll add them).

The reason HTML has that check is that we want to abort the loading process as early as possible when we know that it will fail: in case of

import "./valid.js";
import "invalid specifier";

we want to avoid starting the network request for ./valid.js, since we can easily tell that the process will fail anyway.

It seems like we can get the same behavior by moving those from "create a XXX module script" to the beginning of HostLoadImportedModule, validating referrer.[[RequestedModules]] before doing anything else. There is no observable work happening between when "create a JavaScript module script" returns and when the first HostLoadImportedModule call for its dependencies happen, so the only difference is that we'd do this pre-validation only when we know that we want to load the dependencies of that module.

Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now looks good to me, I'm somewhat confident that it preserve all the existing behavior.

Btw, there are some problems with the Wattsi server. Yesterday I was going to send a long review explaining how the changes were still wrong because the Wattsi preview was stuck at an old commit rather than at the last one that passed the build.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a few nits; please check that they didn't change anything.

I have two questions:

  • I'm confused on the name of the [WASMESM] specification. It calls itself "WebAssembly JavaScript Interface" in the heading of https://webassembly.github.io/esm-integration/js-api/index.html . But that name is already taken by our [WASMJS] reference, https://webassembly.github.io/spec/js-api/ ? This PR seems to call it "WebAssembly JavaScript Module Integration" but I cannot find any evidence of that being an official referenceable name.

  • Can you summarize why we relocated all the dependency checking to HostResolveImportedModule? That's a large change.

Additionally, if you could update the OP with as helpful a commit message as possible, that would be great. Explaining what exactly changes, what the boundary is between the two specs, maybe the different import phases.

hubot pushed a commit to v8/v8 that referenced this pull request Sep 2, 2024
Add source phase import support in import statements with necessary
embedder APIs. And resolve `WebAssembly.Module` as a ModuleSource
with static source phase imports in d8.

In HTML draft integration spec, importing `WebAssembly.Module` at
source phase doesn't need any import attribute to indicate the module
type. The default type `javascript-or-wasm` is distinguished by
resource's MIME type. However, whether or not a requested module is JS
or WASM is determined by file extensions in d8.

Refs: whatwg/html#10380
Bug: 42204365
Change-Id: I095dcb4375c4980c9aa37ff85365ee44b3823cba
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5783137
Reviewed-by: Shu-yu Guo <[email protected]>
Commit-Queue: Chengzhong Wu (legendecas) <[email protected]>
Cr-Commit-Position: refs/heads/main@{#95914}
@guybedford
Copy link
Contributor Author

@domenic nits all look good to me. I do have a slight preference for Module Record over Cyclic Module Record terminology, but being clear works just as good. I've updated the top-line of the PR comment to include the answers to your questions, let me know if that answers things for you?

If there's a better way to handle the Phase 3 status of the ESM Integration in references, I'm open to alternatives.

@domenic
Copy link
Member

domenic commented Sep 4, 2024

If there's a better way to handle the Phase 3 status of the ESM Integration in references, I'm open to alternatives.

Could you give the document a different title, instead of having two documents with the same title?

@guybedford
Copy link
Contributor Author

Sure, I've pushed a commit to update the ESM integration reference to be the spec repo instead of the spec document, and set the title as the direct proposal's name.

@domenic
Copy link
Member

domenic commented Sep 4, 2024

Sorry, that wasn't what I meant; I meant, can you update the spec document? I'm just very surprised the WASM CG is putting out two specs with the same name but different contents, and am unsure whether we should be referencing WASM CG documents if that's their status.

Edit: to be clear, I mean, can you update this line: https://github.com/WebAssembly/esm-integration/blob/8dbbb4dd19b479299cbe2f501a8b5ced9b61fbe8/document/js-api/index.bs#L2

@guybedford
Copy link
Contributor Author

Okay, reverted the last commit and it seems the exceptions proposal did similar here previously in altering the title to include the proposal name. I've landed the title update in WebAssembly/esm-integration#93, but still need to get the spec document to update properly through CI, so will fix that tomorrow and ping back here then. Let me know if there's anything else, and thanks for the reviews!

@guybedford
Copy link
Contributor Author

@domenic the title on the spec is now updated, so we should finally be there now.

@domenic domenic added addition/proposal New features or enhancements topic: script labels Sep 4, 2024
@domenic domenic merged commit 10ed38e into whatwg:main Sep 4, 2024
2 checks passed
@nicolo-ribaudo
Copy link
Contributor

I don't see a comment answering

Can you summarize why we relocated all the dependency checking to HostResolveImportedModule? That's a large change.

The summary is that we can split module loading in two phases:

  1. Load a module
  2. Load its dependencies

The first one ends at the end of the "create a X module" steps, and the second one is in HostLoadImportedModule.

Somewhere between 1 and 2 we need to validate those dependencies. Whether it was at the end of 1 or at the beginning of 2 was originally not observable. With source phase imports it is observable, because we can stop after 1, and we only want the validation to happen when we know that 2 is going to happen.

@LePichu
Copy link

LePichu commented Sep 6, 2024

I know I am kinda late but I haven't seen this anywhere, shouldn't this work to interop work on Import Assertions too in someway? Historically bundlers have allowed you to bundle binary blobs like PNGs, and WASM blobs inline, and text data like JSON too, Import Assertions were supposed to allow for better readability and trying to make a standard way of importing non-JS modules into JS, shouldn't this do something like import { foo } from "./bar.wasm" with { type: "wasm" } instead?

@nicolo-ribaudo
Copy link
Contributor

This has been discussed multiple times — a goal here is making it possible to swap JS modules with wasm modules in a transparent way.

The goal of import attributes on the web was security: JSON and CSS have less capabilities than JS, and the attribute is there to make sure that somebody doesn't try to attack you by providing a JS file where you expect a JSON one. Wasm instead can import JS, and it runs at the same privilege level of JS.

This is similar to how there is no type: "JS".

Note that bundlers can decide to support their own custom attributes, as long as they strip them away when bundling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: script
Development

Successfully merging this pull request may close these issues.

8 participants