REGRESSION (r271970): [iOS] ASSERTION FAILED: Completion handler should always be called under WebKit::VideoFullscreenManagerProxy::forEachSession

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

Seeing the following test consistently assert on iOS debug bots:

    TestWebKitAPI.WKWebViewCloseAllMediaPresentations.VideoFullscreen
        ASSERTION FAILED: Completion handler should always be called
        !m_function
        /Volumes/Data/slave/ios-simulator-14-debug/build/WebKitBuild/Debug-iphonesimulator/usr/local/include/wtf/CompletionHandler.h(58) : WTF::CompletionHandler<void ()>::~CompletionHandler()
        1   0x1119776e9 WTFCrash
        2   0x1197453f7 WTF::CompletionHandler<void ()>::~CompletionHandler()
        3   0x1197429a5 WTF::CompletionHandler<void ()>::~CompletionHandler()
        4   0x11a5a9728 auto -[WKWebView closeAllMediaPresentations:]::$_4::operator()<WebKit::VideoFullscreenModelContext, WebCore::VideoFullscreenInterfaceAVKit>(WebKit::VideoFullscreenModelContext&, WebCore::VideoFullscreenInterfaceAVKit&)
        5   0x11a5a9618 WTF::Detail::CallableWrapper<-[WKWebView closeAllMediaPresentations:]::$_4, void, WebKit::VideoFullscreenModelContext&, WebCore::VideoFullscreenInterfaceAVKit&>::call(WebKit::VideoFullscreenModelContext&, WebCore::VideoFullscreenInterfaceAVKit&)
        6   0x11a711fb7 WTF::Function<void (WebKit::VideoFullscreenModelContext&, WebCore::VideoFullscreenInterfaceAVKit&)>::operator()(WebKit::VideoFullscreenModelContext&, WebCore::VideoFullscreenInterfaceAVKit&) const
        7   0x11a711b82 WebKit::VideoFullscreenManagerProxy::forEachSession(WTF::Function<void (WebKit::VideoFullscreenModelContext&, WebCore::VideoFullscreenInterfaceAVKit&)>&&)
        8   0x11a573a65 -[WKWebView closeAllMediaPresentations:]
        9   0x10e549b60 WKWebViewCloseAllMediaPresentations_VideoFullscreen_Test::TestBody()
        10  0x10e5e1d84 void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        11  0x10e5bedeb void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        12  0x10e5bed26 testing::Test::Run()
        13  0x10e5bfb5a testing::TestInfo::Run()
        14  0x10e5c0984 testing::TestCase::Run()
        15  0x10e5cb7f8 testing::internal::UnitTestImpl::RunAllTests()
        16  0x10e5e6454 bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)
        17  0x10e5cb30b bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)
        18  0x10e5cb1dd testing::UnitTest::Run()
        19  0x10e3fc821 RUN_ALL_TESTS()
        20  0x10e3fc7b2 TestWebKitAPI::TestsController::run(int, char**)
        21  0x10e591d8d main
        22  0x147002409 start
        23  0x2
        Child process terminated with signal 11: Segmentation fault

https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.WKWebViewCloseAllMediaPresentations.VideoFullscreen

[Ryan Haddad]

This seems to have started with https://trac.webkit.org/changeset/271970/webkit

[Radar WebKit Bug Importer]

<rdar://problem/73939450>

[Kate Cheney]

Created attachment 419171 [details]
Patch

[Chris Dumez]

Comment on attachment 419171 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:756
> +        completionHandler();

Seems odd to call the completion handler before calling requestFullscreenMode..

[Kate Cheney]

Created attachment 419178 [details]
Patch

[Kate Cheney]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 419171 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419171&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:756
> > +        completionHandler();
> 
> Seems odd to call the completion handler before calling
> requestFullscreenMode..

You’re right. That got me thinking that I should change the name also because requestFullscreenModeWithCallback makes it sound like the completion handler works for cases other than requesting VideoFullscreenModeNone, but it does not.

[Peng Liu]

Comment on attachment 419171 [details]
Patch

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

>>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:756
>>> +        completionHandler();
>> 
>> Seems odd to call the completion handler before calling requestFullscreenMode..
> 
> You’re right. That got me thinking that I should change the name also because requestFullscreenModeWithCallback makes it sound like the completion handler works for cases other than requesting VideoFullscreenModeNone, but it does not.

Maybe we can reorganize the code to avoid the oddness. Like below:
```
if (...) {
    m_closeCompletionHandlers.append(WTFMove(completionHandler));
    requestFullscreenMode(contextId, mode, finishedWithMedia);
    return;
}

completionHandler();
```

[Peng Liu]

Comment on attachment 419171 [details]
Patch

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

>>>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:756
>>>> +        completionHandler();
>>> 
>>> Seems odd to call the completion handler before calling requestFullscreenMode..
>> 
>> You’re right. That got me thinking that I should change the name also because requestFullscreenModeWithCallback makes it sound like the completion handler works for cases other than requesting VideoFullscreenModeNone, but it does not.
> 
> Maybe we can reorganize the code to avoid the oddness. Like below:
> ```
> if (...) {
>     m_closeCompletionHandlers.append(WTFMove(completionHandler));
>     requestFullscreenMode(contextId, mode, finishedWithMedia);
>     return;
> }
> 
> completionHandler();
> ```

Oh, just saw the new patch! :-)

[youenn fablet]

Comment on attachment 419178 [details]
Patch

LGTM.
Maybe an enum could be used for finishedMedia in a follow-up.

[EWS]

commit-queue failed to commit attachment 419178 [details] to WebKit repository. To retry, please set cq+ flag again.

[EWS]

Committed r272425: <https://trac.webkit.org/changeset/272425>

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