Drop non thread-safe usage of WeakPtr in PlaybackSessionInterfaceAVKit
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 12 Jul 2019 02:35:07 +0000 (02:35 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 12 Jul 2019 02:35:07 +0000 (02:35 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199698

Reviewed by Eric Carlson.

The PlaybackSessionInterfaceAVKit 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 r243337, 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_playbackSessionModel data
member. This gives the sames guarantees than WeakPtr but in a thread-safe way.

* platform/cocoa/PlaybackSessionModel.h:
(WebCore::PlaybackSessionModelClient::modelDestroyed):
* platform/ios/PlaybackSessionInterfaceAVKit.h:
* platform/ios/PlaybackSessionInterfaceAVKit.mm:
(WebCore::PlaybackSessionInterfaceAVKit::PlaybackSessionInterfaceAVKit):
(WebCore::PlaybackSessionInterfaceAVKit::~PlaybackSessionInterfaceAVKit):
(WebCore::PlaybackSessionInterfaceAVKit::playbackSessionModel const):
(WebCore::PlaybackSessionInterfaceAVKit::modelDestroyed):
* platform/ios/WebVideoFullscreenControllerAVKit.mm:
(VideoFullscreenControllerContext::~VideoFullscreenControllerContext):
(VideoFullscreenControllerContext::addClient):
(VideoFullscreenControllerContext::removeClient):

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

Source/WebCore/ChangeLog
Source/WebCore/platform/cocoa/PlaybackSessionModel.h
Source/WebCore/platform/ios/PlaybackSessionInterfaceAVKit.h
Source/WebCore/platform/ios/PlaybackSessionInterfaceAVKit.mm
Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm

index ba3fa89..76bb0fb 100644 (file)
@@ -1,3 +1,34 @@
+2019-07-11  Chris Dumez  <cdumez@apple.com>
+
+        Drop non thread-safe usage of WeakPtr in PlaybackSessionInterfaceAVKit
+        https://bugs.webkit.org/show_bug.cgi?id=199698
+
+        Reviewed by Eric Carlson.
+
+        The PlaybackSessionInterfaceAVKit 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 r243337, 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_playbackSessionModel data
+        member. This gives the sames guarantees than WeakPtr but in a thread-safe way.
+
+        * platform/cocoa/PlaybackSessionModel.h:
+        (WebCore::PlaybackSessionModelClient::modelDestroyed):
+        * platform/ios/PlaybackSessionInterfaceAVKit.h:
+        * platform/ios/PlaybackSessionInterfaceAVKit.mm:
+        (WebCore::PlaybackSessionInterfaceAVKit::PlaybackSessionInterfaceAVKit):
+        (WebCore::PlaybackSessionInterfaceAVKit::~PlaybackSessionInterfaceAVKit):
+        (WebCore::PlaybackSessionInterfaceAVKit::playbackSessionModel const):
+        (WebCore::PlaybackSessionInterfaceAVKit::modelDestroyed):
+        * platform/ios/WebVideoFullscreenControllerAVKit.mm:
+        (VideoFullscreenControllerContext::~VideoFullscreenControllerContext):
+        (VideoFullscreenControllerContext::addClient):
+        (VideoFullscreenControllerContext::removeClient):
+
 2019-07-11  Simon Fraser  <simon.fraser@apple.com>
 
         Fix builds where HAVE_DESIGN_SYSTEM_UI_FONTS is not defined.
index 2e97710..021dd0f 100644 (file)
@@ -110,6 +110,7 @@ public:
     virtual void isPictureInPictureSupportedChanged(bool) { }
     virtual void pictureInPictureActiveChanged(bool) { }
     virtual void ensureControlsManager() { }
+    virtual void modelDestroyed() { }
 };
 
 }
index d7fdb7b..7dee152 100644 (file)
@@ -77,6 +77,7 @@ public:
     WEBCORE_EXPORT void wirelessVideoPlaybackDisabledChanged(bool) override;
     WEBCORE_EXPORT void mutedChanged(bool) override;
     WEBCORE_EXPORT void volumeChanged(double) override;
+    WEBCORE_EXPORT void modelDestroyed() override;
 
     WEBCORE_EXPORT virtual void invalidate();
 
@@ -86,7 +87,7 @@ protected:
     WEBCORE_EXPORT PlaybackSessionInterfaceAVKit(PlaybackSessionModel&);
 
     RetainPtr<WebAVPlayerController> m_playerController;
-    WeakPtr<PlaybackSessionModel> m_playbackSessionModel;
+    PlaybackSessionModel* m_playbackSessionModel { nullptr };
 };
 
 }
index c064d60..fcab26f 100644 (file)
@@ -51,8 +51,9 @@ using namespace PAL;
 
 PlaybackSessionInterfaceAVKit::PlaybackSessionInterfaceAVKit(PlaybackSessionModel& model)
     : m_playerController(adoptNS([[WebAVPlayerController alloc] init]))
-    , m_playbackSessionModel(makeWeakPtr(model))
+    , m_playbackSessionModel(&model)
 {
+    ASSERT(isUIThread());
     model.addClient(*this);
     [m_playerController setPlaybackSessionInterface:this];
     [m_playerController setDelegate:&model];
@@ -71,6 +72,7 @@ PlaybackSessionInterfaceAVKit::PlaybackSessionInterfaceAVKit(PlaybackSessionMode
 
 PlaybackSessionInterfaceAVKit::~PlaybackSessionInterfaceAVKit()
 {
+    ASSERT(isUIThread());
     [m_playerController setPlaybackSessionInterface:nullptr];
     [m_playerController setExternalPlaybackActive:false];
 
@@ -79,7 +81,7 @@ PlaybackSessionInterfaceAVKit::~PlaybackSessionInterfaceAVKit()
 
 PlaybackSessionModel* PlaybackSessionInterfaceAVKit::playbackSessionModel() const
 {
-    return m_playbackSessionModel.get();
+    return m_playbackSessionModel;
 }
 
 void PlaybackSessionInterfaceAVKit::durationChanged(double duration)
@@ -219,6 +221,13 @@ void PlaybackSessionInterfaceAVKit::invalidate()
     m_playbackSessionModel = nullptr;
 }
 
+void PlaybackSessionInterfaceAVKit::modelDestroyed()
+{
+    ASSERT(isUIThread());
+    invalidate();
+    ASSERT(!m_playbackSessionModel);
+}
+
 }
 
 #endif // HAVE(AVKIT)
index 691cbf8..d894c94 100644 (file)
@@ -109,6 +109,8 @@ public:
         return adoptRef(*new VideoFullscreenControllerContext);
     }
 
+    ~VideoFullscreenControllerContext();
+
     void setController(WebVideoFullscreenController* controller) { m_controller = controller; }
     void setUpFullscreen(HTMLVideoElement&, UIView *, HTMLMediaElementEnums::VideoFullscreenMode);
     void exitFullscreen();
@@ -216,6 +218,18 @@ private:
     RetainPtr<WebVideoFullscreenController> m_controller;
 };
 
+VideoFullscreenControllerContext::~VideoFullscreenControllerContext()
+{
+    auto notifyClientsModelWasDestroyed = [this] {
+        while (!m_playbackClients.isEmpty())
+            (*m_playbackClients.begin())->modelDestroyed();
+    };
+    if (isUIThread())
+        notifyClientsModelWasDestroyed();
+    else
+        dispatch_sync(dispatch_get_main_queue(), WTFMove(notifyClientsModelWasDestroyed));
+}
+
 #pragma mark VideoFullscreenChangeObserver
 
 void VideoFullscreenControllerContext::requestUpdateInlineRect()
@@ -543,12 +557,14 @@ void VideoFullscreenControllerContext::volumeChanged(double volume)
 
 void VideoFullscreenControllerContext::addClient(VideoFullscreenModelClient& client)
 {
+    ASSERT(isUIThread());
     ASSERT(!m_fullscreenClients.contains(&client));
     m_fullscreenClients.add(&client);
 }
 
 void VideoFullscreenControllerContext::removeClient(VideoFullscreenModelClient& client)
 {
+    ASSERT(isUIThread());
     ASSERT(m_fullscreenClients.contains(&client));
     m_fullscreenClients.remove(&client);
 }