Changeset 247292 in webkit

Timestamp:

Jul 9, 2019 9:19:48 PM (5 years ago)

Author:

Chris Dumez

Message:

Stop using GenericTaskQueue from multiple threads
​https://bugs.webkit.org/show_bug.cgi?id=199652

Reviewed by Geoffrey Garen.

WebCoreAVFLoaderDelegate was calling GenericTaskQueue::enqueueTask() from a background thread,
which is not safe because the implementation of enqueueTask() calls makeWeakPtr() on the
GenericTaskQueue (a main thread object).

Update WebCoreAVFLoaderDelegate to use callOnMainThread() instead.

• platform/GenericTaskQueue.h:

Stop last template parameter which was used exclusively by WebCoreAVFLoaderDelegate to try and
make GenericTaskQueue thread-safe.

• platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:

(-[WebCoreAVFMovieObserver metadataLoaded]):
(-[WebCoreAVFMovieObserver didEnd:]):
(-[WebCoreAVFMovieObserver observeValueForKeyPath:ofObject:change:context:]):
(-[WebCoreAVFMovieObserver legibleOutput:didOutputAttributedStrings:nativeSampleBuffers:forItemTime:]):
(-[WebCoreAVFMovieObserver outputSequenceWasFlushed:]):
(-[WebCoreAVFLoaderDelegate resourceLoader:shouldWaitForLoadingOfRequestedResource:]):
(-[WebCoreAVFLoaderDelegate resourceLoader:didCancelLoadingRequest:]):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/GenericTaskQueue.h (modified) (2 diffs)
• platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm (modified) (9 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-07-09  Chris Dumez  <cdumez@apple.com>
+
+        Stop using GenericTaskQueue from multiple threads
+        https://bugs.webkit.org/show_bug.cgi?id=199652
+
+        Reviewed by Geoffrey Garen.
+
+        WebCoreAVFLoaderDelegate was calling GenericTaskQueue::enqueueTask() from a background thread,
+        which is not safe because the implementation of enqueueTask() calls makeWeakPtr() on the
+        GenericTaskQueue (a main thread object).
+
+        Update WebCoreAVFLoaderDelegate to use callOnMainThread() instead.
+
+        * platform/GenericTaskQueue.h:
+        Stop last template parameter which was used exclusively by WebCoreAVFLoaderDelegate to try and
+        make GenericTaskQueue thread-safe.
+
+        * platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:
+        (-[WebCoreAVFMovieObserver metadataLoaded]):
+        (-[WebCoreAVFMovieObserver didEnd:]):
+        (-[WebCoreAVFMovieObserver observeValueForKeyPath:ofObject:change:context:]):
+        (-[WebCoreAVFMovieObserver legibleOutput:didOutputAttributedStrings:nativeSampleBuffers:forItemTime:]):
+        (-[WebCoreAVFMovieObserver outputSequenceWasFlushed:]):
+        (-[WebCoreAVFLoaderDelegate resourceLoader:shouldWaitForLoadingOfRequestedResource:]):
+        (-[WebCoreAVFLoaderDelegate resourceLoader:didCancelLoadingRequest:]):
+
 2019-07-09  Tim Horton  <timothy_horton@apple.com>
 

trunk/Source/WebCore/platform/GenericTaskQueue.h

@@ -72 +72 @@
 };
 
-template <typename T, typename C = unsigned>
-class GenericTaskQueue : public CanMakeWeakPtr<GenericTaskQueue<T, C>> {
+template <typename T>
+class GenericTaskQueue : public CanMakeWeakPtr<GenericTaskQueue<T>> {
 public:
     GenericTaskQueue()
@@ -125 +125 @@
 private:
     TaskDispatcher<T> m_dispatcher;
-    C m_pendingTasks { 0 };
+    unsigned m_pendingTasks { 0 };
     bool m_isClosed { false };
 };

trunk/Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm

@@ -170 +170 @@
 {
     WeakPtr<MediaPlayerPrivateAVFoundationObjC> m_player;
-    GenericTaskQueue<Timer, std::atomic<unsigned>> m_taskQueue;
     int m_delayCallbacks;
 }
@@ -185 +184 @@
 @interface WebCoreAVFLoaderDelegate : NSObject<AVAssetResourceLoaderDelegate> {
     WeakPtr<MediaPlayerPrivateAVFoundationObjC> m_player;
-    GenericTaskQueue<Timer, std::atomic<unsigned>> m_taskQueue;
 }
 - (id)initWithPlayer:(WeakPtr<MediaPlayerPrivateAVFoundationObjC>&&)player;
@@ -3288 +3286 @@
 - (void)metadataLoaded
 {
-    m_taskQueue.enqueueTask([player = m_player] {
+    callOnMainThread([player = m_player] {
         if (player)
             player->metadataLoaded();
@@ -3297 +3295 @@
 {
     UNUSED_PARAM(unusedNotification);
-    m_taskQueue.enqueueTask([player = m_player] {
+    callOnMainThread([player = m_player] {
         if (player)
             player->didEnd();
@@ -3305 +3303 @@
 - (void)observeValueForKeyPath:keyPath ofObject:(id)object change:(NSDictionary *)change context:(MediaPlayerAVFoundationObservationContext)context
 {
-    m_taskQueue.enqueueTask([player = m_player, keyPath = retainPtr(keyPath), change = retainPtr(change), object = retainPtr(object), context] {
+    callOnMainThread([player = m_player, keyPath = retainPtr(keyPath), change = retainPtr(change), object = retainPtr(object), context] {
         if (!player)
             return;
@@ -3408 +3406 @@
     UNUSED_PARAM(output);
 
-    m_taskQueue.enqueueTask([player = m_player, strings = retainPtr(strings), nativeSamples = retainPtr(nativeSamples), itemTime] {
+    callOnMainThread([player = m_player, strings = retainPtr(strings), nativeSamples = retainPtr(nativeSamples), itemTime] {
         if (!player)
             return;
@@ -3420 +3418 @@
     UNUSED_PARAM(output);
 
-    m_taskQueue.enqueueTask([player = m_player] {
+    callOnMainThread([player = m_player] {
         if (player)
             player->flushCues();
@@ -3447 +3445 @@
         return NO;
 
-    m_taskQueue.enqueueTask([player = m_player, loadingRequest = retainPtr(loadingRequest)] {
+    callOnMainThread([player = m_player, loadingRequest = retainPtr(loadingRequest)] {
         if (!player) {
             [loadingRequest finishLoadingWithError:nil];
@@ -3471 +3469 @@
 {
     UNUSED_PARAM(resourceLoader);
-    m_taskQueue.enqueueTask([player = m_player, loadingRequest = retainPtr(loadingRequest)] {
+    callOnMainThread([player = m_player, loadingRequest = retainPtr(loadingRequest)] {
         if (player)
             player->didCancelLoadingRequest(loadingRequest.get());