Bug 220038

Summary: Unable to postMessage a WebAssembly module to a worklet
Product: WebKit Reporter: Richard Newman <rnewman>
Component: Web AudioAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: ahmad.saleem792, alecflett, annulen, beidson, benjamin, calvaris, cdumez, eric.carlson, esprehn+autocc, ews-watchlist, fpizlo, ggaren, glenn, gyuyoung.kim, Ian.kettlewell, jer.noble, jsbell, kangil.han, letz, lolwebkitbugtracker, philipj, rniwa, ryuan.choi, sergio, smoley, tibor.klajnscek, webkit-bug-importer, youennf, ysuzuki
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=237144
Bug Depends on: 220627    
Bug Blocks:    
Attachments:
Description Flags
WIP Patch
none
Test case
none
WIP patch
none
WIP patch
none
WIP patch
ews-feeder: commit-queue-
WIP patch
ews-feeder: commit-queue-
WIP patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch none

Description Richard Newman 2020-12-19 09:50:16 PST
WebAssembly modules can be sent between workers and pages via postMessage.

Both Firefox and Chrome also allow WebAssembly modules to be sent from a page to an AudioWorklet via its MessagePort:

  https://developer.mozilla.org/en-US/docs/Web/API/AudioWorkletNode/port

E.g.,

```
  this.port.postMessage({ message: 'hey', module });
```

in Safari Technology Preview (117, WebKit 15611.1.7.2), this throws 

```
  DataCloneError: The object can not be cloned.
```

(FWIW, adding `module` to the transfer list doesn't help; I guess Safari doesn't support transfer lists either?)

The lack of this capability prevents a common code loading approach for AudioWorklets, where some large chunk of WebAssembly is loaded in a worker and sent via the page down to the AudioWorklet via the MessagePort. 

I have not yet verified whether there's a workaround by posting a multi-megabyte ArrayBuffer and compiling it inside the worklet.
Comment 1 Radar WebKit Bug Importer 2020-12-22 12:28:34 PST
<rdar://problem/72596556>
Comment 2 Yusuke Suzuki 2020-12-30 21:47:06 PST
Currently, MessagePort postMessage w/ WebAssembly.Module is not supported.

We need to know the notion of the followings.

1. This pair of MessagePorts are in the same process so that we can send SerializedScriptValue without using IPC back-and-force
2. This pair of MessagePorts are in the different process so that we should just throw an onmessageerror exception.

Chris, do you know about MessagePort's remote/not-remote flags? Is it specified?

Note: https://bugzilla.mozilla.org/show_bug.cgi?id=1605566
Comment 3 Yusuke Suzuki 2020-12-30 22:05:13 PST
Note that LayoutTests/imported/w3c/web-platform-tests/wasm/serialization/module/window-messagechannel-success.html covers this.
Comment 4 Chris Dumez 2021-01-05 08:48:39 PST
(In reply to Yusuke Suzuki from comment #2)
> Currently, MessagePort postMessage w/ WebAssembly.Module is not supported.
> 
> We need to know the notion of the followings.
> 
> 1. This pair of MessagePorts are in the same process so that we can send
> SerializedScriptValue without using IPC back-and-force
> 2. This pair of MessagePorts are in the different process so that we should
> just throw an onmessageerror exception.
> 
> Chris, do you know about MessagePort's remote/not-remote flags? Is it
> specified?
> 
> Note: https://bugzilla.mozilla.org/show_bug.cgi?id=1605566

@Yusuke: I don't quite understand the need for "different-process". Our worklets always run in process.

The only case where we use message port cross-process is with service workers, which are different from worklets.
Comment 5 Chris Dumez 2021-01-05 09:47:04 PST
(In reply to Chris Dumez from comment #4)
> (In reply to Yusuke Suzuki from comment #2)
> > Currently, MessagePort postMessage w/ WebAssembly.Module is not supported.
> > 
> > We need to know the notion of the followings.
> > 
> > 1. This pair of MessagePorts are in the same process so that we can send
> > SerializedScriptValue without using IPC back-and-force
> > 2. This pair of MessagePorts are in the different process so that we should
> > just throw an onmessageerror exception.
> > 
> > Chris, do you know about MessagePort's remote/not-remote flags? Is it
> > specified?
> > 
> > Note: https://bugzilla.mozilla.org/show_bug.cgi?id=1605566
> 
> @Yusuke: I don't quite understand the need for "different-process". Our
> worklets always run in process.
> 
> The only case where we use message port cross-process is with service
> workers, which are different from worklets.

Based on the error, it looks like nobody implemented the serialization of modules in SerializedScriptValue? If so, sending the module to an iframe via postMessage would fail too (has this been tested?)
Comment 6 Richard Newman 2021-01-05 09:55:59 PST
I have not tested iframe messaging.

Web worker to page seems to work correctly.
Comment 7 Chris Dumez 2021-01-05 09:59:44 PST
(In reply to Richard Newman from comment #6)
> I have not tested iframe messaging.
> 
> Web worker to page seems to work correctly.

Oh I see. If worker to page works then I would imagine iframe messaging would work too. This must be specific to worklets somehow.
Comment 8 Chris Dumez 2021-01-05 10:05:01 PST
(In reply to Chris Dumez from comment #7)
> (In reply to Richard Newman from comment #6)
> > I have not tested iframe messaging.
> > 
> > Web worker to page seems to work correctly.
> 
> Oh I see. If worker to page works then I would imagine iframe messaging
> would work too. This must be specific to worklets somehow.

In SerializedScriptValue:

                if (m_context != SerializationContext::WorkerPostMessage) {
                    code = SerializationReturnCode::DataCloneError;
                    return true;
                }

When encoding wasm modules. We are likely not using the WorkerPostMessage SerializationContext when messaging audio worklets?
Comment 9 Chris Dumez 2021-01-05 10:08:27 PST
(In reply to Chris Dumez from comment #8)
> (In reply to Chris Dumez from comment #7)
> > (In reply to Richard Newman from comment #6)
> > > I have not tested iframe messaging.
> > > 
> > > Web worker to page seems to work correctly.
> > 
> > Oh I see. If worker to page works then I would imagine iframe messaging
> > would work too. This must be specific to worklets somehow.
> 
> In SerializedScriptValue:
> 
>                 if (m_context != SerializationContext::WorkerPostMessage) {
>                     code = SerializationReturnCode::DataCloneError;
>                     return true;
>                 }
> 
> When encoding wasm modules. We are likely not using the WorkerPostMessage
> SerializationContext when messaging audio worklets?

MessagePort::postMessage() seems to use the default serialization context, no matter in which context the message port is used:

===
ExceptionOr<void> MessagePort::postMessage(JSC::JSGlobalObject& state, JSC::JSValue messageValue, PostMessageOptions&& options)
{
    LOG(MessagePorts, "Attempting to post message to port %s (to be received by port %s)", m_identifier.logString().utf8().data(), m_remoteIdentifier.logString().utf8().data());

    registerLocalActivity();

    Vector<RefPtr<MessagePort>> ports;
    auto messageData = SerializedScriptValue::create(state, messageValue, WTFMove(options.transfer), ports);
===
Comment 10 Yusuke Suzuki 2021-01-05 12:52:16 PST
(In reply to Chris Dumez from comment #9)
> (In reply to Chris Dumez from comment #8)
> > (In reply to Chris Dumez from comment #7)
> > > (In reply to Richard Newman from comment #6)
> > > > I have not tested iframe messaging.
> > > > 
> > > > Web worker to page seems to work correctly.
> > > 
> > > Oh I see. If worker to page works then I would imagine iframe messaging
> > > would work too. This must be specific to worklets somehow.
> > 
> > In SerializedScriptValue:
> > 
> >                 if (m_context != SerializationContext::WorkerPostMessage) {
> >                     code = SerializationReturnCode::DataCloneError;
> >                     return true;
> >                 }
> > 
> > When encoding wasm modules. We are likely not using the WorkerPostMessage
> > SerializationContext when messaging audio worklets?
> 
> MessagePort::postMessage() seems to use the default serialization context,
> no matter in which context the message port is used:
> 
> ===
> ExceptionOr<void> MessagePort::postMessage(JSC::JSGlobalObject& state,
> JSC::JSValue messageValue, PostMessageOptions&& options)
> {
>     LOG(MessagePorts, "Attempting to post message to port %s (to be received
> by port %s)", m_identifier.logString().utf8().data(),
> m_remoteIdentifier.logString().utf8().data());
> 
>     registerLocalActivity();
> 
>     Vector<RefPtr<MessagePort>> ports;
>     auto messageData = SerializedScriptValue::create(state, messageValue,
> WTFMove(options.transfer), ports);
> ===

I changed it to WorkerPostMessage serialization context, and it didn't work because MessagePort always serialize (via IPC encoder) SerializedScriptValue and deserialized it even though both ports are used by the workers/worklets in the same process.
The problem is that we do not support full serialization of WebAssembly.Module.

1. It becomes super huge size
2. It is really complicated
3. Serialize and deserialize it will double memory size while WebAssembly.Module can be shared between multiple threads safely (it is designed to be a thread-safe).

So, we need a notion of "MessageChannel is not used among different processes", and we should avoid using IPC encoder / decoder to serialize SerializedScriptValue for MessagePort. And I think this is what gecko is doing.
Comment 11 Chris Dumez 2021-01-05 17:48:19 PST
(In reply to Yusuke Suzuki from comment #10)
> (In reply to Chris Dumez from comment #9)
> > (In reply to Chris Dumez from comment #8)
> > > (In reply to Chris Dumez from comment #7)
> > > > (In reply to Richard Newman from comment #6)
> > > > > I have not tested iframe messaging.
> > > > > 
> > > > > Web worker to page seems to work correctly.
> > > > 
> > > > Oh I see. If worker to page works then I would imagine iframe messaging
> > > > would work too. This must be specific to worklets somehow.
> > > 
> > > In SerializedScriptValue:
> > > 
> > >                 if (m_context != SerializationContext::WorkerPostMessage) {
> > >                     code = SerializationReturnCode::DataCloneError;
> > >                     return true;
> > >                 }
> > > 
> > > When encoding wasm modules. We are likely not using the WorkerPostMessage
> > > SerializationContext when messaging audio worklets?
> > 
> > MessagePort::postMessage() seems to use the default serialization context,
> > no matter in which context the message port is used:
> > 
> > ===
> > ExceptionOr<void> MessagePort::postMessage(JSC::JSGlobalObject& state,
> > JSC::JSValue messageValue, PostMessageOptions&& options)
> > {
> >     LOG(MessagePorts, "Attempting to post message to port %s (to be received
> > by port %s)", m_identifier.logString().utf8().data(),
> > m_remoteIdentifier.logString().utf8().data());
> > 
> >     registerLocalActivity();
> > 
> >     Vector<RefPtr<MessagePort>> ports;
> >     auto messageData = SerializedScriptValue::create(state, messageValue,
> > WTFMove(options.transfer), ports);
> > ===
> 
> I changed it to WorkerPostMessage serialization context, and it didn't work
> because MessagePort always serialize (via IPC encoder) SerializedScriptValue
> and deserialized it even though both ports are used by the workers/worklets
> in the same process.
> The problem is that we do not support full serialization of
> WebAssembly.Module.
> 
> 1. It becomes super huge size
> 2. It is really complicated
> 3. Serialize and deserialize it will double memory size while
> WebAssembly.Module can be shared between multiple threads safely (it is
> designed to be a thread-safe).
> 
> So, we need a notion of "MessageChannel is not used among different
> processes", and we should avoid using IPC encoder / decoder to serialize
> SerializedScriptValue for MessagePort. And I think this is what gecko is
> doing.

From MessagePort, you can query identifier() and remoteIdentifier(). Each returns a MessagePortIdentifier which is a struct that contains both a processIdentifier and a portIdentifier. It should therefore be trivial to figure out if the local and remote ports are hosted in the same process check comparing identifier().processIdentifier and remoteIdentifier().processIdentifier.
Comment 12 Chris Dumez 2021-01-14 10:31:08 PST
(In reply to Chris Dumez from comment #11)
> (In reply to Yusuke Suzuki from comment #10)
> > (In reply to Chris Dumez from comment #9)
> > > (In reply to Chris Dumez from comment #8)
> > > > (In reply to Chris Dumez from comment #7)
> > > > > (In reply to Richard Newman from comment #6)
> > > > > > I have not tested iframe messaging.
> > > > > > 
> > > > > > Web worker to page seems to work correctly.
> > > > > 
> > > > > Oh I see. If worker to page works then I would imagine iframe messaging
> > > > > would work too. This must be specific to worklets somehow.
> > > > 
> > > > In SerializedScriptValue:
> > > > 
> > > >                 if (m_context != SerializationContext::WorkerPostMessage) {
> > > >                     code = SerializationReturnCode::DataCloneError;
> > > >                     return true;
> > > >                 }
> > > > 
> > > > When encoding wasm modules. We are likely not using the WorkerPostMessage
> > > > SerializationContext when messaging audio worklets?
> > > 
> > > MessagePort::postMessage() seems to use the default serialization context,
> > > no matter in which context the message port is used:
> > > 
> > > ===
> > > ExceptionOr<void> MessagePort::postMessage(JSC::JSGlobalObject& state,
> > > JSC::JSValue messageValue, PostMessageOptions&& options)
> > > {
> > >     LOG(MessagePorts, "Attempting to post message to port %s (to be received
> > > by port %s)", m_identifier.logString().utf8().data(),
> > > m_remoteIdentifier.logString().utf8().data());
> > > 
> > >     registerLocalActivity();
> > > 
> > >     Vector<RefPtr<MessagePort>> ports;
> > >     auto messageData = SerializedScriptValue::create(state, messageValue,
> > > WTFMove(options.transfer), ports);
> > > ===
> > 
> > I changed it to WorkerPostMessage serialization context, and it didn't work
> > because MessagePort always serialize (via IPC encoder) SerializedScriptValue
> > and deserialized it even though both ports are used by the workers/worklets
> > in the same process.
> > The problem is that we do not support full serialization of
> > WebAssembly.Module.
> > 
> > 1. It becomes super huge size
> > 2. It is really complicated
> > 3. Serialize and deserialize it will double memory size while
> > WebAssembly.Module can be shared between multiple threads safely (it is
> > designed to be a thread-safe).
> > 
> > So, we need a notion of "MessageChannel is not used among different
> > processes", and we should avoid using IPC encoder / decoder to serialize
> > SerializedScriptValue for MessagePort. And I think this is what gecko is
> > doing.
> 
> From MessagePort, you can query identifier() and remoteIdentifier(). Each
> returns a MessagePortIdentifier which is a struct that contains both a
> processIdentifier and a portIdentifier. It should therefore be trivial to
> figure out if the local and remote ports are hosted in the same process
> check comparing identifier().processIdentifier and
> remoteIdentifier().processIdentifier.

Indeed, it seems we are going to the NetworkProcess via IPC even though the source and destination processes are identical. This is definitely suboptimal in general and breaking for Wasm modules since those are currently not IPC-encoded.
Comment 13 Chris Dumez 2021-01-14 11:49:31 PST
With the WIP patch at Bug 220627 (to avoid IPC when both MessagePorts are in the same process) and with a change to MessagePort::postMessage() to use SerializationContext::WorkerPostMessage when necessary, LayoutTests/imported/w3c/web-platform-tests/wasm/serialization/module/window-messagechannel-success.html starts to PASS.
Comment 14 Yusuke Suzuki 2021-01-14 12:42:06 PST
(In reply to Chris Dumez from comment #13)
> With the WIP patch at Bug 220627 (to avoid IPC when both MessagePorts are in
> the same process) and with a change to MessagePort::postMessage() to use
> SerializationContext::WorkerPostMessage when necessary,
> LayoutTests/imported/w3c/web-platform-tests/wasm/serialization/module/window-
> messagechannel-success.html starts to PASS.

Perfect!
Comment 15 Chris Dumez 2021-01-28 17:06:09 PST
Created attachment 418682 [details]
WIP Patch
Comment 16 Chris Dumez 2021-01-28 17:18:02 PST
Comment on attachment 418682 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418682&action=review

> Source/WebCore/dom/MessagePort.cpp:141
> +    // FIXME: At the time of serialization, we don't know if the target is a window, a worker or something else. We will only know at the time of

@Yusuke: Not sure how to best deal with this. Passing SerializationContext::WorkerPostMessage unconditionally here is a hack.
Comment 17 Yusuke Suzuki 2021-02-01 02:03:23 PST
Comment on attachment 418682 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418682&action=review

> LayoutTests/imported/w3c/web-platform-tests/wasm/serialization/module/window-messagechannel-success-expected.txt:2
> +PASS postMessaging to a dedicated worker via MessageChannel allows them to instantiate

Can we have a test case that is posting wasm module to message-ports tied to the different process and getting an error in deserialization side?
Ideally, we should throw an error, and we should report onmessageerror since we failed deserialization.
Maybe, we should add a test for checking that error, and after that, we should file a bug for `onmessageerror` handler invocation and implement it.
https://developer.mozilla.org/en-US/docs/Web/API/Worker/onmessageerror
https://developer.mozilla.org/en-US/docs/Web/API/MessagePort/onmessageerror

>> Source/WebCore/dom/MessagePort.cpp:141
>> +    // FIXME: At the time of serialization, we don't know if the target is a window, a worker or something else. We will only know at the time of
> 
> @Yusuke: Not sure how to best deal with this. Passing SerializationContext::WorkerPostMessage unconditionally here is a hack.

This hack looks OK to me! If we attempt to deserialize non-sharable-between-processes objects, then we will get an error at deserialization (not causing catastrophic crash) since our SerializedScriptValue's deserialization is robust enough against that.
Comment 18 letz 2021-08-12 09:39:46 PDT
Created attachment 435429 [details]
Test case
Comment 19 letz 2021-08-12 09:42:17 PDT
Note that WebAssembly modules should be serialisable to be used in processorOptions at construction time between an AudioWorkletNote and and AudioWorkletProcessor.
Comment 20 Chris Dumez 2021-08-18 14:10:06 PDT
Comment on attachment 418682 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418682&action=review

>> LayoutTests/imported/w3c/web-platform-tests/wasm/serialization/module/window-messagechannel-success-expected.txt:2
>> +PASS postMessaging to a dedicated worker via MessageChannel allows them to instantiate
> 
> Can we have a test case that is posting wasm module to message-ports tied to the different process and getting an error in deserialization side?
> Ideally, we should throw an error, and we should report onmessageerror since we failed deserialization.
> Maybe, we should add a test for checking that error, and after that, we should file a bug for `onmessageerror` handler invocation and implement it.
> https://developer.mozilla.org/en-US/docs/Web/API/Worker/onmessageerror
> https://developer.mozilla.org/en-US/docs/Web/API/MessagePort/onmessageerror

WebKit currently doesn't support onmessageerror AFAIK
Comment 21 Chris Dumez 2021-08-18 14:39:25 PDT
Created attachment 435798 [details]
WIP patch
Comment 22 Chris Dumez 2021-08-18 14:45:03 PDT
Created attachment 435799 [details]
WIP patch
Comment 23 Chris Dumez 2021-08-18 16:09:52 PDT
Created attachment 435812 [details]
WIP patch
Comment 24 Chris Dumez 2021-08-18 17:18:06 PDT
Created attachment 435820 [details]
WIP patch
Comment 25 Chris Dumez 2021-08-19 08:25:04 PDT
Created attachment 435860 [details]
WIP patch
Comment 26 Chris Dumez 2021-08-19 08:42:54 PDT
Created attachment 435862 [details]
Patch
Comment 27 Chris Dumez 2021-08-19 08:59:22 PDT
Created attachment 435865 [details]
Patch
Comment 28 Chris Dumez 2021-08-19 10:40:38 PDT
Created attachment 435882 [details]
Patch
Comment 29 Chris Dumez 2021-08-23 08:11:05 PDT
Created attachment 436196 [details]
Patch
Comment 30 Chris Dumez 2021-08-23 10:21:14 PDT
Created attachment 436207 [details]
Patch
Comment 31 Ian Kettlewell 2021-12-18 09:59:52 PST
A key detail worth noting: It looks like WebAssembly.Memory objects cannot be sent between workers via postMessage either.

An important use for postMessaging WebAssembly modules / memory is to use an Audio Worklet in a way similar to how a non-web dedicated audio thread would work. The Audio Worklet runs a Wasm instance backed by a SharedArrayBuffer that communicates with the main thread via Wasm atomics. That flow makes it *much* easier to for things like game engines to share audio backend code between non-web and web.

I think (but haven't verified) the module being un-postMessageable can be worked around by postMessaging the code and re-instantiating the module on the Worklet.

However as far as I'm aware the WebAssembly.Memory object being non-postMessagable does not have a workaround. That blocks the use-case I've described here.
Comment 32 letz 2022-01-27 01:01:17 PST
Any news on this issue? Any expected date for a proper fix to appear? Thanks.
Comment 33 Tibor Klajnscek 2022-03-09 04:20:29 PST
+1 for a status update - this is blocking us from using our audio worklet backend which is critical to consistent and crackle-free audio in our products.
Comment 34 Attila Haraszti 2022-04-19 03:16:37 PDT
This is currently a blocker for us, a fix would be highly appreciated.
Comment 35 Tibor Klajnscek 2022-07-22 03:46:39 PDT
This seems to work in the latest Safari dev preview, but I haven't found any release notes mentioning it. 

Is it just a fluke or has it been fixed? And if so, will it make it into Safari 16?
Comment 36 Ahmad Saleem 2022-09-30 08:15:36 PDT
@Chris - it seems this patch passes all tests and only have "style" issues? Is it something on your radar and "see also" bug is also fixed? Thanks!
Comment 37 Chris Dumez 2022-11-17 11:05:20 PST
Pull request: https://github.com/WebKit/WebKit/pull/6596
Comment 38 EWS 2022-11-18 13:31:52 PST
Committed 256853@main (a9d61c9d9e2e): <https://commits.webkit.org/256853@main>

Reviewed commits have been landed. Closing PR #6596 and removing active labels.