Bug 200599

Summary: Possible non-thread safe usage of RefCounted in ~VideoFullscreenControllerContext()
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: MediaAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, eric.carlson, ggaren, jer.noble, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 200507    
Attachments:
Description Flags
Patch none

Description Chris Dumez 2019-08-09 16:25:18 PDT
Possible non-thread safe usage of RefCounted in ~VideoFullscreenControllerContext():

ASSERTION FAILED: !areThreadingCheckedEnabled() || m_isOwnedByMainThread == isMainThread()
        /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug-iphonesimulator/usr/local/include/wtf/RefCounted.h(117) : bool WTF::RefCountedBase::derefBase() const
        1   0x1106c60c9 WTFCrash
        2   0x11e5e8dbb WTFCrashWithInfo(int, char const*, char const*, int)
        3   0x11e5fd0be WTF::RefCountedBase::derefBase() const
        4   0x11e6a457f WTF::RefCounted<WebCore::EventListener>::deref() const
        5   0x12248ce25 void WTF::derefIfNotNull<WebCore::PlaybackSessionModelMediaElement>(WebCore::PlaybackSessionModelMediaElement*)
        6   0x12248cde9 WTF::RefPtr<WebCore::PlaybackSessionModelMediaElement, WTF::DumbPtrTraits<WebCore::PlaybackSessionModelMediaElement> >::~RefPtr()
        7   0x12247b215 WTF::RefPtr<WebCore::PlaybackSessionModelMediaElement, WTF::DumbPtrTraits<WebCore::PlaybackSessionModelMediaElement> >::~RefPtr()
        8   0x12247aef5 VideoFullscreenControllerContext::~VideoFullscreenControllerContext()
        9   0x12247b275 VideoFullscreenControllerContext::~VideoFullscreenControllerContext()
        10  0x12247b319 VideoFullscreenControllerContext::~VideoFullscreenControllerContext()
        11  0x12248cd82 WTF::ThreadSafeRefCounted<VideoFullscreenControllerContext, (WTF::DestructionThread)0>::deref() const::'lambda'()::operator()() const
        12  0x12248cd3d WTF::ThreadSafeRefCounted<VideoFullscreenControllerContext, (WTF::DestructionThread)0>::deref() const
        13  0x12248cfc5 void WTF::derefIfNotNull<VideoFullscreenControllerContext>(VideoFullscreenControllerContext*)
        14  0x12248cf89 WTF::RefPtr<VideoFullscreenControllerContext, WTF::DumbPtrTraits<VideoFullscreenControllerContext> >::~RefPtr()
        15  0x12247bbe5 WTF::RefPtr<VideoFullscreenControllerContext, WTF::DumbPtrTraits<VideoFullscreenControllerContext> >::~RefPtr()
        16  0x12248afc5 VideoFullscreenControllerContext::volumeChanged(double)::$_21::~$_21()
        17  0x122480725 VideoFullscreenControllerContext::volumeChanged(double)::$_21::~$_21()
        18  0x122480709 __destroy_helper_block_e8_32c64_ZTSKZN32VideoFullscreenControllerContext13volumeChangedEdE4$_21
        19  0x7fff52a089c2 _Block_release
        20  0x7fff529777f9 _dispatch_client_callout
        21  0x7fff52983d22 _dispatch_main_queue_callback_4CF
        22  0x7fff23b72e19 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__
        23  0x7fff23b6da79 __CFRunLoopRun
        24  0x7fff23b6ce36 CFRunLoopRunSpecific
        25  0x7fff2574803f -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
        26  0x10f6575c6 TestWebKitAPI::Util::run(bool*)
        27  0x10f1f4cd4 TestWebKitAPI::WebKitLegacy_AudioSessionCategoryIOS_Test::TestBody()
        28  0x10f794dee void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        29  0x10f7735eb void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        30  0x10f773516 testing::Test::Run()
        31  0x10f774395 testing::TestInfo::Run()
Comment 1 Chris Dumez 2019-08-09 16:32:36 PDT
Likely the ref'ing of |this| here:
void VideoFullscreenControllerContext::volumeChanged(double volume)
{
    if (WebThreadIsCurrent()) {
        dispatch_async(dispatch_get_main_queue(), [protectedThis = makeRefPtr(this), volume] {
            protectedThis->volumeChanged(volume);
        });
        return;
    }
Comment 2 Chris Dumez 2019-08-09 16:35:49 PDT
So we end up destroying the VideoFullscreenControllerContext on the UIThread, which is fine. However, this ends up deref'ing a PlaybackSessionModelMediaElement on the UIThread, even though PlaybackSessionModelMediaElement objects are WebThread objects (and we're not holding the WebThreadLock).
Comment 3 Chris Dumez 2019-08-09 16:38:15 PDT
This is likely VideoFullscreenControllerContext::m_playbackModel, which is a RefPtr<PlaybackSessionModelMediaElement> and PlaybackSessionModelMediaElement is RefCounted (not ThreadSafeRefCounted).

We could mark it as ThreadSafeRefCounted but I doubt this is the right fix here since we may end up destroying the object on the wrong thread. It is more likely I need to make sure the object gets destroyed on the WebThread, or grab the WebThreadLock.
Comment 4 Chris Dumez 2019-08-09 16:54:30 PDT
Created attachment 375980 [details]
Patch
Comment 5 Geoffrey Garen 2019-08-09 17:14:38 PDT
Comment on attachment 375980 [details]
Patch

r=me
Comment 6 WebKit Commit Bot 2019-08-09 17:56:37 PDT
Comment on attachment 375980 [details]
Patch

Clearing flags on attachment: 375980

Committed r248492: <https://trac.webkit.org/changeset/248492>
Comment 7 WebKit Commit Bot 2019-08-09 17:56:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-08-09 17:57:15 PDT
<rdar://problem/54150479>