Changeset 278306 in webkit

Timestamp:

Jun 1, 2021, 7:56:04 AM (5 years ago)

Author:

Chris Dumez

Message:

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

Reviewed by Youenn Fablet.

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

• setFormat() was modifying m_muted / m_sourceNumberOfChannels / m_sourceSampleRate on the main thread without locking, even though those data members are accessed from the rendering thread.
• process() was accessing m_muted / m_sourceNumberOfChannels / m_sourceSampleRate on the rendering thread *before* locking.

• Modules/webaudio/MediaElementAudioSourceNode.cpp:

(WebCore::MediaElementAudioSourceNode::setFormat):
(WebCore::MediaElementAudioSourceNode::process):

• Modules/webaudio/MediaElementAudioSourceNode.h:

Location:

trunk/Source/WebCore

Files:

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

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-06-01  Chris Dumez  <cdumez@apple.com>
+
+        Fix thread safety issues in MediaElementAudioSourceNode
+        https://bugs.webkit.org/show_bug.cgi?id=226475
+
+        Reviewed by Youenn Fablet.
+
+        Adopt thread safety analysis annotations in MediaElementAudioSourceNode and fix
+        bugs found by clang. In particular, the following issues were fixed:
+        - setFormat() was modifying m_muted / m_sourceNumberOfChannels / m_sourceSampleRate
+          on the main thread without locking, even though those data members are accessed
+          from the rendering thread.
+        - process() was accessing  m_muted / m_sourceNumberOfChannels / m_sourceSampleRate
+          on the rendering thread *before* locking.
+
+        * Modules/webaudio/MediaElementAudioSourceNode.cpp:
+        (WebCore::MediaElementAudioSourceNode::setFormat):
+        (WebCore::MediaElementAudioSourceNode::process):
+        * Modules/webaudio/MediaElementAudioSourceNode.h:
+
 2021-06-01  Antti Koivisto  <antti@apple.com>
 

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

@@ -84 +84 @@
 {
     auto protectedThis = makeRef(*this);
+
+    // Synchronize with process().
+    Locker locker { m_processLock };
+
     m_muted = wouldTaintOrigin();
 
@@ -97 +101 @@
         m_sourceNumberOfChannels = numberOfChannels;
         m_sourceSampleRate = sourceSampleRate;
-
-        // Synchronize with process().
-        Locker locker { m_processLock };
 
         if (sourceSampleRate != sampleRate()) {
@@ -151 +152 @@
     AudioBus* outputBus = output(0)->bus();
 
-    if (m_muted || !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 HTMLMediaElement must be in the middle of
@@ -166 +162 @@
 
     Locker locker { AdoptLock, m_processLock };
-    if (m_sourceNumberOfChannels != outputBus->numberOfChannels()) {
+
+    if (m_muted || !m_sourceNumberOfChannels || !m_sourceSampleRate || m_sourceNumberOfChannels != outputBus->numberOfChannels()) {
         outputBus->zero();
         return;

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

@@ -73 +73 @@
     Lock m_processLock;
 
-    unsigned m_sourceNumberOfChannels { 0 };
-    double m_sourceSampleRate { 0 };
-    bool m_muted { false };
+    unsigned m_sourceNumberOfChannels WTF_GUARDED_BY_LOCK(m_processLock) { 0 };
+    double m_sourceSampleRate WTF_GUARDED_BY_LOCK(m_processLock) { 0 };
+    bool m_muted WTF_GUARDED_BY_LOCK(m_processLock) { false };
 
-    std::unique_ptr<MultiChannelResampler> m_multiChannelResampler;
+    std::unique_ptr<MultiChannelResampler> m_multiChannelResampler WTF_GUARDED_BY_LOCK(m_processLock);
 };