Allow to remove MediaStreamPrivate observers when iterating over observers

https://bugs.webkit.org/show_bug.cgi?id=187256

Reviewed by Eric Carlson.

Migrate the observer list from a Vector to a HashSet.
This is more robust to multiple observing and keeping of order of observers is not required.
Copy the set of observers to a vector before iterating over it.
This allows to remove an observer while iterating, which is now used in UserMediaRequest.

Covered by existing tests.

* Modules/mediastream/UserMediaRequest.cpp:
(WebCore::UserMediaRequest::mediaStreamIsReady):
* platform/mediastream/MediaStreamPrivate.cpp:
(WebCore::MediaStreamPrivate::addObserver):
(WebCore::MediaStreamPrivate::removeObserver):
(WebCore::MediaStreamPrivate::forEachObserver const):
(WebCore::MediaStreamPrivate::updateActiveState):
(WebCore::MediaStreamPrivate::addTrack):
(WebCore::MediaStreamPrivate::removeTrack):
(WebCore::MediaStreamPrivate::characteristicsChanged):
* platform/mediastream/MediaStreamPrivate.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@238181 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 0cbb16c..144009b 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,5 +1,31 @@
 2018-11-14  Youenn Fablet  <youenn@apple.com>
 
 2018-11-14  Youenn Fablet  <youenn@apple.com>
 

+        Allow to remove MediaStreamPrivate observers when iterating over observers
+        https://bugs.webkit.org/show_bug.cgi?id=187256
+
+        Reviewed by Eric Carlson.
+
+        Migrate the observer list from a Vector to a HashSet.
+        This is more robust to multiple observing and keeping of order of observers is not required.
+        Copy the set of observers to a vector before iterating over it.
+        This allows to remove an observer while iterating, which is now used in UserMediaRequest.
+
+        Covered by existing tests.
+
+        * Modules/mediastream/UserMediaRequest.cpp:
+        (WebCore::UserMediaRequest::mediaStreamIsReady):
+        * platform/mediastream/MediaStreamPrivate.cpp:
+        (WebCore::MediaStreamPrivate::addObserver):
+        (WebCore::MediaStreamPrivate::removeObserver):
+        (WebCore::MediaStreamPrivate::forEachObserver const):
+        (WebCore::MediaStreamPrivate::updateActiveState):
+        (WebCore::MediaStreamPrivate::addTrack):
+        (WebCore::MediaStreamPrivate::removeTrack):
+        (WebCore::MediaStreamPrivate::characteristicsChanged):
+        * platform/mediastream/MediaStreamPrivate.h:
+
+2018-11-14  Youenn Fablet  <youenn@apple.com>
+

         Calling removeTrack on different RTCPeerConnection should throw InvalidAccessError
         https://bugs.webkit.org/show_bug.cgi?id=191603
 
         Calling removeTrack on different RTCPeerConnection should throw InvalidAccessError
         https://bugs.webkit.org/show_bug.cgi?id=191603
 

diff --git a/Source/WebCore/Modules/mediastream/UserMediaRequest.cpp b/Source/WebCore/Modules/mediastream/UserMediaRequest.cpp
index f616eb7..e10b96b 100644 (file)
--- a/Source/WebCore/Modules/mediastream/UserMediaRequest.cpp
+++ b/Source/WebCore/Modules/mediastream/UserMediaRequest.cpp
@@ -365,8 +365,7 @@ void UserMediaRequest::mediaStreamIsReady(Ref<MediaStream>&& stream)
     RELEASE_LOG(MediaStream, "UserMediaRequest::mediaStreamIsReady");
     stream->document()->setHasCaptureMediaStreamTrack();
     m_promise.resolve(WTFMove(stream));
     RELEASE_LOG(MediaStream, "UserMediaRequest::mediaStreamIsReady");
     stream->document()->setHasCaptureMediaStreamTrack();
     m_promise.resolve(WTFMove(stream));

-    // We are in an observer iterator loop, we do not want to change the observers within this loop.
-    callOnMainThread([stream = WTFMove(m_pendingActivationMediaStream)] { });
+    m_pendingActivationMediaStream = nullptr;

 }
 
 void UserMediaRequest::mediaStreamDidFail(RealtimeMediaSource::Type type)
 }
 
 void UserMediaRequest::mediaStreamDidFail(RealtimeMediaSource::Type type)

diff --git a/Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp b/Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp
index 965229e..bd8eb88 100644 (file)
--- a/Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp
+++ b/Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp
@@ -84,14 +84,21 @@ MediaStreamPrivate::~MediaStreamPrivate()
 
 void MediaStreamPrivate::addObserver(MediaStreamPrivate::Observer& observer)
 {
 
 void MediaStreamPrivate::addObserver(MediaStreamPrivate::Observer& observer)
 {

-    m_observers.append(&observer);
+    m_observers.add(&observer);

 }
 
 void MediaStreamPrivate::removeObserver(MediaStreamPrivate::Observer& observer)
 {
 }
 
 void MediaStreamPrivate::removeObserver(MediaStreamPrivate::Observer& observer)
 {

-    size_t pos = m_observers.find(&observer);
-    if (pos != notFound)
-        m_observers.remove(pos);
+    m_observers.remove(&observer);
+}
+
+void MediaStreamPrivate::forEachObserver(const WTF::Function<void(Observer&)>& apply) const
+{
+    for (auto* observer : copyToVector(m_observers)) {
+        if (!m_observers.contains(observer))
+            continue;
+        apply(*observer);
+    }

 }
 
 MediaStreamTrackPrivateVector MediaStreamPrivate::tracks() const
 }
 
 MediaStreamTrackPrivateVector MediaStreamPrivate::tracks() const


@@ -118,8 +125,9 @@ void MediaStreamPrivate::updateActiveState(NotifyClientOption notifyClientOption
     m_isActive = newActiveState;
 
     if (notifyClientOption == NotifyClientOption::Notify) {
     m_isActive = newActiveState;
 
     if (notifyClientOption == NotifyClientOption::Notify) {

-        for (auto& observer : m_observers)
-            observer->activeStatusChanged();
+        forEachObserver([](auto& observer) {
+            observer.activeStatusChanged();
+        });

     }
 }
 
     }
 }
 


@@ -132,8 +140,9 @@ void MediaStreamPrivate::addTrack(RefPtr<MediaStreamTrackPrivate>&& track, Notif
     m_trackSet.add(track->id(), track);
 
     if (notifyClientOption == NotifyClientOption::Notify) {
     m_trackSet.add(track->id(), track);
 
     if (notifyClientOption == NotifyClientOption::Notify) {

-        for (auto& observer : m_observers)
-            observer->didAddTrack(*track.get());
+        forEachObserver([&track](auto& observer) {
+            observer.didAddTrack(*track.get());
+        });

     }
 
     updateActiveState(notifyClientOption);
     }
 
     updateActiveState(notifyClientOption);


@@ -148,8 +157,9 @@ void MediaStreamPrivate::removeTrack(MediaStreamTrackPrivate& track, NotifyClien
     track.removeObserver(*this);
 
     if (notifyClientOption == NotifyClientOption::Notify) {
     track.removeObserver(*this);
 
     if (notifyClientOption == NotifyClientOption::Notify) {

-        for (auto& observer : m_observers)
-            observer->didRemoveTrack(track);
+        forEachObserver([&track](auto& observer) {
+            observer.didRemoveTrack(track);
+        });

     }
 
     updateActiveState(NotifyClientOption::Notify);
     }
 
     updateActiveState(NotifyClientOption::Notify);


@@ -256,8 +266,9 @@ void MediaStreamPrivate::updateActiveVideoTrack()
 
 void MediaStreamPrivate::characteristicsChanged()
 {
 
 void MediaStreamPrivate::characteristicsChanged()
 {

-    for (auto& observer : m_observers)
-        observer->characteristicsChanged();
+    forEachObserver([](auto& observer) {
+        observer.characteristicsChanged();
+    });

 }
 
 void MediaStreamPrivate::trackMutedChanged(MediaStreamTrackPrivate&)
 }
 
 void MediaStreamPrivate::trackMutedChanged(MediaStreamTrackPrivate&)

diff --git a/Source/WebCore/platform/mediastream/MediaStreamPrivate.h b/Source/WebCore/platform/mediastream/MediaStreamPrivate.h
index dc65ad8..e146c68 100644 (file)
--- a/Source/WebCore/platform/mediastream/MediaStreamPrivate.h
+++ b/Source/WebCore/platform/mediastream/MediaStreamPrivate.h
@@ -117,8 +117,9 @@ private:
     void updateActiveVideoTrack();
 
     void scheduleDeferredTask(Function<void ()>&&);
     void updateActiveVideoTrack();
 
     void scheduleDeferredTask(Function<void ()>&&);

+    void forEachObserver(const WTF::Function<void(Observer&)>&) const;

 
 

-    Vector<Observer*> m_observers;
+    HashSet<Observer*> m_observers;

     String m_id;
     MediaStreamTrackPrivate* m_activeVideoTrack { nullptr };
     HashMap<String, RefPtr<MediaStreamTrackPrivate>> m_trackSet;
     String m_id;
     MediaStreamTrackPrivate* m_activeVideoTrack { nullptr };
     HashMap<String, RefPtr<MediaStreamTrackPrivate>> m_trackSet;