Title: [278289] trunk/Source/WebCore
- Revision
- 278289
- Author
- cdu...@apple.com
- Date
- 2021-05-31 17:09:42 -0700 (Mon, 31 May 2021)
Log Message
Fix thread safety issues in ConvolverNode
https://bugs.webkit.org/show_bug.cgi?id=226449
Reviewed by Darin Adler.
Adopt thread safety annotations in ConvolverNode and fix bugs found by clang.
In particular, the following issues were found and fixed:
- tailTime() / latencyTime() were accessing m_reverb on the audio thread without
locking even though m_reverb gets modified on the main thread.
- checkNumberOfChannelsForInput() was accessing m_buffer on the audio thread
without locking even though m_buffer gets modified on the main thread.
* Modules/webaudio/ConvolverNode.cpp:
(WebCore::ConvolverNode::create):
(WebCore::ConvolverNode::setBufferForBindings):
(WebCore::ConvolverNode::setNormalizeForBindings):
(WebCore::ConvolverNode::tailTime const):
(WebCore::ConvolverNode::latencyTime const):
(WebCore::ConvolverNode::checkNumberOfChannelsForInput):
* Modules/webaudio/ConvolverNode.h:
* Modules/webaudio/ConvolverNode.idl:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (278288 => 278289)
--- trunk/Source/WebCore/ChangeLog 2021-06-01 00:08:20 UTC (rev 278288)
+++ trunk/Source/WebCore/ChangeLog 2021-06-01 00:09:42 UTC (rev 278289)
@@ -1,5 +1,29 @@
2021-05-31 Chris Dumez <cdu...@apple.com>
+ Fix thread safety issues in ConvolverNode
+ https://bugs.webkit.org/show_bug.cgi?id=226449
+
+ Reviewed by Darin Adler.
+
+ Adopt thread safety annotations in ConvolverNode and fix bugs found by clang.
+ In particular, the following issues were found and fixed:
+ - tailTime() / latencyTime() were accessing m_reverb on the audio thread without
+ locking even though m_reverb gets modified on the main thread.
+ - checkNumberOfChannelsForInput() was accessing m_buffer on the audio thread
+ without locking even though m_buffer gets modified on the main thread.
+
+ * Modules/webaudio/ConvolverNode.cpp:
+ (WebCore::ConvolverNode::create):
+ (WebCore::ConvolverNode::setBufferForBindings):
+ (WebCore::ConvolverNode::setNormalizeForBindings):
+ (WebCore::ConvolverNode::tailTime const):
+ (WebCore::ConvolverNode::latencyTime const):
+ (WebCore::ConvolverNode::checkNumberOfChannelsForInput):
+ * Modules/webaudio/ConvolverNode.h:
+ * Modules/webaudio/ConvolverNode.idl:
+
+2021-05-31 Chris Dumez <cdu...@apple.com>
+
Drop PendingActivity data member from BaseAudioContext
https://bugs.webkit.org/show_bug.cgi?id=226445
Modified: trunk/Source/WebCore/Modules/webaudio/ConvolverNode.cpp (278288 => 278289)
--- trunk/Source/WebCore/Modules/webaudio/ConvolverNode.cpp 2021-06-01 00:08:20 UTC (rev 278288)
+++ trunk/Source/WebCore/Modules/webaudio/ConvolverNode.cpp 2021-06-01 00:09:42 UTC (rev 278289)
@@ -63,9 +63,9 @@
if (result.hasException())
return result.releaseException();
- node->setNormalize(!options.disableNormalization);
+ node->setNormalizeForBindings(!options.disableNormalization);
- result = node->setBuffer(WTFMove(options.buffer));
+ result = node->setBufferForBindings(WTFMove(options.buffer));
if (result.hasException())
return result.releaseException();
@@ -110,7 +110,7 @@
}
}
-ExceptionOr<void> ConvolverNode::setBuffer(RefPtr<AudioBuffer>&& buffer)
+ExceptionOr<void> ConvolverNode::setBufferForBindings(RefPtr<AudioBuffer>&& buffer)
{
ASSERT(isMainThread());
@@ -160,19 +160,33 @@
return { };
}
-AudioBuffer* ConvolverNode::buffer()
+AudioBuffer* ConvolverNode::bufferForBindings() WTF_IGNORES_THREAD_SAFETY_ANALYSIS
{
ASSERT(isMainThread());
return m_buffer.get();
}
+void ConvolverNode::setNormalizeForBindings(bool normalize)
+{
+ ASSERT(isMainThread());
+ m_normalize = normalize;
+}
+
double ConvolverNode::tailTime() const
{
+ ASSERT(context().isAudioThread());
+ if (!m_processLock.tryLock())
+ return std::numeric_limits<double>::infinity();
+ Locker locker { AdoptLock, m_processLock };
return m_reverb ? m_reverb->impulseResponseLength() / static_cast<double>(sampleRate()) : 0;
}
double ConvolverNode::latencyTime() const
{
+ ASSERT(context().isAudioThread());
+ if (!m_processLock.tryLock())
+ return std::numeric_limits<double>::infinity();
+ Locker locker { AdoptLock, m_processLock };
return m_reverb ? m_reverb->latencyFrames() / static_cast<double>(sampleRate()) : 0;
}
@@ -199,9 +213,15 @@
void ConvolverNode::checkNumberOfChannelsForInput(AudioNodeInput* input)
{
ASSERT(context().isAudioThread() && context().isGraphOwner());
+ std::optional<unsigned> numberOfBufferChannels;
+ if (m_processLock.tryLock()) {
+ Locker locker { AdoptLock, m_processLock };
+ if (m_buffer)
+ numberOfBufferChannels = m_buffer->numberOfChannels();
+ }
- if (m_buffer) {
- unsigned numberOfOutputChannels = computeNumberOfOutputChannels(input->numberOfChannels(), m_buffer->numberOfChannels());
+ if (numberOfBufferChannels) {
+ unsigned numberOfOutputChannels = computeNumberOfOutputChannels(input->numberOfChannels(), *numberOfBufferChannels);
if (isInitialized() && numberOfOutputChannels != output(0)->numberOfChannels()) {
// We're already initialized but the channel count has changed.
Modified: trunk/Source/WebCore/Modules/webaudio/ConvolverNode.h (278288 => 278289)
--- trunk/Source/WebCore/Modules/webaudio/ConvolverNode.h 2021-06-01 00:08:20 UTC (rev 278288)
+++ trunk/Source/WebCore/Modules/webaudio/ConvolverNode.h 2021-06-01 00:09:42 UTC (rev 278289)
@@ -41,11 +41,11 @@
virtual ~ConvolverNode();
- ExceptionOr<void> setBuffer(RefPtr<AudioBuffer>&&);
- AudioBuffer* buffer();
+ ExceptionOr<void> setBufferForBindings(RefPtr<AudioBuffer>&&);
+ AudioBuffer* bufferForBindings(); // Only safe to call on the main thread.
- bool normalize() const { return m_normalize; }
- void setNormalize(bool normalize) { m_normalize = normalize; }
+ bool normalizeForBindings() const WTF_IGNORES_THREAD_SAFETY_ANALYSIS { ASSERT(isMainThread()); return m_normalize; }
+ void setNormalizeForBindings(bool);
ExceptionOr<void> setChannelCount(unsigned) final;
ExceptionOr<void> setChannelCountMode(ChannelCountMode) final;
@@ -60,14 +60,14 @@
void process(size_t framesToProcess) final;
void checkNumberOfChannelsForInput(AudioNodeInput*) final;
- std::unique_ptr<Reverb> m_reverb;
- RefPtr<AudioBuffer> m_buffer;
+ std::unique_ptr<Reverb> m_reverb WTF_GUARDED_BY_LOCK(m_processLock); // Only modified on the main thread but accessed on the audio thread.
+ RefPtr<AudioBuffer> m_buffer WTF_GUARDED_BY_LOCK(m_processLock); // Only modified on the main thread but accessed on the audio thread.
// This synchronizes dynamic changes to the convolution impulse response with process().
mutable Lock m_processLock;
// Normalize the impulse response or not.
- bool m_normalize { true };
+ bool m_normalize { true }; // Only used on the main thread.
};
} // namespace WebCore
Modified: trunk/Source/WebCore/Modules/webaudio/ConvolverNode.idl (278288 => 278289)
--- trunk/Source/WebCore/Modules/webaudio/ConvolverNode.idl 2021-06-01 00:08:20 UTC (rev 278288)
+++ trunk/Source/WebCore/Modules/webaudio/ConvolverNode.idl 2021-06-01 00:09:42 UTC (rev 278289)
@@ -31,6 +31,6 @@
] interface ConvolverNode : AudioNode {
[EnabledBySetting=WebAudio] constructor (BaseAudioContext context, optional ConvolverOptions options);
- attribute AudioBuffer? buffer;
- attribute boolean normalize;
+ [ImplementedAs=bufferForBindings] attribute AudioBuffer? buffer;
+ [ImplementedAs=normalizeForBindings] attribute boolean normalize;
};
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
