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

WebAssembly JS and Web integration spec in Bikeshed #591

Merged
merged 16 commits into from
Dec 6, 2017

Conversation

littledan
Copy link
Collaborator

Initial checkin, JS.bs is converted from JS.md with
updates to reference the current Wasm specification.

Initial checkin, JS.bs is converted from JS.md with
updates to reference the current Wasm specification.
@littledan littledan changed the title WebAssembly JS integraton spec in Bikeshed WebAssembly JS integration spec in Bikeshed Oct 22, 2017
@binji
Copy link
Member

binji commented Oct 22, 2017

Awesome! Is there a link to the generated page online somewhere?

@littledan
Copy link
Collaborator Author

littledan commented Oct 22, 2017

You can see a generated copy here: https://littledan.github.io/spec/document/js-api/index.html . I was imagining that we'd include it in gh-pages for WebAssembly/spec when this lands.

Edit: And see https://littledan.github.io/spec/document/web/index.html

- Ensure that not passing a maximum Memory size in the object will not
  create a maximum in Wasm.
- Clean up notes about object deduplication in instantiate.
- Refer to Promises guide everywhere.
- Remove duplicate buffer cloning.
@rossberg
Copy link
Member

Great work! Here is a first round of comments (sorry, was reading th IO rendering, too tedious to break up into individual GH comments):

Section 1:

  • s/demo.was/demo.wat/ (.wat is the recommended extension for the text format)

Section 2:

  • What is the meaning of DOMString in outside the web?

  • Definition of valid module: Step 2 makes no sense, since the AST contains no UTF-8. The constraint is enforced at decoding time from binaries.

  • Definition of compile(bytes): Nit: call the result moduleObject, to be consistent with def of "compile a module"?

  • Definition of instantiate: nesting of steps seems to be broken, e.g. step 7 or step 26

  • "Note: ... is checked by instantiate_module below" (occurs multiple times) -- Nit: say "by instantiate_module as invoked below" for clarity

  • Def of instantiate, step 20.3.3: formatting error

  • Def of instantiate, step 34: formatting error

  • Definition of instantiate(bytes): nesting of steps seems to be broken, e.g. step 3

Section 2.1:

  • Definition of exports: this should go through the module_exports function.

  • Definition of imports: this should go through the module_imports function.

Section 2.2:

  • IDL: typo "Contructor"

  • Add link for "instantiates the WebAssembly module"

Section 2.3:

  • Note on StructuredSerializeWithTransfer: type "to to". But in fact, I think this note should not be here, since it is web-specific.

  • Def of grow: maybe the core spec should add mem_size to for hygiene.

  • Def of grow, step 10: formatting error

  • Def of grow, step 11: typo, missing "to"

Section 2.4:

  • Def of creating, step 1: formatting error

  • Def of Table constructor, step 3: m could be undefined

  • Def of grow: Nit: switch steps 5 and 6, so that we don't bake in the assumption that the store is unmutated in the case of an error

  • Def of length: formatting error; typo "reciever"

  • Def of set: Nit: same as above, switch steps 6 and 7

Section 2.5:

  • Def of create, step 10: function addresses don't have an intrinsic or unique function index associated with them. It would probably be cleaner to make it a parameter to the create algorithm

  • Def of call: broen formatting of nested steps, e.g. step 7

Section 2.7:

  • Can we avoid mentioning DOMException here, which does not exist outside the web?

Section 3:

  • The wording shouldn't assume that the embedder is a browser.

Section 4:

  • s/Wasm/WebAssembly/

  • "each WA memory instance is identified with a JavaScript Data Block" -- only a subset is, because a memory that's not exported will never be visible in JS; also, should this say ECMAScript?

@rossberg
Copy link
Member

PS: I didn't see the latest commit in time, so some comments may already be obsolete.

@rossberg
Copy link
Member

One infra comment: before landing, can you move this to document/js-api/JS.html? The idea is to mirror the ./core vs ./js-api vs ./web directory structure in test/. Then we should adopt a similar structure on the gh-pages side and add a front page linking to the different spec documents in the subdirectories.

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.

Got about halfway through before I have to run to an appointment. Seems like there might still be some polish ongoing in the later sections especially? Let me know if it'd be helpful to continue.

document/JS.bs Outdated
Repository: WebAssembly/spec
Abstract: This document provides an explicit JavaScript API for interacting with WebAssembly.
Markup Shorthands: css no, markdown yes
Ignored Terms: h1, h2, h3, h4, h5, h6, xmp
Copy link
Member

Choose a reason for hiding this comment

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

Why would these ignored terms be necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deleted (cargo-culting another spec).

document/JS.bs Outdated

<pre class='biblio'>
{
"ECMA-262": {
Copy link
Member

Choose a reason for hiding this comment

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

This is not necessary. Just use [ECMASCRIPT] and it'll work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

More cargo-culting; fixed.

document/JS.bs Outdated
"publisher": "ECMA TC39",
"status": "Current Editor's Draft"
},
"WEBASSEMBLY": {
Copy link
Member

Choose a reason for hiding this comment

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

We should work on getting this added to specref so you don't need it here. I can't remember what the procedure is for new specs but I think there are docs.

document/JS.bs Outdated
type: exception; for: ECMAScript
text: Error; url: sec-error-objects
text: NativeError; url: sec-nativeerror-constructors
type: dfn
Copy link
Member

Choose a reason for hiding this comment

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

You're getting hit heavily by speced/bikeshed#809 and speced/bikeshed#861 here. Not much you can do right now, just a heads-up.

document/JS.bs Outdated
Memory or Table)
WebAssemblyRuntimeValue;

typedef (record&lt;DOMString, WebAssemblyRuntimeValue&gt;) InstanceExportsMap;
Copy link
Member

Choose a reason for hiding this comment

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

Parens are unnecessary here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed.

document/JS.bs Outdated
1. Perform ! CreateDataProperty(obj, `"module"`, |module|).
1. Perform ! CreateDataProperty(obj, `"name"`, |name|).
1. Perform ! CreateDataProperty(obj, `"kind"`, |kind|).
1. Append |obj</obj> to the end of |imports|.
Copy link
Member

Choose a reason for hiding this comment

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

Bad variable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

document/JS.bs Outdated
1. Perform ! CreateDataProperty(obj, `"name"`, |name|).
1. Perform ! CreateDataProperty(obj, `"kind"`, |kind|).
1. Append |obj</obj> to the end of |imports|.
1. Return ! CreateArrayFromList(|imports|).
Copy link
Member

Choose a reason for hiding this comment

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

No need to CreateArrayFromList. This is a Web IDL function, which returns sequence<>s, which are lists. The Web IDL layer handles the conversion to JS for you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

document/JS.bs Outdated
* `"table"` if |export|.desc is of the form 𝗍𝖺𝖻𝗅𝖾 tableidx
* `"memory"` if |export|.desc is of the form 𝗆𝖾𝗆 memidx
* `"global"` if |export|.desc is of the form 𝗀𝗅𝗈𝖻𝖺𝗅 globalidx
1. Let |obj| be ! ObjectCreate(%ObjectPrototype%).
Copy link
Member

Choose a reason for hiding this comment

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

Dictionary instances are not JS objects, so you should not be creating JS objects here. Instead you can return an Infra struct, or just something like "a ModuleExportDescriptor whose name is |name| and kind is |kind|".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Used something like the second wording.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I use ordered map operations on the import and export objects? Treating them this way probably will be an observable change, on the importing side, as it was based on getting specific things that were asked for. If I don't make that change, is there any IDL i can use to stand for an uninterpreted object?

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the problem a bit more? How would it be an observable change? What change are you proposing, now that this seems to be fixed?

document/JS.bs Outdated
1. Let |bytes| be |module|.\[[Bytes]].
1. Let |customSections| be an empty List.
1. For each [=custom section=] |customSection| in the binary format of |bytes|,
1. Let |name| be the <code>name</code> of the custom section, decoded as UTF-8.
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

document/JS.bs Outdated
The <dfn constructor for="Module">Module(bytes)</dfn> constructor, when invoked, performs the follwing steps:

1. Let |stableBytes| be a [=get a copy of the buffer source|copy of the bytes held by the buffer=] |bytes|.
1. [=Compile a WebAssembly module|compiles the WebAssembly module=] |stableBytes| and returns the result.
Copy link
Member

Choose a reason for hiding this comment

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

Weird linking text

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

document/JS.bs Outdated
This API is, initially, the only API for accessing WebAssembly [[WEBASSEMBLY]] from the Web Platform, through a bridge to explicitly construct modules from ECMAScript [[ECMASCRIPT]].

Issue: In future versions, WebAssembly
be loaded and run directly from an HTML `<script type='module'>` tag—and
Copy link
Member

Choose a reason for hiding this comment

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

may be

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

document/JS.bs Outdated

<p><em>This section is non-normative.</em></p>

Given `demo.was` (encoded to `demo.wasm`):
Copy link
Member

Choose a reason for hiding this comment

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

demo.wat

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

document/JS.bs Outdated
-->

<div algorithm>
A WebAssembly module AST |module| is said to be <dfn>valid</dfn> if:
Copy link
Member

Choose a reason for hiding this comment

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

Just WebAssembly |module|

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed any mention of AST (this particular one disappeared in a refactoring)

@littledan
Copy link
Collaborator Author

Thanks for the extremely prompt and detailed reviews. I apologize for the typographical errors!

s/demo.was/demo.wat/ (.wat is the recommended extension for the text format)

Fixed

What is the meaning of DOMString in outside the web?

It just means String, in WebIDL-speak. I filed a bug to make this clickable.

Definition of valid module: Step 2 makes no sense, since the AST contains no UTF-8. The constraint is enforced at decoding time from binaries.

Fixed.

Definition of compile(bytes): Nit: call the result moduleObject, to be consistent with def of "compile a module"?

Done

Definition of instantiate: nesting of steps seems to be broken, e.g. step 7 or step 26

Fixed.

"Note: ... is checked by instantiate_module below" (occurs multiple times) -- Nit: say "by instantiate_module as invoked below" for clarity

I wasn't sure how much duplication to leave in. The notes that are there in this version are much more brief than those in JS.md. Are any of these needed at all?

Def of instantiate, step 20.3.3: formatting error

Fixed

Def of instantiate, step 34: formatting error

Fixed

Definition of instantiate(bytes): nesting of steps seems to be broken, e.g. step 3

Fixed

Definition of exports: this should go through the module_exports function.

Done

Definition of imports: this should go through the module_imports function.

Done

IDL: typo "Contructor"

Fixed

Add link for "instantiates the WebAssembly module"

Done

Note on StructuredSerializeWithTransfer: type "to to". But in fact, I think this note should not be here, since it is web-specific.

Made the note more generic, and wrote what was there before in a comment.

Def of grow: maybe the core spec should add mem_size to for hygiene.

It could; this is pretty minor though. Another thing that the spec could do for hygiene is make grow_mem return the old length of the memory, as the internal operation does.

Def of grow, step 10: formatting error

I'm not sure how to do a vertical bar in this markdown dialect; it's probably not so clear anyway. Switched to saying "the length of".

Def of grow, step 11: typo, missing "to"

Fixed

Def of creating, step 1: formatting error

Switched to not use vertical bars.

Def of Table constructor, step 3: m could be undefined

Fixed.

Def of grow: Nit: switch steps 5 and 6, so that we don't bake in the assumption that the store is unmutated in the case of an error

I don't get it; seems like the assumption is baked into the signature of grow_mem. How could this be addressed within JS.bs? Switching steps wouldn't really make sense.

Def of length: formatting error; typo "reciever"

Fixed (cleaning up the wording)

Def of set: Nit: same as above, switch steps 6 and 7

Same as above

Def of create, step 10: function addresses don't have an intrinsic or unique function index associated with them. It would probably be cleaner to make it a parameter to the create algorithm

You win the prize--you found the sloppy wording that I was hoping no one would notice! I'm not sure how to make it a parameter--ExportedFunctions are created from all sorts of places, like when a Table exists on module exports, during instantiation. Maybe reviving #560 could help?

Def of call: broen formatting of nested steps, e.g. step 7

Fixed

Can we avoid mentioning DOMException here, which does not exist outside the web?

Removed

The wording shouldn't assume that the embedder is a browser.

Added "another runtime system" as a further possibility.

s/Wasm/WebAssembly/

Done

"each WA memory instance is identified with a JavaScript Data Block" -- only a subset is, because a memory that's not exported will never be visible in JS; also, should this say ECMAScript?

Heh, WHATWG specs stubbornly refuse to say ECMAScript; I was following that school of thought.. I don't have an opinion here one way or another.

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.

Made it to the bottom this time, I think.

One overall thing is that header casing is inconsistent. I think most web specs use sentence case, i.e. only the first word is capitalized. This spec uses a mix of that and Title Case.

document/JS.bs Outdated
1. Let |module| be [=decode_module=](|bytes|). If |module| is [=error=], return [=error=].
1. If [=validate_module=](|module|) is [=error=], return [=error=].
1. For each <code>name</code> production in |bytes|, including in a custom <code>name</code> section if present, with the subsequence identified as |nameBytes|:
1. If running the [=UTF-8 decoder=] algorithm on |nameBytes| leads to an error, return [=error=].
Copy link
Member

Choose a reason for hiding this comment

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

I think you're not supposed to use this directly. It seems like you want https://encoding.spec.whatwg.org/#utf-8-decode-without-bom-or-fail instead. The naming, at least, is much more explicit.

This happens multiple times throughout.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, specifications should only use the provided hooks and ideally only those with UTF-8 in the name. Interesting that wasm also wants the hard failure mode, just like WebSocket. Most of the platform uses never-failing UTF-8 decode.

Copy link
Member

Choose a reason for hiding this comment

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

Just delete this entire step, decode_module already enforces it.

Copy link
Collaborator Author

@littledan littledan Oct 24, 2017

Choose a reason for hiding this comment

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

@rossberg I see, somehow I missed the name validation in https://webassembly.github.io/spec/binary/values.html#binary-name . Thanks for the pointer. This section doesn't reference the Encoding spec that @domenic mentions; should it?

Copy link
Member

Choose a reason for hiding this comment

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

That's primarily a spec for web en/decoding APIs, which the Wasm core does not need and has no reason to depend on. The Unicode spec is the primary source of truth for the few aspects that matter there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should the name section be validated? I see the specific note,

If an implementation interprets the contents of a custom section, then errors in that contents, or the placement of the section, must not invalidate the module.

If I just remove this section, I don't see how the names will be validated. OTOH, this text currently doesn't seem sufficient already for validating the names section. I'm drafting text to be more strict about it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that comment is important, and what you observe is intentional. The name section, like any custom section, is essentially allowed to be totally broken, which of course implies that names occurring in it may be malformed just as well. The JS API must not enforce anything there either. In fact, I see no reason why the name section should even be mentioned in the API spec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, adding a note that specifically calls out the name section as not validated.

Copy link
Member

@domenic domenic Oct 24, 2017

Choose a reason for hiding this comment

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

That's primarily a spec for web en/decoding APIs

That's not correct.

(It seems like this point is moot though if the section is slated to be removed.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The comment has been removed.

document/JS.bs Outdated

<div algorithm>
The <dfn method for="WebAssembly">validate(bytes)</dfn> method, when invoked, performs the following steps:
1. [=Compile a WebAssembly module|compile=] |bytes| as a WebAssembly module and store the results as |module|.
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize

document/JS.bs Outdated

6. Let |imports| be an empty [=list=] of [=external value=]s.
1. For each (|moduleName|, |componentName|, |externtype|) in [=module_imports=](|module|), do
1. Let |o| be ? [=Get=](|importObject|, |moduleName|).
Copy link
Member

Choose a reason for hiding this comment

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

Again, Web IDL has covered this for you. importObject is not a JS object anymore, but instead an ordered map of (JS string ->(JS string -> (double or ExportedFunction or Memory or Table)).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, this will definitely be an observable semantic change. In particular, we'd no longer search up the prototype chain. I'm not sure how bad that would be, given the point Wasm is at in shipping.

Copy link
Member

Choose a reason for hiding this comment

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

Why wouldn't you search up the prototype chain? Web IDL definitely does that, during the initial conversion steps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The conversion algorithm for records seems to be based on reading all the own property keys as the first step. It seems like this would reach a different result than the algorithm here, which looks at the module to find what to look for and does a simple Get on those, regardless of whether they are own or not. Currently, I have InstanceImportsMap as a record, though maybe there's a better type.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry about that, I guess I just totally forgot how records worked.

If you do need to drop down to JS, you can use the object type. I'm not sure what would be cleaner; it seems kind of nice to do all the JS object stuff ahead of time in one fell swoop then operate on the unobservable spec/implementation data structures, but on the other hand it sounds like that might involve unnecessary work, and changing implementations is annoying.

The ahead-of-time version seems a lot nicer for the async algorithm version (not yet in this document).

I haven't checked that the previous spec was well-followed or detailed enough before your version; that might also be worth investigating. E.g. maybe someone is already using a record-like algorithm today! (We could test with proxies.)

Anyway, the above is just a lot of thoughts. I don't have strong feelings, just hoping we weigh all the options.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I asked at the WebAssembly WG meeting about asynchronous instantiation, and the response was pretty unequivocal: no, that would not make sense. I'll switch to object here for now; I filed a WebIDL bug about another feature mismatch.

Copy link
Member

Choose a reason for hiding this comment

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

I meant for instantiateStreaming. The question is do we delay grabbing object properties until we post a task back to the main thread, or do we do it immediately when the function is called, and let the later work operate without worrying about JS objects. In the former case, we could e.g. change the object's properties while the Response was coming from the network, and expect that to be detected; for the latter, doing so would have no effect.

Copy link
Member

Choose a reason for hiding this comment

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

@littledan the IDL bug you point to is about return values, but this seems to be about something else?

document/JS.bs Outdated
1. Set the current agent cluster's [=associated store=] to |store|.
1. Let |externfunc| be the [=external value=] 𝖿𝗎𝗇𝖼 |funcaddr|
1. [=Append=] |externfunc| to |imports|.
1. If |externtype| is of the form 𝗀𝗅𝗈𝖻𝖺𝗅 |globaltype|,
Copy link
Member

Choose a reason for hiding this comment

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

Nesting seems off here as these steps reference v but appear misnested in the output

document/JS.bs Outdated
1. Let |value| be |memory|.
1. Otherwise, |externtype| is of the form 𝗍𝖺𝖻𝗅𝖾 |tabletype|,
1. Let |value| be the unique |table| in |tables| such that |table|.\[[Table]] is |externval|.
1. Let |status| be ! [=CreateDataProperty=](|exports|, |name|, |value|).
Copy link
Member

Choose a reason for hiding this comment

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

exports -> exportsObject, right?

document/JS.bs Outdated
1. Let |result| be [=grow_table=](|store|, |tableaddr|, |d|).
1. If |result| is [=error=], throw a [=RangeError=] exception.

Note: The above exception may happen due to either insufficient memory or an invalid size parameter.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: since this directly relates to the step above, indenting it so that it's part of the step would look nicer, I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Applying this here and a few more places around the spec.

document/JS.bs Outdated
</div>

<div algorithm>
The <dfn for="Table">grow(d)</dfn> method, when invoked, performs the following steps:
Copy link
Member

Choose a reason for hiding this comment

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

This and others are missing method inside their <dfn open tag. You can tell because in the output they are not monospace.

document/JS.bs Outdated

<pre class='idl'>
[NoInterfaceObject]
interface ExportedFunction : Function { };
Copy link
Member

Choose a reason for hiding this comment

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

Are these Web IDL semantics actually what's intended? We define a new class, not exposed to the global, as if via the following?

class ExportedFunction extends Function {
  constructor() {
    throw new TypeError("Illegal invocation");
  }
}

And the APIs accepting import/export records will throw if they do not pass the brand check for ExportedFunction?

I think this probably shouldn't be a Web IDL type, and you should just use Function in the typedefs for import/export records.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, that's not intended. Changing it to Function and adding the appropriate TypeErrors/checks explicitly. (Actually there was a typo--for imports, we allow ordinary functions, not just Exported Functions).

document/JS.bs Outdated
1. If |funcs| contains any |func| where |func|.\[[ExportedAddress]] is |funcaddr|,
1. Let |func| be that function object.
1. Otherwise:
1. Let |func| be a [=create an exported function|a new exported function=] created from |funcaddr|.
Copy link
Member

Choose a reason for hiding this comment

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

One place uses the text "a new exported function" and another "create an Exported Function"; best to be consistent.

document/JS.bs Outdated
</div>

<div algorithm>
To <dfn>call an Exported Function</dfn> |funcaddr| with arguments |argValues|, perform the following steps:
Copy link
Member

Choose a reason for hiding this comment

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

Although usually I don't declare the types of my arguments in spec algorithms, I think here it might help. I'm guessing this is a WebAssembly function Address and a list of ... JS values?

@littledan littledan changed the title WebAssembly JS integration spec in Bikeshed WebAssembly JS and Web integration spec in Bikeshed Oct 24, 2017
@rossberg
Copy link
Member

rossberg commented Oct 24, 2017 via email

@littledan
Copy link
Collaborator Author

That's good, but now that I think about it, I realise that this must be
stronger than a note/recommendation. There needs to be normative language
somewhere saying that an ArrayBuffer that is used with a memory becomes
undetachable, i.e., any attempt of detaching it must be rejected (either
by ignoring or by throwing an error).

I'm not really sure how to make this stronger than a recommendation. JavaScript doesn't expose any way to detach ArrayBuffers. WebAssembly actually does need to detach the memory here, so the core detach function has to keep accepting it. I could make a new abstract operation in the ECMAScript spec which checks if something is a Memory, and detaches if not, and call this from the HTML spec, but I don't see a way I could prohibit anyone from making another environment which embeds JS and calls the wrong one. It seems equivalent either way, and operationally, it's easier to get this change into HTML, so that's where I was thinking of writing it.

Patch coming shortly to fix the other issues...

@tabatkins
Copy link

One overall thing is that header casing is inconsistent. I think most web specs use sentence case, i.e. only the first word is capitalized. This spec uses a mix of that and Title Case.

CSS has a (unofficial, but maintained fairly strictly) policy to do "This Heading Is In Title Case: the 'text-transform' property"; titlecase before the colon; lowercase afterward. I find it very readable.

@annevk
Copy link
Member

annevk commented Oct 25, 2017

JavaScript doesn't expose any way to detach ArrayBuffers.

postMessage() can do that. What doesn't exist however is a way to prevent detaching. It's either detached or not, there's no third state and I doubt we'd want to introduce that. It'd be good to know in some more detail what this is about.

@rossberg
Copy link
Member

rossberg commented Oct 25, 2017 via email

@annevk
Copy link
Member

annevk commented Oct 25, 2017

@rossberg if you actually need a new hook in ECMAScript please do create a PR at https://github.com/tc39/ecma262. It seems rather bad to fork the way ArrayBuffer works without making that clear to them.

@littledan
Copy link
Collaborator Author

Thanks for the additional feedback; I've addressed the comments in the latest version.

1. Let |module| be |moduleObject|.\[[Module]].
1. Let |imports| be an empty [=list=].
1. For each (|moduleBytes|, |nameBytes|, |type|) in [=module_imports=](|module|),
1. Let |module| be |moduleBytes| [=UTF-8 decode without BOM or fail|decoded as UTF-8=].
Copy link
Member

Choose a reason for hiding this comment

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

Can you redeclare module like that? This seems problematic.

Copy link
Member

Choose a reason for hiding this comment

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

Especially since below in the assert you want to reference the object again.

1. For each (|moduleBytes|, |nameBytes|, |type|) in [=module_imports=](|module|),
1. Let |module| be |moduleBytes| [=UTF-8 decode without BOM or fail|decoded as UTF-8=].
1. Let |name| be |nameBytes| [=UTF-8 decode without BOM or fail|decoded as UTF-8=].
1. Assert: The previous operations did not fail, as |module| is [=valid=].
Copy link
Member

Choose a reason for hiding this comment

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

Again I think this would be better with asserting that neither name nor module is failure.


Note: extra parameters are not allowed, including the empty `` `application/wasm;` ``.

3. If |response| is not [=CORS-same-origin=], [=reject=] |returnValue| with a {{TypeError}} and abort these substeps.
Copy link
Member

Choose a reason for hiding this comment

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

3 -> 1?

Copy link
Collaborator Author

@littledan littledan Nov 10, 2017

Choose a reason for hiding this comment

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

This is done so the numbering continues after the note, though now it's off by one, so fixing it.


Note: Although it is specified here that the response is consumed entirely before compilation proceeds, that is purely for ease of specification; implementations are likely to instead perform processing in a streaming fashion. The different is unobservable, and thus the simpler model is specified. <!-- Using consume is a bit silly as it creates an ArrayBuffer but then we just want the underlying bytes. This is because of how streams is specced in terms of promises and JS objects whereas we want to operate more directly on the underlying concept. We can revisit this if things change in the Streams/Fetch specs. -->

6. [=Upon fulfillment=] of |bodyPromise| with value |bodyArrayBuffer|:
Copy link
Member

Choose a reason for hiding this comment

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

6 -> 1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is done so the numbering continues after the note.

Copy link
Member

@lukewagner lukewagner left a comment

Choose a reason for hiding this comment

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

Sorry for taking to long to read carefully. Overall looks really good!

To <dfn>construct a WebAssembly module object</dfn> from a module |module|, perform the following steps:

1. Let |moduleObject| be a new {{Module}} object.
1. Set |moduleObject|.\[[Module]] to |module|.
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a "Each Module object has two internal slots: " declaration in the Module object section (analogous to Memory et al)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


1. Let |moduleObject| be a new {{Module}} object.
1. Set |moduleObject|.\[[Module]] to |module|.
1. Set |moduleObject|.\[[Bytes]] to |bytes|.
Copy link
Member

Choose a reason for hiding this comment

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

"bytes" wasn't declared as a parameter to this procedure and it also isn't being explicitly passed at the callsite.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, and added a missing call from the Module constructor

<div algorithm>
To <dfn>instantiate a WebAssembly module</dfn> from a {{Module}} |moduleObject| and imports |importObject|, perform the following steps:
1. Let |module| be |moduleObject|.\[[Module]].
1. If |module|.imports is not an empty list, and |importObject| is undefined, throw a {{TypeError}} exception.
Copy link
Member

Choose a reason for hiding this comment

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

Here and below for other core spec record accesses, it would be nice to have a special typeface and link to the field in the core spec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed many of them; will add more links in a follow-on update.

1. Return |promise|
</div>

Note: A follow-on streaming API is documented [in the WebAssembly design repository](https://github.com/WebAssembly/design/blob/master/Web.md#additional-web-embedding-api).
Copy link
Member

Choose a reason for hiding this comment

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

I think this note and link could be updated now to point to your new Web embedding spec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

1. Let |o| be ? [=Get=](|importObject|, |moduleName|).
1. If [=Type=](|o|) is not [=Object=], throw a {{TypeError}} exception.
1. Let |v| be ? [=Get=](|o|, |componentName|)
1. If |externtype| is of the form 𝖿𝗎𝗇𝖼 |functype|,
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the request for record accesses, it'd be nice to give constructors like func a special typeface and link to core spec def.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the typeface is fixed for everything now, and I've added some links. Will add more links soon.

1. If |value| does not have a \[[FunctionAddress]] internal slot, throw a {{TypeError}} exception.
1. Let |funcaddr| be |value|.\[[FunctionAddress]].
1. Let |store| be the current agent cluster's [=associated store=].
1. Let |store| be [=write_table=](|store|, |tableaddr|, |index|, |funcaddr|).
Copy link
Member

Choose a reason for hiding this comment

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

... and similarly s/funcaddr/elem/

1. If |funcinst| is of the form {𝗍𝗒𝗉𝖾 |functype|, 𝗁𝗈𝗌𝗍𝖼𝗈𝖽𝖾 |hostfunc|},
1. Assert: |hostfunc| is a JavaScript object and [=IsCallable=](|hostfunc|) is true.
1. Let |name| be ? [=Get=](|hostfunc|, "name").
1. Return ? [=ToString=](|name|).
Copy link
Member

Choose a reason for hiding this comment

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

In Firefox and Chrome (testing on Linux just now; probably the others too), there is no special case here for host functions and in all cases the function index is used as the name of an exported wasm function.

Copy link
Collaborator Author

@littledan littledan Dec 6, 2017

Choose a reason for hiding this comment

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

I don't understand, how do host functions get a function index?

Copy link
Member

Choose a reason for hiding this comment

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

Their function index is the function index of the function import that was re-exported.

1. Let |arity| be the length of |arguments|.
1. Perform ! [=DefinePropertyOrThrow=](|function|, `"length"`, PropertyDescriptor {\[[Value]]: |arity|, \[[Writable]]: false, \[[Enumerable]]: false, \[[Configurable]]: true}).
1. Let |name| be the [=name of the WebAssembly function=] |funcaddr|.
1. Perform ! [=SetFunctionName=](|function|, ! [=ToString=](|index|)).
Copy link
Member

Choose a reason for hiding this comment

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

No ToString and s/index/name/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

1. For each type |t| of |parameters|,
1. If the length of |argValues| &gt; |i|, let |arg| be |argValues|[i].
1. Otherwise, let |arg| be undefined.
1. [=Append=] [=ToWebAssemblyValue=](|arg|, |t|) to |args|.
Copy link
Member

Choose a reason for hiding this comment

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

Loop needs to increment i.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

1. Let |jsArguments| be an empty [=list=].
1. For each |arg| in |arguments|,
1. [=Append=] [=ToJSValue=](|arg|) to |jsArguments|.
1. Let |ret| be ? [=Call=](|func|, undefined, |jsArguments|). If an exception is thrown, trigger a WebAssembly trap, and propagate the exception to the enclosing JavaScript.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the clause "trigger a WebAssembly trap" is meaningful here. In other places a "wasm trap" is specifically a trap that occurs from within wasm code and necessarily generates a RuntimeError. Can we just unconditionally say here that if an exception is thrown, unwind the entire enclosing wasm activation to the innermost enclosing invoke_func? If so, then we should simplify the line "If ret is error, throw an exception. This exception should be a WebAssembly RuntimeError exception, unless otherwise indicated by the WebAssembly error mapping." that goes after the two invoke_funcs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good idea; I'll make that change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, to clarify, if there's a call from Wasm to JS which throws an arbitrary JS value as an exception, should this be replaced by a RuntimeError, or left as is? I don't quite understand what change needs to be made.

Copy link
Member

Choose a reason for hiding this comment

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

The thrown JS value is left as-is; RuntimeErrors should only be thrown by wasm-internal traps. I'm not sure a change is needed here, I was just suggesting wording clarification.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thinking about this some more, I'm still uncomfortable with changing the wording from trap to "unwind". Wasm has specific semantics here--throwing an exception doesn't just throw the computation. In particular, updates to the store stick around. A WebAssembly trap explains how this happens within the core spec formalism.

If I were to change this section for clarification, I might change it in the other direction of making it more formal--there could be a global variable which stores the "current exception", the host function triggers a trap, and then calling an exported function reads the global variable to decide which exception type to throw. But I'm not sure if this helps anything--would wording like that be any more clear, or would it just be more jumble-of-words to dig through?

Copy link
Member

Choose a reason for hiding this comment

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

That's a good idea! Instead of the global variable, though, maybe we could:

  • change the trap instruction to have an optional hostvalue (symmetric to hostfunc)
  • when a core wasm trap executes, it creates a trap ϵ
  • we add some core language saying that when calling a hostfunc, the hostfunc can result in an error with a hostvalue v, in which case the call evaluates to a trap v.
  • in invoke_func, the hostvalue is produced in the case of error
  • the JS API can then pipe in the thrown exception and throw the resulting host value on the other side.

@rossberg Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

@lukewagner, sounds reasonable, though it raises the question whether traps and exceptions are the same thing within Wasm. With the exception proposal they might not be. But I suppose that does not matter for the time being.

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, isn't the wording in Section 4 enough to handwave over this for the time being?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was imagining something that wouldn't require any changes to the core spec:

  • When doing the underlying Call for a host function, if an exception is thrown, stash that exception in a per-agent variable.
  • When calling invoke_func, if the result is error, check that variable. If it's set, clear that variable and throw the object that it contained. If it's not set, throw a RuntimeError (or other appropriate exception for OOM or stack overflow).

Anyway, I'm not really sure if this would add anything vs the current wording. What would you think?

Copy link
Member

Choose a reason for hiding this comment

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

Re-reading Section 4 from scratch, I agree that what we have now is clear enough and there's no real benefit to adding anything like I was suggesting above until we have some form of EH or trap handlers.

@littledan
Copy link
Collaborator Author

Thanks for all the comments. I am about to leave for vacation, so unfortunately I won't be able to address these for two weeks.

@ericprud
Copy link

ericprud commented Nov 19, 2017

@littledan , in case it comes to a vote before you return, are you OK with publishing it as a First Public Working Draft before you finish?

FPWD is basically the WG's way of declaring to the public what the spec is about. It should be sufficient to start the IP disclosure process but is not expected to be a finished document. IMO, this doc has been ready for FPWD since the initial checkin.

Just for orientation, after publication as FPWD, the group may publish as often as it likes (multiple times per minute might be viewed as a DOS attack). It is only when we publish the document as a Candidate Rec that all issues raised by the WG must either be addressed or deliberately postponed.

@littledan
Copy link
Collaborator Author

I don't know enough about W3C process to have an opinion. I don't understand the significance of getting to that point extremely quickly given that this was developed under a CG, but I am fine with this in theory as long as I can still make changes due to the comments. I have not reviewed the comments yet.

@littledan
Copy link
Collaborator Author

Thanks for this detailed review, @lukewagner . I've left questions about a few tricky pieces; I plan to upload fixes for everything else tomorrow. If you get a chance to leave any more clarifications, that will be very helpful.

@littledan
Copy link
Collaborator Author

I've addressed some of @lukewagner 's comments. There are still some more changes to make that require a bit more thought. However, these are all editorial or minor normative fixes; I don't think they have much IP content. So, I would be comfortable going to FPWD if the rest of the group is.

@littledan
Copy link
Collaborator Author

In the WebAssembly WG meeting today, we decided to move this PR to First Public Working Draft status. Should we merge it then?

@flagxor
Copy link
Member

flagxor commented Dec 6, 2017

lgtm

@flagxor flagxor merged commit d15cabb into WebAssembly:master Dec 6, 2017
@domenic
Copy link
Member

domenic commented Dec 12, 2017

Is this published anywhere besides littledan.github.io?

@littledan
Copy link
Collaborator Author

@domenic If it is, I can't find it either. Seems like I should extend the CI here to publish the spec in the right place.

littledan added a commit to littledan/spec that referenced this pull request Dec 13, 2017
WebAssembly names are in the AST as strings of Unicode code points,
rather than UTF-8-encoded byte sequences. This patch uses WebIDL's
USVString type to encode this--the type is sequences of code points
in IDL-land, which is then converted to an ECMAScript value by
encoding in UTF-16. The place where this is inapplicable is custom
sections, which don't have a decoded name, but whose name is still
validated as UTF-8.

Thanks to @lukewagner for pointing this out in
WebAssembly#591 (comment)
littledan added a commit to littledan/spec that referenced this pull request Dec 13, 2017
Previously, the instantiate function had some kind of complicated logic
to make it so just one JS object per memory, table or exported function
would be created per address in the store without having a global mapping.
This patch switches to a global mapping to make the specification
easier to read and modify over time.

The change here was discussed in this comment thread:
WebAssembly#591 (comment)
@rossberg
Copy link
Member

rossberg commented Dec 13, 2017 via email

@rossberg
Copy link
Member

Okay, finally managed to work around the issues, see #624. @littledan, left the actual build rules for the Bikeshed stuff as TODOs in the makefiles. Can you fill them in in a follow-up?

littledan added a commit to littledan/spec that referenced this pull request Dec 22, 2017
WebAssembly names are in the AST as strings of Unicode code points,
rather than UTF-8-encoded byte sequences. This patch uses WebIDL's
USVString type to encode this--the type is sequences of code points
in IDL-land, which is then converted to an ECMAScript value by
encoding in UTF-16. The place where this is inapplicable is custom
sections, which don't have a decoded name, but whose name is still
validated as UTF-8.

Thanks to @lukewagner for pointing this out in
WebAssembly#591 (comment)
littledan added a commit to littledan/spec that referenced this pull request Dec 22, 2017
Previously, the instantiate function had some kind of complicated logic
to make it so just one JS object per memory, table or exported function
would be created per address in the store without having a global mapping.
This patch switches to a global mapping to make the specification
easier to read and modify over time.

The change here was discussed in this comment thread:
WebAssembly#591 (comment)
littledan added a commit that referenced this pull request Jan 3, 2018
* Editorial: Use USVString rather than DOMString to explain encoding

WebAssembly names are in the AST as strings of Unicode code points,
rather than UTF-8-encoded byte sequences. This patch uses WebIDL's
USVString type to encode this--the type is sequences of code points
in IDL-land, which is then converted to an ECMAScript value by
encoding in UTF-16. The place where this is inapplicable is custom
sections, which don't have a decoded name, but whose name is still
validated as UTF-8.

Thanks to @lukewagner for pointing this out in
#591 (comment)

* Editorial: Clean up caching JS objects

Previously, the instantiate function had some kind of complicated logic
to make it so just one JS object per memory, table or exported function
would be created per address in the store without having a global mapping.
This patch switches to a global mapping to make the specification
easier to read and modify over time.

The change here was discussed in this comment thread:
#591 (comment)

* Editorial: Improve comment

* Editorial: Further wording improvements

Suggested by @rossberg

* Editorial: Consolidate storage sections

* Editorial: Fix link to the moved core WebAssembly specification

* Editorial: Further changes from @rossberg's review
@Ms2ger Ms2ger deleted the new-js.bs branch September 22, 2022 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants