Bug 202425 – Stressing webkitSetPresentationMode leads to wrong inline video dimensions

• Component: Media
• Status: RESOLVED FIXED

[Carlos Bentzen]

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

[Carlos Bentzen]

Created attachment 379930 [details]
Screenshot

[Carlos Bentzen]

(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/

[Carlos Bentzen]

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:]

[Radar WebKit Bug Importer]

<rdar://problem/55887384>

[Peng Liu]

Created attachment 400739 [details]
Patch

[Eric Carlson]

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?

[Peng Liu]

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.

[Peng Liu]

Created attachment 400781 [details]
Revise the patch based on Eric's comments and fix test failures

[Peng Liu]

Created attachment 400786 [details]
Fix WK1 test failures

[Peng Liu]

Created attachment 400853 [details]
Fix an API test failure

[EWS]

Committed r262456: <https://trac.webkit.org/changeset/262456>

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

[Truitt Savell]

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