Changeset 247380 in webkit

Timestamp:

Jul 11, 2019 7:35:07 PM (5 years ago)

Author:

Chris Dumez

Message:

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):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/cocoa/PlaybackSessionModel.h (modified) (1 diff)
• platform/ios/PlaybackSessionInterfaceAVKit.h (modified) (2 diffs)
• platform/ios/PlaybackSessionInterfaceAVKit.mm (modified) (4 diffs)
• platform/ios/WebVideoFullscreenControllerAVKit.mm (modified) (4 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/platform/cocoa/PlaybackSessionModel.h

@@ -111 +111 @@
     virtual void pictureInPictureActiveChanged(bool) { }
     virtual void ensureControlsManager() { }
+    virtual void modelDestroyed() { }
 };
 

trunk/Source/WebCore/platform/ios/PlaybackSessionInterfaceAVKit.h

@@ -78 +78 @@
     WEBCORE_EXPORT void mutedChanged(bool) override;
     WEBCORE_EXPORT void volumeChanged(double) override;
+    WEBCORE_EXPORT void modelDestroyed() override;
 
     WEBCORE_EXPORT virtual void invalidate();
@@ -87 +88 @@
 
     RetainPtr<WebAVPlayerController> m_playerController;
-    WeakPtr<PlaybackSessionModel> m_playbackSessionModel;
+    PlaybackSessionModel* m_playbackSessionModel { nullptr };
 };
 

trunk/Source/WebCore/platform/ios/PlaybackSessionInterfaceAVKit.mm

@@ -52 +52 @@
 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];
@@ -72 +73 @@
 PlaybackSessionInterfaceAVKit::~PlaybackSessionInterfaceAVKit()
 {
+    ASSERT(isUIThread());
     [m_playerController setPlaybackSessionInterface:nullptr];
     [m_playerController setExternalPlaybackActive:false];
@@ -80 +82 @@
 PlaybackSessionModel* PlaybackSessionInterfaceAVKit::playbackSessionModel() const
 {
-    return m_playbackSessionModel.get();
+    return m_playbackSessionModel;
 }
 
@@ -220 +222 @@
 }
 
+void PlaybackSessionInterfaceAVKit::modelDestroyed()
+{
+    ASSERT(isUIThread());
+    invalidate();
+    ASSERT(!m_playbackSessionModel);
+}
+
 }
 

trunk/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm

@@ -109 +109 @@
         return adoptRef(*new VideoFullscreenControllerContext);
     }
+
+    ~VideoFullscreenControllerContext();
 
     void setController(WebVideoFullscreenController* controller) { m_controller = controller; }
@@ -217 +219 @@
 };
 
+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
 
@@ -544 +558 @@
 void VideoFullscreenControllerContext::addClient(VideoFullscreenModelClient& client)
 {
+    ASSERT(isUIThread());
     ASSERT(!m_fullscreenClients.contains(&client));
     m_fullscreenClients.add(&client);
@@ -550 +565 @@
 void VideoFullscreenControllerContext::removeClient(VideoFullscreenModelClient& client)
 {
+    ASSERT(isUIThread());
     ASSERT(m_fullscreenClients.contains(&client));
     m_fullscreenClients.remove(&client);