- Revision
- 273542
- Author
- [email protected]
- Date
- 2021-02-25 22:43:34 -0800 (Thu, 25 Feb 2021)
Log Message
Fix threading issue in ScriptProcessorNode::process()
https://bugs.webkit.org/show_bug.cgi?id=222447
<rdar://74700526>
Reviewed by Jer Noble.
It was possible for the audio thread (in ScriptProcessorNode::process())
and the main thread (via ScriptProcessorNode::fireProcessEvent()) to
access the same m_inputBuffers / m_outputBuffers concurrently, causing
crashes. The m_processLock was supposed to avoid this. However, the
scope of the locking in ScriptProcessorNode::process() was insufficient
to protect us. ScriptProcessorNode::process() now grabs the lock very
early, before interacting with any buffers and we make sure not to
modify the buffers when we are unable to grab the lock.
Also, to make sure that the m_inputBuffers / m_outputBuffers are modified
by the main thread only during the scope of the fireProcessEvent() function
(during which we hold the processLock), we no longer pass our internal
buffers to the JS process event handler. Instead, we pass new buffers to
JS and memcpy to and from them. While this is less efficient, this is
actually required because the script could store the buffers it is given
and modify them outside the scope of the process event handler.
* Modules/webaudio/AudioBuffer.cpp:
(WebCore::AudioBuffer::AudioBuffer):
(WebCore::AudioBuffer::topologyMatches const):
(WebCore::AudioBuffer::copyTo const):
(WebCore::AudioBuffer::clone const):
* Modules/webaudio/AudioBuffer.h:
* Modules/webaudio/ScriptProcessorNode.cpp:
(WebCore::ScriptProcessorNode::createInputBufferForJS const):
(WebCore::ScriptProcessorNode::createOutputBufferForJS const):
(WebCore::ScriptProcessorNode::process):
(WebCore::ScriptProcessorNode::fireProcessEvent):
* Modules/webaudio/ScriptProcessorNode.h:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (273541 => 273542)
--- trunk/Source/WebCore/ChangeLog 2021-02-26 05:06:47 UTC (rev 273541)
+++ trunk/Source/WebCore/ChangeLog 2021-02-26 06:43:34 UTC (rev 273542)
@@ -1,3 +1,41 @@
+2021-02-25 Chris Dumez <[email protected]>
+
+ Fix threading issue in ScriptProcessorNode::process()
+ https://bugs.webkit.org/show_bug.cgi?id=222447
+ <rdar://74700526>
+
+ Reviewed by Jer Noble.
+
+ It was possible for the audio thread (in ScriptProcessorNode::process())
+ and the main thread (via ScriptProcessorNode::fireProcessEvent()) to
+ access the same m_inputBuffers / m_outputBuffers concurrently, causing
+ crashes. The m_processLock was supposed to avoid this. However, the
+ scope of the locking in ScriptProcessorNode::process() was insufficient
+ to protect us. ScriptProcessorNode::process() now grabs the lock very
+ early, before interacting with any buffers and we make sure not to
+ modify the buffers when we are unable to grab the lock.
+
+ Also, to make sure that the m_inputBuffers / m_outputBuffers are modified
+ by the main thread only during the scope of the fireProcessEvent() function
+ (during which we hold the processLock), we no longer pass our internal
+ buffers to the JS process event handler. Instead, we pass new buffers to
+ JS and memcpy to and from them. While this is less efficient, this is
+ actually required because the script could store the buffers it is given
+ and modify them outside the scope of the process event handler.
+
+ * Modules/webaudio/AudioBuffer.cpp:
+ (WebCore::AudioBuffer::AudioBuffer):
+ (WebCore::AudioBuffer::topologyMatches const):
+ (WebCore::AudioBuffer::copyTo const):
+ (WebCore::AudioBuffer::clone const):
+ * Modules/webaudio/AudioBuffer.h:
+ * Modules/webaudio/ScriptProcessorNode.cpp:
+ (WebCore::ScriptProcessorNode::createInputBufferForJS const):
+ (WebCore::ScriptProcessorNode::createOutputBufferForJS const):
+ (WebCore::ScriptProcessorNode::process):
+ (WebCore::ScriptProcessorNode::fireProcessEvent):
+ * Modules/webaudio/ScriptProcessorNode.h:
+
2021-02-25 Myles C. Maxfield <[email protected]>
Emoji sequences with constituents in the UBLOCK_SYMBOLS_AND_PICTOGRAPHS_EXTENDED_A Unicode block don't get combined properly
Modified: trunk/Source/WebCore/Modules/webaudio/AudioBuffer.cpp (273541 => 273542)
--- trunk/Source/WebCore/Modules/webaudio/AudioBuffer.cpp 2021-02-26 05:06:47 UTC (rev 273541)
+++ trunk/Source/WebCore/Modules/webaudio/AudioBuffer.cpp 2021-02-26 06:43:34 UTC (rev 273542)
@@ -84,6 +84,7 @@
AudioBuffer::AudioBuffer(unsigned numberOfChannels, size_t length, float sampleRate, LegacyPreventDetaching preventDetaching)
: m_sampleRate(sampleRate)
, m_originalLength(length)
+ , m_isDetachable(preventDetaching == LegacyPreventDetaching::No)
{
m_channels.reserveCapacity(numberOfChannels);
@@ -267,6 +268,34 @@
return false;
}
+bool AudioBuffer::topologyMatches(const AudioBuffer& other) const
+{
+ return numberOfChannels() == other.numberOfChannels() && length() == other.length() && sampleRate() == other.sampleRate();
+}
+
+bool AudioBuffer::copyTo(AudioBuffer& other) const
+{
+ if (!topologyMatches(other))
+ return false;
+
+ if (hasDetachedChannelBuffer() || other.hasDetachedChannelBuffer())
+ return false;
+
+ for (unsigned channelIndex = 0; channelIndex < numberOfChannels(); ++channelIndex)
+ memcpy(other.rawChannelData(channelIndex), m_channels[channelIndex]->data(), length() * sizeof(float));
+
+ return true;
+}
+
+Ref<AudioBuffer> AudioBuffer::clone(ShouldCopyChannelData shouldCopyChannelData) const
+{
+ auto clone = AudioBuffer::create(numberOfChannels(), length(), sampleRate(), m_isDetachable ? LegacyPreventDetaching::No : LegacyPreventDetaching::Yes);
+ ASSERT(clone);
+ if (shouldCopyChannelData == ShouldCopyChannelData::Yes)
+ copyTo(*clone);
+ return clone.releaseNonNull();
+}
+
} // namespace WebCore
#endif // ENABLE(WEB_AUDIO)
Modified: trunk/Source/WebCore/Modules/webaudio/AudioBuffer.h (273541 => 273542)
--- trunk/Source/WebCore/Modules/webaudio/AudioBuffer.h 2021-02-26 05:06:47 UTC (rev 273541)
+++ trunk/Source/WebCore/Modules/webaudio/AudioBuffer.h 2021-02-26 06:43:34 UTC (rev 273542)
@@ -76,7 +76,14 @@
size_t memoryCost() const;
template<typename Visitor> void visitChannelWrappers(Visitor&);
+
+ bool copyTo(AudioBuffer&) const;
+
+ enum class ShouldCopyChannelData : bool { No, Yes };
+ Ref<AudioBuffer> clone(ShouldCopyChannelData = ShouldCopyChannelData::Yes) const;
+ bool topologyMatches(const AudioBuffer&) const;
+
private:
AudioBuffer(unsigned numberOfChannels, size_t length, float sampleRate, LegacyPreventDetaching = LegacyPreventDetaching::No);
explicit AudioBuffer(AudioBus&);
@@ -90,6 +97,7 @@
size_t m_originalLength;
Vector<RefPtr<Float32Array>> m_channels;
Vector<JSValueInWrappedObject> m_channelWrappers;
+ bool m_isDetachable { true };
};
} // namespace WebCore
Modified: trunk/Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp (273541 => 273542)
--- trunk/Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp 2021-02-26 05:06:47 UTC (rev 273541)
+++ trunk/Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp 2021-02-26 06:43:34 UTC (rev 273542)
@@ -101,6 +101,31 @@
AudioNode::initialize();
}
+RefPtr<AudioBuffer> ScriptProcessorNode::createInputBufferForJS(AudioBuffer* inputBuffer) const
+{
+ if (!inputBuffer)
+ return nullptr;
+
+ // As an optimization, we reuse the same buffer as last time when possible.
+ if (!m_cachedInputBufferForJS || !m_cachedInputBufferForJS->topologyMatches(*inputBuffer))
+ m_cachedInputBufferForJS = inputBuffer->clone();
+ else
+ inputBuffer->copyTo(*m_cachedInputBufferForJS);
+
+ return m_cachedInputBufferForJS;
+}
+
+RefPtr<AudioBuffer> ScriptProcessorNode::createOutputBufferForJS(AudioBuffer& outputBuffer) const
+{
+ // As an optimization, we reuse the same buffer as last time when possible.
+ if (!m_cachedOutputBufferForJS || m_cachedOutputBufferForJS->topologyMatches(outputBuffer))
+ m_cachedOutputBufferForJS = outputBuffer.clone(AudioBuffer::ShouldCopyChannelData::No);
+ else
+ m_cachedOutputBufferForJS->zero();
+
+ return m_cachedOutputBufferForJS;
+}
+
void ScriptProcessorNode::uninitialize()
{
if (!isInitialized())
@@ -131,6 +156,14 @@
AudioBus* inputBus = this->input(0)->bus();
AudioBus* outputBus = this->output(0)->bus();
+ auto locker = tryHoldLock(m_processLock);
+ if (!locker) {
+ // We're late in handling the previous request. The main thread must be
+ // very busy. The best we can do is clear out the buffer ourself here.
+ outputBus->zero();
+ return;
+ }
+
// Get input and output buffers. We double-buffer both the input and output sides.
unsigned doubleBufferIndex = this->doubleBufferIndex();
bool isDoubleBufferIndexGood = doubleBufferIndex < 2 && doubleBufferIndex < m_inputBuffers.size() && doubleBufferIndex < m_outputBuffers.size();
@@ -192,14 +225,6 @@
});
semaphore.wait();
} else {
- auto locker = tryHoldLock(m_processLock);
- if (!locker) {
- // We're late in handling the previous request. The main thread must be
- // very busy. The best we can do is clear out the buffer ourself here.
- outputBuffer->zero();
- return;
- }
-
callOnMainThread([this, doubleBufferIndex = m_doubleBufferIndex, protector = makeRef(*this)] {
auto locker = holdLock(m_processLock);
fireProcessEvent(doubleBufferIndex);
@@ -226,14 +251,21 @@
return;
// Avoid firing the event if the document has already gone away.
- if (!context().isStopped()) {
- // Calculate playbackTime with the buffersize which needs to be processed each time when onaudioprocess is called.
- // The outputBuffer being passed to JS will be played after exhausting previous outputBuffer by double-buffering.
- double playbackTime = (context().currentSampleFrame() + m_bufferSize) / static_cast<double>(context().sampleRate());
+ if (context().isStopped())
+ return;
- // Call the _javascript_ event handler which will do the audio processing.
- dispatchEvent(AudioProcessingEvent::create(inputBuffer, outputBuffer, playbackTime));
- }
+ // Calculate playbackTime with the buffersize which needs to be processed each time when onaudioprocess is called.
+ // The outputBuffer being passed to JS will be played after exhausting previous outputBuffer by double-buffering.
+ double playbackTime = (context().currentSampleFrame() + m_bufferSize) / static_cast<double>(context().sampleRate());
+
+ auto inputBufferForJS = createInputBufferForJS(inputBuffer);
+ auto outputBufferForJS = createOutputBufferForJS(*outputBuffer);
+
+ // Call the _javascript_ event handler which will do the audio processing.
+ dispatchEvent(AudioProcessingEvent::create(inputBufferForJS.get(), outputBufferForJS.get(), playbackTime));
+
+ if (!outputBufferForJS->copyTo(*outputBuffer))
+ outputBuffer->zero();
}
ExceptionOr<void> ScriptProcessorNode::setChannelCount(unsigned channelCount)
Modified: trunk/Source/WebCore/Modules/webaudio/ScriptProcessorNode.h (273541 => 273542)
--- trunk/Source/WebCore/Modules/webaudio/ScriptProcessorNode.h 2021-02-26 05:06:47 UTC (rev 273541)
+++ trunk/Source/WebCore/Modules/webaudio/ScriptProcessorNode.h 2021-02-26 06:43:34 UTC (rev 273542)
@@ -80,6 +80,9 @@
void eventListenersDidChange() final;
void fireProcessEvent(unsigned doubleBufferIndex);
+ RefPtr<AudioBuffer> createInputBufferForJS(AudioBuffer*) const;
+ RefPtr<AudioBuffer> createOutputBufferForJS(AudioBuffer&) const;
+
// Double buffering
unsigned doubleBufferIndex() const { return m_doubleBufferIndex; }
void swapBuffers() { m_doubleBufferIndex = 1 - m_doubleBufferIndex; }
@@ -86,6 +89,8 @@
unsigned m_doubleBufferIndex { 0 };
Vector<RefPtr<AudioBuffer>> m_inputBuffers;
Vector<RefPtr<AudioBuffer>> m_outputBuffers;
+ mutable RefPtr<AudioBuffer> m_cachedInputBufferForJS;
+ mutable RefPtr<AudioBuffer> m_cachedOutputBufferForJS;
size_t m_bufferSize;
unsigned m_bufferReadWriteIndex { 0 };
