Bug 221343 - REGRESSION (r271970): [iOS] ASSERTION FAILED: Completion handler should always be called under WebKit::VideoFullscreenManagerProxy::forEachSession
Summary: REGRESSION (r271970): [iOS] ASSERTION FAILED: Completion handler should alway...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-03 10:45 PST by Ryan Haddad
Modified: 2021-02-05 10:45 PST (History)
12 users (show)

See Also:


Attachments
Patch (1.83 KB, patch)
2021-02-03 12:26 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (6.91 KB, patch)
2021-02-03 13:05 PST, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Haddad 2021-02-03 10:45:23 PST
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
Comment 1 Ryan Haddad 2021-02-03 10:47:14 PST
This seems to have started with https://trac.webkit.org/changeset/271970/webkit
Comment 2 Radar WebKit Bug Importer 2021-02-03 10:47:31 PST
<rdar://problem/73939450>
Comment 3 Kate Cheney 2021-02-03 12:26:46 PST
Created attachment 419171 [details]
Patch
Comment 4 Chris Dumez 2021-02-03 12:30:04 PST
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..
Comment 5 Kate Cheney 2021-02-03 13:05:07 PST
Created attachment 419178 [details]
Patch
Comment 6 Kate Cheney 2021-02-03 13:06:34 PST
(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.
Comment 7 Peng Liu 2021-02-03 13:21:26 PST
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();
```
Comment 8 Peng Liu 2021-02-03 13:22:56 PST
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! :-)
Comment 9 youenn fablet 2021-02-05 10:16:02 PST
Comment on attachment 419178 [details]
Patch

LGTM.
Maybe an enum could be used for finishedMedia in a follow-up.
Comment 10 EWS 2021-02-05 10:39:21 PST
commit-queue failed to commit attachment 419178 [details] to WebKit repository. To retry, please set cq+ flag again.
Comment 11 EWS 2021-02-05 10:45:00 PST
Committed r272425: <https://trac.webkit.org/changeset/272425>

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