Bug 209680 – REGRESSION (r259095): ASSERTION FAILED: m_videoFullscreenMode != VideoFullscreenModeNone seen with TestWebKitAPI.WebKitLegacy.AudioSessionCategoryIOS

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ryan Haddad]

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

[Radar WebKit Bug Importer]

<rdar://problem/60983033>

[Ryan Haddad]

I think this may have started with https://trac.webkit.org/changeset/259095/webkit

[Peng Liu]

Created attachment 394776 [details]
Patch

[Daniel Bates]

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?

[Peng Liu]

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

[Peng Liu]

Created attachment 395004 [details]
Patch

[Darin Adler]

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

[Eric Carlson]

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.

[Peng Liu]

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.

[Darin Adler]

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.

[Peng Liu]

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!

[Peng Liu]

Created attachment 395128 [details]
Revise the patch based on review comments and add an API test

[Eric Carlson]

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?

[Peng Liu]

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.

[Peng Liu]

*** Bug 209823 has been marked as a duplicate of this bug. ***

[Peng Liu]

Created attachment 395323 [details]
Simplify the page for API test after discussion with Jer and Eric

[Alexey Proskuryakov]

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.

[Peng Liu]

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.

[Peng Liu]

Created attachment 395442 [details]
Patch for landing

[Peng Liu]

Created attachment 395443 [details]
Patch for landing - v2

[EWS]

Committed r259531: <https://trac.webkit.org/changeset/259531>

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