Closed Bug 1312817 Opened 4 years ago Closed 4 years ago

support postMessage() of WebAssembly.Module

Categories

(Core :: DOM: Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: luke, Assigned: baku)

References

Details

Attachments

(2 files)

Bug 1276029 adds a threadsafe refcounted JS::WasmModule interface to JS API that allows WebAssembly.Module objects to be stored in an IDBObjectStore.  To support full structured clone, we also need the structure clone integration to allow Module to be sent via postMessage().
There are many postMessages. window to window, window to worker, MessageChannel and BroadcastChannel.
We divide them based on the 'contexts': SameProcessSameThread (window to window), SameProcessDifferentThread (workers) and DifferentProcess (MessageChannel and BroadcastChannel).

I suspect we want to support the first 2 contexts. Am I right?
This should be described in the spec, btw.
I think the first two contexts would be immediately useful and sufficient, so what we should do in this bug, but eventually I think it'd probably be good to allow structured-cloning between all contexts (since we have the ability to serialize).

There isn't a fully explicit spec yet, we just have a hand-wavy paragraph saying "it should be structured cloneable, equivalent to if the original bytecode was copied and recompiled":
  https://github.com/WebAssembly/design/blob/master/JS.md#structured-clone-of-a-webassemblymodule
Are there more details that a proper spec would need to fill in to describe?  Do you have any links to examples to follow?
Assignee: nobody → amarchesini
Attached patch wasm.patchSplinter Review
qdot: are you familiar with StructuredCloneHolder?

like: it works \o/ but not window to iframe, because JS::IsWasmModuleObject(aObj) returns false. gdb says that it returns false because instead being a WasmModule, it is a ProxyObject. Should I unwrap something first?
Attachment #8805042 - Flags: review?(luke)
Attachment #8805042 - Flags: review?(kyle)
Comment on attachment 8805042 [details] [diff] [review]
wasm.patch

Review of attachment 8805042 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful!  I think we just need a CheckedUnwrap() in JS::IsWasmModuleObject/JS::GetWasmModule to handle the transparent-cross-compartment-wrapper case; I'll post a patch for that real quick and then perhaps you can validate with an iframe testcase?
Attachment #8805042 - Flags: review?(luke) → review+
Attachment #8805100 - Flags: review?(bbouvier)
Attachment #8805100 - Flags: feedback?(amarchesini)
Attachment #8805100 - Flags: review?(bbouvier) → review+
Attachment #8805100 - Flags: feedback?(amarchesini) → feedback+
Comment on attachment 8805042 [details] [diff] [review]
wasm.patch

Review of attachment 8805042 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/StructuredCloneTags.h
@@ +56,5 @@
>    // This tag is used by both main thread and workers.
>    SCTAG_DOM_URLSEARCHPARAMS,
>  
> +  // Wasm Modules
> +  SCTAG_DOM_WASM_MODULE,

Is this intentionally different than the SCTAG_DOM_WASM :janv added in bug 1311466?  (It's up above because IndexedDB needs a stable tag identifier for persistence.  Although based on your line numbers I think your checkout is not up-to-date and you need to rebase.)
Comment on attachment 8805042 [details] [diff] [review]
wasm.patch

Review of attachment 8805042 [details] [diff] [review]:
-----------------------------------------------------------------

Haven't worked with StructuredClone too much, but I read through the patch and the current state of the file and this seems to make sense.
Attachment #8805042 - Flags: review?(kyle) → review+
Thanks Andrew! Funny thing is that I introduced that tag, but I didn't know janv already landed those patches.
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19e79becc540
support {window,worker}.postMessage() of WebAssembly.Module, r=qdot, r=luke
Luke, I landed my patch. When you want, can you land yours plus:

https://hg.mozilla.org/integration/mozilla-inbound/file/tip/dom/base/test/test_postMessages.html#l371

this set to true?
Flags: needinfo?(luke)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79635ac69f44
Backed out changeset 19e79becc540 for failures in own test on android
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36deeea122f9
support {window,worker}.postMessage() of WebAssembly.Module, r=qdot, r=luke
Flags: needinfo?(amarchesini)
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc216bb3f2c7
Handle wrappers in JS::WasmModule APIs, enable {window,iframe}.postMessage() (r=bbouvier)
Done, thanks for all the help!
Flags: needinfo?(luke)
https://hg.mozilla.org/mozilla-central/rev/36deeea122f9
https://hg.mozilla.org/mozilla-central/rev/dc216bb3f2c7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.