Title: [278306] trunk/Source/WebCore
Revision
278306
Author
cdu...@apple.com
Date
2021-06-01 07:56:04 -0700 (Tue, 01 Jun 2021)

Log 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:

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (278305 => 278306)


--- trunk/Source/WebCore/ChangeLog	2021-06-01 13:58:23 UTC (rev 278305)
+++ trunk/Source/WebCore/ChangeLog	2021-06-01 14:56:04 UTC (rev 278306)
@@ -1,3 +1,23 @@
+2021-06-01  Chris Dumez  <cdu...@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  <an...@apple.com>
 
         Rename ComplexLineLayout to LegacyLineLayout

Modified: trunk/Source/WebCore/Modules/webaudio/MediaElementAudioSourceNode.cpp (278305 => 278306)


--- trunk/Source/WebCore/Modules/webaudio/MediaElementAudioSourceNode.cpp	2021-06-01 13:58:23 UTC (rev 278305)
+++ trunk/Source/WebCore/Modules/webaudio/MediaElementAudioSourceNode.cpp	2021-06-01 14:56:04 UTC (rev 278306)
@@ -83,6 +83,10 @@
 void MediaElementAudioSourceNode::setFormat(size_t numberOfChannels, float sourceSampleRate)
 {
     auto protectedThis = makeRef(*this);
+
+    // Synchronize with process().
+    Locker locker { m_processLock };
+
     m_muted = wouldTaintOrigin();
 
     if (numberOfChannels != m_sourceNumberOfChannels || sourceSampleRate != m_sourceSampleRate) {
@@ -97,9 +101,6 @@
         m_sourceNumberOfChannels = numberOfChannels;
         m_sourceSampleRate = sourceSampleRate;
 
-        // Synchronize with process().
-        Locker locker { m_processLock };
-
         if (sourceSampleRate != sampleRate()) {
             double scaleFactor = sourceSampleRate / sampleRate();
             m_multiChannelResampler = makeUnique<MultiChannelResampler>(scaleFactor, numberOfChannels, AudioUtilities::renderQuantumSize, std::bind(&MediaElementAudioSourceNode::provideInput, this, std::placeholders::_1, std::placeholders::_2));
@@ -150,11 +151,6 @@
 {
     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
     // reconfiguring its playback engine, so we output silence in this case.
@@ -165,7 +161,8 @@
     }
 
     Locker locker { AdoptLock, m_processLock };
-    if (m_sourceNumberOfChannels != outputBus->numberOfChannels()) {
+
+    if (m_muted || !m_sourceNumberOfChannels || !m_sourceSampleRate || m_sourceNumberOfChannels != outputBus->numberOfChannels()) {
         outputBus->zero();
         return;
     }

Modified: trunk/Source/WebCore/Modules/webaudio/MediaElementAudioSourceNode.h (278305 => 278306)


--- trunk/Source/WebCore/Modules/webaudio/MediaElementAudioSourceNode.h	2021-06-01 13:58:23 UTC (rev 278305)
+++ trunk/Source/WebCore/Modules/webaudio/MediaElementAudioSourceNode.h	2021-06-01 14:56:04 UTC (rev 278306)
@@ -72,11 +72,11 @@
     Ref<HTMLMediaElement> m_mediaElement;
     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);
 };
 
 } // namespace WebCore
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to