Attachment 345950 Details for Bug 188097 – Patch

[patch] Patch

bug-188097-20180727135337.patch (text/plain), 18.90 KB, created by Chris Dumez on 2018-07-27 13:53:37 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-07-27 13:53:37 PDT

Size: 18.90 KB

patch

obsolete

>Subversion Revision: 234310
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 0ff7fc87e61852842c1494a36168dcf108441ef7..823a01479b2b6866c3a347c7f12bf38172d9e087 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,51 @@
>+2018-07-27  Chris Dumez  <cdumez@apple.com>
>+
>+        Fix thread-safety issues related to RealtimeMediaSource::audioSamplesAvailable()
>+        https://bugs.webkit.org/show_bug.cgi?id=188097
>+        <rdar://problem/42558823>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-26  Andy VanWagoner  <andy@vanwagoner.family>
> 
>         [INTL] Remove INTL sub-feature compile flags
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 5eb8f0509446dec27822ed99294dec2b590ba41f..3f7ea801737ca6ac393c99f985228d1a959d456d 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,13 @@
>+2018-07-27  Chris Dumez  <cdumez@apple.com>
>+
>+        Fix thread-safety issues related to RealtimeMediaSource::audioSamplesAvailable()
>+        https://bugs.webkit.org/show_bug.cgi?id=188097
>+        <rdar://problem/42558823>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:
>+
> 2018-07-27  Chris Dumez  <cdumez@apple.com>
> 
>         Loading a file URL and then issuing a reload right away causes the load to fail due to sandboxing
>diff --git a/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp b/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp
>index d7abbdf02e450d8b3c16027b5b3b6e3cf1a83299..c60571fbc3fdaab75b47aab1f72d05c3a698efa6 100644
>--- a/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp
>+++ b/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp
>@@ -63,16 +63,32 @@ MediaStreamTrackPrivate::~MediaStreamTrackPrivate()
>     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 @@ void MediaStreamTrackPrivate::setEnabled(bool enabled)
>     // 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 @@ void MediaStreamTrackPrivate::endTrack()
> 
>     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 @@ AudioSourceProvider* MediaStreamTrackPrivate::audioSourceProvider()
> 
> 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 @@ void MediaStreamTrackPrivate::sourceStopped()
>     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 @@ void MediaStreamTrackPrivate::videoSampleAvailable(MediaSample& mediaSample)
>         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 @@ void MediaStreamTrackPrivate::audioSamplesAvailable(const MediaTime& mediaTime,
>         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 @@ void MediaStreamTrackPrivate::updateReadyState()
>         return;
> 
>     m_readyState = state;
>-    for (auto& observer : m_observers)
>-        observer->readyStateChanged(*this);
>+    forEachObserver([this](auto& observer) {
>+        observer.readyStateChanged(*this);
>+    });
> }
> 
> } // namespace WebCore
>diff --git a/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h b/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h
>index 3a816fd30722d0c690c85da71cc6efbc25a5ece1..d4ff4d40faaf747809453e75aa38a7082f005f69 100644
>--- a/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h
>+++ b/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h
>@@ -50,8 +50,10 @@ public:
>         virtual void trackSettingsChanged(MediaStreamTrackPrivate&) = 0;
>         virtual void trackEnabledChanged(MediaStreamTrackPrivate&) = 0;
>         virtual void sampleBufferUpdated(MediaStreamTrackPrivate&, MediaSample&) { };
>-        virtual void audioSamplesAvailable(MediaStreamTrackPrivate&, const MediaTime&, const PlatformAudioData&, const AudioStreamDescription&, size_t) { };
>         virtual void readyStateChanged(MediaStreamTrackPrivate&) { };
>+
>+        // May get called on a background thread.
>+        virtual void audioSamplesAvailable(MediaStreamTrackPrivate&, const MediaTime&, const PlatformAudioData&, const AudioStreamDescription&, size_t) { };
>     };
> 
>     static Ref<MediaStreamTrackPrivate> create(Ref<RealtimeMediaSource>&&);
>@@ -116,7 +118,10 @@ private:
> 
>     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;
>diff --git a/Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp b/Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp
>index be625a3700104396abbf26390341183656e40806..bfe4d6de1c8340682a1f7ee29e8c94585fe0bce8 100644
>--- a/Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp
>+++ b/Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp
>@@ -61,16 +61,16 @@ RealtimeMediaSource::RealtimeMediaSource(const String& id, Type type, const Stri
> 
> 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 @@ void RealtimeMediaSource::notifyMutedChange(bool muted)
>     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 @@ void RealtimeMediaSource::settingsDidChange()
> 
>     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 @@ void RealtimeMediaSource::start()
>     if (!m_isProducingData)
>         return;
> 
>-    for (Observer& observer : m_observers)
>+    forEachObserver([](auto& observer) {
>         observer.sourceStarted();
>+    });
> }
> 
> void RealtimeMediaSource::stop()
>@@ -173,17 +194,20 @@ void RealtimeMediaSource::requestStop(Observer* callingObserver)
>     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 @@ void RealtimeMediaSource::captureFailed()
>     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>)
>diff --git a/Source/WebCore/platform/mediastream/RealtimeMediaSource.h b/Source/WebCore/platform/mediastream/RealtimeMediaSource.h
>index 1d0129477c2369c1bcbcbbbcee4f1835352dbc9c..c29ab19c16df3b1fd01a0040b92fa5a723b48b0a 100644
>--- a/Source/WebCore/platform/mediastream/RealtimeMediaSource.h
>+++ b/Source/WebCore/platform/mediastream/RealtimeMediaSource.h
>@@ -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,13 +254,16 @@ private:
>     virtual void startProducingData() { }
>     virtual void stopProducingData() { }
> 
>+    void forEachObserver(const WTF::Function<void(Observer&)>&) const;
>+
>     bool m_muted { false };
> 
>     String m_id;
>     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 };
>diff --git a/Source/WebCore/platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp b/Source/WebCore/platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp
>index 87a65f26b1378a8f4714626739bf01f0a0394209..f6de2ea54dc8670de62265206894b86f953df77e 100644
>--- a/Source/WebCore/platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp
>+++ b/Source/WebCore/platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp
>@@ -165,12 +165,9 @@ AudioComponentInstance AudioTrackPrivateMediaStreamCocoa::createAudioUnit(CAAudi
>     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) {
>diff --git a/Source/WebCore/platform/mediastream/mac/RealtimeOutgoingAudioSourceCocoa.cpp b/Source/WebCore/platform/mediastream/mac/RealtimeOutgoingAudioSourceCocoa.cpp
>index 236667617dd1973c2e553f3e412c0f14b75e6b97..4ab406d58955e2fcb683447e6586b571640fd30b 100644
>--- a/Source/WebCore/platform/mediastream/mac/RealtimeOutgoingAudioSourceCocoa.cpp
>+++ b/Source/WebCore/platform/mediastream/mac/RealtimeOutgoingAudioSourceCocoa.cpp
>@@ -81,6 +81,7 @@ bool RealtimeOutgoingAudioSourceCocoa::hasBufferedEnoughData()
>     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) {
>diff --git a/Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm b/Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm
>index d4e973b9928f8ebe9496b3fab9671963380a88ad..94efae695ddd292cf8f4214e3c3d04d181638e35 100644
>--- a/Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm
>+++ b/Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm
>@@ -159,6 +159,7 @@ void WebAudioSourceProviderAVFObjC::unprepare()
>     }
> }
> 
>+// 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())
>diff --git a/Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp b/Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp
>index f47f16ee29273a011ac84b2f246c657f717500cb..f343a05b6dad1233b61a13665f56e52c6f493090 100644
>--- a/Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp
>+++ b/Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp
>@@ -80,6 +80,7 @@ public:
>         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);

Actions: View | Formatted Diff | Diff

Attachments on bug 188097: 345943 | 345950