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: | 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
Ryan Haddad
2020-03-27 13:46:41 PDT
I think this may have started with https://trac.webkit.org/changeset/259095/webkit Created attachment 394776 [details]
Patch
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? (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. Created attachment 395004 [details]
Patch
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 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 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 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 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! Created attachment 395128 [details]
Revise the patch based on review comments and add an API test
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 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. *** Bug 209823 has been marked as a duplicate of this bug. *** Created attachment 395323 [details]
Simplify the page for API test after discussion with Jer and Eric
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 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. Created attachment 395442 [details]
Patch for landing
Created attachment 395443 [details]
Patch for landing - v2
Committed r259531: <https://trac.webkit.org/changeset/259531> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395443 [details]. |