From: youenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date: Wed, 14 Nov 2018 17:50:36 +0000 (+0000)
Subject: Allow to remove MediaStreamPrivate observers when iterating over observers
X-Git-Url: http://git.webkit.org/?p=WebKit-https.git;a=commitdiff_plain;h=2a69597cae3c2e908b8d465e6312c14d01bf100c

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
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,5 +1,31 @@
 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
 
diff --git a/Source/WebCore/Modules/mediastream/UserMediaRequest.cpp b/Source/WebCore/Modules/mediastream/UserMediaRequest.cpp
index f616eb7..e10b96b 100644
--- 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));
-    // 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)
diff --git a/Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp b/Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp
index 965229e..bd8eb88 100644
--- 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)
 {
-    m_observers.append(&observer);
+    m_observers.add(&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
@@ -118,8 +125,9 @@ void MediaStreamPrivate::updateActiveState(NotifyClientOption notifyClientOption
     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) {
-        for (auto& observer : m_observers)
-            observer->didAddTrack(*track.get());
+        forEachObserver([&track](auto& observer) {
+            observer.didAddTrack(*track.get());
+        });
     }
 
     updateActiveState(notifyClientOption);
@@ -148,8 +157,9 @@ void MediaStreamPrivate::removeTrack(MediaStreamTrackPrivate& track, NotifyClien
     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);
@@ -256,8 +266,9 @@ void MediaStreamPrivate::updateActiveVideoTrack()
 
 void MediaStreamPrivate::characteristicsChanged()
 {
-    for (auto& observer : m_observers)
-        observer->characteristicsChanged();
+    forEachObserver([](auto& observer) {
+        observer.characteristicsChanged();
+    });
 }
 
 void MediaStreamPrivate::trackMutedChanged(MediaStreamTrackPrivate&)
diff --git a/Source/WebCore/platform/mediastream/MediaStreamPrivate.h b/Source/WebCore/platform/mediastream/MediaStreamPrivate.h
index dc65ad8..e146c68 100644
--- 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 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;