SharedArrayBuffer posted to AudioWorkletProcessor is not actually shared with the main thread

• Component: Web Audio
• Status: RESOLVED FIXED

[tasnim]

Created attachment 453112 [details]
Screenshot of steps in safari

When SharedArrayBuffer is posted to AudioWorkletProcessor from main thread, changing value of an element from main thread should be reflected within AudioWorkletProcessor (which is running in its own Web Audio render thread). This doesn't seem to be happening in Safari but works perfectly fine in Chrome. Moreover, when SharedArrayBuffer is posted to a Worker, the changed value is noticed in Safari and chrome (both).

Minimal steps to reproduce:

In the same directory, need 3 files: safari.html, white-noise-processor.js & server.py (this one is for overriding response headers to enable cross origin isolation). The first two can be found here: https://jsfiddle.net/vzh29gk1/2/ . Copy them in a directory and create server.py there like below:

#!/usr/bin/env python
try:
    from http import server # Python 3
except ImportError:
    import SimpleHTTPServer as server # Python 2

class MyHTTPRequestHandler(server.SimpleHTTPRequestHandler):
    def end_headers(self):
        self.send_my_headers()

        server.SimpleHTTPRequestHandler.end_headers(self)

    def send_my_headers(self):
        self.send_header("Access-Control-Allow-Origin", "*")
        self.send_header("Cross-Origin-Opener-Policy", "same-origin")
        self.send_header("Cross-Origin-Embedder-Policy", "require-corp")

if __name__ == '__main__':
    server.test(HandlerClass=MyHTTPRequestHandler)

On terminal, in the same directory which has all 3 above files, type: 'python3 -m server' and then open URL: localhost:8000/safari.html on browser. Keep the Developer Console tab. Now, click anywhere in the screen. This should be enough to kick-off the worklet processor. In the console tab, try tweaking the value of an element of SharedArrayBuffer and notice that its still not changed within AudioWorkletProcessor. Have attached the image on how the steps look like in safari.

[tasnim]

Created attachment 453114 [details]
Same steps in chrome

Attached the same screenshot in chrome where changing value from main thread can be immediately noticed in the AudioWorkletProcessor.

[Chris Dumez]

Test seems to rely on AudioWorkletNodeOptions.processorOptions:
- https://webaudio.github.io/web-audio-api/#AudioWorkletNodeOptions

Looking at our implementation, we don't seem to support that. Not sure if this is a recent addition to the spec or if we missed it. My bet is that if the SharedArrayBuffer was sent to the AudioWorklet via postMessage(), it would work.

[Chris Dumez]

(In reply to Chris Dumez from comment #2)
> Test seems to rely on AudioWorkletNodeOptions.processorOptions:
> - https://webaudio.github.io/web-audio-api/#AudioWorkletNodeOptions
> 
> Looking at our implementation, we don't seem to support that. Not sure if
> this is a recent addition to the spec or if we missed it. My bet is that if
> the SharedArrayBuffer was sent to the AudioWorklet via postMessage(), it
> would work.

Actually, maybe we do support processorOptions after all, I haven't tested but we do have it in our IDL.

[tasnim]

Right, I had posted my findings with processorOptions but I just tested with postMessage too. Doesn't work there. See https://jsfiddle.net/6ua5o3jk/
Works perfectly fine in chrome.

The SharedArrayBuffer may be getting copied to the other thread and behaving like a normal ArrayBuffer. I verified this when initially setting some values (before sab was transported to the other thread) and later, able to see the same value in the other thread.

(Note in both the test, at step-2 in the attached screenshot if you click anywhere in the browser to activate AudioWorkletProcessor, it may not work directly coz of autoplay issue and then you'll have to reload the browser and check again)

[Chris Dumez]

Ok, so when constructing a SerializedScriptValue, the default SerializationContext  is SerializationContext::Default and it is likely what we end up using for sending the options to the AudioWorklet. However, looking at our implementation, SharedArrayBuffer are only serialized as shared when SerializationContext is SerializationContext::WorkerPostMessage. I am not sure why that is (Yusuke, do you know?).

In anyway, I bet that using SerializationContext::WorkerPostMessage as serialization context for AudioWorklet will work. I'll test.

[Radar WebKit Bug Importer]

<rdar://problem/89479641>

[Chris Dumez]

Created attachment 453228 [details]
WIP Patch

[Chris Dumez]

Created attachment 453239 [details]
Patch

[Darin Adler]

Comment on attachment 453239 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioWorkletNode.cpp:94
> -        serializedOptions = SerializedScriptValue::create(globalObject, jsOptions, SerializationErrorMode::NonThrowing);
> -        if (!serializedOptions)
> +        Vector<RefPtr<MessagePort>> ports;
> +        auto serializedValue = SerializedScriptValue::create(globalObject, jsOptions, { }, ports, SerializationContext::WorkerPostMessage);

I’m finding this interface confusing. Could this difference in semantics be expressed as two differently named functions rather than having both be named "create"? Why does SerializedScriptValue::create do something so different when you pass it different arguments, particularly in a case like this where all the arguments are empty values and empty vectors. Is the key difference SerializationContext::WorkerPostMessage?

[Chris Dumez]

Created attachment 453253 [details]
Patch

[Chris Dumez]

(In reply to Darin Adler from comment #9)
> Comment on attachment 453239 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453239&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioWorkletNode.cpp:94
> > -        serializedOptions = SerializedScriptValue::create(globalObject, jsOptions, SerializationErrorMode::NonThrowing);
> > -        if (!serializedOptions)
> > +        Vector<RefPtr<MessagePort>> ports;
> > +        auto serializedValue = SerializedScriptValue::create(globalObject, jsOptions, { }, ports, SerializationContext::WorkerPostMessage);
> 
> I’m finding this interface confusing. Could this difference in semantics be
> expressed as two differently named functions rather than having both be
> named "create"? Why does SerializedScriptValue::create do something so
> different when you pass it different arguments, particularly in a case like
> this where all the arguments are empty values and empty vectors. Is the key
> difference SerializationContext::WorkerPostMessage?

Will investigate in a follow-up as it is not trivial it seems. There are a lot of things that the other create() function doesn't serialize for some reason.

[Chris Dumez]

Committed r290531 (247811@trunk): <https://commits.webkit.org/247811@trunk>

[tasnim]

Thanks for the quick fix Chris. Much appreciated.
Wanted to ask how soon can I try this? Probably, I need to wait for this to show up in https://developer.apple.com/safari/technology-preview/ for trying early, right?

[Chris Dumez]

(In reply to tasnim from comment #13)
> Thanks for the quick fix Chris. Much appreciated.
> Wanted to ask how soon can I try this? Probably, I need to wait for this to
> show up in https://developer.apple.com/safari/technology-preview/ for trying
> early, right?

Yes, you’d have to wait until the next Safari technology preview build (or the one after that if unlucky). Note that I only fixed the sending of the shared array buffer via the options objects like in your original example.

Sadly, sending the shared array buffer via postMessage() (like in your updated example) is still broken due to bug 220038. I will try and take a stab at it soon but it is a little more complicated to fix because of our current architecture.

[tasnim]

Cool, thanks! We send shared array buffer only via the options object. So, should be fine for us.

[Chris Dumez]

(In reply to Darin Adler from comment #9)
> Comment on attachment 453239 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453239&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioWorkletNode.cpp:94
> > -        serializedOptions = SerializedScriptValue::create(globalObject, jsOptions, SerializationErrorMode::NonThrowing);
> > -        if (!serializedOptions)
> > +        Vector<RefPtr<MessagePort>> ports;
> > +        auto serializedValue = SerializedScriptValue::create(globalObject, jsOptions, { }, ports, SerializationContext::WorkerPostMessage);
> 
> I’m finding this interface confusing. Could this difference in semantics be
> expressed as two differently named functions rather than having both be
> named "create"? Why does SerializedScriptValue::create do something so
> different when you pass it different arguments, particularly in a case like
> this where all the arguments are empty values and empty vectors. Is the key
> difference SerializationContext::WorkerPostMessage?

Following up in Bug 237241 for the SerializedScriptValue construction.

[tasnim]

Just wanted to follow up on which next stable build of safari will this fix make in? And, how many estimated weeks away are we from that?

I have already verified on safari preview build but am waiting for the next safari stable release when I will be able to verify on iOS and decide on launching our web app.