Bug 221886 - Cleanup exit/enterFullScreenHandler and how the global states are set.
Summary: Cleanup exit/enterFullScreenHandler and how the global states are set.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jean-Yves Avenard [:jya]
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-14 21:14 PST by Jean-Yves Avenard [:jya]
Modified: 2021-02-15 09:02 PST (History)
8 users (show)

See Also:


Attachments
Patch (8.20 KB, patch)
2021-02-14 22:16 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>