[EME] Possible deadlock when aborting a SourceBufferPrivateAVFObjC append operation
authorjer.noble@apple.com <jer.noble@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Dec 2017 19:25:35 +0000 (19:25 +0000)
committerjer.noble@apple.com <jer.noble@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Dec 2017 19:25:35 +0000 (19:25 +0000)
https://bugs.webkit.org/show_bug.cgi?id=180486

Reviewed by Eric Carlson.

It's possible that an abort() operation which is blocked on the ongoing appendBuffer()
operation completing will deadlock forever, with either the willProvideContentKeyRequest
or didProvideContentKeyRequest callbacks blocking waiting to be run on the main thread
(which is itself blocked waiting for the append operation to complete).

To break this deadlock, add a new abortSemaphore which is signaled in the abort() method
and have the willProvide... and didProvide... methods block both on their own semaphores
as well as the abortSemaphore, allowing them to break out even if their main thread calls
never get a chance to execute.

* platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:
(-[WebAVStreamDataParserListener streamDataParserWillProvideContentKeyRequestInitializationData:forTrackID:]):
(-[WebAVStreamDataParserListener streamDataParser:didProvideContentKeyRequestInitializationData:forTrackID:]):
(WebCore::SourceBufferPrivateAVFObjC::SourceBufferPrivateAVFObjC):
(WebCore::SourceBufferPrivateAVFObjC::abort):

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm

index 36ce043..8926ece 100644 (file)
@@ -1,3 +1,26 @@
+2017-12-07  Jer Noble  <jer.noble@apple.com>
+
+        [EME] Possible deadlock when aborting a SourceBufferPrivateAVFObjC append operation
+        https://bugs.webkit.org/show_bug.cgi?id=180486
+
+        Reviewed by Eric Carlson.
+
+        It's possible that an abort() operation which is blocked on the ongoing appendBuffer()
+        operation completing will deadlock forever, with either the willProvideContentKeyRequest
+        or didProvideContentKeyRequest callbacks blocking waiting to be run on the main thread
+        (which is itself blocked waiting for the append operation to complete).
+
+        To break this deadlock, add a new abortSemaphore which is signaled in the abort() method
+        and have the willProvide... and didProvide... methods block both on their own semaphores
+        as well as the abortSemaphore, allowing them to break out even if their main thread calls
+        never get a chance to execute.
+
+        * platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:
+        (-[WebAVStreamDataParserListener streamDataParserWillProvideContentKeyRequestInitializationData:forTrackID:]):
+        (-[WebAVStreamDataParserListener streamDataParser:didProvideContentKeyRequestInitializationData:forTrackID:]):
+        (WebCore::SourceBufferPrivateAVFObjC::SourceBufferPrivateAVFObjC):
+        (WebCore::SourceBufferPrivateAVFObjC::abort):
+
 2017-12-07  Eric Carlson  <eric.carlson@apple.com>
 
         Simplify log channel configuration UI
index 6f30a99..fc17a35 100644 (file)
@@ -108,9 +108,11 @@ SOFT_LINK_CONSTANT(AVFoundation, AVSampleBufferDisplayLayerFailedToDecodeNotific
 
 @interface WebAVStreamDataParserListener : NSObject<AVStreamDataParserOutputHandling> {
     WeakPtr<WebCore::SourceBufferPrivateAVFObjC> _parent;
+    OSObjectPtr<dispatch_semaphore_t> _abortSemaphore;
     AVStreamDataParser* _parser;
 }
 @property (assign) WeakPtr<WebCore::SourceBufferPrivateAVFObjC> parent;
+@property (assign) OSObjectPtr<dispatch_semaphore_t> abortSemaphore;
 - (id)initWithParser:(AVStreamDataParser*)parser parent:(WeakPtr<WebCore::SourceBufferPrivateAVFObjC>)parent;
 @end
 
@@ -129,6 +131,7 @@ SOFT_LINK_CONSTANT(AVFoundation, AVSampleBufferDisplayLayerFailedToDecodeNotific
 }
 
 @synthesize parent=_parent;
+@synthesize abortSemaphore=_abortSemaphore;
 
 - (void)dealloc
 {
@@ -203,10 +206,22 @@ SOFT_LINK_CONSTANT(AVFoundation, AVSampleBufferDisplayLayerFailedToDecodeNotific
 
     // We must call synchronously to the main thread, as the AVStreamSession must be associated
     // with the streamDataParser before the delegate method returns.
-    dispatch_sync(dispatch_get_main_queue(), [parent = _parent, trackID]() {
+    OSObjectPtr<dispatch_semaphore_t> respondedSemaphore = adoptOSObject(dispatch_semaphore_create(0));
+    callOnMainThread([parent = _parent, trackID, respondedSemaphore]() {
         if (parent)
             parent->willProvideContentKeyRequestInitializationDataForTrackID(trackID);
+        dispatch_semaphore_signal(respondedSemaphore.get());
     });
+
+    while (true) {
+        if (!dispatch_semaphore_wait(respondedSemaphore.get(), dispatch_time(DISPATCH_TIME_NOW, NSEC_PER_MSEC * 100)))
+            return;
+
+        if (!dispatch_semaphore_wait(_abortSemaphore.get(), dispatch_time(DISPATCH_TIME_NOW, NSEC_PER_MSEC * 100))) {
+            dispatch_semaphore_signal(_abortSemaphore.get());
+            return;
+        }
+    }
 }
 
 - (void)streamDataParser:(AVStreamDataParser *)streamDataParser didProvideContentKeyRequestInitializationData:(NSData *)initData forTrackID:(CMPersistentTrackID)trackID
@@ -218,7 +233,16 @@ SOFT_LINK_CONSTANT(AVFoundation, AVSampleBufferDisplayLayerFailedToDecodeNotific
         if (parent)
             parent->didProvideContentKeyRequestInitializationDataForTrackID(protectedInitData.get(), trackID, hasSessionSemaphore);
     });
-    dispatch_semaphore_wait(hasSessionSemaphore.get(), DISPATCH_TIME_FOREVER);
+
+    while (true) {
+        if (!dispatch_semaphore_wait(hasSessionSemaphore.get(), dispatch_time(DISPATCH_TIME_NOW, NSEC_PER_MSEC * 100)))
+            return;
+
+        if (!dispatch_semaphore_wait(_abortSemaphore.get(), dispatch_time(DISPATCH_TIME_NOW, NSEC_PER_MSEC * 100))) {
+            dispatch_semaphore_signal(_abortSemaphore.get());
+            return;
+        }
+    }
 }
 @end
 
@@ -487,6 +511,7 @@ SourceBufferPrivateAVFObjC::SourceBufferPrivateAVFObjC(MediaSourcePrivateAVFObjC
     , m_mediaSource(parent)
 {
     CMNotificationCenterAddListener(CMNotificationCenterGetDefaultLocalCenter(), this, bufferWasConsumedCallback, kCMSampleBufferConsumerNotification_BufferConsumed, nullptr, 0);
+    m_delegate.get().abortSemaphore = adoptOSObject(dispatch_semaphore_create(0));
 }
 
 SourceBufferPrivateAVFObjC::~SourceBufferPrivateAVFObjC()
@@ -713,9 +738,11 @@ void SourceBufferPrivateAVFObjC::abort()
     // semaphore, the m_isAppendingGroup wait operation will deadlock.
     if (m_hasSessionSemaphore)
         dispatch_semaphore_signal(m_hasSessionSemaphore.get());
+    dispatch_semaphore_signal(m_delegate.get().abortSemaphore.get());
     dispatch_group_wait(m_isAppendingGroup.get(), DISPATCH_TIME_FOREVER);
     m_appendWeakFactory.revokeAll();
     m_delegate.get().parent = m_appendWeakFactory.createWeakPtr(*this);
+    m_delegate.get().abortSemaphore = adoptOSObject(dispatch_semaphore_create(0));
 }
 
 void SourceBufferPrivateAVFObjC::resetParserState()