Changeset 278309 in webkit

Timestamp:

Jun 1, 2021, 8:34:54 AM (5 years ago)

Author:

Chris Dumez

Message:

Fix thread safety issues in MediaStreamAudioSourceNode
​https://bugs.webkit.org/show_bug.cgi?id=226476

Reviewed by Youenn Fablet.

Adopt thread safety analysis annotations in MediaStreamAudioSourceNode and fix
bugs found by clang. In particular, the following issues were fixed:

• setFormat() could modify m_sourceNumberOfChannels before locking on the main thread.
• process() was accessing m_sourceNumberOfChannels / m_sourceSampleRate on the rendering thread *before* locking.

• Modules/webaudio/MediaStreamAudioSourceNode.cpp:

(WebCore::MediaStreamAudioSourceNode::setFormat):
(WebCore::MediaStreamAudioSourceNode::process):

• Modules/webaudio/MediaStreamAudioSourceNode.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• Modules/webaudio/MediaStreamAudioSourceNode.cpp (modified) (4 diffs)
• Modules/webaudio/MediaStreamAudioSourceNode.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-06-01  Chris Dumez  <cdumez@apple.com>
+
+        Fix thread safety issues in MediaStreamAudioSourceNode
+        https://bugs.webkit.org/show_bug.cgi?id=226476
+
+        Reviewed by Youenn Fablet.
+
+        Adopt thread safety analysis annotations in MediaStreamAudioSourceNode and fix
+        bugs found by clang. In particular, the following issues were fixed:
+        - setFormat() could modify m_sourceNumberOfChannels before locking on the main
+          thread.
+        - process() was accessing m_sourceNumberOfChannels / m_sourceSampleRate
+          on the rendering thread *before* locking.
+
+        * Modules/webaudio/MediaStreamAudioSourceNode.cpp:
+        (WebCore::MediaStreamAudioSourceNode::setFormat):
+        (WebCore::MediaStreamAudioSourceNode::process):
+        * Modules/webaudio/MediaStreamAudioSourceNode.h:
+
 2021-06-01  Chris Dumez  <cdumez@apple.com>
 

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

@@ -90 +90 @@
 void MediaStreamAudioSourceNode::setFormat(size_t numberOfChannels, float sourceSampleRate)
 {
-    float sampleRate = this->sampleRate();
+    // Synchronize with process().
+    Locker locker { m_processLock };
+
     if (numberOfChannels == m_sourceNumberOfChannels && sourceSampleRate == m_sourceSampleRate)
         return;
@@ -102 +104 @@
     }
 
-    // Synchronize with process().
-    Locker locker { m_processLock };
-
     m_sourceNumberOfChannels = numberOfChannels;
     m_sourceSampleRate = sourceSampleRate;
 
+    float sampleRate = this->sampleRate();
     if (sourceSampleRate == sampleRate)
         m_multiChannelResampler = nullptr;
@@ -135 +135 @@
     AudioBus* outputBus = output(0)->bus();
 
-    if (!mediaStream() || !m_sourceNumberOfChannels || !m_sourceSampleRate) {
-        outputBus->zero();
-        return;
-    }
-
     // Use tryLock() to avoid contention in the real-time audio thread.
     // If we fail to acquire the lock then the MediaStream must be in the middle of
@@ -149 +144 @@
     }
     Locker locker { AdoptLock, m_processLock };
-    if (m_sourceNumberOfChannels != outputBus->numberOfChannels()) {
+
+    if (!m_sourceNumberOfChannels || !m_sourceSampleRate || m_sourceNumberOfChannels != outputBus->numberOfChannels()) {
         outputBus->zero();
         return;

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

@@ -47 +47 @@
     ~MediaStreamAudioSourceNode();
 
-    MediaStream* mediaStream() { return &m_mediaStream.get(); }
+    MediaStream& mediaStream() { return m_mediaStream; }
 
 private:
@@ -68 +68 @@
     Ref<MediaStream> m_mediaStream;
     Ref<WebAudioSourceProvider> m_provider;
-    std::unique_ptr<MultiChannelResampler> m_multiChannelResampler;
+    std::unique_ptr<MultiChannelResampler> m_multiChannelResampler WTF_GUARDED_BY_LOCK(m_processLock);
 
     Lock m_processLock;
 
-    unsigned m_sourceNumberOfChannels { 0 };
-    double m_sourceSampleRate { 0 };
+    unsigned m_sourceNumberOfChannels WTF_GUARDED_BY_LOCK(m_processLock) { 0 };
+    double m_sourceSampleRate WTF_GUARDED_BY_LOCK(m_processLock) { 0 };
 };