Changeset 278289 in webkit

Timestamp:

May 31, 2021, 5:09:42 PM (5 years ago)

Author:

Chris Dumez

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:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• Modules/webaudio/ConvolverNode.cpp (modified) (6 diffs)
• Modules/webaudio/ConvolverNode.h (modified) (3 diffs)
• Modules/webaudio/ConvolverNode.idl (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-05-31  Chris Dumez  <cdumez@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  <cdumez@apple.com>
 

trunk/Source/WebCore/Modules/webaudio/ConvolverNode.cpp

@@ -64 +64 @@
         return result.releaseException();
 
-    node->setNormalize(!options.disableNormalization);
-
-    result = node->setBuffer(WTFMove(options.buffer));
+    node->setNormalizeForBindings(!options.disableNormalization);
+
+    result = node->setBufferForBindings(WTFMove(options.buffer));
     if (result.hasException())
         return result.releaseException();
@@ -111 +111 @@
 }
 
-ExceptionOr<void> ConvolverNode::setBuffer(RefPtr<AudioBuffer>&& buffer)
+ExceptionOr<void> ConvolverNode::setBufferForBindings(RefPtr<AudioBuffer>&& buffer)
 {
     ASSERT(isMainThread());
@@ -161 +161 @@
 }
 
-AudioBuffer* ConvolverNode::buffer()
+AudioBuffer* ConvolverNode::bufferForBindings() WTF_IGNORES_THREAD_SAFETY_ANALYSIS
 {
     ASSERT(isMainThread());
@@ -167 +167 @@
 }
 
+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;
 }
@@ -174 +184 @@
 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;
 }
@@ -200 +214 @@
 {
     ASSERT(context().isAudioThread() && context().isGraphOwner());
-
-    if (m_buffer) {
-        unsigned numberOfOutputChannels = computeNumberOfOutputChannels(input->numberOfChannels(), m_buffer->numberOfChannels());
+    std::optional<unsigned> numberOfBufferChannels;
+    if (m_processLock.tryLock()) {
+        Locker locker { AdoptLock, m_processLock };
+        if (m_buffer)
+            numberOfBufferChannels = m_buffer->numberOfChannels();
+    }
+
+    if (numberOfBufferChannels) {
+        unsigned numberOfOutputChannels = computeNumberOfOutputChannels(input->numberOfChannels(), *numberOfBufferChannels);
 
         if (isInitialized() && numberOfOutputChannels != output(0)->numberOfChannels()) {

trunk/Source/WebCore/Modules/webaudio/ConvolverNode.h

@@ -42 +42 @@
     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;
@@ -61 +61 @@
     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().
@@ -68 +68 @@
 
     // Normalize the impulse response or not.
-    bool m_normalize { true };
+    bool m_normalize { true }; // Only used on the main thread.
 };
 

trunk/Source/WebCore/Modules/webaudio/ConvolverNode.idl

@@ -32 +32 @@
     [EnabledBySetting=WebAudio] constructor (BaseAudioContext context, optional ConvolverOptions options);
 
-    attribute AudioBuffer? buffer;
-    attribute boolean normalize;
+    [ImplementedAs=bufferForBindings] attribute AudioBuffer? buffer;
+    [ImplementedAs=normalizeForBindings] attribute boolean normalize;
 };