VideoFullscreenInterfaceAVKit is leaking when a video element enters and exits fullscreen/picture-in-picture
<rdar://problem/63561002>
Created attachment 400100 [details] Patch
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
Created attachment 400302 [details] Patch for landing
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.
Created attachment 400311 [details] patch for landing v2
Committed r262185: <https://trac.webkit.org/changeset/262185> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400311 [details].