Drop non thread-safe usage of WeakPtr in VideoFullscreenInterfaceAVKit
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 13 Jul 2019 15:28:51 +0000 (15:28 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 13 Jul 2019 15:28:51 +0000 (15:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199775

Reviewed by Eric Carlson.

The VideoFullscreenInterfaceAVKit constructor was making a weakPtr on the UI Thread
of an WebThread object. The WeakPtr would then be used as a data member throughout
the class on the UIThread. This is not thread-safe.

This patch switches to using a raw pointer instead of a WeakPtr. This is a partial
rollout of r243298, which turned the raw pointer into a WeakPtr for hardening
purposes. For extra safety, this patch updates the VideoFullscreenControllerContext
so that it notifies its clients (i.e. PlaybackSessionInterfaceAVKit) that it is
getting destroyed, so that they can null-out their m_videoFullscreenModel &
m_fullscreenChangeObserver data members. This gives the sames guarantees as WeakPtr
but in a thread-safe way.

This is very similar to the fix that was done for PlaybackSessionInterfaceAVKit in
r247380.

* platform/cocoa/VideoFullscreenModel.h:
(WebCore::VideoFullscreenModelClient::modelDestroyed):
* platform/ios/VideoFullscreenInterfaceAVKit.h:
* platform/ios/VideoFullscreenInterfaceAVKit.mm:
(VideoFullscreenInterfaceAVKit::setVideoFullscreenModel):
(VideoFullscreenInterfaceAVKit::setVideoFullscreenChangeObserver):
(VideoFullscreenInterfaceAVKit::modelDestroyed):
* platform/ios/WebVideoFullscreenControllerAVKit.mm:
(VideoFullscreenControllerContext::~VideoFullscreenControllerContext):

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

Source/WebCore/ChangeLog
Source/WebCore/platform/cocoa/VideoFullscreenModel.h
Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h
Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm
Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm

index 52ca927..9e5d9f3 100644 (file)
@@ -1,3 +1,35 @@
+2019-07-13  Chris Dumez  <cdumez@apple.com>
+
+        Drop non thread-safe usage of WeakPtr in VideoFullscreenInterfaceAVKit
+        https://bugs.webkit.org/show_bug.cgi?id=199775
+
+        Reviewed by Eric Carlson.
+
+        The VideoFullscreenInterfaceAVKit constructor was making a weakPtr on the UI Thread
+        of an WebThread object. The WeakPtr would then be used as a data member throughout
+        the class on the UIThread. This is not thread-safe.
+
+        This patch switches to using a raw pointer instead of a WeakPtr. This is a partial
+        rollout of r243298, which turned the raw pointer into a WeakPtr for hardening
+        purposes. For extra safety, this patch updates the VideoFullscreenControllerContext
+        so that it notifies its clients (i.e. PlaybackSessionInterfaceAVKit) that it is
+        getting destroyed, so that they can null-out their m_videoFullscreenModel &
+        m_fullscreenChangeObserver data members. This gives the sames guarantees as WeakPtr
+        but in a thread-safe way.
+
+        This is very similar to the fix that was done for PlaybackSessionInterfaceAVKit in
+        r247380.
+
+        * platform/cocoa/VideoFullscreenModel.h:
+        (WebCore::VideoFullscreenModelClient::modelDestroyed):
+        * platform/ios/VideoFullscreenInterfaceAVKit.h:
+        * platform/ios/VideoFullscreenInterfaceAVKit.mm:
+        (VideoFullscreenInterfaceAVKit::setVideoFullscreenModel):
+        (VideoFullscreenInterfaceAVKit::setVideoFullscreenChangeObserver):
+        (VideoFullscreenInterfaceAVKit::modelDestroyed):
+        * platform/ios/WebVideoFullscreenControllerAVKit.mm:
+        (VideoFullscreenControllerContext::~VideoFullscreenControllerContext):
+
 2019-07-13  Zalan Bujtas  <zalan@apple.com>
 
         Cannot bring up custom media controls at all on v.youku.com
index 7b2bfa6..3f457d3 100644 (file)
@@ -83,6 +83,7 @@ public:
     virtual void failedToEnterPictureInPicture() { }
     virtual void willExitPictureInPicture() { }
     virtual void didExitPictureInPicture() { }
+    virtual void modelDestroyed() { }
 };
 
 WEBCORE_EXPORT bool supportsPictureInPicture();
index 5d494f8..e7485cb 100644 (file)
@@ -74,6 +74,7 @@ public:
     // VideoFullscreenModelClient
     WEBCORE_EXPORT void hasVideoChanged(bool) final;
     WEBCORE_EXPORT void videoDimensionsChanged(const FloatSize&) final;
+    WEBCORE_EXPORT void modelDestroyed() final;
 
     // PlaybackSessionModelClient
     WEBCORE_EXPORT void externalPlaybackChanged(bool enabled, PlaybackSessionModel::ExternalPlaybackTargetType, const String& localizedDeviceName) final;
@@ -127,7 +128,7 @@ public:
     Mode m_currentMode;
     Mode m_targetMode;
 
-    VideoFullscreenModel* videoFullscreenModel() const { return m_videoFullscreenModel.get(); }
+    VideoFullscreenModel* videoFullscreenModel() const { return m_videoFullscreenModel; }
     bool shouldExitFullscreenWithReason(ExitFullScreenReason);
     HTMLMediaElementEnums::VideoFullscreenMode mode() const { return m_currentMode.mode(); }
     bool allowsPictureInPicturePlayback() const { return m_allowsPictureInPicturePlayback; }
@@ -171,8 +172,8 @@ protected:
     Ref<PlaybackSessionInterfaceAVKit> m_playbackSessionInterface;
     RetainPtr<WebAVPlayerViewControllerDelegate> m_playerViewControllerDelegate;
     RetainPtr<WebAVPlayerViewController> m_playerViewController;
-    WeakPtr<VideoFullscreenModel> m_videoFullscreenModel;
-    WeakPtr<VideoFullscreenChangeObserver> m_fullscreenChangeObserver;
+    VideoFullscreenModel* m_videoFullscreenModel { nullptr };
+    VideoFullscreenChangeObserver* m_fullscreenChangeObserver { nullptr };
 
     // These are only used when fullscreen is presented in a separate window.
     RetainPtr<UIWindow> m_window;
index 5739561..5a7465d 100644 (file)
@@ -760,7 +760,7 @@ void VideoFullscreenInterfaceAVKit::setVideoFullscreenModel(VideoFullscreenModel
     if (m_videoFullscreenModel)
         m_videoFullscreenModel->removeClient(*this);
 
-    m_videoFullscreenModel = makeWeakPtr(model);
+    m_videoFullscreenModel = model;
 
     if (m_videoFullscreenModel) {
         m_videoFullscreenModel->addClient(*this);
@@ -779,7 +779,7 @@ void VideoFullscreenInterfaceAVKit::setVideoFullscreenModel(VideoFullscreenModel
 
 void VideoFullscreenInterfaceAVKit::setVideoFullscreenChangeObserver(VideoFullscreenChangeObserver* observer)
 {
-    m_fullscreenChangeObserver = makeWeakPtr(observer);
+    m_fullscreenChangeObserver = observer;
 }
 
 void VideoFullscreenInterfaceAVKit::hasVideoChanged(bool hasVideo)
@@ -930,6 +930,12 @@ void VideoFullscreenInterfaceAVKit::invalidate()
     cleanupFullscreen();
 }
 
+void VideoFullscreenInterfaceAVKit::modelDestroyed()
+{
+    ASSERT(isUIThread());
+    invalidate();
+}
+
 void VideoFullscreenInterfaceAVKit::requestHideAndExitFullscreen()
 {
     if (m_currentMode.hasPictureInPicture())
index d894c94..436a970 100644 (file)
@@ -223,6 +223,8 @@ VideoFullscreenControllerContext::~VideoFullscreenControllerContext()
     auto notifyClientsModelWasDestroyed = [this] {
         while (!m_playbackClients.isEmpty())
             (*m_playbackClients.begin())->modelDestroyed();
+        while (!m_fullscreenClients.isEmpty())
+            (*m_fullscreenClients.begin())->modelDestroyed();
     };
     if (isUIThread())
         notifyClientsModelWasDestroyed();