Attachment #400100 for bug #212293

View Details Raw Unified Return to Bug 212293

| Differences between



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



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



#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;

class VideoFullscreenInterfaceAVKit final
    : public VideoFullscreenModelClient
    , public PlaybackSessionModelClient
    , public ThreadSafeRefCounted<VideoFullscreenInterfaceAVKit>
    , public CanMakeWeakPtr<VideoFullscreenInterfaceAVKit> {
public:
    WEBCORE_EXPORT static Ref<VideoFullscreenInterfaceAVKit> create(PlaybackSessionInterfaceAVKit&);
    virtual ~VideoFullscreenInterfaceAVKit();



#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>

@class WebAVMediaSelectionOption;

@interface WebAVPlayerViewControllerDelegate : NSObject <AVPlayerViewControllerDelegate_WebKitOnly> {
    WeakPtr<VideoFullscreenInterfaceAVKit> _fullscreenInterface;
}
@property (assign) VideoFullscreenInterfaceAVKit* fullscreenInterface;
- (BOOL)playerViewController:(AVPlayerViewController *)playerViewController shouldExitFullScreenWithReason:(AVPlayerViewControllerExitFullScreenReason)reason;


- (void)setFullscreenInterface:(VideoFullscreenInterfaceAVKit*)fullscreenInterface
{
    _fullscreenInterface = makeWeakPtr(*fullscreenInterface);
}

- (void)playerViewControllerWillStartPictureInPicture:(AVPlayerViewController *)playerViewController

    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;

    
    if (m_fullscreenChangeObserver)
        m_fullscreenChangeObserver->didCleanupFullscreen();

    [playerController() setHasEnabledVideo:false];
    [playerController() setHasVideo:false];
}

void VideoFullscreenInterfaceAVKit::invalidate()

Lines 537-542 void VideoFullscreenManagerProxy::setHasVideo(uint64_t contextId, bool hasVideo) a/Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm_sec1
- a/Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm +6 lines
537	if (m_mockVideoPresentationModeEnabled)	537	if (m_mockVideoPresentationModeEnabled)
538	return;	538	return;
539		539
		540	if (!m_contextMap.contains(contextId))
		541	return;
		542
540	ensureInterface(contextId).hasVideoChanged(hasVideo);	543	ensureInterface(contextId).hasVideoChanged(hasVideo);
541	}	544	}
542		545
Lines 546-551 void VideoFullscreenManagerProxy::setVideoDimensions(uint64_t contextId, const F a/Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm_sec2
546	if (m_mockVideoPresentationModeEnabled)	549	if (m_mockVideoPresentationModeEnabled)
547	return;	550	return;
548		551
		552	if (!m_contextMap.contains(contextId))
		553	return;
		554
549	ensureInterface(contextId).videoDimensionsChanged(videoDimensions);	555	ensureInterface(contextId).videoDimensionsChanged(videoDimensions);
550	}	556	}
551		557

Return to Bug 212293

Lines 45-50 a/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm_sec1
- a/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm -2 / +10 lines
45	#import <pal/spi/cocoa/AVKitSPI.h>	45	#import <pal/spi/cocoa/AVKitSPI.h>
46	#import <pal/spi/cocoa/QuartzCoreSPI.h>	46	#import <pal/spi/cocoa/QuartzCoreSPI.h>
47	#import <pal/spi/ios/UIKitSPI.h>	47	#import <pal/spi/ios/UIKitSPI.h>
		48	#import <wtf/RefPtr.h>
48	#import <wtf/RetainPtr.h>	49	#import <wtf/RetainPtr.h>
49	#import <wtf/text/CString.h>	50	#import <wtf/text/CString.h>
50	#import <wtf/text/WTFString.h>	51	#import <wtf/text/WTFString.h>
Lines 94-100 @end a/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm_sec2
94	@class WebAVMediaSelectionOption;	95	@class WebAVMediaSelectionOption;
95		96
96	@interface WebAVPlayerViewControllerDelegate : NSObject <AVPlayerViewControllerDelegate_WebKitOnly> {	97	@interface WebAVPlayerViewControllerDelegate : NSObject <AVPlayerViewControllerDelegate_WebKitOnly> {
97	RefPtr<VideoFullscreenInterfaceAVKit> _fullscreenInterface;	98	WeakPtr<VideoFullscreenInterfaceAVKit> _fullscreenInterface;
98	}	99	}
99	@property (assign) VideoFullscreenInterfaceAVKit* fullscreenInterface;	100	@property (assign) VideoFullscreenInterfaceAVKit* fullscreenInterface;
100	- (BOOL)playerViewController:(AVPlayerViewController *)playerViewController shouldExitFullScreenWithReason:(AVPlayerViewControllerExitFullScreenReason)reason;	101	- (BOOL)playerViewController:(AVPlayerViewController *)playerViewController shouldExitFullScreenWithReason:(AVPlayerViewControllerExitFullScreenReason)reason;
Lines 108-114 - (VideoFullscreenInterfaceAVKit*)fullscreenInterface a/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm_sec3
108		109
109	- (void)setFullscreenInterface:(VideoFullscreenInterfaceAVKit*)fullscreenInterface	110	- (void)setFullscreenInterface:(VideoFullscreenInterfaceAVKit*)fullscreenInterface
110	{	111	{
111	_fullscreenInterface = fullscreenInterface;	112	_fullscreenInterface = makeWeakPtr(*fullscreenInterface);
112	}	113	}
113		114
114	- (void)playerViewControllerWillStartPictureInPicture:(AVPlayerViewController *)playerViewController	115	- (void)playerViewControllerWillStartPictureInPicture:(AVPlayerViewController *)playerViewController
Lines 891-896 void VideoFullscreenInterfaceAVKit::setupFullscreen(UIView& videoView, const Int a/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm_sec4
891	ASSERT(standby \|\| mode != HTMLMediaElementEnums::VideoFullscreenModeNone);	892	ASSERT(standby \|\| mode != HTMLMediaElementEnums::VideoFullscreenModeNone);
892	LOG(Fullscreen, "VideoFullscreenInterfaceAVKit::setupFullscreen(%p)", this);	893	LOG(Fullscreen, "VideoFullscreenInterfaceAVKit::setupFullscreen(%p)", this);
893		894
		895	[playerController() setHasEnabledVideo:true];
		896	[playerController() setHasVideo:true];
		897	[playerController() setContentDimensions:initialRect.size()];
		898
894	m_allowsPictureInPicturePlayback = allowsPictureInPicturePlayback;	899	m_allowsPictureInPicturePlayback = allowsPictureInPicturePlayback;
895	m_videoView = &videoView;	900	m_videoView = &videoView;
896	m_parentView = parentView;	901	m_parentView = parentView;
Lines 968-973 void VideoFullscreenInterfaceAVKit::cleanupFullscreen() a/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm_sec5
968		973
969	if (m_fullscreenChangeObserver)	974	if (m_fullscreenChangeObserver)
970	m_fullscreenChangeObserver->didCleanupFullscreen();	975	m_fullscreenChangeObserver->didCleanupFullscreen();
		976
		977	[playerController() setHasEnabledVideo:false];
		978	[playerController() setHasVideo:false];
971	}	979	}
972		980
973	void VideoFullscreenInterfaceAVKit::invalidate()	981	void VideoFullscreenInterfaceAVKit::invalidate()

Lines 1-3 a/Source/WebCore/ChangeLog_sec1
- a/Source/WebCore/ChangeLog +27 lines
		1	2020-05-22 Peng Liu <peng.liu6@apple.com>
		2
		3	VideoFullscreenInterfaceAVKit is leaking when a video element enters and exits fullscreen/picture-in-picture
		4	https://bugs.webkit.org/show_bug.cgi?id=212293
		5
		6	Reviewed by NOBODY (OOPS!).
		7
		8	WebAVPlayerViewControllerDelegate is created and retained by VideoFullscreenInterfaceAVKit,
		9	but it has a RefPtr to VideoFullscreenInterfaceAVKit. This leads to a memory leak
		10	when a video element enters and exit fullscreen or Picture-in-Picture. This patch
		11	replaces the RefPtr with a WeakPtr to fix the leak.
		12
		13	With this patch, we config playerController in VideoFullscreenInterfaceAVKit::setupFullscreen()
		14	and VideoFullscreenInterfaceAVKit::cleanupFullscreen(), so that we can avoid relying on
		15	VideoFullscreenManagerProxy::setHasVideo() and VideoFullscreenManagerProxy::setVideoDimensions().
		16	Those two functions are driven by IPC messages from the Web process, which may come before
		17	VideoFullscreenInterfaceAVKit is constructed or after VideoFullscreenInterfaceAVKit
		18	is destroyed.
		19
		20	Manually tested.
		21
		22	* platform/ios/VideoFullscreenInterfaceAVKit.h:
		23	* platform/ios/VideoFullscreenInterfaceAVKit.mm:
		24	(-[WebAVPlayerViewControllerDelegate setFullscreenInterface:]):
		25	(VideoFullscreenInterfaceAVKit::setupFullscreen):
		26	(VideoFullscreenInterfaceAVKit::cleanupFullscreen):
		27
1	2020-05-21 Simon Fraser <simon.fraser@apple.com>	28	2020-05-21 Simon Fraser <simon.fraser@apple.com>
2		29
3	Scrolling thread scrolls on sync-scrolling scrollers don't get to the main thread	30	Scrolling thread scrolls on sync-scrolling scrollers don't get to the main thread

Lines 1-3 a/Source/WebKit/ChangeLog_sec1
- a/Source/WebKit/ChangeLog +20 lines
		1	2020-05-22 Peng Liu <peng.liu6@apple.com>
		2
		3	VideoFullscreenInterfaceAVKit is leaking when a video element enters and exits fullscreen/picture-in-picture
		4	https://bugs.webkit.org/show_bug.cgi?id=212293
		5
		6	VideoFullscreenManagerProxy::ensureInterface() makes sure a fullscreen interface object exists (an object
		7	will be created if it does not exit). That means an extra object will be created by an IPC message from
		8	the Web process after the original video fullscreen interface object has been destroyed (thats happens
		9	when a video element is returning to inline from fullscreen or picture-in-picture).
		10
		11	Reviewed by NOBODY (OOPS!).
		12
		13	* UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:
		14	(WebKit::VideoFullscreenManagerProxy::setHasVideo):
		15	Don't call VideoFullscreenInterface[AVKit\|Mac]::hasVideoChanged() before the instance of
		16	VideoFullscreenInterface[AVKit\|Mac] is not ready yet.
		17	(WebKit::VideoFullscreenManagerProxy::setVideoDimensions):
		18	Don't call VideoFullscreenInterface[AVKit\|Mac]::videoDimensionsChanged() after the instance of
		19	VideoFullscreenInterface[AVKit\|Mac] is destroyed.
		20
1	2020-05-21 Peng Liu <peng.liu6@apple.com>	21	2020-05-21 Peng Liu <peng.liu6@apple.com>
2		22
3	Fix issues of the Picture-in-Picture API under stress tests	23	Fix issues of the Picture-in-Picture API under stress tests

Lines 36-45 a/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h_sec1
- a/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h -4 / +4 lines
36	#include <objc/objc.h>	36	#include <objc/objc.h>
37	#include <wtf/Forward.h>	37	#include <wtf/Forward.h>
38	#include <wtf/Function.h>	38	#include <wtf/Function.h>
39	#include <wtf/RefCounted.h>
40	#include <wtf/RefPtr.h>
41	#include <wtf/RetainPtr.h>	39	#include <wtf/RetainPtr.h>
42	#include <wtf/RunLoop.h>	40	#include <wtf/RunLoop.h>
		41	#include <wtf/ThreadSafeRefCounted.h>
		42	#include <wtf/WeakPtr.h>
43		43
44	OBJC_CLASS UIViewController;	44	OBJC_CLASS UIViewController;
45	OBJC_CLASS UIWindow;	45	OBJC_CLASS UIWindow;
Lines 61-68 class VideoFullscreenChangeObserver; a/Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h_sec2
61	class VideoFullscreenInterfaceAVKit final	61	class VideoFullscreenInterfaceAVKit final
62	: public VideoFullscreenModelClient	62	: public VideoFullscreenModelClient
63	, public PlaybackSessionModelClient	63	, public PlaybackSessionModelClient
64	, public ThreadSafeRefCounted<VideoFullscreenInterfaceAVKit> {	64	, public ThreadSafeRefCounted<VideoFullscreenInterfaceAVKit>
65		65	, public CanMakeWeakPtr<VideoFullscreenInterfaceAVKit> {
66	public:	66	public:
67	WEBCORE_EXPORT static Ref<VideoFullscreenInterfaceAVKit> create(PlaybackSessionInterfaceAVKit&);	67	WEBCORE_EXPORT static Ref<VideoFullscreenInterfaceAVKit> create(PlaybackSessionInterfaceAVKit&);
68	virtual ~VideoFullscreenInterfaceAVKit();	68	virtual ~VideoFullscreenInterfaceAVKit();