Changeset 238181 in webkit

Timestamp:

Nov 14, 2018 9:50:36 AM (5 years ago)

Author:

youenn@apple.com

Message:

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:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• Modules/mediastream/UserMediaRequest.cpp (modified) (1 diff)
• platform/mediastream/MediaStreamPrivate.cpp (modified) (5 diffs)
• platform/mediastream/MediaStreamPrivate.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/Modules/mediastream/UserMediaRequest.cpp

@@ -366 +366 @@
     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;
 }
 

trunk/Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp

@@ -85 +85 @@
 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);
+    }
 }
 
@@ -119 +126 @@
 
     if (notifyClientOption == NotifyClientOption::Notify) {
-        for (auto& observer : m_observers)
-            observer->activeStatusChanged();
+        forEachObserver([](auto& observer) {
+            observer.activeStatusChanged();
+        });
     }
 }
@@ -133 +141 @@
 
     if (notifyClientOption == NotifyClientOption::Notify) {
-        for (auto& observer : m_observers)
-            observer->didAddTrack(*track.get());
+        forEachObserver([&track](auto& observer) {
+            observer.didAddTrack(*track.get());
+        });
     }
 
@@ -149 +158 @@
 
     if (notifyClientOption == NotifyClientOption::Notify) {
-        for (auto& observer : m_observers)
-            observer->didRemoveTrack(track);
+        forEachObserver([&track](auto& observer) {
+            observer.didRemoveTrack(track);
+        });
     }
 
@@ -257 +267 @@
 void MediaStreamPrivate::characteristicsChanged()
 {
-    for (auto& observer : m_observers)
-        observer->characteristicsChanged();
+    forEachObserver([](auto& observer) {
+        observer.characteristicsChanged();
+    });
 }
 

trunk/Source/WebCore/platform/mediastream/MediaStreamPrivate.h

@@ -118 +118 @@
 
     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 };