Bug 212293 – VideoFullscreenInterfaceAVKit is leaking when a video element enters and exits fullscreen/picture-in-picture

• Component: Media
• Status: RESOLVED FIXED

[Peng Liu]

VideoFullscreenInterfaceAVKit is leaking when a video element enters and exits fullscreen/picture-in-picture

[Radar WebKit Bug Importer]

<rdar://problem/63561002>

[Peng Liu]

Created attachment 400100 [details]
Patch

[youenn fablet]

Comment on attachment 400100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400100&action=review

> Source/WebKit/ChangeLog:16
> +        VideoFullscreenInterface[AVKit|Mac] is not ready yet.

s/not ready yet/ready/

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:64
> +    , public ThreadSafeRefCounted<VideoFullscreenInterfaceAVKit>

This might be left to another patch but do we even need this one to be ref counted?
Can we make it a unique_ptr and replace all the Ref<VideoFullscreenInterfaceAVKit> by WeakPtr<VideoFullscreenInterfaceAVKit> except for the owner of the VideoFullscreenInterfaceAVKit?

If we need it to be refcounted, is there a possibility VideoFullscreenInterfaceAVKit get destroyed in a background thread?
Given it has string, we probably do not want that. We could make it ThreadSafeRefCounted<VideoFullscreenInterfaceAVKit, DestructionThread::Main (or MainRunLoop)>

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:48
> +#import <wtf/RefPtr.h>

Is this one really needed, I would expect WTFString.h to include it somehow.

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:98
> +    WeakPtr<VideoFullscreenInterfaceAVKit> _fullscreenInterface;

Are we sure _fullscreenInterface will only be used in the main thread?

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:112
> +    _fullscreenInterface = makeWeakPtr(*fullscreenInterface);

Let's add an ASSERT(IsMainThread()) here.

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:540
> +    if (!m_contextMap.contains(contextId))

It would be nice to use an ObjectIdentifier<> instead of a uint64_t, as a follow-up patch.
That would make the code more type-aware and allow removing the MESSAGE_CHECK_CONTEXTID(contextId).

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:543
>      ensureInterface(contextId).hasVideoChanged(hasVideo);

We are searching m_contextMap twice probably.
Can we add a routine so that we can write :
if (auto* interface = interfaceIfFromIdentifier(contextId))
    interface->hasVideoChanged(hasVideo);

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:553
> +        return;

Ditto

[Peng Liu]

Created attachment 400302 [details]
Patch for landing

[Peng Liu]

Comment on attachment 400100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400100&action=review

>> Source/WebKit/ChangeLog:16
>> +        VideoFullscreenInterface[AVKit|Mac] is not ready yet.
> 
> s/not ready yet/ready/

Fixed.

>> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:64
>> +    , public ThreadSafeRefCounted<VideoFullscreenInterfaceAVKit>
> 
> This might be left to another patch but do we even need this one to be ref counted?
> Can we make it a unique_ptr and replace all the Ref<VideoFullscreenInterfaceAVKit> by WeakPtr<VideoFullscreenInterfaceAVKit> except for the owner of the VideoFullscreenInterfaceAVKit?

Did a simple search. Currently, WebAVPlayerLayer, VideoFullscreenControllerContext and
WKFullScreenViewControllerVideoFullscreenModelClient have RefPtr of VideoFullscreenInterfaceAVKit. In addition, VideoFullscreenManagerProxy has a hash map of ModelInterfaceTuple, which includes RefPtr of VideoFullscreenInterfaceAVKit. We may need to refactor some classes to get rid of the RefPtr of VideoFullscreenInterfaceAVKit and we must be very careful to avoid any timing issue. Also, if we do the refactoring for VideoFullscreenInterfaceAVKit, we had better do the same thing for VideoFullscreenInterfaceMac as well.

> 
> If we need it to be refcounted, is there a possibility VideoFullscreenInterfaceAVKit get destroyed in a background thread?
> Given it has string, we probably do not want that. We could make it ThreadSafeRefCounted<VideoFullscreenInterfaceAVKit, DestructionThread::Main (or MainRunLoop)>

Good point! Make it ThreadSafeRefCounted<VideoFullscreenInterfaceAVKit, DestructionThread::MainRunLoop>.

>> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:48
>> +#import <wtf/RefPtr.h>
> 
> Is this one really needed, I would expect WTFString.h to include it somehow.

It should be included indirectly somehow because there was no building errors.

>> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:98
>> +    WeakPtr<VideoFullscreenInterfaceAVKit> _fullscreenInterface;
> 
> Are we sure _fullscreenInterface will only be used in the main thread?

Not sure. Let's add ASSERT(isMainThread()).

>> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:112
>> +    _fullscreenInterface = makeWeakPtr(*fullscreenInterface);
> 
> Let's add an ASSERT(IsMainThread()) here.

Done.

>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:540
>> +    if (!m_contextMap.contains(contextId))
> 
> It would be nice to use an ObjectIdentifier<> instead of a uint64_t, as a follow-up patch.
> That would make the code more type-aware and allow removing the MESSAGE_CHECK_CONTEXTID(contextId).

Filed a bug for it webkit.org/b/212392.

>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:543
>>      ensureInterface(contextId).hasVideoChanged(hasVideo);
> 
> We are searching m_contextMap twice probably.
> Can we add a routine so that we can write :
> if (auto* interface = interfaceIfFromIdentifier(contextId))
>     interface->hasVideoChanged(hasVideo);

Added VideoFullscreenManagerProxy::findInterface() for this purpose.

>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:553
>> +        return;
> 
> Ditto

Same as the above comment.

[Peng Liu]

Created attachment 400311 [details]
patch for landing v2

[EWS]

Committed r262185: <https://trac.webkit.org/changeset/262185>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400311 [details].