Created attachment 379929 [details] Example If webkitSetPresentationMode is stressed quickly enough, the video element ends up in an inconsistent state where it is inline, but with wrong dimensions. With the attached web page (note: needs to add a valid video src), one can reproduce by clicking the button successively fast enough. Safari versions tested: - Version 13.0.1 (13608.2.11.1.10) - SafariForWebKitDevelopment, same version, WebKit r250365 MacOS: High Sierra 10.13.6
Created attachment 379930 [details] Screenshot
(In reply to Carlos Eduardo Ramalho from comment #0) > Created attachment 379929 [details] > Example > > If webkitSetPresentationMode is stressed quickly enough, the video element > ends up in an inconsistent state where it is inline, but with wrong > dimensions. s/state where it is inline/state as if it's in PIP mode/
Running with a debug build, we hit an assertion: ASSERTION FAILED: !_pipViewController ./platform/mac/VideoFullscreenInterfaceMac.mm(187) : -[WebVideoFullscreenInterfaceMacObjC setUpPIPForVideoView:withFrame:inWindow:] 1 0x10bd7a979 WTFCrash 2 0x117d8b44b WTFCrashWithInfo(int, char const*, char const*, int) 3 0x1187c0caa -[WebVideoFullscreenInterfaceMacObjC setUpPIPForVideoView:withFrame:inWindow:] 4 0x1187c3afb WebCore::VideoFullscreenInterfaceMac::setupFullscreen(NSView&, WebCore::IntRect const&, NSWindow*, unsigned int, bool) 5 0x111db5702 WebKit::VideoFullscreenManagerProxy::setupFullscreenWithID(unsigned long long, unsigned int, WebCore::IntRect const&, float, unsigned int, bool, bool) 6 0x11274e852 void IPC::callMemberFunctionImpl<WebKit::VideoFullscreenManagerProxy, void (WebKit::VideoFullscreenManagerProxy::*)(unsigned long long, unsigned int, WebCore::IntRect const&, float, unsigned int, bool, bool), std::__1::tuple<unsigned long long, unsigned int, WebCore::IntRect, float, unsigned int, bool, bool>, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul>(WebKit::VideoFullscreenManagerProxy*, void (WebKit::VideoFullscreenManagerProxy::*)(unsigned long long, unsigned int, WebCore::IntRect const&, float, unsigned int, bool, bool), std::__1::tuple<unsigned long long, unsigned int, WebCore::IntRect, float, unsigned int, bool, bool>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul>) 7 0x11274b650 void IPC::callMemberFunction<WebKit::VideoFullscreenManagerProxy, void (WebKit::VideoFullscreenManagerProxy::*)(unsigned long long, unsigned int, WebCore::IntRect const&, float, unsigned int, bool, bool), std::__1::tuple<unsigned long long, unsigned int, WebCore::IntRect, float, unsigned int, bool, bool>, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul> >(std::__1::tuple<unsigned long long, unsigned int, WebCore::IntRect, float, unsigned int, bool, bool>&&, WebKit::VideoFullscreenManagerProxy*, void (WebKit::VideoFullscreenManagerProxy::*)(unsigned long long, unsigned int, WebCore::IntRect const&, float, unsigned int, bool, bool)) 8 0x112749c04 void IPC::handleMessage<Messages::VideoFullscreenManagerProxy::SetupFullscreenWithID, WebKit::VideoFullscreenManagerProxy, void (WebKit::VideoFullscreenManagerProxy::*)(unsigned long long, unsigned int, WebCore::IntRect const&, float, unsigned int, bool, bool)>(IPC::Decoder&, WebKit::VideoFullscreenManagerProxy*, void (WebKit::VideoFullscreenManagerProxy::*)(unsigned long long, unsigned int, WebCore::IntRect const&, float, unsigned int, bool, bool)) 9 0x112749480 WebKit::VideoFullscreenManagerProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) 10 0x1114c0a3a IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) 11 0x111cffcd4 WebKit::AuxiliaryProcessProxy::dispatchMessage(IPC::Connection&, IPC::Decoder&) 12 0x111ff75ba WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) 13 0x11144fcdc IPC::Connection::dispatchMessage(IPC::Decoder&) 14 0x111443345 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) 15 0x11144d491 IPC::Connection::dispatchIncomingMessages() 16 0x11146a082 IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_7::operator()() 17 0x111469fa9 WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_7, void>::call() 18 0x10bda46ed WTF::Function<void ()>::operator()() const 19 0x10be0beb3 WTF::RunLoop::performWork() 20 0x10be0c914 WTF::RunLoop::performWork(void*) 21 0x7fff4694c551 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ 22 0x7fff46a06b1c __CFRunLoopDoSource0 23 0x7fff4692efb0 __CFRunLoopDoSources0 24 0x7fff4692e42d __CFRunLoopRun 25 0x7fff4692dc93 CFRunLoopRunSpecific 26 0x7fff45c18d96 RunCurrentEventLoopInMode 27 0x7fff45c18b06 ReceiveNextEventCommon 28 0x7fff45c18884 _BlockUntilNextEventMatchingListInModeWithFilter 29 0x7fff43ec8a73 _DPSNextEvent 30 0x7fff4465ee34 -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] 31 0x105e8b5c8 -[BrowserApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
<rdar://problem/55887384>
Created attachment 400739 [details] Patch
Comment on attachment 400739 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400739&action=review > Source/WebCore/html/HTMLVideoElement.cpp:463 > + if (!m_isChangingPresentationMode && !m_isEnteringOrExitingPictureInPicture) Is there any reason to not do this check in setFullscreenMode? > Source/WebCore/html/HTMLVideoElement.cpp:492 > + if (mode != fullscreenMode()) { > + m_isChangingPresentationMode = true; > + enterFullscreen(mode); > + } We also call enterFullscreen() from HTMLVideoElement::webkitEnterFullscreen, but instead of also setting this there does it make sense to do it in HTMLMediaElement?
Comment on attachment 400739 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400739&action=review >> Source/WebCore/html/HTMLVideoElement.cpp:463 >> + if (!m_isChangingPresentationMode && !m_isEnteringOrExitingPictureInPicture) > > Is there any reason to not do this check in setFullscreenMode? Actually no reason. I will move the checking to setFullscreenMode() and keep the API implementation simple. >> Source/WebCore/html/HTMLVideoElement.cpp:492 >> + } > > We also call enterFullscreen() from HTMLVideoElement::webkitEnterFullscreen, but instead of also setting this there does it make sense to do it in HTMLMediaElement? I plan to add the checking in HTMLVideoElement::webkitEnterFullscreen() with another patch. There is a bug report for it. Maybe it is a better idea to mark that bug as a duplicated one and fix both of them in one patch? Technically, we can add the checking in HTMLMediaElement::enterFullscreen(). I chose to do that in HTMLVideoElement for two reasons: 1). HTMLMediaElement::enterFullscreen() is not only used by the API implementation (webkitEnterFullscreen and webkitSetPresentationMode), but also used by some internal modules, such as VideoFullscreenControllerContext::didSetupFullscreen(), WebVideoFullscreenController:_requestEnter, AccessibilityMediaObject::enterFullscreen() and WebAccessibilityObjectWrapper:accessibilityVideoEnterFullscreen. However, this patch only tries to prevent excessive requests to corrupt the state machine. So I did not try to change the behavior of the function used by internal modules. 2). The flag m_isChangingPresentationMode is closely related to m_isEnteringOrExitingPictureInPicture and I would like to keep them together.
Created attachment 400781 [details] Revise the patch based on Eric's comments and fix test failures
Created attachment 400786 [details] Fix WK1 test failures
Created attachment 400853 [details] Fix an API test failure
Committed r262456: <https://trac.webkit.org/changeset/262456> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400853 [details].
The changes in https://trac.webkit.org/changeset/262456/webkit caused media/modern-media-controls/media-controller/media-controller-inline-to-fullscreen-to-pip-to-inline.html to time out very frequently. this is being tracked in https://bugs.webkit.org/show_bug.cgi?id=212694