Attachment 400100 Details for Bug 212293 – Patch

[patch] Patch

bug-212293-20200522191304.patch (text/plain), 7.87 KB, created by Peng Liu on 2020-05-22 19:13:05 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Peng Liu

Created: 2020-05-22 19:13:05 PDT

Size: 7.87 KB

patch

obsolete

>Subversion Revision: 262043
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 5aed6cd38a6d0978e22adb907a0910346dcc118c..df6bfcc8599f433daa4f5f980a485c5cddf02bd2 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,30 @@
>+2020-05-22  Peng Liu  <peng.liu6@apple.com>
>+
>+        VideoFullscreenInterfaceAVKit is leaking when a video element enters and exits fullscreen/picture-in-picture
>+        https://bugs.webkit.org/show_bug.cgi?id=212293
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        WebAVPlayerViewControllerDelegate is created and retained by VideoFullscreenInterfaceAVKit,
>+        but it has a RefPtr to VideoFullscreenInterfaceAVKit. This leads to a memory leak
>+        when a video element enters and exit fullscreen or Picture-in-Picture. This patch
>+        replaces the RefPtr with a WeakPtr to fix the leak.
>+
>+        With this patch, we config playerController in VideoFullscreenInterfaceAVKit::setupFullscreen()
>+        and VideoFullscreenInterfaceAVKit::cleanupFullscreen(), so that we can avoid relying on
>+        VideoFullscreenManagerProxy::setHasVideo() and VideoFullscreenManagerProxy::setVideoDimensions().
>+        Those two functions are driven by IPC messages from the Web process, which may come before
>+        VideoFullscreenInterfaceAVKit is constructed or after VideoFullscreenInterfaceAVKit
>+        is destroyed.
>+
>+        Manually tested.
>+
>+        * platform/ios/VideoFullscreenInterfaceAVKit.h:
>+        * platform/ios/VideoFullscreenInterfaceAVKit.mm:
>+        (-[WebAVPlayerViewControllerDelegate setFullscreenInterface:]):
>+        (VideoFullscreenInterfaceAVKit::setupFullscreen):
>+        (VideoFullscreenInterfaceAVKit::cleanupFullscreen):
>+
> 2020-05-21  Simon Fraser  <simon.fraser@apple.com>
> 
>         Scrolling thread scrolls on sync-scrolling scrollers don't get to the main thread
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index fce6b013082e7ce183772d4a40f866f278716bf6..f6c115dff414a3263adc3b65ae211d180b1aebb4 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,23 @@
>+2020-05-22  Peng Liu  <peng.liu6@apple.com>
>+
>+        VideoFullscreenInterfaceAVKit is leaking when a video element enters and exits fullscreen/picture-in-picture
>+        https://bugs.webkit.org/show_bug.cgi?id=212293
>+
>+        VideoFullscreenManagerProxy::ensureInterface() makes sure a fullscreen interface object exists (an object
>+        will be created if it does not exit). That means an extra object will be created by an IPC message from
>+        the Web process after the original video fullscreen interface object has been destroyed (thats happens
>+        when a video element is returning to inline from fullscreen or picture-in-picture).
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:
>+        (WebKit::VideoFullscreenManagerProxy::setHasVideo):
>+        Don't call VideoFullscreenInterface[AVKit|Mac]::hasVideoChanged() before the instance of
>+        VideoFullscreenInterface[AVKit|Mac] is not ready yet.
>+        (WebKit::VideoFullscreenManagerProxy::setVideoDimensions):
>+        Don't call VideoFullscreenInterface[AVKit|Mac]::videoDimensionsChanged() after the instance of
>+        VideoFullscreenInterface[AVKit|Mac] is destroyed.
>+
> 2020-05-21  Peng Liu  <peng.liu6@apple.com>
> 
>         Fix issues of the Picture-in-Picture API under stress tests
>diff --git a/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h b/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h
>index d6ac4ca05f2ddfc88e30c8e1b3bfe6a27cae4238..79aa85a6de7c6297c5a169b8ae344a07c283cd1c 100644
>--- a/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h
>+++ b/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h
>@@ -36,10 +36,10 @@
> #include <objc/objc.h>
> #include <wtf/Forward.h>
> #include <wtf/Function.h>
>-#include <wtf/RefCounted.h>
>-#include <wtf/RefPtr.h>
> #include <wtf/RetainPtr.h>
> #include <wtf/RunLoop.h>
>+#include <wtf/ThreadSafeRefCounted.h>
>+#include <wtf/WeakPtr.h>
> 
> OBJC_CLASS UIViewController;
> OBJC_CLASS UIWindow;
>@@ -61,8 +61,8 @@ class VideoFullscreenChangeObserver;
> class VideoFullscreenInterfaceAVKit final
>     : public VideoFullscreenModelClient
>     , public PlaybackSessionModelClient
>-    , public ThreadSafeRefCounted<VideoFullscreenInterfaceAVKit> {
>-
>+    , public ThreadSafeRefCounted<VideoFullscreenInterfaceAVKit>
>+    , public CanMakeWeakPtr<VideoFullscreenInterfaceAVKit> {
> public:
>     WEBCORE_EXPORT static Ref<VideoFullscreenInterfaceAVKit> create(PlaybackSessionInterfaceAVKit&);
>     virtual ~VideoFullscreenInterfaceAVKit();
>diff --git a/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm b/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm
>index 6ce176f2f7604be56d41d145d0ca5365c9d4cc27..15cfc8b24bfb35e45141e2010f3958ec71ac96c1 100644
>--- a/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm
>+++ b/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm
>@@ -45,6 +45,7 @@
> #import <pal/spi/cocoa/AVKitSPI.h>
> #import <pal/spi/cocoa/QuartzCoreSPI.h>
> #import <pal/spi/ios/UIKitSPI.h>
>+#import <wtf/RefPtr.h>
> #import <wtf/RetainPtr.h>
> #import <wtf/text/CString.h>
> #import <wtf/text/WTFString.h>
>@@ -94,7 +95,7 @@ @end
> @class WebAVMediaSelectionOption;
> 
> @interface WebAVPlayerViewControllerDelegate : NSObject <AVPlayerViewControllerDelegate_WebKitOnly> {
>-    RefPtr<VideoFullscreenInterfaceAVKit> _fullscreenInterface;
>+    WeakPtr<VideoFullscreenInterfaceAVKit> _fullscreenInterface;
> }
> @property (assign) VideoFullscreenInterfaceAVKit* fullscreenInterface;
> - (BOOL)playerViewController:(AVPlayerViewController *)playerViewController shouldExitFullScreenWithReason:(AVPlayerViewControllerExitFullScreenReason)reason;
>@@ -108,7 +109,7 @@ - (VideoFullscreenInterfaceAVKit*)fullscreenInterface
> 
> - (void)setFullscreenInterface:(VideoFullscreenInterfaceAVKit*)fullscreenInterface
> {
>-    _fullscreenInterface = fullscreenInterface;
>+    _fullscreenInterface = makeWeakPtr(*fullscreenInterface);
> }
> 
> - (void)playerViewControllerWillStartPictureInPicture:(AVPlayerViewController *)playerViewController
>@@ -891,6 +892,10 @@ void VideoFullscreenInterfaceAVKit::setupFullscreen(UIView& videoView, const Int
>     ASSERT(standby || mode != HTMLMediaElementEnums::VideoFullscreenModeNone);
>     LOG(Fullscreen, "VideoFullscreenInterfaceAVKit::setupFullscreen(%p)", this);
> 
>+    [playerController() setHasEnabledVideo:true];
>+    [playerController() setHasVideo:true];
>+    [playerController() setContentDimensions:initialRect.size()];
>+
>     m_allowsPictureInPicturePlayback = allowsPictureInPicturePlayback;
>     m_videoView = &videoView;
>     m_parentView = parentView;
>@@ -968,6 +973,9 @@ void VideoFullscreenInterfaceAVKit::cleanupFullscreen()
>     
>     if (m_fullscreenChangeObserver)
>         m_fullscreenChangeObserver->didCleanupFullscreen();
>+
>+    [playerController() setHasEnabledVideo:false];
>+    [playerController() setHasVideo:false];
> }
> 
> void VideoFullscreenInterfaceAVKit::invalidate()
>diff --git a/Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm b/Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm
>index 41d94d3926d07cf5378e7f6fc098c46a341b4d9a..ae8574e3ef2dd6a8d4d52d543ed5cd2c8a985594 100644
>--- a/Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm
>+++ b/Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm
>@@ -537,6 +537,9 @@ void VideoFullscreenManagerProxy::setHasVideo(uint64_t contextId, bool hasVideo)
>     if (m_mockVideoPresentationModeEnabled)
>         return;
> 
>+    if (!m_contextMap.contains(contextId))
>+        return;
>+
>     ensureInterface(contextId).hasVideoChanged(hasVideo);
> }
> 
>@@ -546,6 +549,9 @@ void VideoFullscreenManagerProxy::setVideoDimensions(uint64_t contextId, const F
>     if (m_mockVideoPresentationModeEnabled)
>         return;
> 
>+    if (!m_contextMap.contains(contextId))
>+        return;
>+
>     ensureInterface(contextId).videoDimensionsChanged(videoDimensions);
> }
>

Flags:

youennf: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 212293: 400100 | 400302 | 400311