Bug 209680

Summary: REGRESSION (r259095): ASSERTION FAILED: m_videoFullscreenMode != VideoFullscreenModeNone seen with TestWebKitAPI.WebKitLegacy.AudioSessionCategoryIOS
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: New BugsAssignee: Peng Liu <peng.liu6>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, cdumez, darin, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, Lawrence.j, peng.liu6, philipj, sergio, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=209610
https://bugs.webkit.org/show_bug.cgi?id=209823
https://bugs.webkit.org/show_bug.cgi?id=211792
Attachments:
Description Flags
Patch
none
Patch
none
Revise the patch based on review comments and add an API test
none
Simplify the page for API test after discussion with Jer and Eric
jer.noble: review+
Patch for landing
none
Patch for landing - v2 none

Description Ryan Haddad 2020-03-27 13:46:41 PDT
Seeing the following API test crash on iOS Debug bots:

    TestWebKitAPI.WebKitLegacy.AudioSessionCategoryIOS
        MRMediaRemoteSetNowPlayingApplicationPlaybackStateForOrigin(stopped) failed with error 3
        MRMediaRemoteSetNowPlayingApplicationPlaybackStateForOrigin(stopped) failed with error 3
        MRMediaRemoteSetNowPlayingApplicationPlaybackStateForOrigin(stopped) failed with error 3
        MRMediaRemoteSetNowPlayingApplicationPlaybackStateForOrigin(stopped) failed with error 3
        MRMediaRemoteSetNowPlayingApplicationPlaybackStateForOrigin(stopped) failed with error 3
        MRMediaRemoteSetNowPlayingApplicationPlaybackStateForOrigin(playing) failed with error 3
        2020-03-27 01:17:47.601 TestWebKitAPI[49624:5509678] *** Warning: <AVPlayerViewController: 0x7f9d5d035e00> is trying to enter full screen, but is not in its view's window's view controller hierarchy. This results in undefined behavior.
        MRMediaRemoteSetNowPlayingApplicationPlaybackStateForOrigin(stopped) failed with error 3
        MRMediaRemoteSetNowPlayingApplicationPlaybackStateForOrigin(stopped) failed with error 3
        MRMediaRemoteSetNowPlayingApplicationPlaybackStateForOrigin(stopped) failed with error 3
        MRMediaRemoteSetNowPlayingApplicationPlaybackStateForOrigin(stopped) failed with error 3
        ASSERTION FAILED: m_videoFullscreenMode != VideoFullscreenModeNone
        ./html/HTMLMediaElement.cpp(6105) : void WebCore::HTMLMediaElement::exitFullscreen()
        1   0x107ff9909 WTFCrash
        2   0x120762f3b WTFCrashWithInfo(int, char const*, char const*, int)
        3   0x1231fe11f WebCore::HTMLMediaElement::exitFullscreen()
        4   0x111e13c4b -[WebView(WebViewInternal) _enterVideoFullscreenForVideoElement:mode:]
        5   0x111e3487d WebChromeClient::enterVideoFullscreenForVideoElement(WebCore::HTMLVideoElement&, unsigned int, bool)
        6   0x12325d7b6 WebCore::HTMLMediaElement::enterFullscreen(unsigned int)::$_37::operator()() const
        7   0x12325d539 WTF::Detail::CallableWrapper<WebCore::HTMLMediaElement::enterFullscreen(unsigned int)::$_37, void>::call()
        8   0x1207772aa WTF::Function<void ()>::operator()() const
        9   0x120a72940 WebCore::GenericTaskQueue<WebCore::Timer>::enqueueTask(WTF::Function<void ()>&&)::'lambda'()::operator()() const
        10  0x120a72789 WTF::Detail::CallableWrapper<WebCore::GenericTaskQueue<WebCore::Timer>::enqueueTask(WTF::Function<void ()>&&)::'lambda'(), void>::call()
        11  0x1207772aa WTF::Function<void ()>::operator()() const
        12  0x123cb7f3e WebCore::TaskDispatcher<WebCore::Timer>::dispatchOneTask()
        13  0x123cb7be5 WebCore::TaskDispatcher<WebCore::Timer>::sharedTimerFired()
        14  0x123cbd921 WebCore::TaskDispatcher<WebCore::Timer>::sharedTimer()::$_1::operator()() const
        15  0x123cbd8e9 WTF::Detail::CallableWrapper<WebCore::TaskDispatcher<WebCore::Timer>::sharedTimer()::$_1, void>::call()
        16  0x1207772aa WTF::Function<void ()>::operator()() const
        17  0x120841c69 WebCore::Timer::fired()
        18  0x123d0e6fa WebCore::ThreadTimers::sharedTimerFiredInternal()
        19  0x123d176b1 WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0::operator()() const
        20  0x123d17669 WTF::Detail::CallableWrapper<WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0, void>::call()
        21  0x1207772aa WTF::Function<void ()>::operator()() const
        22  0x123ccfc97 WebCore::MainThreadSharedTimer::fired()
        23  0x123d78f06 WebCore::timerFired(__CFRunLoopTimer*, void*)
        24  0x10fc803f4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
        25  0x10fc8008e __CFRunLoopDoTimer
        26  0x10fc7f6ea __CFRunLoopDoTimers
        27  0x10fc7a33e __CFRunLoopRun
        28  0x10fc79884 CFRunLoopRunSpecific
        29  0x12217080e RunWebThread(void*)
        30  0x110f6c109 _pthread_start
        31  0x110f67b8b thread_start
        Child process terminated with signal 11: Segmentation fault

https://build.webkit.org/builders/Apple%20iOS%2013%20Simulator%20Debug%20WK2%20(Tests)/builds/2822
Comment 1 Radar WebKit Bug Importer 2020-03-27 13:46:54 PDT
<rdar://problem/60983033>
Comment 2 Ryan Haddad 2020-03-27 13:48:37 PDT
I think this may have started with https://trac.webkit.org/changeset/259095/webkit
Comment 3 Peng Liu 2020-03-27 18:20:34 PDT
Created attachment 394776 [details]
Patch
Comment 4 Daniel Bates 2020-03-29 09:02:15 PDT
Comment on attachment 394776 [details]
Patch

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

The patch fixes the test failures, but can't this assert still happen on actual web sites? If not, why not?

Out of curiosity, what are the differences between webkit-playsinline and playsinline?

> Tools/ChangeLog:3
> +        REGRESSION (r259095): ASSERTION FAILED: m_videoFullscreenMode != VideoFullscreenModeNone seen with TestWebKitAPI.WebKitLegacy.AudioSessionCategoryIOS

How does r259095 cause this regression?
Comment 5 Peng Liu 2020-03-30 18:38:42 PDT
(In reply to Daniel Bates from comment #4)
> Comment on attachment 394776 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394776&action=review
> 
> The patch fixes the test failures, but can't this assert still happen on
> actual web sites? If not, why not?
> 
> Out of curiosity, what are the differences between webkit-playsinline and
> playsinline?
> 
> > Tools/ChangeLog:3
> > +        REGRESSION (r259095): ASSERTION FAILED: m_videoFullscreenMode != VideoFullscreenModeNone seen with TestWebKitAPI.WebKitLegacy.AudioSessionCategoryIOS
> 
> How does r259095 cause this regression?

Thanks for the review and sorry for the confusion.

Actually, this patch only fixes the test failures, but not the real problem. I will upload the patch to fix the real problem, and clean up the usage of webkit-playsinline in another patch.
Comment 6 Peng Liu 2020-03-30 19:10:14 PDT
Created attachment 395004 [details]
Patch
Comment 7 Darin Adler 2020-03-31 10:26:50 PDT
Comment on attachment 395004 [details]
Patch

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

Does seem smarter to start using RetainPtr

> Source/WebKitLegacy/mac/WebView/WebView.mm:9338
> +        // This previous call will trigger _exitFullscreen
> +        // which has to clear an item in _private->fullscreenControllersExiting.

I don’t understand this comment. I suggest just omitting it.

> Source/WebKitLegacy/mac/WebView/WebView.mm:9340
> +        _private->fullscreenControllersExiting.append(_private->fullscreenController);
> +        _private->fullscreenController = nil;

What guarantees _private->fullscreenController is not nil here? I don’t understand what limits there are on what HTMLVIdeoElement::exitFullscreen might do. Couldn’t it do some arbitrary set of work that might end up nilling out _private->fullscreenController?

We can avoid a tiny bit of reference count churn by writing it this way:

    _private->fullscreenControllersExiting.append(std::exchange(_private->fullscreenController, nil));

That will move the RetainPtr instead of first copying it and then nulling it out.

> Source/WebKitLegacy/mac/WebView/WebView.mm:9365
> +        auto fullscreenControler = _private->fullscreenControllersExiting.first();
> +        [fullscreenControler exitFullscreen];
> +        _private->fullscreenControllersExiting.remove(0);
>          return;

This seems a bit fragile. Is it really better to call exitFullscreen first, and *then* remove the first thing in the vector *unconditionally*?

Local variable name has a misspelling in it: "controler" with one "r". Also if we don’t change the sequence here, I suggest not using a local variable:

    [_private->fullscreenControllersExiting.first() exitFullscreen];
Comment 8 Eric Carlson 2020-03-31 10:39:19 PDT
Comment on attachment 395004 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Call fullscreenModeChanged(VideoFullscreenModeNone) right before call the

s/before call the/before calling the/

> Source/WebKitLegacy/mac/ChangeLog:10
> +        With this patch, the WebKit-Legacy can support multiple video elements request
> +        to enter video fullscreen almost at the same time, and only the last one will succeed.
> +        Also, this patch fixes webkit.org/b/209610 for WebKit-Legacy.

This should have a test.
Comment 9 Peng Liu 2020-03-31 11:59:31 PDT
Comment on attachment 395004 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        Call fullscreenModeChanged(VideoFullscreenModeNone) right before call the
> 
> s/before call the/before calling the/

Right, will fix it.

>> Source/WebKitLegacy/mac/ChangeLog:10
>> +        Also, this patch fixes webkit.org/b/209610 for WebKit-Legacy.
> 
> This should have a test.

Agree.  We need a dedicated test for this fix. I will add a layout test.
Currently TestWebKitAPI.WebKitLegacy.AudioSessionCategoryIOS will test the fix, but that is not its purpose.

>> Source/WebKitLegacy/mac/WebView/WebView.mm:9338
>> +        // which has to clear an item in _private->fullscreenControllersExiting.
> 
> I don’t understand this comment. I suggest just omitting it.

Got it. Will remove it.

>> Source/WebKitLegacy/mac/WebView/WebView.mm:9340
>> +        _private->fullscreenController = nil;
> 
> What guarantees _private->fullscreenController is not nil here? I don’t understand what limits there are on what HTMLVIdeoElement::exitFullscreen might do. Couldn’t it do some arbitrary set of work that might end up nilling out _private->fullscreenController?
> 
> We can avoid a tiny bit of reference count churn by writing it this way:
> 
>     _private->fullscreenControllersExiting.append(std::exchange(_private->fullscreenController, nil));
> 
> That will move the RetainPtr instead of first copying it and then nulling it out.

The current implementation of HTMLVIdeoElement::exitFullscreen() guarantees the _private->fullscreenController will be changed to nil in a later run loop. The only way to change _private->fullscreenController to nil is -[WebView(WebViewInternal) _exitVideoFullscreen], which will be called in HTMLMediaElement::dispatchEvent() after dispatching the "webkitendfullscreenEvent" event. Therefore, when we call _private->fullscreenControllersExiting.append() here, the _private->fullscreenController should not be nil.

Great idea! I will update the code.

>> Source/WebKitLegacy/mac/WebView/WebView.mm:9365
>>          return;
> 
> This seems a bit fragile. Is it really better to call exitFullscreen first, and *then* remove the first thing in the vector *unconditionally*?
> 
> Local variable name has a misspelling in it: "controler" with one "r". Also if we don’t change the sequence here, I suggest not using a local variable:
> 
>     [_private->fullscreenControllersExiting.first() exitFullscreen];

All the fullscreenControllers in the vector should call exitFullscreen before they can be released. I chose to do it in the FIFO order, the ordering does not matter though. Also, the exitFullscreen function won't change the vector.

Good idea! I will fix it.
Comment 10 Darin Adler 2020-03-31 12:10:30 PDT
Comment on attachment 395004 [details]
Patch

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

>>> Source/WebKitLegacy/mac/WebView/WebView.mm:9365
>>>          return;
>> 
>> This seems a bit fragile. Is it really better to call exitFullscreen first, and *then* remove the first thing in the vector *unconditionally*?
>> 
>> Local variable name has a misspelling in it: "controler" with one "r". Also if we don’t change the sequence here, I suggest not using a local variable:
>> 
>>     [_private->fullscreenControllersExiting.first() exitFullscreen];
> 
> All the fullscreenControllers in the vector should call exitFullscreen before they can be released. I chose to do it in the FIFO order, the ordering does not matter though. Also, the exitFullscreen function won't change the vector.
> 
> Good idea! I will fix it.

Saying they "should call exitFullscreen before they can be released" is one thing, but my point is more about why wouldn’t we move the controller into a RetainPtr local variable *before* calling exitFullscreen. It will still not be released, because the local variable will be there. It would de a "take first item off the vector and process it" model, rather than a "process first item in the vector, then remove first item from the vector" model. Seems much easier to get right, to me.

Something to consider changing later even if we land it like this.

> Source/WebKitLegacy/mac/WebView/WebView.mm:9369
>      [_private->fullscreenController exitFullscreen];
> -    [_private->fullscreenController release];
>      _private->fullscreenController = nil;

Same idea here; I would move this into a local variable first, then call exitFullscreen, for the same reason.

Unless there’s a good reason to not do it this way.
Comment 11 Peng Liu 2020-03-31 12:29:47 PDT
Comment on attachment 395004 [details]
Patch

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

>>>> Source/WebKitLegacy/mac/WebView/WebView.mm:9365
>>>>          return;
>>> 
>>> This seems a bit fragile. Is it really better to call exitFullscreen first, and *then* remove the first thing in the vector *unconditionally*?
>>> 
>>> Local variable name has a misspelling in it: "controler" with one "r". Also if we don’t change the sequence here, I suggest not using a local variable:
>>> 
>>>     [_private->fullscreenControllersExiting.first() exitFullscreen];
>> 
>> All the fullscreenControllers in the vector should call exitFullscreen before they can be released. I chose to do it in the FIFO order, the ordering does not matter though. Also, the exitFullscreen function won't change the vector.
>> 
>> Good idea! I will fix it.
> 
> Saying they "should call exitFullscreen before they can be released" is one thing, but my point is more about why wouldn’t we move the controller into a RetainPtr local variable *before* calling exitFullscreen. It will still not be released, because the local variable will be there. It would de a "take first item off the vector and process it" model, rather than a "process first item in the vector, then remove first item from the vector" model. Seems much easier to get right, to me.
> 
> Something to consider changing later even if we land it like this.

Ah, got your point! Make sense. I will update the patch. Thanks!

>> Source/WebKitLegacy/mac/WebView/WebView.mm:9369
>>      _private->fullscreenController = nil;
> 
> Same idea here; I would move this into a local variable first, then call exitFullscreen, for the same reason.
> 
> Unless there’s a good reason to not do it this way.

Will fix. Thanks!
Comment 12 Peng Liu 2020-03-31 21:59:16 PDT
Created attachment 395128 [details]
Revise the patch based on review comments and add an API test
Comment 13 Eric Carlson 2020-04-01 09:37:16 PDT
Comment on attachment 395128 [details]
Revise the patch based on review comments and add an API test

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

> Source/WebKitLegacy/mac/WebView/WebView.mm:9334
> +        _private->fullscreenControllersExiting.append(std::exchange(_private->fullscreenController, nil));

"fullscreenControllersExiting" is a slightly confusing name. Maybe just "fullscreenControllers" (see comment below).

> Source/WebKitLegacy/mac/WebView/WebView.mm:9352
> +    if (!_private->fullscreenController && _private->fullscreenControllersExiting.isEmpty())

Don't you want `||` here?

> Source/WebKitLegacy/mac/WebView/WebView.mm:9357
> +        auto controller = _private->fullscreenControllersExiting.first();
> +        _private->fullscreenControllersExiting.remove(0);

You could use removeFirst here:

    _private->fullscreenControllersExiting.removeFirst(&controller)

> Source/WebKitLegacy/mac/WebView/WebViewData.h:324
> +    RetainPtr<WebVideoFullscreenController> fullscreenController;
> +    Vector<RetainPtr<WebVideoFullscreenController>> fullscreenControllersExiting;

Do we really need both? Why not just a vector, which will only have one controller in the typical case?
Comment 14 Peng Liu 2020-04-01 10:44:33 PDT
Comment on attachment 395128 [details]
Revise the patch based on review comments and add an API test

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

>> Source/WebKitLegacy/mac/WebView/WebView.mm:9334
>> +        _private->fullscreenControllersExiting.append(std::exchange(_private->fullscreenController, nil));
> 
> "fullscreenControllersExiting" is a slightly confusing name. Maybe just "fullscreenControllers" (see comment below).

The fullscreenControllers in the vector are the ones in the process to exit fullscreen, while the fullscreenController is the one in the fullscreen mode currently.

>> Source/WebKitLegacy/mac/WebView/WebView.mm:9352
>> +    if (!_private->fullscreenController && _private->fullscreenControllersExiting.isEmpty())
> 
> Don't you want `||` here?

The purpose of this function is to ask one fullscreenController in the vector to exit fullscreen if the vector is not empty. If the vector is empty, then ask _private->fullscreenController to exit fullscreen if it is not nil. So we need to use '&&' here.

>> Source/WebKitLegacy/mac/WebView/WebView.mm:9357
>> +        _private->fullscreenControllersExiting.remove(0);
> 
> You could use removeFirst here:
> 
>     _private->fullscreenControllersExiting.removeFirst(&controller)

Right. Is removeFirst more efficient in this case?

>> Source/WebKitLegacy/mac/WebView/WebViewData.h:324
>> +    Vector<RetainPtr<WebVideoFullscreenController>> fullscreenControllersExiting;
> 
> Do we really need both? Why not just a vector, which will only have one controller in the typical case?

We still need a variable to remember which fullscreenController is in fullscreen currently.
Comment 15 Peng Liu 2020-04-01 17:16:56 PDT
*** Bug 209823 has been marked as a duplicate of this bug. ***
Comment 16 Peng Liu 2020-04-02 16:50:53 PDT
Created attachment 395323 [details]
Simplify the page for API test after discussion with Jer and Eric
Comment 17 Alexey Proskuryakov 2020-04-03 12:14:48 PDT
Comment on attachment 395323 [details]
Simplify the page for API test after discussion with Jer and Eric

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

> Source/WebCore/ChangeLog:8
> +        API test: WebKitLegacy.VideoFullscreenStress

This patch should also remove the crashing expectation for media/media-fullscreen-return-to-inline.html.
Comment 18 Peng Liu 2020-04-03 22:09:13 PDT
Comment on attachment 395323 [details]
Simplify the page for API test after discussion with Jer and Eric

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

>> Source/WebCore/ChangeLog:8
>> +        API test: WebKitLegacy.VideoFullscreenStress
> 
> This patch should also remove the crashing expectation for media/media-fullscreen-return-to-inline.html.

Right! Thanks for your reminder.
Will update the patch to do that.
Comment 19 Peng Liu 2020-04-04 01:24:30 PDT
Created attachment 395442 [details]
Patch for landing
Comment 20 Peng Liu 2020-04-04 02:10:28 PDT
Created attachment 395443 [details]
Patch for landing - v2
Comment 21 EWS 2020-04-04 10:06:27 PDT
Committed r259531: <https://trac.webkit.org/changeset/259531>

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