Diff
Modified: trunk/Source/WebCore/ChangeLog (234333 => 234334)
--- trunk/Source/WebCore/ChangeLog 2018-07-27 22:32:25 UTC (rev 234333)
+++ trunk/Source/WebCore/ChangeLog 2018-07-27 22:33:52 UTC (rev 234334)
@@ -1,3 +1,51 @@
+2018-07-27 Chris Dumez <[email protected]>
+
+ Fix thread-safety issues related to RealtimeMediaSource::audioSamplesAvailable()
+ https://bugs.webkit.org/show_bug.cgi?id=188097
+ <rdar://problem/42558823>
+
+ Reviewed by Eric Carlson.
+
+ Fix thread-safety issues related to RealtimeMediaSource::audioSamplesAvailable(). RealtimeMediaSource::audioSamplesAvailable()
+ is called on a background thread but iterates over observers that may get destroyed concurrently on the main thread.
+
+ To address the issue:
+ - Introduce a Lock to protect m_observer accesses
+ - Copy observers to a Vector before iterating over them
+
+ * platform/mediastream/MediaStreamTrackPrivate.cpp:
+ (WebCore::MediaStreamTrackPrivate::forEachObserver const):
+ (WebCore::MediaStreamTrackPrivate::addObserver):
+ (WebCore::MediaStreamTrackPrivate::removeObserver):
+ (WebCore::MediaStreamTrackPrivate::setEnabled):
+ (WebCore::MediaStreamTrackPrivate::endTrack):
+ (WebCore::MediaStreamTrackPrivate::sourceStarted):
+ (WebCore::MediaStreamTrackPrivate::sourceStopped):
+ (WebCore::MediaStreamTrackPrivate::sourceMutedChanged):
+ (WebCore::MediaStreamTrackPrivate::sourceSettingsChanged):
+ (WebCore::MediaStreamTrackPrivate::videoSampleAvailable):
+ (WebCore::MediaStreamTrackPrivate::audioSamplesAvailable):
+ (WebCore::MediaStreamTrackPrivate::updateReadyState):
+ * platform/mediastream/MediaStreamTrackPrivate.h:
+ (WebCore::MediaStreamTrackPrivate::Observer::sampleBufferUpdated):
+ (WebCore::MediaStreamTrackPrivate::Observer::audioSamplesAvailable):
+ * platform/mediastream/RealtimeMediaSource.cpp:
+ (WebCore::RealtimeMediaSource::addObserver):
+ (WebCore::RealtimeMediaSource::removeObserver):
+ (WebCore::RealtimeMediaSource::forEachObserver const):
+ (WebCore::RealtimeMediaSource::notifyMutedObservers const):
+ (WebCore::RealtimeMediaSource::settingsDidChange):
+ (WebCore::RealtimeMediaSource::videoSampleAvailable):
+ (WebCore::RealtimeMediaSource::audioSamplesAvailable):
+ (WebCore::RealtimeMediaSource::start):
+ (WebCore::RealtimeMediaSource::requestStop):
+ (WebCore::RealtimeMediaSource::captureFailed):
+ * platform/mediastream/RealtimeMediaSource.h:
+ * platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp:
+ (WebCore::AudioTrackPrivateMediaStreamCocoa::audioSamplesAvailable):
+ * platform/mediastream/mac/RealtimeOutgoingAudioSourceCocoa.cpp:
+ * platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:
+
2018-07-27 Alex Christensen <[email protected]>
Don't include WebPageProxy.h just for UndoOrRedo
Modified: trunk/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp (234333 => 234334)
--- trunk/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp 2018-07-27 22:32:25 UTC (rev 234333)
+++ trunk/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp 2018-07-27 22:33:52 UTC (rev 234334)
@@ -63,16 +63,32 @@
m_source->removeObserver(*this);
}
+void MediaStreamTrackPrivate::forEachObserver(const WTF::Function<void(Observer&)>& apply) const
+{
+ Vector<Observer*> observersCopy;
+ {
+ auto locker = holdLock(m_observersLock);
+ observersCopy = copyToVector(m_observers);
+ }
+ for (auto* observer : observersCopy) {
+ auto locker = holdLock(m_observersLock);
+ // Make sure the observer has not been destroyed.
+ if (!m_observers.contains(observer))
+ continue;
+ apply(*observer);
+ }
+}
+
void MediaStreamTrackPrivate::addObserver(MediaStreamTrackPrivate::Observer& observer)
{
- m_observers.append(&observer);
+ auto locker = holdLock(m_observersLock);
+ m_observers.add(&observer);
}
void MediaStreamTrackPrivate::removeObserver(MediaStreamTrackPrivate::Observer& observer)
{
- size_t pos = m_observers.find(&observer);
- if (pos != notFound)
- m_observers.remove(pos);
+ auto locker = holdLock(m_observersLock);
+ m_observers.remove(&observer);
}
const String& MediaStreamTrackPrivate::label() const
@@ -98,8 +114,9 @@
// Always update the enabled state regardless of the track being ended.
m_isEnabled = enabled;
- for (auto& observer : m_observers)
- observer->trackEnabledChanged(*this);
+ forEachObserver([this](auto& observer) {
+ observer.trackEnabledChanged(*this);
+ });
}
void MediaStreamTrackPrivate::endTrack()
@@ -115,8 +132,9 @@
m_source->requestStop(this);
- for (auto& observer : m_observers)
- observer->trackEnded(*this);
+ forEachObserver([this](auto& observer) {
+ observer.trackEnded(*this);
+ });
}
Ref<MediaStreamTrackPrivate> MediaStreamTrackPrivate::clone()
@@ -160,8 +178,9 @@
void MediaStreamTrackPrivate::sourceStarted()
{
- for (auto& observer : m_observers)
- observer->trackStarted(*this);
+ forEachObserver([this](auto& observer) {
+ observer.trackStarted(*this);
+ });
}
void MediaStreamTrackPrivate::sourceStopped()
@@ -172,20 +191,23 @@
m_isEnded = true;
updateReadyState();
- for (auto& observer : m_observers)
- observer->trackEnded(*this);
+ forEachObserver([this](auto& observer) {
+ observer.trackEnded(*this);
+ });
}
void MediaStreamTrackPrivate::sourceMutedChanged()
{
- for (auto& observer : m_observers)
- observer->trackMutedChanged(*this);
+ forEachObserver([this](auto& observer) {
+ observer.trackMutedChanged(*this);
+ });
}
void MediaStreamTrackPrivate::sourceSettingsChanged()
{
- for (auto& observer : m_observers)
- observer->trackSettingsChanged(*this);
+ forEachObserver([this](auto& observer) {
+ observer.trackSettingsChanged(*this);
+ });
}
bool MediaStreamTrackPrivate::preventSourceFromStopping()
@@ -205,10 +227,12 @@
return;
mediaSample.setTrackID(id());
- for (auto& observer : m_observers)
- observer->sampleBufferUpdated(*this, mediaSample);
+ forEachObserver([&](auto& observer) {
+ observer.sampleBufferUpdated(*this, mediaSample);
+ });
}
+// May get called on a background thread.
void MediaStreamTrackPrivate::audioSamplesAvailable(const MediaTime& mediaTime, const PlatformAudioData& data, const AudioStreamDescription& description, size_t sampleCount)
{
if (!m_haveProducedData) {
@@ -216,8 +240,9 @@
updateReadyState();
}
- for (auto& observer : m_observers)
- observer->audioSamplesAvailable(*this, mediaTime, data, description, sampleCount);
+ forEachObserver([&](auto& observer) {
+ observer.audioSamplesAvailable(*this, mediaTime, data, description, sampleCount);
+ });
}
@@ -234,8 +259,9 @@
return;
m_readyState = state;
- for (auto& observer : m_observers)
- observer->readyStateChanged(*this);
+ forEachObserver([this](auto& observer) {
+ observer.readyStateChanged(*this);
+ });
}
} // namespace WebCore
Modified: trunk/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h (234333 => 234334)
--- trunk/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h 2018-07-27 22:32:25 UTC (rev 234333)
+++ trunk/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h 2018-07-27 22:33:52 UTC (rev 234334)
@@ -50,8 +50,10 @@
virtual void trackSettingsChanged(MediaStreamTrackPrivate&) = 0;
virtual void trackEnabledChanged(MediaStreamTrackPrivate&) = 0;
virtual void sampleBufferUpdated(MediaStreamTrackPrivate&, MediaSample&) { };
+ virtual void readyStateChanged(MediaStreamTrackPrivate&) { };
+
+ // May get called on a background thread.
virtual void audioSamplesAvailable(MediaStreamTrackPrivate&, const MediaTime&, const PlatformAudioData&, const AudioStreamDescription&, size_t) { };
- virtual void readyStateChanged(MediaStreamTrackPrivate&) { };
};
static Ref<MediaStreamTrackPrivate> create(Ref<RealtimeMediaSource>&&);
@@ -116,7 +118,10 @@
void updateReadyState();
- Vector<Observer*> m_observers;
+ void forEachObserver(const WTF::Function<void(Observer&)>&) const;
+
+ mutable RecursiveLock m_observersLock;
+ HashSet<Observer*> m_observers;
Ref<RealtimeMediaSource> m_source;
String m_id;
Modified: trunk/Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp (234333 => 234334)
--- trunk/Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp 2018-07-27 22:32:25 UTC (rev 234333)
+++ trunk/Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp 2018-07-27 22:33:52 UTC (rev 234334)
@@ -61,16 +61,16 @@
void RealtimeMediaSource::addObserver(RealtimeMediaSource::Observer& observer)
{
- m_observers.append(observer);
+ auto locker = holdLock(m_observersLock);
+ m_observers.add(&observer);
}
void RealtimeMediaSource::removeObserver(RealtimeMediaSource::Observer& observer)
{
- m_observers.removeFirstMatching([&observer](auto anObserver) {
- return &anObserver.get() == &observer;
- });
+ auto locker = holdLock(m_observersLock);
- if (!m_observers.size())
+ m_observers.remove(&observer);
+ if (m_observers.isEmpty())
stop();
}
@@ -110,10 +110,27 @@
notifyMutedObservers();
}
+void RealtimeMediaSource::forEachObserver(const WTF::Function<void(Observer&)>& apply) const
+{
+ Vector<Observer*> observersCopy;
+ {
+ auto locker = holdLock(m_observersLock);
+ observersCopy = copyToVector(m_observers);
+ }
+ for (auto* observer : observersCopy) {
+ auto locker = holdLock(m_observersLock);
+ // Make sure the observer has not been destroyed.
+ if (!m_observers.contains(observer))
+ continue;
+ apply(*observer);
+ }
+}
+
void RealtimeMediaSource::notifyMutedObservers() const
{
- for (Observer& observer : m_observers)
+ forEachObserver([](auto& observer) {
observer.sourceMutedChanged();
+ });
}
void RealtimeMediaSource::settingsDidChange()
@@ -127,21 +144,24 @@
scheduleDeferredTask([this] {
m_pendingSettingsDidChangeNotification = false;
- for (Observer& observer : m_observers)
+ forEachObserver([](auto& observer) {
observer.sourceSettingsChanged();
+ });
});
}
void RealtimeMediaSource::videoSampleAvailable(MediaSample& mediaSample)
{
- for (Observer& observer : m_observers)
+ forEachObserver([&](auto& observer) {
observer.videoSampleAvailable(mediaSample);
+ });
}
void RealtimeMediaSource::audioSamplesAvailable(const MediaTime& time, const PlatformAudioData& audioData, const AudioStreamDescription& description, size_t numberOfFrames)
{
- for (Observer& observer : m_observers)
+ forEachObserver([&](auto& observer) {
observer.audioSamplesAvailable(time, audioData, description, numberOfFrames);
+ });
}
void RealtimeMediaSource::start()
@@ -155,8 +175,9 @@
if (!m_isProducingData)
return;
- for (Observer& observer : m_observers)
+ forEachObserver([](auto& observer) {
observer.sourceStarted();
+ });
}
void RealtimeMediaSource::stop()
@@ -173,17 +194,20 @@
if (!m_isProducingData)
return;
- for (Observer& observer : m_observers) {
+ bool hasObserverPreventingStopping = false;
+ forEachObserver([&](auto& observer) {
if (observer.preventSourceFromStopping())
- return;
- }
+ hasObserverPreventingStopping = true;
+ });
+ if (hasObserverPreventingStopping)
+ return;
stop();
- for (Observer& observer : m_observers) {
+ forEachObserver([callingObserver](auto& observer) {
if (&observer != callingObserver)
observer.sourceStopped();
- }
+ });
}
void RealtimeMediaSource::captureFailed()
@@ -193,8 +217,9 @@
m_isProducingData = false;
m_captureDidFailed = true;
- for (Observer& observer : m_observers)
+ forEachObserver([](auto& observer) {
observer.sourceStopped();
+ });
}
bool RealtimeMediaSource::supportsSizeAndFrameRate(std::optional<int>, std::optional<int>, std::optional<double>)
Modified: trunk/Source/WebCore/platform/mediastream/RealtimeMediaSource.h (234333 => 234334)
--- trunk/Source/WebCore/platform/mediastream/RealtimeMediaSource.h 2018-07-27 22:32:25 UTC (rev 234333)
+++ trunk/Source/WebCore/platform/mediastream/RealtimeMediaSource.h 2018-07-27 22:33:52 UTC (rev 234334)
@@ -41,6 +41,7 @@
#include "MediaSample.h"
#include "PlatformLayer.h"
#include "RealtimeMediaSourceCapabilities.h"
+#include <wtf/RecursiveLockAdapter.h>
#include <wtf/ThreadSafeRefCounted.h>
#include <wtf/Vector.h>
#include <wtf/WeakPtr.h>
@@ -253,6 +254,8 @@
virtual void startProducingData() { }
virtual void stopProducingData() { }
+ void forEachObserver(const WTF::Function<void(Observer&)>&) const;
+
bool m_muted { false };
String m_id;
@@ -259,7 +262,8 @@
String m_persistentID;
Type m_type;
String m_name;
- Vector<std::reference_wrapper<Observer>> m_observers;
+ mutable RecursiveLock m_observersLock;
+ HashSet<Observer*> m_observers;
IntSize m_size;
double m_frameRate { 30 };
double m_aspectRatio { 0 };
Modified: trunk/Source/WebCore/platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp (234333 => 234334)
--- trunk/Source/WebCore/platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp 2018-07-27 22:32:25 UTC (rev 234333)
+++ trunk/Source/WebCore/platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp 2018-07-27 22:33:52 UTC (rev 234334)
@@ -165,12 +165,9 @@
return remoteIOUnit;
}
+// May get called on a background thread.
void AudioTrackPrivateMediaStreamCocoa::audioSamplesAvailable(const MediaTime& sampleTime, const PlatformAudioData& audioData, const AudioStreamDescription& description, size_t sampleCount)
{
- // This function is called on a background thread. The following protectedThis object ensures the object is not
- // destroyed on the main thread before this function exits.
- Ref<AudioTrackPrivateMediaStreamCocoa> protectedThis { *this };
-
ASSERT(description.platformDescription().type == PlatformDescription::CAAudioStreamBasicType);
if (!m_inputDescription || *m_inputDescription != description) {
Modified: trunk/Source/WebCore/platform/mediastream/mac/RealtimeOutgoingAudioSourceCocoa.cpp (234333 => 234334)
--- trunk/Source/WebCore/platform/mediastream/mac/RealtimeOutgoingAudioSourceCocoa.cpp 2018-07-27 22:32:25 UTC (rev 234333)
+++ trunk/Source/WebCore/platform/mediastream/mac/RealtimeOutgoingAudioSourceCocoa.cpp 2018-07-27 22:33:52 UTC (rev 234334)
@@ -81,6 +81,7 @@
return writtenAudioDuration >= readAudioDuration + 0.01;
}
+// May get called on a background thread.
void RealtimeOutgoingAudioSourceCocoa::audioSamplesAvailable(const MediaTime&, const PlatformAudioData& audioData, const AudioStreamDescription& streamDescription, size_t sampleCount)
{
if (m_inputStreamDescription != streamDescription) {
Modified: trunk/Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm (234333 => 234334)
--- trunk/Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm 2018-07-27 22:32:25 UTC (rev 234333)
+++ trunk/Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm 2018-07-27 22:33:52 UTC (rev 234334)
@@ -159,6 +159,7 @@
}
}
+// May get called on a background thread.
void WebAudioSourceProviderAVFObjC::audioSamplesAvailable(MediaStreamTrackPrivate& track, const MediaTime&, const PlatformAudioData& data, const AudioStreamDescription& description, size_t frameCount)
{
if (!track.enabled())
Modified: trunk/Source/WebKit/ChangeLog (234333 => 234334)
--- trunk/Source/WebKit/ChangeLog 2018-07-27 22:32:25 UTC (rev 234333)
+++ trunk/Source/WebKit/ChangeLog 2018-07-27 22:33:52 UTC (rev 234334)
@@ -1,3 +1,13 @@
+2018-07-27 Chris Dumez <[email protected]>
+
+ Fix thread-safety issues related to RealtimeMediaSource::audioSamplesAvailable()
+ https://bugs.webkit.org/show_bug.cgi?id=188097
+ <rdar://problem/42558823>
+
+ Reviewed by Eric Carlson.
+
+ * UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:
+
2018-07-27 Alex Christensen <[email protected]>
Don't include WebPageProxy.h just for UndoOrRedo
Modified: trunk/Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp (234333 => 234334)
--- trunk/Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp 2018-07-27 22:32:25 UTC (rev 234333)
+++ trunk/Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp 2018-07-27 22:33:52 UTC (rev 234334)
@@ -80,6 +80,7 @@
m_manager.process().send(Messages::UserMediaCaptureManager::SourceSettingsChanged(m_id, m_source->settings()), 0);
}
+ // May get called on a background thread.
void audioSamplesAvailable(const MediaTime& time, const PlatformAudioData& audioData, const AudioStreamDescription& description, size_t numberOfFrames) final {
if (m_description != description) {
ASSERT(description.platformDescription().type == PlatformDescription::CAAudioStreamBasicType);
