- Revision
- 262185
- Author
- peng.l...@apple.com
- Date
- 2020-05-27 00:50:44 -0700 (Wed, 27 May 2020)
Log Message
VideoFullscreenInterfaceAVKit is leaking when a video element enters and exits fullscreen/picture-in-picture
https://bugs.webkit.org/show_bug.cgi?id=212293
Source/WebCore:
Reviewed by Youenn Fablet.
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):
Source/WebKit:
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 Youenn Fablet.
* UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:
(WebKit::VideoFullscreenManagerProxy::setHasVideo):
Don't call VideoFullscreenInterface[AVKit|Mac]::hasVideoChanged() before the instance of
VideoFullscreenInterface[AVKit|Mac] is ready.
(WebKit::VideoFullscreenManagerProxy::setVideoDimensions):
Don't call VideoFullscreenInterface[AVKit|Mac]::videoDimensionsChanged() after the instance of
VideoFullscreenInterface[AVKit|Mac] is destroyed.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (262184 => 262185)
--- trunk/Source/WebCore/ChangeLog 2020-05-27 07:49:41 UTC (rev 262184)
+++ trunk/Source/WebCore/ChangeLog 2020-05-27 07:50:44 UTC (rev 262185)
@@ -1,3 +1,30 @@
+2020-05-27 Peng Liu <peng.l...@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 Youenn Fablet.
+
+ 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-27 Carlos Garcia Campos <cgar...@igalia.com>
[GTK][WTR] EventSender: stop using GdkEvent API in preparation for GTK4
Modified: trunk/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h (262184 => 262185)
--- trunk/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h 2020-05-27 07:49:41 UTC (rev 262184)
+++ trunk/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h 2020-05-27 07:50:44 UTC (rev 262185)
@@ -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 VideoFullscreenInterfaceAVKit final
: public VideoFullscreenModelClient
, public PlaybackSessionModelClient
- , public ThreadSafeRefCounted<VideoFullscreenInterfaceAVKit> {
-
+ , public ThreadSafeRefCounted<VideoFullscreenInterfaceAVKit, WTF::DestructionThread::MainRunLoop>
+ , public CanMakeWeakPtr<VideoFullscreenInterfaceAVKit> {
public:
WEBCORE_EXPORT static Ref<VideoFullscreenInterfaceAVKit> create(PlaybackSessionInterfaceAVKit&);
virtual ~VideoFullscreenInterfaceAVKit();
Modified: trunk/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm (262184 => 262185)
--- trunk/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm 2020-05-27 07:49:41 UTC (rev 262184)
+++ trunk/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm 2020-05-27 07:50:44 UTC (rev 262185)
@@ -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 @@
@class WebAVMediaSelectionOption;
@interface WebAVPlayerViewControllerDelegate : NSObject <AVPlayerViewControllerDelegate_WebKitOnly> {
- RefPtr<VideoFullscreenInterfaceAVKit> _fullscreenInterface;
+ WeakPtr<VideoFullscreenInterfaceAVKit> _fullscreenInterface;
}
@property (assign) VideoFullscreenInterfaceAVKit* fullscreenInterface;
- (BOOL)playerViewController:(AVPlayerViewController *)playerViewController shouldExitFullScreenWithReason:(AVPlayerViewControllerExitFullScreenReason)reason;
@@ -103,12 +104,14 @@
@implementation WebAVPlayerViewControllerDelegate
- (VideoFullscreenInterfaceAVKit*)fullscreenInterface
{
+ ASSERT(isMainThread());
return _fullscreenInterface.get();
}
- (void)setFullscreenInterface:(VideoFullscreenInterfaceAVKit*)fullscreenInterface
{
- _fullscreenInterface = fullscreenInterface;
+ ASSERT(isMainThread());
+ _fullscreenInterface = makeWeakPtr(*fullscreenInterface);
}
- (void)playerViewControllerWillStartPictureInPicture:(AVPlayerViewController *)playerViewController
@@ -891,6 +894,10 @@
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;
@@ -973,6 +980,9 @@
if (m_fullscreenChangeObserver)
m_fullscreenChangeObserver->didCleanupFullscreen();
+
+ [playerController() setHasEnabledVideo:false];
+ [playerController() setHasVideo:false];
}
void VideoFullscreenInterfaceAVKit::invalidate()
Modified: trunk/Source/WebKit/ChangeLog (262184 => 262185)
--- trunk/Source/WebKit/ChangeLog 2020-05-27 07:49:41 UTC (rev 262184)
+++ trunk/Source/WebKit/ChangeLog 2020-05-27 07:50:44 UTC (rev 262185)
@@ -1,3 +1,23 @@
+2020-05-27 Peng Liu <peng.l...@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 Youenn Fablet.
+
+ * UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:
+ (WebKit::VideoFullscreenManagerProxy::setHasVideo):
+ Don't call VideoFullscreenInterface[AVKit|Mac]::hasVideoChanged() before the instance of
+ VideoFullscreenInterface[AVKit|Mac] is ready.
+ (WebKit::VideoFullscreenManagerProxy::setVideoDimensions):
+ Don't call VideoFullscreenInterface[AVKit|Mac]::videoDimensionsChanged() after the instance of
+ VideoFullscreenInterface[AVKit|Mac] is destroyed.
+
2020-05-27 Carlos Garcia Campos <cgar...@igalia.com>
[GTK][WTR] EventSender: stop using GdkEvent API in preparation for GTK4
Modified: trunk/Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.h (262184 => 262185)
--- trunk/Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.h 2020-05-27 07:49:41 UTC (rev 262184)
+++ trunk/Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.h 2020-05-27 07:50:44 UTC (rev 262185)
@@ -151,6 +151,7 @@
ModelInterfaceTuple& ensureModelAndInterface(uint64_t contextId);
VideoFullscreenModelContext& ensureModel(uint64_t contextId);
PlatformVideoFullscreenInterface& ensureInterface(uint64_t contextId);
+ PlatformVideoFullscreenInterface* findInterface(uint64_t contextId);
void addClientForContext(uint64_t contextId);
void removeClientForContext(uint64_t contextId);
Modified: trunk/Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm (262184 => 262185)
--- trunk/Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm 2020-05-27 07:49:41 UTC (rev 262184)
+++ trunk/Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm 2020-05-27 07:50:44 UTC (rev 262185)
@@ -441,6 +441,15 @@
return *std::get<1>(ensureModelAndInterface(contextId));
}
+PlatformVideoFullscreenInterface* VideoFullscreenManagerProxy::findInterface(uint64_t contextId)
+{
+ auto it = m_contextMap.find(contextId);
+ if (it == m_contextMap.end())
+ return nullptr;
+
+ return std::get<1>(it->value).get();
+}
+
void VideoFullscreenManagerProxy::addClientForContext(uint64_t contextId)
{
auto addResult = m_clientCounts.add(contextId, 1);
@@ -547,7 +556,8 @@
if (m_mockVideoPresentationModeEnabled)
return;
- ensureInterface(contextId).hasVideoChanged(hasVideo);
+ if (auto* interface = findInterface(contextId))
+ interface->hasVideoChanged(hasVideo);
}
void VideoFullscreenManagerProxy::setVideoDimensions(uint64_t contextId, const FloatSize& videoDimensions)
@@ -556,7 +566,8 @@
if (m_mockVideoPresentationModeEnabled)
return;
- ensureInterface(contextId).videoDimensionsChanged(videoDimensions);
+ if (auto* interface = findInterface(contextId))
+ interface->videoDimensionsChanged(videoDimensions);
}
void VideoFullscreenManagerProxy::enterFullscreen(uint64_t contextId)
