Bug 209680

Summary:

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

Product:

WebKit

Reporter:

Ryan Haddad <ryanhaddad>

Component:

New Bugs

Assignee:

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:

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

Ryan Haddad

Reported 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

Attachments
Patch (5.27 KB, patch) 2020-03-27 18:20 PDT, Peng Liu	no flags	Details Formatted Diff Diff
Patch (7.53 KB, patch) 2020-03-30 19:10 PDT, Peng Liu	no flags	Details Formatted Diff Diff
Revise the patch based on review comments and add an API test (21.73 KB, patch) 2020-03-31 21:59 PDT, Peng Liu	no flags	Details Formatted Diff Diff
Simplify the page for API test after discussion with Jer and Eric (21.25 KB, patch) 2020-04-02 16:50 PDT, Peng Liu	jer.noble: review+	Details Formatted Diff Diff
Patch for landing (23.48 KB, patch) 2020-04-04 01:24 PDT, Peng Liu	no flags	Details Formatted Diff Diff
Patch for landing - v2 (22.95 KB, patch) 2020-04-04 02:10 PDT, Peng Liu	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2020-03-27 13:46:54 PDT

<rdar://problem/60983033>

Ryan Haddad

Comment 2 2020-03-27 13:48:37 PDT

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

Peng Liu

Comment 3 2020-03-27 18:20:34 PDT

Created attachment 394776 [details] Patch

Daniel Bates

Comment 4 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?

Peng Liu

Comment 5 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.

Peng Liu

Comment 6 2020-03-30 19:10:14 PDT

Created attachment 395004 [details] Patch

Darin Adler

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

Eric Carlson

Comment 8 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.

Peng Liu

Comment 9 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.

Darin Adler

Comment 10 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.

Peng Liu

Comment 11 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!

Peng Liu

Comment 12 2020-03-31 21:59:16 PDT

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

Eric Carlson

Comment 13 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?

Peng Liu

Comment 14 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.

Peng Liu

Comment 15 2020-04-01 17:16:56 PDT

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

Peng Liu

Comment 16 2020-04-02 16:50:53 PDT

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

Alexey Proskuryakov

Comment 17 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.

Peng Liu

Comment 18 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.

Peng Liu

Comment 19 2020-04-04 01:24:30 PDT

Created attachment 395442 [details] Patch for landing

Peng Liu

Comment 20 2020-04-04 02:10:28 PDT

Created attachment 395443 [details] Patch for landing - v2

EWS

Comment 21 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].

Note You need to log in before you can comment on or make changes to this bug.