Fix unsafe usage of makeWeakPtr() in CMTimebaseEffectiveRateChangedCallback()
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Jul 2019 17:34:19 +0000 (17:34 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Jul 2019 17:34:19 +0000 (17:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199656

Reviewed by Eric Carlson.

CMTimebaseEffectiveRateChangedCallback() is getting called on a background thread and would call
makeWeakPtr() on the MediaPlayerPrivateMediaSourceAVFObjC object, which is not safe because
MediaPlayerPrivateMediaSourceAVFObjC is a main thread object.

To address the issue, move the logic for listening to effective rate changes to its own
Listener class which is ThreadSafeRefCounted. Instead of using makeWeakPtr() on the background
thread, we now merely ref the thread-safe listener.

* platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.h:
* platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:
(WebCore::EffectiveRateChangedListener::create):
(WebCore::EffectiveRateChangedListener::effectiveRateChanged):
(WebCore::CMTimebaseEffectiveRateChangedCallback):
(WebCore::EffectiveRateChangedListener::stop):
(WebCore::EffectiveRateChangedListener::EffectiveRateChangedListener):
(WebCore::MediaPlayerPrivateMediaSourceAVFObjC::MediaPlayerPrivateMediaSourceAVFObjC):
(WebCore::MediaPlayerPrivateMediaSourceAVFObjC::~MediaPlayerPrivateMediaSourceAVFObjC):

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.h
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm

index e3ca994..714d4a0 100644 (file)
@@ -1,3 +1,28 @@
+2019-07-10  Chris Dumez  <cdumez@apple.com>
+
+        Fix unsafe usage of makeWeakPtr() in CMTimebaseEffectiveRateChangedCallback()
+        https://bugs.webkit.org/show_bug.cgi?id=199656
+
+        Reviewed by Eric Carlson.
+
+        CMTimebaseEffectiveRateChangedCallback() is getting called on a background thread and would call
+        makeWeakPtr() on the MediaPlayerPrivateMediaSourceAVFObjC object, which is not safe because
+        MediaPlayerPrivateMediaSourceAVFObjC is a main thread object.
+
+        To address the issue, move the logic for listening to effective rate changes to its own
+        Listener class which is ThreadSafeRefCounted. Instead of using makeWeakPtr() on the background
+        thread, we now merely ref the thread-safe listener.
+
+        * platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.h:
+        * platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:
+        (WebCore::EffectiveRateChangedListener::create):
+        (WebCore::EffectiveRateChangedListener::effectiveRateChanged):
+        (WebCore::CMTimebaseEffectiveRateChangedCallback):
+        (WebCore::EffectiveRateChangedListener::stop):
+        (WebCore::EffectiveRateChangedListener::EffectiveRateChangedListener):
+        (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::MediaPlayerPrivateMediaSourceAVFObjC):
+        (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::~MediaPlayerPrivateMediaSourceAVFObjC):
+
 2019-07-10  Antti Koivisto  <antti@apple.com>
 
         Remove TouchActionData
index bf38721..d0210a4 100644 (file)
@@ -49,6 +49,7 @@ typedef struct __CVBuffer *CVOpenGLTextureRef;
 namespace WebCore {
 
 class CDMSessionMediaSourceAVFObjC;
+class EffectiveRateChangedListener;
 class MediaSourcePrivateAVFObjC;
 class PixelBufferConformerCV;
 class PlatformClockCM;
@@ -329,6 +330,8 @@ private:
 #endif
     std::unique_ptr<VideoFullscreenLayerManagerObjC> m_videoFullscreenLayerManager;
 
+    Ref<EffectiveRateChangedListener> m_effectiveRateChangedListener;
+
 #if !RELEASE_LOG_DISABLED
     Ref<const Logger> m_logger;
     const void* m_logIdentifier;
index 0fd6700..f195156 100644 (file)
@@ -85,14 +85,46 @@ String convertEnumerationToString(MediaPlayerPrivateMediaSourceAVFObjC::SeekStat
 #pragma mark -
 #pragma mark MediaPlayerPrivateMediaSourceAVFObjC
 
+class EffectiveRateChangedListener : public ThreadSafeRefCounted<EffectiveRateChangedListener> {
+public:
+    static Ref<EffectiveRateChangedListener> create(MediaPlayerPrivateMediaSourceAVFObjC& client, CMTimebaseRef timebase)
+    {
+        return adoptRef(*new EffectiveRateChangedListener(client, timebase));
+    }
+
+    void effectiveRateChanged()
+    {
+        callOnMainThread([this, protectedThis = makeRef(*this)] {
+            if (m_client)
+                m_client->effectiveRateChanged();
+        });
+    }
+
+    void stop(CMTimebaseRef);
+
+private:
+    EffectiveRateChangedListener(MediaPlayerPrivateMediaSourceAVFObjC&, CMTimebaseRef);
+
+    WeakPtr<MediaPlayerPrivateMediaSourceAVFObjC> m_client;
+};
+
 static void CMTimebaseEffectiveRateChangedCallback(CMNotificationCenterRef, const void *listener, CFStringRef, const void *, CFTypeRef)
 {
-    MediaPlayerPrivateMediaSourceAVFObjC* player = (MediaPlayerPrivateMediaSourceAVFObjC*)const_cast<void*>(listener);
-    callOnMainThread([weakThis = makeWeakPtr(player)] {
-        if (!weakThis)
-            return;
-        weakThis->effectiveRateChanged();
-    });
+    auto* effectiveRateChangedListener = (EffectiveRateChangedListener*)const_cast<void*>(listener);
+    effectiveRateChangedListener->effectiveRateChanged();
+}
+
+void EffectiveRateChangedListener::stop(CMTimebaseRef timebase)
+{
+    CMNotificationCenterRef nc = CMNotificationCenterGetDefaultLocalCenter();
+    CMNotificationCenterRemoveListener(nc, this, CMTimebaseEffectiveRateChangedCallback, kCMTimebaseNotification_EffectiveRateChanged, timebase);
+}
+
+EffectiveRateChangedListener::EffectiveRateChangedListener(MediaPlayerPrivateMediaSourceAVFObjC& client, CMTimebaseRef timebase)
+    : m_client(makeWeakPtr(client))
+{
+    CMNotificationCenterRef nc = CMNotificationCenterGetDefaultLocalCenter();
+    CMNotificationCenterAddListener(nc, this, CMTimebaseEffectiveRateChangedCallback, kCMTimebaseNotification_EffectiveRateChanged, timebase, 0);
 }
 
 MediaPlayerPrivateMediaSourceAVFObjC::MediaPlayerPrivateMediaSourceAVFObjC(MediaPlayer* player)
@@ -106,15 +138,12 @@ MediaPlayerPrivateMediaSourceAVFObjC::MediaPlayerPrivateMediaSourceAVFObjC(Media
     , m_seeking(false)
     , m_loadingProgressed(false)
     , m_videoFullscreenLayerManager(std::make_unique<VideoFullscreenLayerManagerObjC>())
+    , m_effectiveRateChangedListener(EffectiveRateChangedListener::create(*this, [m_synchronizer timebase]))
 #if !RELEASE_LOG_DISABLED
     , m_logger(player->mediaPlayerLogger())
     , m_logIdentifier(player->mediaPlayerLogIdentifier())
 #endif
 {
-    CMTimebaseRef timebase = [m_synchronizer timebase];
-    CMNotificationCenterRef nc = CMNotificationCenterGetDefaultLocalCenter();
-    CMNotificationCenterAddListener(nc, this, CMTimebaseEffectiveRateChangedCallback, kCMTimebaseNotification_EffectiveRateChanged, timebase, 0);
-
     auto logSiteIdentifier = LOGIDENTIFIER;
     ALWAYS_LOG(logSiteIdentifier);
     UNUSED_PARAM(logSiteIdentifier);
@@ -150,9 +179,7 @@ MediaPlayerPrivateMediaSourceAVFObjC::~MediaPlayerPrivateMediaSourceAVFObjC()
 {
     ALWAYS_LOG(LOGIDENTIFIER);
 
-    CMTimebaseRef timebase = [m_synchronizer timebase];
-    CMNotificationCenterRef nc = CMNotificationCenterGetDefaultLocalCenter();
-    CMNotificationCenterRemoveListener(nc, this, CMTimebaseEffectiveRateChangedCallback, kCMTimebaseNotification_EffectiveRateChanged, timebase);
+    m_effectiveRateChangedListener->stop([m_synchronizer timebase]);
 
     if (m_timeJumpedObserver)
         [m_synchronizer removeTimeObserver:m_timeJumpedObserver.get()];