Bug 221886

Summary:

Cleanup exit/enterFullScreenHandler and how the global states are set.

Product:

WebKit

Reporter:

Jean-Yves Avenard [:jya] <jean-yves.avenard>

Component:

Media

Assignee:

Jean-Yves Avenard [:jya] <jean-yves.avenard>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=221863

Attachments:

Description	Flags
Patch	none

Description Jean-Yves Avenard [:jya] 2021-02-14 21:14:43 PST

This is a follow-up to bug 221863 that saw crashes due to an infinite recursion happening when entering/exiting full screen.

The crash occurs when VideoFullscreenInterfaceAVKit::doEnterFullscreen is called, attempts to enter full screen by calling [AVPlayerViewController _transitionFromFullScreenAnimated] which errors causing doEnterFullScreen to be called again.


```
Thread 0 name:  Dispatch queue: com.apple.main-thread
Thread 0 Crashed ↩:
0   libsystem_trace.dylib         	0x00000001a0fc92fc _os_log_fmt_flatten_to_blob + 68 (format.c:739)
1   libsystem_trace.dylib         	0x00000001a0fce608 _os_log_impl_flatten_and_send + 2188 (format.c:839)
2   libsystem_trace.dylib         	0x00000001a0fce608 _os_log_impl_flatten_and_send + 2188 (format.c:839)
3   libsystem_trace.dylib         	0x00000001a0fd098c _os_log_impl_dynamic + 316 (log.c:2346)
4   libsystem_trace.dylib         	0x00000001a0fd0db4 _os_log_with_args_impl + 160 (log.c:2611)
5   JavaScriptCore                	0x0000000195090d9c vprintf_stderr_common(char const*, char*) + 296 (Assertions.cpp:152)
6   JavaScriptCore                	0x000000019508f6e0 vprintf_stderr_with_trailing_newline(char const*, char*) + 284 (Assertions.cpp:204)
7   JavaScriptCore                	0x000000019508f794 WTFLogAlways + 28 (Assertions.cpp:478)
8   WebCore                       	0x0000000199749a80 WebCore::VideoFullscreenInterfaceAVKit::enterFullscreenHandler(bool, NSError*) + 68 (VideoFullscreenInterfaceAVKit.mm:1573)
9   AVKit                         	0x00000001a0696ff8 -[AVPlayerViewController _transitionToFullScreenAnimated:interactive:completionHandler:] + 388 (AVPlayerViewController_Mobile.m:2066)
10  AVKit                         	0x00000001a069d418 -[AVPlayerViewController(AVPlayerViewController_WebKitOnly) enterFullScreenAnimated:completionHandler:] + 884 (AVPlayerViewController_Mobile.m:2848)
11  WebCore                       	0x00000001997488d8 WebCore::VideoFullscreenInterfaceAVKit::doEnterFullscreen() + 212 (VideoFullscreenInterfaceAVKit.mm:1445)
12  AVKit                         	0x00000001a0696ff8 -[AVPlayerViewController _transitionToFullScreenAnimated:interactive:completionHandler:] + 388 (AVPlayerViewController_Mobile.m:2066)
13  AVKit                         	0x00000001a069d418 -[AVPlayerViewController(AVPlayerViewController_WebKitOnly) enterFullScreenAnimated:completionHandler:] + 884 (AVPlayerViewController_Mobile.m:2848)
14  WebCore                       	0x00000001997488d8 WebCore::VideoFullscreenInterfaceAVKit::doEnterFullscreen() + 212 (VideoFullscreenInterfaceAVKit.mm:1445)
15  AVKit                         	0x00000001a0696ff8 -[AVPlayerViewController _transitionToFullScreenAnimated:interactive:completionHandler:] + 388 (AVPlayerViewController_Mobile.m:2066)
16  AVKit                         	0x00000001a069d418 -[AVPlayerViewController(AVPlayerViewController_WebKitOnly) enterFullScreenAnimated:completionHandler:] + 884 (AVPlayerViewController_Mobile.m:2848)
17  WebCore                       	0x00000001997488d8 WebCore::VideoFullscreenInterfaceAVKit::doEnterFullscreen() + 212 (VideoFullscreenInterfaceAVKit.mm:1445)
18  AVKit                         	0x00000001a0696ff8 -[AVPlayerViewController _transitionToFullScreenAnimated:interactive:completionHandler:] + 388 (AVPlayerViewController_Mobile.m:2066)
19  AVKit                         	0x00000001a069d418 -[AVPlayerViewController(AVPlayerViewController_WebKitOnly) enterFullScreenAnimated:completionHandler:] + 884 (AVPlayerViewController_Mobile.m:2848)
20  WebCore                       	0x00000001997488d8 WebCore::VideoFullscreenInterfaceAVKit::doEnterFullscreen() + 212 (VideoFullscreenInterfaceAVKit.mm:1445)
21  AVKit                         	0x00000001a0696ff8 -[AVPlayerViewController _transitionToFullScreenAnimated:interactive:completionHandler:] + 388 (AVPlayerViewController_Mobile.m:2066)
22  AVKit                         	0x00000001a069d418 -[AVPlayerViewController(AVPlayerViewController_WebKitOnly) enterFullScreenAnimated:completionHandler:] + 884 (AVPlayerViewController_Mobile.m:2848)
23  WebCore                       	0x00000001997488d8 WebCore::VideoFullscreenInterfaceAVKit::doEnterFullscreen() + 212 (VideoFullscreenInterfaceAVKit.mm:1445)
24  AVKit                         	0x00000001a0696ff8 -[AVPlayerViewController _transitionToFullScreenAnimated:interactive:completionHandler:] + 388 (AVPlayerViewController_Mobile.m:2066)
25  AVKit                         	0x00000001a069d418 -[AVPlayerViewController(AVPlayerViewController_WebKitOnly) enterFullScreenAnimated:completionHandler:] + 884 (AVPlayerViewController_Mobile.m:2848)
26  WebCore                       	0x00000001997488d8 WebCore::VideoFullscreenInterfaceAVKit::doEnterFullscreen() + 212 (VideoFullscreenInterfaceAVKit.mm:1445)
27  AVKit                         	0x00000001a0696ff8 -[AVPlayerViewController _transitionToFullScreenAnimated:interactive:completionHandler:] + 388 (AVPlayerViewController_Mobile.m:2066)
28  AVKit                         	0x00000001a069d418 -[AVPlayerViewController(AVPlayerViewController_WebKitOnly) enterFullScreenAnimated:completionHandler:] + 884 (AVPlayerViewController_Mobile.m:2848)
29  WebCore                       	0x00000001997488d8 WebCore::VideoFullscreenInterfaceAVKit::doEnterFullscreen() + 212 (VideoFullscreenInterfaceAVKit.mm:1445)
30  AVKit                         	0x00000001a0696ff8 -[AVPlayerViewController _transitionToFullScreenAnimated:interactive:completionHandler:] + 388 (AVPlayerViewController_Mobile.m:2066)
31  AVKit                         	0x00000001a069d418 -[AVPlayerViewController(AVPlayerViewController_WebKitOnly) enterFullScreenAnimated:completionHandler:] + 884 (AVPlayerViewController_Mobile.m:2848)
32  WebCore                       	0x00000001997488d8 WebCore::VideoFullscreenInterfaceAVKit::doEnterFullscreen() + 212 (VideoFullscreenInterfaceAVKit.mm:1445)
33  AVKit                         	0x00000001a0696ff8 -[AVPlayerViewController _transitionToFullScreenAnimated:interactive:completionHandler:] + 388 (AVPlayerViewController_Mobile.m:2066)
34  AVKit                         	0x00000001a069d418 -[AVPlayerViewController(AVPlayerViewController_WebKitOnly) enterFullScreenAnimated:completionHandler:] + 884 (AVPlayerViewController_Mobile.m:2848)
35  WebCore                       	0x00000001997488d8 WebCore::VideoFullscreenInterfaceAVKit::doEnterFullscreen() + 212 (VideoFullscreenInterfaceAVKit.mm:1445)
...
```

As described in bug 221863 comment 3, when switching from fullscreen to PiP; I see the following stack trace occurring:
```
    frame #0: 0x0000000109c4c734 WebCore`WebCore::VideoFullscreenInterfaceAVKit::exitFullscreenHandler(this=0x0000000112e85c18, success=NO, error=domain: "AVKitErrorDomain" - code: 0) at VideoFullscreenInterfaceAVKit.mm:1553:115 [opt]
    frame #1: 0x00000001b9300074 AVKit`-[AVPlayerViewController _transitionFromFullScreenAnimated:interactive:completionHandler:] + 484
    frame #2: 0x00000001b930bfa8 AVKit`-[AVPlayerViewController(AVPlayerViewController_WebKitOnly) exitFullScreenAnimated:completionHandler:] + 1196
    frame #3: 0x0000000109c4b7c0 WebCore`WebCore::VideoFullscreenInterfaceAVKit::doEnterFullscreen(this=0x0000000112e85c18) at VideoFullscreenInterfaceAVKit.mm:1474:9 [opt]
    frame #4: 0x0000000105cafe10 WebKit`WebKit::VideoFullscreenManagerProxy::enterFullscreen(this=0x0000000112eb9448, contextId=(m_identifier = 353)) at VideoFullscreenManagerProxy.mm:603:15 [opt]
    frame #5: 0x0000000103ea3f70 JavaScriptCore`WTF::RunLoop::performWork() [inlined] WTF::Function<void ()>::operator(this=<unavailable>)() const at Function.h:83:35 [opt]
    frame #6: 0x0000000103ea3f5c JavaScriptCore`WTF::RunLoop::performWork(this=0x0000000112edc000) at RunLoop.cpp:128 [opt]
  * frame #7: 0x0000000103ea49f4 JavaScriptCore`WTF::RunLoop::performWork(context=<unavailable>) at RunLoopCF.cpp:46:37 [opt]
    frame #8: 0x00000001a2ddc858 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 28
    frame #9: 0x00000001a2ddc754 CoreFoundation`__CFRunLoopDoSource0 + 208
    frame #10: 0x00000001a2ddba58 CoreFoundation`__CFRunLoopDoSources0 + 268
    frame #11: 0x00000001a2dd5a38 CoreFoundation`__CFRunLoopRun + 820
    frame #12: 0x00000001a2dd51d0 CoreFoundation`CFRunLoopRunSpecific + 600
    frame #13: 0x00000001baa4a734 GraphicsServices`GSEventRunModal + 164
    frame #14: 0x00000001a583bfb8 UIKitCore`-[UIApplication _run] + 1072
    frame #15: 0x00000001a5841828 UIKitCore`UIApplicationMain + 168
    frame #16: 0x0000000102ca535c MobileSafari`main(argc=<unavailable>, argv=<unavailable>) at main.m:123:5 [opt]
    frame #17: 0x00000001a2a91d54 libdyld.dylib`start + 4
```

For some reasons when going into PiP mode from fullscreen, we get a call to -[AVPlayerViewController _transitionFromFullScreenAnimated:interactive:completionHandler:] which will error.
I have a working theory on how bug 221863 could occur: if that method got to get called while we were still in the middle of entering full screen, we would have VideoFullscreenInterfaceAVKit::m_enterFullscreenNeedsEnterFullscreen set to true as the handler is called asynchronously.

The aim of that boolean is to have enterFullScreenHandler set to call again doEnterFullScreen; however it is possible that it is set when the new call to WebKit::VideoFullscreenManagerProxy::enterFullscreen occurs; this would erroneously cause the recursion.

Using member variables to control how a future method will behave is dangerous in asynchronous mode. By limiting the scope of that state to only that specific callback instance we will prevent the above case to occur.

Comment 1 Jean-Yves Avenard [:jya] 2021-02-14 21:16:43 PST

Forgot to add in the description above (and I unfortunately don't have edit rights): I have the strong suspicion than with the crash report attached to bug 221863, those stack traces are truncated. And that the original called was WebKit::VideoFullscreenManagerProxy::enterFullscreen
following use of PiP

Comment 2 Jean-Yves Avenard [:jya] 2021-02-14 22:16:10 PST

Created attachment 420270 [details]
Patch

Comment 3 Jean-Yves Avenard [:jya] 2021-02-15 01:49:44 PST

Comment on attachment 420270 [details]
Patch

This is ios code only

Comment 4 Darin Adler 2021-02-15 08:43:00 PST

Comment on attachment 420270 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420270&action=review

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:236
> +    void exitFullscreenHandler(BOOL success, NSError *, NextActions = NextActions());
> +    void enterFullscreenHandler(BOOL success, NSError *, NextActions = NextActions());

Could consider { } instead of NextActions()

Comment 5 EWS 2021-02-15 09:01:25 PST

Committed r272853: <https://commits.webkit.org/r272853>

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

Comment 6 Radar WebKit Bug Importer 2021-02-15 09:02:16 PST

<rdar://problem/74351997>