Changeset 278285 in webkit

Timestamp:

May 31, 2021, 5:03:36 PM (5 years ago)

Author:

Chris Dumez

Message:

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

Reviewed by Darin Adler.

Adopt thread safety annotations in OscillatorNode and fix bugs found by clang.
In particular, propagatesSilence() was failing to grab the lock before accessing
m_periodicWave, which gets modified on the main thread.

• Modules/webaudio/OscillatorNode.cpp:

(WebCore::OscillatorNode::create):
(WebCore::OscillatorNode::setTypeForBindings):
(WebCore::OscillatorNode::propagatesSilence const):

• Modules/webaudio/OscillatorNode.h:
• Modules/webaudio/OscillatorNode.idl:

Location:

trunk/Source/WebCore

Files:

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

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-05-31  Chris Dumez  <cdumez@apple.com>
+
+        Fix thread safety issues in OscillatorNode
+        https://bugs.webkit.org/show_bug.cgi?id=226450
+
+        Reviewed by Darin Adler.
+
+        Adopt thread safety annotations in OscillatorNode and fix bugs found by clang.
+        In particular, propagatesSilence() was failing to grab the lock before accessing
+        m_periodicWave, which gets modified on the main thread.
+
+        * Modules/webaudio/OscillatorNode.cpp:
+        (WebCore::OscillatorNode::create):
+        (WebCore::OscillatorNode::setTypeForBindings):
+        (WebCore::OscillatorNode::propagatesSilence const):
+        * Modules/webaudio/OscillatorNode.h:
+        * Modules/webaudio/OscillatorNode.idl:
+
 2021-05-31  Chris Dumez  <cdumez@apple.com>
 

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

@@ -73 +73 @@
         oscillator->setPeriodicWave(*options.periodicWave);
     else {
-        result = oscillator->setType(options.type);
+        result = oscillator->setTypeForBindings(options.type);
         if (result.hasException())
             return result.releaseException();
@@ -98 +98 @@
 }
 
-ExceptionOr<void> OscillatorNode::setType(OscillatorType type)
+ExceptionOr<void> OscillatorNode::setTypeForBindings(OscillatorType type)
 {
     ALWAYS_LOG(LOGIDENTIFIER, type);
+    ASSERT(isMainThread());
 
     if (type == OscillatorType::Custom) {
@@ -439 +440 @@
 bool OscillatorNode::propagatesSilence() const
 {
-    return !isPlayingOrScheduled() || hasFinished() || !m_periodicWave.get();
+    ASSERT(context().isAudioThread());
+    if (!isPlayingOrScheduled() || hasFinished())
+        return true;
+    if (!m_processLock.tryLock())
+        return false; // Assume we have a periodic wave if we are unable to grab the lock.
+    Locker locker { AdoptLock, m_processLock };
+    return !m_periodicWave.get();
 }
 

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

@@ -45 +45 @@
     const char* activeDOMObjectName() const final { return "OscillatorNode"; }
 
-    OscillatorType type() const { return m_type; }
-    ExceptionOr<void> setType(OscillatorType);
+    OscillatorType typeForBindings() const { ASSERT(isMainThread()); return m_type; }
+    ExceptionOr<void> setTypeForBindings(OscillatorType);
 
     AudioParam* frequency() { return m_frequency.get(); }
@@ -63 +63 @@
 
     // Returns true if there are sample-accurate timeline parameter changes.
-    bool calculateSampleAccuratePhaseIncrements(size_t framesToProcess);
+    bool calculateSampleAccuratePhaseIncrements(size_t framesToProcess) WTF_REQUIRES_LOCK(m_processLock);
 
-    double processARate(int, float* destP, double virtualReadIndex, float* phaseIncrements);
-    double processKRate(int, float* destP, double virtualReadIndex);
+    double processARate(int, float* destP, double virtualReadIndex, float* phaseIncrements) WTF_REQUIRES_LOCK(m_processLock);
+    double processKRate(int, float* destP, double virtualReadIndex) WTF_REQUIRES_LOCK(m_processLock);
 
     bool propagatesSilence() const final;
 
     // One of the waveform types defined in the enum.
-    OscillatorType m_type;
+    OscillatorType m_type; // Only used on the main thread.
     
     // Frequency value in Hertz.
@@ -92 +92 @@
     AudioFloatArray m_detuneValues;
     
-    RefPtr<PeriodicWave> m_periodicWave;
+    RefPtr<PeriodicWave> m_periodicWave WTF_GUARDED_BY_LOCK(m_processLock);
 };
 

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

@@ -32 +32 @@
     [EnabledBySetting=WebAudio] constructor (BaseAudioContext context, optional OscillatorOptions options);
 
-    attribute OscillatorType type;
+    [ImplementedAs=typeForBindings] attribute OscillatorType type;
 
     readonly attribute AudioParam frequency; // in Hertz