AudioWorkletNode port.postMessage not sending message with SharedArrayBuffer as argument
Reported by
pszewczy...@gmail.com,
Oct 4
|
|||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.35 Safari/537.36 Steps to reproduce the problem: 1. Download attached zipped and extract it 2. Enter extracted folder content and start any http local server (i.e in terminal type npx http-server) 3. Open Chrome Beta Version 70.0.3538.35 using command line: /Applications/Google\ Chrome\ Beta.app/Contents/MacOS/Google\ Chrome --js-flags=--experimental-wasm-threads --enable-features=WebAssembly,SharedArrayBuffer --disable-features=WebAssemblyTrapHandler 4. Go to localhost:8080 (default for http-server) 5. Open developer tools, refresh browser and observe that port.onmessage in worklet.js in never called What is the expected behavior? port.onmessage for AudioWorkletProcessor instance is invoked when AudioWorkletNode instance port.postMessage is called with SharedArrayBuffer as argument What went wrong? port.onmessage for AudioWorkletProcessor instance is not invoked when AudioWorkletNode instance port.postMessage is called with SharedArrayBuffer as argument; it works fine for other types of data like Object or ArrayBuffers Did this work before? Yes 69.0.3497.100 Does this work in other browsers? N/A Chrome version: 70.0.3538.35 Channel: beta OS Version: OS X 10.13.6 Flash Version:
,
Oct 4
,
Oct 4
,
Oct 4
First off, thanks for the great repro zip, with included steps! It was very helpful.
I took a look, and I see the behavior you describe, even with ToT Chrome.
If you add the following code to the worklet constructor, you see that the message was sent, but produced an error:
```
this.port.onmessageerror = (e) => {
console.log('error', e)
}
```
This is occurring because the spec currently doesn't allow transferring a SharedArrayBuffer to a worklet. See https://html.spec.whatwg.org/multipage/webappapis.html#integration-with-the-javascript-agent-cluster-formalism
This bounces through a number of GH issues, but eventually ends up here: https://github.com/w3c/css-houdini-drafts/issues/380
Where passing SAB to AudioWorklets is mentioned explicitly, but there seems to be no resolution. I'll follow up on that thread.
,
Oct 4
binji@ I think this is a regression. It's working in the current stable (69.0.3497) and I even wrote an article/example for this feature. But this doesn't work anymore in the current Canary (71.0.3570.0) without any visible error or warning. Please try this example with Stable and Canary: https://googlechromelabs.github.io/web-audio-samples/audio-worklet/design-pattern/shared-buffer/ I was not aware of the change in agent cluster in the spec - but now sure why Worklet's owner cannot be undefined. ikilpatrick@ should know more about this. * FYI the article: https://developers.google.com/web/updates/2018/06/audio-worklet-design-pattern#webaudio_powerhouse_audio_worklet_and_sharedarraybuffer
,
Oct 4
binji@ What's the change that excluded the Worklet from the agent cluster? Can we defer that change until we have a clear resolution on the spec?
,
Oct 4
hongchan@: It was added in https://chromium-review.googlesource.com/1130505. The relevant code is at https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/workers/worklet_global_scope.h?sq=package:chromium&g=0&l=56, where it gets the AgentClusterId. I see what you mean that it is a regression; though the previous behavior was actually not spec compliant, as we allowed SABs to be transferred where they should not be allowed. I think to fix this we'd want to do something similar to Worker, where we pass the AgentClusterId down from the document that created it. Not sure where that happens, however.
,
Oct 4
Thanks for the link. I agree that it's bad for being non-compliant but I am concerned that there are some projects already rely on this feature. I hope we can reach the resolution before it gets out to the stable.
,
Oct 4
,
Oct 4
I have a WIP CL here https://chromium-review.googlesource.com/c/chromium/src/+/1262932, currently missing tests. I'm not sure this is the correct behavior, however.
,
Oct 5
Tried testing the issue on reported chrome version #70.0.3538.35 using Mac OS 10.13.6 by following below steps. Steps: ===== 1.Launched chrome by" --js-flags=--experimental-wasm-threads --enable-features=WebAssembly,SharedArrayBuffer --disable-features=WebAssemblyTrapHandler". 2.Ran a local host and opened the file. 3.Opened dev tools and refreshed the page, observed blank page Note: Tested same on chrome version #69.0.3497.100 but unable to see any difference between both. Attached screencast for reference. @reporter: Could you please verify above screencast and let us know if anything is being missed here. Request you to provide a screencast/screenshot of the issue so that it would be really helpful for better triaging of issue. Thanks.!
,
Oct 5
Re #11: This issue is confirmed, and I believe it is kind of a regression but more of "external dependency" due to the spec change that needs to be made. I will follow up on the spec issue and report back here.
,
Oct 8
Is there a temporary workaround one can employ until the spec is hashed out?
,
Oct 8
Re #13 There is no known workaround for this issue in our audio processing solution. We're using SAB for data control flow and as audio data feed for AudioWorklet.
,
Oct 8
,
Oct 8
+domenic@ We have a plan to change the spec so the worklet agent can be supported officially. I believe it was a simple disconnection between two spec documents. https://github.com/w3c/css-houdini-drafts/issues/380#issuecomment-427443234
,
Oct 9
I am currently trying to decide if we should actually block the launch of Chrome 70 to stable because of this or release the fix on a follow-up Chrome 70 stable push. hongchan@ et. al. who else is using such a setup and might be affected by the behavior change?
,
Oct 9
A couple of audio developers is using this scheme, since I published the article a while ago (2018 June). I don't think it is widely used because of the complexity of the design, but it would be great if we don't break the known behavior just because the spec was not fully updated.
,
Oct 9
We have an approval for the spec change: https://github.com/whatwg/html/pull/4069#pullrequestreview-163099529 The CL + tests can be landed now.
,
Oct 10
Great, please merge to 70 and 71 (I think this might miss branch cut tomorrow)
,
Oct 10
,
Oct 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/97167bddb398b0adb7e06f6f5c68e9f5d556ad96 commit 97167bddb398b0adb7e06f6f5c68e9f5d556ad96 Author: Ben Smith <binji@chromium.org> Date: Thu Oct 11 01:24:49 2018 Allow posting a SharedArrayBuffer to AudioWorklet The HTML spec only allows passing SharedArrayBuffers to an agent in the same agent cluster. Worklets currently do not belong to any agent cluster, so a SharedArrayBuffer can not be shared with a worklet. However, it is intended that this is possible; see, for example, https://github.com/w3c/css-houdini-drafts/issues/380 and the AudioWorklet article here: https://developers.google.com/web/updates/2018/06/audio-worklet-design-pattern#webaudio_powerhouse_audio_worklet_and_sharedarraybuffer This change funnels the agent cluster ID through when creating a ThreadedWorklet, so a SharedArrayBuffer can be shared as long as the creator of the Worklet's thread is in the same agent cluster. It is not clear that this is the behavior that will be specified, however. Bug: chromium:892067 Change-Id: If1a2187ae38da41f2389538c07e7b04921c6128f Reviewed-on: https://chromium-review.googlesource.com/c/1262932 Commit-Queue: Ben Smith <binji@chromium.org> Reviewed-by: Hongchan Choi <hongchan@chromium.org> Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Cr-Commit-Position: refs/heads/master@{#598619} [add] https://crrev.com/97167bddb398b0adb7e06f6f5c68e9f5d556ad96/third_party/WebKit/LayoutTests/external/wpt/webaudio/the-audio-api/the-audioworklet-interface/audioworklet-postmessage-sharedarraybuffer.https.html [add] https://crrev.com/97167bddb398b0adb7e06f6f5c68e9f5d556ad96/third_party/WebKit/LayoutTests/external/wpt/webaudio/the-audio-api/the-audioworklet-interface/processors/sharedarraybuffer-processor.js [modify] https://crrev.com/97167bddb398b0adb7e06f6f5c68e9f5d556ad96/third_party/blink/renderer/core/workers/threaded_worklet_messaging_proxy.cc [modify] https://crrev.com/97167bddb398b0adb7e06f6f5c68e9f5d556ad96/third_party/blink/renderer/core/workers/worklet_global_scope.cc [modify] https://crrev.com/97167bddb398b0adb7e06f6f5c68e9f5d556ad96/third_party/blink/renderer/core/workers/worklet_global_scope.h
,
Oct 11
Please merge to 70 and 71 after this has enough canary coverage.
,
Oct 11
,
Oct 13
Looks like this made the M71 branch cut. I'll merge back to M70 the beginning of next week.
,
Oct 15
Pls merge your change to M71 branch #3578 ASAP so we can pick it up for next M71 dev release. Thank you.
,
Oct 15
FYI M70 stable is tomorrow. This has to be in M70 by today in order to make it to stable.
,
Oct 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/db54921dea346d5d2676be91eba718a311916db1 commit db54921dea346d5d2676be91eba718a311916db1 Author: Ben Smith <binji@chromium.org> Date: Mon Oct 15 15:22:07 2018 Allow posting a SharedArrayBuffer to AudioWorklet The HTML spec only allows passing SharedArrayBuffers to an agent in the same agent cluster. Worklets currently do not belong to any agent cluster, so a SharedArrayBuffer can not be shared with a worklet. However, it is intended that this is possible; see, for example, https://github.com/w3c/css-houdini-drafts/issues/380 and the AudioWorklet article here: https://developers.google.com/web/updates/2018/06/audio-worklet-design-pattern#webaudio_powerhouse_audio_worklet_and_sharedarraybuffer This change funnels the agent cluster ID through when creating a ThreadedWorklet, so a SharedArrayBuffer can be shared as long as the creator of the Worklet's thread is in the same agent cluster. It is not clear that this is the behavior that will be specified, however. Bug: chromium:892067 Change-Id: If1a2187ae38da41f2389538c07e7b04921c6128f Reviewed-on: https://chromium-review.googlesource.com/c/1262932 Commit-Queue: Ben Smith <binji@chromium.org> Reviewed-by: Hongchan Choi <hongchan@chromium.org> Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#598619}(cherry picked from commit 97167bddb398b0adb7e06f6f5c68e9f5d556ad96) Reviewed-on: https://chromium-review.googlesource.com/c/1280843 Reviewed-by: Ben Smith <binji@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#998} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [add] https://crrev.com/db54921dea346d5d2676be91eba718a311916db1/third_party/WebKit/LayoutTests/external/wpt/webaudio/the-audio-api/the-audioworklet-interface/audioworklet-postmessage-sharedarraybuffer.https.html [add] https://crrev.com/db54921dea346d5d2676be91eba718a311916db1/third_party/WebKit/LayoutTests/external/wpt/webaudio/the-audio-api/the-audioworklet-interface/processors/sharedarraybuffer-processor.js [modify] https://crrev.com/db54921dea346d5d2676be91eba718a311916db1/third_party/blink/renderer/core/workers/threaded_worklet_messaging_proxy.cc [modify] https://crrev.com/db54921dea346d5d2676be91eba718a311916db1/third_party/blink/renderer/core/workers/worklet_global_scope.cc [modify] https://crrev.com/db54921dea346d5d2676be91eba718a311916db1/third_party/blink/renderer/core/workers/worklet_global_scope.h
,
Oct 15
Here's a summary of the rules that were executed: - OnlyMergeApprovedChange: Rule Failed -- Revision db54921dea346d5d2676be91eba718a311916db1 was merged to refs/branch-heads/3538 branch with no merge approval from a TPM! Please explain why this change was merged to the branch! - AcknowledgeMerge: Notification Required --
,
Oct 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/db54921dea346d5d2676be91eba718a311916db1 Commit: db54921dea346d5d2676be91eba718a311916db1 Author: binji@chromium.org Commiter: binji@chromium.org Date: 2018-10-15 15:22:07 +0000 UTC Allow posting a SharedArrayBuffer to AudioWorklet The HTML spec only allows passing SharedArrayBuffers to an agent in the same agent cluster. Worklets currently do not belong to any agent cluster, so a SharedArrayBuffer can not be shared with a worklet. However, it is intended that this is possible; see, for example, https://github.com/w3c/css-houdini-drafts/issues/380 and the AudioWorklet article here: https://developers.google.com/web/updates/2018/06/audio-worklet-design-pattern#webaudio_powerhouse_audio_worklet_and_sharedarraybuffer This change funnels the agent cluster ID through when creating a ThreadedWorklet, so a SharedArrayBuffer can be shared as long as the creator of the Worklet's thread is in the same agent cluster. It is not clear that this is the behavior that will be specified, however. Bug: chromium:892067 Change-Id: If1a2187ae38da41f2389538c07e7b04921c6128f Reviewed-on: https://chromium-review.googlesource.com/c/1262932 Commit-Queue: Ben Smith <binji@chromium.org> Reviewed-by: Hongchan Choi <hongchan@chromium.org> Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#598619}(cherry picked from commit 97167bddb398b0adb7e06f6f5c68e9f5d556ad96) Reviewed-on: https://chromium-review.googlesource.com/c/1280843 Reviewed-by: Ben Smith <binji@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#998} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
,
Oct 15
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 15
,
Oct 15
|
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by hablich@chromium.org
, Oct 4