Skip to content

Commit 3d951ff

Browse files
cdumezcdumez
committed
AudioWorkletProcessor::process() may get called on non-audio worklet thread and crash
https://bugs.webkit.org/show_bug.cgi?id=219209 Reviewed by Geoffrey Garen. Source/WebCore: If the AudioContext is already running when the AudioWorklet becomes ready, then the AudioWorkletNode::process() was getting called on the CoreAudio rendering thread instead of the AudioWorklet thread. This was causing us to run the AudioWorkletProcessor's JS on the wrong thread, which would cause flaky crashes in release and assertion hits in debug. To address the issue, I updated AudioWorkletNode::process() to output silence when it is called on another thread than the AudioWorklet thread. Also, if the AudioContext is already running when the AudioWorklet becomes ready, we need to restart the AudioDestination so that we switch the audio rendering from the CoreAudio rendering thread to the AudioWorklet thread. Test: http/wpt/webaudio/the-audio-api/the-audioworklet-interface/context-already-rendering.html * Modules/webaudio/AudioDestinationNode.h: (WebCore::AudioDestinationNode::restartRendering): * Modules/webaudio/AudioWorkletNode.cpp: (WebCore::AudioWorkletNode::isWorkletThread): (WebCore::AudioWorkletNode::process): * Modules/webaudio/AudioWorkletNode.h: * Modules/webaudio/BaseAudioContext.cpp: (WebCore::BaseAudioContext::addAudioParamDescriptors): (WebCore::BaseAudioContext::workletIsReady): * Modules/webaudio/BaseAudioContext.h: * Modules/webaudio/DefaultAudioDestinationNode.cpp: (WebCore::DefaultAudioDestinationNode::uninitialize): (WebCore::DefaultAudioDestinationNode::startRendering): (WebCore::DefaultAudioDestinationNode::resume): (WebCore::DefaultAudioDestinationNode::suspend): (WebCore::DefaultAudioDestinationNode::restartRendering): (WebCore::DefaultAudioDestinationNode::setChannelCount): * Modules/webaudio/DefaultAudioDestinationNode.h: LayoutTests: Add layout test coverage. * http/wpt/webaudio/the-audio-api/the-audioworklet-interface/context-already-rendering-expected.txt: Added. * http/wpt/webaudio/the-audio-api/the-audioworklet-interface/context-already-rendering.html: Added. * http/wpt/webaudio/the-audio-api/the-audioworklet-interface/processors/basic-processor.js: Added. (BasicProcessor.prototype.process): (BasicProcessor): Canonical link: https://commits.webkit.org/231836@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@270127 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 274981e commit 3d951ff

12 files changed

Lines changed: 135 additions & 3 deletions

File tree

‎LayoutTests/ChangeLog‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,18 @@
1+
2020-11-20 Chris Dumez <cdumez@apple.com>
2+
3+
AudioWorkletProcessor::process() may get called on non-audio worklet thread and crash
4+
https://bugs.webkit.org/show_bug.cgi?id=219209
5+
6+
Reviewed by Geoffrey Garen.
7+
8+
Add layout test coverage.
9+
10+
* http/wpt/webaudio/the-audio-api/the-audioworklet-interface/context-already-rendering-expected.txt: Added.
11+
* http/wpt/webaudio/the-audio-api/the-audioworklet-interface/context-already-rendering.html: Added.
12+
* http/wpt/webaudio/the-audio-api/the-audioworklet-interface/processors/basic-processor.js: Added.
13+
(BasicProcessor.prototype.process):
14+
(BasicProcessor):
15+
116
2020-11-20 Lauro Moura <lmoura@igalia.com>
217

318
canvas: drawImage should not raise IndexSizeError on empty sources

‎LayoutTests/http/wpt/webaudio/the-audio-api/the-audioworklet-interface/context-already-rendering-expected.txt‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
2+
PASS Passes if it does not crash
3+

‎LayoutTests/http/wpt/webaudio/the-audio-api/the-audioworklet-interface/context-already-rendering.html‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<!doctype html>
2+
<title>Make sure we don't crash if the AudioWorklet gets constructed while the AudioContext is already rendering.</title>
3+
<script src=/resources/testharness.js></script>
4+
<script src=/resources/testharnessreport.js></script>
5+
<script>
6+
var context;
7+
promise_setup(async (t) => {
8+
context = new AudioContext();
9+
internals.withUserGesture(() => {
10+
context.resume();
11+
});
12+
13+
const filePath = 'processors/basic-processor.js';
14+
await context.audioWorklet.addModule(filePath);
15+
});
16+
17+
const wait_for_audioworklet_rendering = async (node) => {
18+
await new Promise((resolve) => {
19+
node.port.onmessage = resolve;
20+
});
21+
};
22+
23+
promise_test(async (t) => {
24+
const options = {
25+
numberOfInputs: 0,
26+
numberOfOutputs: 1
27+
};
28+
29+
const node = new AudioWorkletNode(context, 'basic-processor', options);
30+
await wait_for_audioworklet_rendering(node);
31+
}, 'Passes if it does not crash');
32+
</script>

‎LayoutTests/http/wpt/webaudio/the-audio-api/the-audioworklet-interface/processors/basic-processor.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
class BasicProcessor extends AudioWorkletProcessor {
2+
process(inputs, outputs) {
3+
this.port.postMessage("did rendering");
4+
return false;
5+
}
6+
}
7+
8+
registerProcessor('basic-processor', BasicProcessor);

‎Source/WebCore/ChangeLog‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,43 @@
1+
2020-11-20 Chris Dumez <cdumez@apple.com>
2+
3+
AudioWorkletProcessor::process() may get called on non-audio worklet thread and crash
4+
https://bugs.webkit.org/show_bug.cgi?id=219209
5+
6+
Reviewed by Geoffrey Garen.
7+
8+
If the AudioContext is already running when the AudioWorklet becomes ready, then the
9+
AudioWorkletNode::process() was getting called on the CoreAudio rendering thread instead
10+
of the AudioWorklet thread. This was causing us to run the AudioWorkletProcessor's JS
11+
on the wrong thread, which would cause flaky crashes in release and assertion hits in
12+
debug.
13+
14+
To address the issue, I updated AudioWorkletNode::process() to output silence when
15+
it is called on another thread than the AudioWorklet thread. Also, if the AudioContext
16+
is already running when the AudioWorklet becomes ready, we need to restart the
17+
AudioDestination so that we switch the audio rendering from the CoreAudio rendering
18+
thread to the AudioWorklet thread.
19+
20+
Test: http/wpt/webaudio/the-audio-api/the-audioworklet-interface/context-already-rendering.html
21+
22+
* Modules/webaudio/AudioDestinationNode.h:
23+
(WebCore::AudioDestinationNode::restartRendering):
24+
* Modules/webaudio/AudioWorkletNode.cpp:
25+
(WebCore::AudioWorkletNode::isWorkletThread):
26+
(WebCore::AudioWorkletNode::process):
27+
* Modules/webaudio/AudioWorkletNode.h:
28+
* Modules/webaudio/BaseAudioContext.cpp:
29+
(WebCore::BaseAudioContext::addAudioParamDescriptors):
30+
(WebCore::BaseAudioContext::workletIsReady):
31+
* Modules/webaudio/BaseAudioContext.h:
32+
* Modules/webaudio/DefaultAudioDestinationNode.cpp:
33+
(WebCore::DefaultAudioDestinationNode::uninitialize):
34+
(WebCore::DefaultAudioDestinationNode::startRendering):
35+
(WebCore::DefaultAudioDestinationNode::resume):
36+
(WebCore::DefaultAudioDestinationNode::suspend):
37+
(WebCore::DefaultAudioDestinationNode::restartRendering):
38+
(WebCore::DefaultAudioDestinationNode::setChannelCount):
39+
* Modules/webaudio/DefaultAudioDestinationNode.h:
40+
141
2020-11-20 Lauro Moura <lmoura@igalia.com>
242

343
canvas: drawImage should not raise IndexSizeError on empty sources

‎Source/WebCore/Modules/webaudio/AudioDestinationNode.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ class AudioDestinationNode : public AudioNode, public AudioIOCallback {
5959
virtual void resume(CompletionHandler<void(Optional<Exception>&&)>&& completionHandler) { completionHandler(WTF::nullopt); }
6060
virtual void suspend(CompletionHandler<void(Optional<Exception>&&)>&& completionHandler) { completionHandler(WTF::nullopt); }
6161
virtual void close(CompletionHandler<void()>&& completionHandler) { completionHandler(); }
62+
virtual void restartRendering() { }
6263

6364
virtual bool isPlaying() { return false; }
6465
void isPlayingDidChange() override;

‎Source/WebCore/Modules/webaudio/AudioWorkletNode.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,7 @@ void AudioWorkletNode::setProcessor(RefPtr<AudioWorkletProcessor>&& processor)
182182
if (processor) {
183183
auto locker = holdLock(m_processLock);
184184
m_processor = WTFMove(processor);
185+
m_workletThread = &Thread::current();
185186
} else
186187
fireProcessorErrorOnMainThread(ProcessorError::ConstructorError);
187188
}
@@ -191,7 +192,7 @@ void AudioWorkletNode::process(size_t framesToProcess)
191192
ASSERT(!isMainThread());
192193

193194
auto locker = tryHoldLock(m_processLock);
194-
if (!locker || !m_processor) {
195+
if (!locker || !m_processor || &Thread::current() != m_workletThread) {
195196
// We're not ready yet or we are getting destroyed. In this case, we output silence.
196197
for (unsigned i = 0; i < numberOfOutputs(); ++i)
197198
output(i)->bus()->zero();

‎Source/WebCore/Modules/webaudio/AudioWorkletNode.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ class AudioWorkletNode : public AudioNode, public ActiveDOMObject {
8787
Lock m_processLock;
8888
RefPtr<AudioWorkletProcessor> m_processor; // Should only be used on the rendering thread.
8989
HashMap<String, std::unique_ptr<AudioFloatArray>> m_paramValuesMap;
90+
Thread* m_workletThread { nullptr };
9091

9192
// Keeps the reference of AudioBus objects from AudioNodeInput and AudioNodeOutput in order
9293
// to pass them to AudioWorkletProcessor.

‎Source/WebCore/Modules/webaudio/BaseAudioContext.cpp‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1040,7 +1040,10 @@ PeriodicWave& BaseAudioContext::periodicWave(OscillatorType type)
10401040
void BaseAudioContext::addAudioParamDescriptors(const String& processorName, Vector<AudioParamDescriptor>&& descriptors)
10411041
{
10421042
ASSERT(!m_parameterDescriptorMap.contains(processorName));
1043+
bool wasEmpty = m_parameterDescriptorMap.isEmpty();
10431044
m_parameterDescriptorMap.add(processorName, WTFMove(descriptors));
1045+
if (wasEmpty)
1046+
workletIsReady();
10441047
}
10451048

10461049
void BaseAudioContext::sourceNodeWillBeginPlayback(AudioNode& node)
@@ -1055,6 +1058,16 @@ void BaseAudioContext::sourceNodeDidFinishPlayback(AudioNode& node)
10551058
m_finishedSourceNodes.append(&node);
10561059
}
10571060

1061+
void BaseAudioContext::workletIsReady()
1062+
{
1063+
ASSERT(isMainThread());
1064+
1065+
// If we're already rendering when the worklet becomes ready, we need to restart
1066+
// rendering in order to switch to the audio worklet thread.
1067+
if (m_destinationNode)
1068+
m_destinationNode->restartRendering();
1069+
}
1070+
10581071
#if !RELEASE_LOG_DISABLED
10591072
WTFLogChannel& BaseAudioContext::logChannel() const
10601073
{

‎Source/WebCore/Modules/webaudio/BaseAudioContext.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,7 @@ class BaseAudioContext
317317
void clear();
318318

319319
void scheduleNodeDeletion();
320+
void workletIsReady();
320321

321322
// When source nodes begin playing, the BaseAudioContext keeps them alive inside m_referencedSourceNodes.
322323
// When the nodes stop playing, they get added to m_finishedSourceNodes. After each rendering quantum,

0 commit comments

Comments
 (0)