Bug 216287 – Returning to element fullscreen from PiP is not stable under stress tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

Return to element fullscreen from PiP is not stable under stress tests

[Radar WebKit Bug Importer]

<rdar://problem/68533983>

[Peng Liu]

Created attachment 408284 [details]
Patch

[Peng Liu]

Created attachment 408510 [details]
Fix an api test failure on Mac

[Jer Noble]

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?

[Peng Liu]

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.

[Jer Noble]

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

[Peng Liu]

Created attachment 408739 [details]
Update the patch based on Jer's comments

[Peng Liu]

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!

[EWS]

Committed r267053: <https://trac.webkit.org/changeset/267053>

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