support postMessage() of WebAssembly.Module

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM: Workers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: luke, Assigned: baku)

Tracking

(Blocks: 1 bug)

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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().
(Assignee)

Comment 1

2 years ago
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.
(Reporter)

Comment 2

2 years ago
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)

Updated

2 years ago
Assignee: nobody → amarchesini
(Assignee)

Comment 3

2 years ago
Created attachment 8805042 [details] [diff] [review]
wasm.patch

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)
(Reporter)

Comment 4

2 years ago
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+
(Reporter)

Comment 5

2 years ago
Created attachment 8805100 [details] [diff] [review]
checked-unwrap-module
Attachment #8805100 - Flags: review?(bbouvier)
Attachment #8805100 - Flags: feedback?(amarchesini)
Attachment #8805100 - Flags: review?(bbouvier) → review+
(Assignee)

Updated

2 years ago
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+
(Assignee)

Comment 8

2 years ago
Thanks Andrew! Funny thing is that I introduced that tag, but I didn't know janv already landed those patches.

Comment 9

2 years ago
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
(Assignee)

Comment 10

2 years ago
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)

Comment 11

2 years ago
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
backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=38357030&repo=mozilla-inbound
Flags: needinfo?(amarchesini)

Comment 13

2 years ago
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
(Assignee)

Updated

2 years ago
Flags: needinfo?(amarchesini)

Comment 14

2 years ago
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)
(Reporter)

Comment 15

2 years ago
Done, thanks for all the help!
Flags: needinfo?(luke)

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/36deeea122f9
https://hg.mozilla.org/mozilla-central/rev/dc216bb3f2c7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.