Changeset 247417 in webkit

Timestamp:

Jul 13, 2019 8:28:51 AM (5 years ago)

Author:

Chris Dumez

Message:

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

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/cocoa/VideoFullscreenModel.h (modified) (1 diff)
• platform/ios/VideoFullscreenInterfaceAVKit.h (modified) (3 diffs)
• platform/ios/VideoFullscreenInterfaceAVKit.mm (modified) (3 diffs)
• platform/ios/WebVideoFullscreenControllerAVKit.mm (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

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

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

@@ -84 +84 @@
     virtual void willExitPictureInPicture() { }
     virtual void didExitPictureInPicture() { }
+    virtual void modelDestroyed() { }
 };
 

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

@@ -75 +75 @@
     WEBCORE_EXPORT void hasVideoChanged(bool) final;
     WEBCORE_EXPORT void videoDimensionsChanged(const FloatSize&) final;
+    WEBCORE_EXPORT void modelDestroyed() final;
 
     // PlaybackSessionModelClient
@@ -128 +129 @@
     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(); }
@@ -172 +173 @@
     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.

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

@@ -761 +761 @@
         m_videoFullscreenModel->removeClient(*this);
 
-    m_videoFullscreenModel = makeWeakPtr(model);
+    m_videoFullscreenModel = model;
 
     if (m_videoFullscreenModel) {
@@ -780 +780 @@
 void VideoFullscreenInterfaceAVKit::setVideoFullscreenChangeObserver(VideoFullscreenChangeObserver* observer)
 {
-    m_fullscreenChangeObserver = makeWeakPtr(observer);
+    m_fullscreenChangeObserver = observer;
 }
 
@@ -929 +929 @@
     
     cleanupFullscreen();
+}
+
+void VideoFullscreenInterfaceAVKit::modelDestroyed()
+{
+    ASSERT(isUIThread());
+    invalidate();
 }
 

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

@@ -224 +224 @@
         while (!m_playbackClients.isEmpty())
             (*m_playbackClients.begin())->modelDestroyed();
+        while (!m_fullscreenClients.isEmpty())
+            (*m_fullscreenClients.begin())->modelDestroyed();
     };
     if (isUIThread())