[iOS][WK1] Unsafe unsafe of WeakPtr<Document> from UIThread under PlaybackSessionInterfaceAVKit::PlaybackSessionInterfaceAVKit()

[Chris Dumez]

Unsafe unsafe of WeakPtr<Document> from UIThread under PlaybackSessionInterfaceAVKit::PlaybackSessionInterfaceAVKit():

TestWebKitAPI.WebKitLegacy.AudioSessionCategoryIOS
        ASSERTION FAILED: Thread::mayBeGCThread() || m_wasConstructedOnMainThread == isMainThread()
        /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug-iphonesimulator/usr/local/include/wtf/WeakPtr.h(65) : typename T::WeakValueType *WTF::WeakPtrImpl::get() [T = WebCore::Document]
        1   0x10f4ded09 WTFCrash
        2   0x11c87d34b WTFCrashWithInfo(int, char const*, char const*, int)
        3   0x11df066cc WebCore::Document::WeakValueType* WTF::WeakPtrImpl::get<WebCore::Document>()
        4   0x11df06617 WTF::WeakPtr<WebCore::Document>::get() const
        5   0x11df686c5 WTF::WeakPtr<WebCore::Document>::operator->() const
        6   0x11f6fe626 WebCore::Quirks::needsQuirks() const
        7   0x11f6fe799 WebCore::Quirks::needsSeekingSupportDisabled() const
        8   0x11ef28c7f WebCore::HTMLMediaElement::supportsSeeking() const
        9   0x11ceb0991 WebCore::PlaybackSessionModelMediaElement::duration() const
        10  0x1206fbce5 VideoFullscreenControllerContext::duration() const
        11  0x11cbbf20d WebCore::PlaybackSessionInterfaceAVKit::PlaybackSessionInterfaceAVKit(WebCore::PlaybackSessionModel&)
        12  0x11cbbf89d WebCore::PlaybackSessionInterfaceAVKit::PlaybackSessionInterfaceAVKit(WebCore::PlaybackSessionModel&)
        13  0x120702811 WebCore::PlaybackSessionInterfaceAVKit::create(WebCore::PlaybackSessionModel&)
        14  0x1206fd5c2 VideoFullscreenControllerContext::setUpFullscreen(WebCore::HTMLVideoElement&, UIView*, unsigned int)::$_42::operator()() const
        15  0x1206fd529 invocation function for block in VideoFullscreenControllerContext::setUpFullscreen(WebCore::HTMLVideoElement&, UIView*, unsigned int)
        16  0x7fff52661888 _dispatch_call_block_and_release
        17  0x7fff526627f9 _dispatch_client_callout
        18  0x7fff5266ed22 _dispatch_main_queue_callback_4CF
        19  0x7fff23b6de19 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__
        20  0x7fff23b68a79 __CFRunLoopRun
        21  0x7fff23b67e36 CFRunLoopRunSpecific
        22  0x7fff2573d26f -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
        23  0x10adf0e46 TestWebKitAPI::Util::run(bool*)
        24  0x10a98f4f0 TestWebKitAPI::WebKitLegacy_AudioSessionCategoryIOS_Test::TestBody()
        25  0x10af2c56e void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        26  0x10af0ad6b void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        27  0x10af0ac96 testing::Test::Run()
        28  0x10af0bb15 testing::TestInfo::Run()
        29  0x10af0c9cf testing::TestCase::Run()
        30  0x10af17d66 testing::internal::UnitTestImpl::RunAllTests()
        31  0x10af3071e bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)
        Child process terminated with signal 11: Segmentation fault

[Chris Dumez]

Created attachment 375268 [details]
Patch

[Ryosuke Niwa]

I think the bug here is that VideoFullscreenControllerContext::setUpFullscreen isn't grabbing the web thread lock. If the web thread lock is held, isMainThread() should return true.

[Joseph Pecoraro]

(In reply to Ryosuke Niwa from comment #2)
> I think the bug here is that
> VideoFullscreenControllerContext::setUpFullscreen isn't grabbing the web
> thread lock. If the web thread lock is held, isMainThread() should return
> true.

Indeed.

Source/WebCore/platform/iOS/WebVideoFullscreenControllerAVKit.mm: `VideoFullscreenControllerContext::setUpFullscreen` has:

    dispatch_async(dispatch_get_main_queue(), [protectedThis = makeRefPtr(this), this, videoElementClientRect, viewRef, mode, allowsPictureInPicture] {
        ASSERT(isUIThread());

        Ref<PlaybackSessionInterfaceAVKit> sessionInterface = PlaybackSessionInterfaceAVKit::create(*this);
        m_interface = VideoFullscreenInterfaceAVKit::create(sessionInterface.get());
        m_interface->setVideoFullscreenChangeObserver(this);
        m_interface->setVideoFullscreenModel(this);
        m_interface->setVideoFullscreenChangeObserver(this);

        m_videoFullscreenView = adoptNS([PAL::allocUIViewInstance() init]);

        m_interface->setupFullscreen(*m_videoFullscreenView.get(), videoElementClientRect, viewRef.get(), mode, allowsPictureInPicture, false);
    });


The block may be doing all kinds of things inside of WebCore on the UIThread without a WebThreadLock which could be unsafe. Plugging a single case (HTMLMediaElement::supportsSeeking) may just hide this issue until another issue later on. It would be better if we were able to use `WebThreadRun` or just took a `WebThreadLock` in here. It looks like this file already makes heavy use of `WebThreadRun` so maybe we can use that!

I find this file rather confusing as the entire file is guarded by `#if PLATFORM(IOS_FAMILY)` but there are many equivalent feature guards within the file. Seems the sub-cases are stale and could simplify the file quite a bit.

[Chris Dumez]

Created attachment 375282 [details]
Patch

[Chris Dumez]

Thanks for the feedback, I am super unfamiliar with the WebThread lock. I tried it and it fixes the crash nicely.

[WebKit Commit Bot]

Comment on attachment 375282 [details]
Patch

Clearing flags on attachment: 375282

Committed r248101: <https://trac.webkit.org/changeset/248101>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53812730>