Possible non-thread safe usage of RefCounted in ~VideoFullscreenControllerContext()

[Chris Dumez]

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()

[Chris Dumez]

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;
    }

[Chris Dumez]

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).

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 375980 [details]
Patch

[Geoffrey Garen]

Comment on attachment 375980 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 375980 [details]
Patch

Clearing flags on attachment: 375980

Committed r248492: <https://trac.webkit.org/changeset/248492>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54150479>