Return to element fullscreen from PiP is not stable under stress tests
<rdar://problem/68533983>
Created attachment 408284 [details] Patch
Created attachment 408510 [details] Fix an api test failure on Mac
Comment on attachment 408510 [details] Fix an api test failure on Mac View in context: https://bugs.webkit.org/attachment.cgi?id=408510&action=review > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mm:700 > + videoFullscreenManager->setClient(&_videoFullscreenManagerProxyClient); Here. > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mm:851 > + videoFullscreenManager->setClient(nullptr); And here. > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mm:932 > + if (![self isFullScreen]) { > + if (auto* videoFullscreenManager = self._videoFullscreenManager) > + videoFullscreenManager->setClient(nullptr); > + } And here. > Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:655 > + if (auto* videoFullscreenManager = self._videoFullscreenManager) > + videoFullscreenManager->setClient(nullptr); And here. > Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:691 > + if (auto* videoFullscreenManager = self._videoFullscreenManager) > + videoFullscreenManager->setClient(&_videoFullscreenManagerProxyClient); And here. What will happen if some other piece of code reset videoFullScreenManager's client, perhaps by requesting fullscreen before this animation is complete? VideoFullscreenManager is a singleton, so would that cause us to get into a bad state in the manager's state machine?
Comment on attachment 408510 [details] Fix an api test failure on Mac View in context: https://bugs.webkit.org/attachment.cgi?id=408510&action=review >> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mm:851 >> + videoFullscreenManager->setClient(nullptr); > > And here. I guess it is better to move this section out of the _repaintCallback? >> Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:691 >> + videoFullscreenManager->setClient(&_videoFullscreenManagerProxyClient); > > And here. > > What will happen if some other piece of code reset videoFullScreenManager's client, perhaps by requesting fullscreen before this animation is complete? VideoFullscreenManager is a singleton, so would that cause us to get into a bad state in the manager's state machine? FullscreenManager has the logic to ignore a new fullscreen mode changing request if the current request is not completed yet. Also, WKFullScreenWindowControllerVideoFullscreenManagerProxyClient is the only one client of VideoFullscreenManagerProxy, and only WKFullScreenWindowController calls VideoFullscreenManagerProxy::setClient(). So I believe the code is safe.
(In reply to Peng Liu from comment #5) > Comment on attachment 408510 [details] > Fix an api test failure on Mac > > View in context: > https://bugs.webkit.org/attachment.cgi?id=408510&action=review > > >> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mm:851 > >> + videoFullscreenManager->setClient(nullptr); > > > > And here. > > I guess it is better to move this section out of the _repaintCallback? > > >> Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:691 > >> + videoFullscreenManager->setClient(&_videoFullscreenManagerProxyClient); > > > > And here. > > > > What will happen if some other piece of code reset videoFullScreenManager's client, perhaps by requesting fullscreen before this animation is complete? VideoFullscreenManager is a singleton, so would that cause us to get into a bad state in the manager's state machine? > > FullscreenManager has the logic to ignore a new fullscreen mode changing > request if the current request is not completed yet. Also, > WKFullScreenWindowControllerVideoFullscreenManagerProxyClient is the only > one client of VideoFullscreenManagerProxy, and only > WKFullScreenWindowController calls VideoFullscreenManagerProxy::setClient(). > So I believe the code is safe. Might be worthwhile to put some ASSERT() statements around the setClient() calls, to catch this case should things ever go wrong. I.e., assert that client is NULL when set, and that it's `this` when cleared.
Created attachment 408739 [details] Update the patch based on Jer's comments
Comment on attachment 408510 [details] Fix an api test failure on Mac View in context: https://bugs.webkit.org/attachment.cgi?id=408510&action=review >>>> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mm:851 >>>> + videoFullscreenManager->setClient(nullptr); >>> >>> And here. >> >> I guess it is better to move this section out of the _repaintCallback? > > Might be worthwhile to put some ASSERT() statements around the setClient() calls, to catch this case should things ever go wrong. I.e., assert that client is NULL when set, and that it's `this` when cleared. Good point!
Committed r267053: <https://trac.webkit.org/changeset/267053> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408739 [details].