Bug 228932 – ThreadSanitizer: data race in WTF::StringImpl::deref() under WebKit::WebProcessPool::setMediaAccessibilityPreferences()

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

ThreadSanitizer: data race in WTF::StringImpl::deref() under WebKit::WebProcessPool::setMediaAccessibilityPreferences().

In Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm, the following code sends a Vector<WTF::String> object from a background thread to the main thread without an isolated copy of the WTF::String objects:

#if HAVE(MEDIA_ACCESSIBILITY_FRAMEWORK)
void WebProcessPool::setMediaAccessibilityPreferences(WebProcessProxy& process)
{
    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), [weakProcess = makeWeakPtr(process)] {
        auto captionDisplayMode = WebCore::CaptionUserPreferencesMediaAF::platformCaptionDisplayMode();
        auto preferredLanguages = WebCore::CaptionUserPreferencesMediaAF::platformPreferredLanguages();
        callOnMainRunLoop([weakProcess, captionDisplayMode, preferredLanguages] {
            if (weakProcess)
                weakProcess->send(Messages::WebProcess::SetMediaAccessibilityPreferences(captionDisplayMode, preferredLanguages), 0);
        });
    });
}
#endif

--

Thread sanitizer warning while running LayoutTests/crypto/crypto-random-values-types.html with WebKit r280760:

WARNING: ThreadSanitizer: data race (pid=46345)
  Read of size 4 at 0x7b080000ea00 by main thread:
    #0 WTF::StringImpl::deref() <null> (WebKit:x86_64+0x7aca)
    #1 WTF::VectorDestructor<true, WTF::String>::destruct(WTF::String*, WTF::String*) <null> (WebKit:x86_64+0x9867)
    #2 WTF::VectorTypeOperations<WTF::String>::destruct(WTF::String*, WTF::String*) <null> (WebKit:x86_64+0x97b0)
    #3 WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::~Vector() <null> (WebKit:x86_64+0x974f)
    #4 WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::~Vector() <null> (WebKit:x86_64+0x6eb9)
    #5 WebKit::WebProcessPool::setMediaAccessibilityPreferences(WebKit::WebProcessProxy&)::$_3::operator()() const::'lambda'()::~() <null> (WebKit:x86_64+0x10cc72a)
    #6 WebKit::WebProcessPool::setMediaAccessibilityPreferences(WebKit::WebProcessProxy&)::$_3::operator()() const::'lambda'()::~() <null> (WebKit:x86_64+0x10cba49)
    #7 WTF::Detail::CallableWrapper<WebKit::WebProcessPool::setMediaAccessibilityPreferences(WebKit::WebProcessProxy&)::$_3::operator()() const::'lambda'(), void>::~CallableWrapper() <null> (WebKit:x86_64+0x10cbe00)
    #8 WTF::Detail::CallableWrapper<WebKit::WebProcessPool::setMediaAccessibilityPreferences(WebKit::WebProcessProxy&)::$_3::operator()() const::'lambda'(), void>::~CallableWrapper() <null> (WebKit:x86_64+0x10cbcc9)
    #9 WTF::Detail::CallableWrapper<WebKit::WebProcessPool::setMediaAccessibilityPreferences(WebKit::WebProcessProxy&)::$_3::operator()() const::'lambda'(), void>::~CallableWrapper() <null> (WebKit:x86_64+0x10cbcf9)
    #10 std::__1::default_delete<WTF::Detail::CallableWrapperBase<void> >::operator()(WTF::Detail::CallableWrapperBase<void>*) const <null> (JavaScriptCore:x86_64+0x149c7)
    #11 std::__1::unique_ptr<WTF::Detail::CallableWrapperBase<void>, std::__1::default_delete<WTF::Detail::CallableWrapperBase<void> > >::reset(WTF::Detail::CallableWrapperBase<void>*) <null> (JavaScriptCore:x86_64+0x1492d)
    #12 std::__1::unique_ptr<WTF::Detail::CallableWrapperBase<void>, std::__1::default_delete<WTF::Detail::CallableWrapperBase<void> > >::~unique_ptr() <null> (JavaScriptCore:x86_64+0x148bb)
    #13 std::__1::unique_ptr<WTF::Detail::CallableWrapperBase<void>, std::__1::default_delete<WTF::Detail::CallableWrapperBase<void> > >::~unique_ptr() <null> (JavaScriptCore:x86_64+0x14889)
    #14 WTF::Function<void ()>::~Function() <null> (JavaScriptCore:x86_64+0x14859)
    #15 WTF::Function<void ()>::~Function() <null> (JavaScriptCore:x86_64+0x134a9)
    #16 WTF::RunLoop::performWork() <null> (JavaScriptCore:x86_64+0x8d9aa)
    #17 WTF::RunLoop::performWork(void*) <null> (JavaScriptCore:x86_64+0x9072a)
    #18 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ <null> (CoreFoundation:x86_64+0x81354)
    #19 WTR::TestController::runUntil(bool&, WTF::Seconds) <null> (WebKitTestRunner:x86_64+0x100054a1b)
    #20 WTR::TestController::resetStateToConsistentValues(WTR::TestOptions const&, WTR::TestController::ResetStage)::$_4::operator()() const <null> (WebKitTestRunner:x86_64+0x10005a0b8)
    #21 WTR::TestController::resetStateToConsistentValues(WTR::TestOptions const&, WTR::TestController::ResetStage) <null> (WebKitTestRunner:x86_64+0x100059032)
    #22 WTR::TestController::ensureViewSupportsOptionsForTest(WTR::TestInvocation const&) <null> (WebKitTestRunner:x86_64+0x1000587ae)
    #23 WTR::TestController::configureViewForTest(WTR::TestInvocation const&) <null> (WebKitTestRunner:x86_64+0x10005b480)
    #24 WTR::TestInvocation::invoke() <null> (WebKitTestRunner:x86_64+0x10009784c)
    #25 WTR::TestController::runTest(char const*) <null> (WebKitTestRunner:x86_64+0x10005b66e)
    #26 WTR::TestController::runTestingServerLoop() <null> (WebKitTestRunner:x86_64+0x10005ba17)
    #27 WTR::TestController::run() <null> (WebKitTestRunner:x86_64+0x10005523d)
    #28 WTR::TestController::TestController(int, char const**) <null> (WebKitTestRunner:x86_64+0x100054d4b)
    #29 WTR::TestController::TestController(int, char const**) <null> (WebKitTestRunner:x86_64+0x1000552e9)
    #30 main <null> (WebKitTestRunner:x86_64+0x10000804a)

  Previous write of size 4 at 0x7b080000ea00 by thread T3:
    #0 WTF::StringImpl::deref() <null> (WebKit:x86_64+0x7ada)
    #1 WTF::VectorDestructor<true, WTF::String>::destruct(WTF::String*, WTF::String*) <null> (WebKit:x86_64+0x9867)
    #2 WTF::VectorTypeOperations<WTF::String>::destruct(WTF::String*, WTF::String*) <null> (WebKit:x86_64+0x97b0)
    #3 WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::~Vector() <null> (WebKit:x86_64+0x974f)
    #4 WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::~Vector() <null> (WebKit:x86_64+0x6eb9)
    #5 WebKit::WebProcessPool::setMediaAccessibilityPreferences(WebKit::WebProcessProxy&)::$_3::operator()() const <null> (WebKit:x86_64+0x10988c3)
    #6 invocation function for block in WebKit::WebProcessPool::setMediaAccessibilityPreferences(WebKit::WebProcessProxy&) <null> (WebKit:x86_64+0x109882d)
    #7 __tsan::invoke_and_release_block(void*) <null> (libclang_rt.tsan_osx_dynamic.dylib:x86_64+0x7377b)
    #8 _dispatch_client_callout <null> (libdispatch.dylib:x86_64+0x34ff)

  Location is heap block of size 24 at 0x7b080000ea00 allocated by thread T3:
    #0 __sanitizer_mz_malloc <null> (libclang_rt.tsan_osx_dynamic.dylib:x86_64+0x5168a)
    #1 _malloc_zone_malloc <null> (libsystem_malloc.dylib:x86_64+0x1cf80)
    #2 bmalloc::Cache::allocateSlowCaseNullCache(bmalloc::HeapKind, unsigned long) <null> (JavaScriptCore:x86_64+0x11d240)
    #3 bmalloc::Cache::allocate(bmalloc::HeapKind, unsigned long) <null> (JavaScriptCore:x86_64+0x37629)
    #4 WTF::fastMalloc(unsigned long) <null> (JavaScriptCore:x86_64+0x36e5b)
    #5 WTF::StringImpl::operator new(unsigned long) <null> (JavaScriptCore:x86_64+0x34b69)
    #6 WTF::StringImpl::adopt(WTF::StringBuffer<unsigned char>&&) <null> (JavaScriptCore:x86_64+0xa6374)
    #7 WTF::String::String(__CFString const*) <null> (JavaScriptCore:x86_64+0x9ca9e)
    #8 WTF::String::String(__CFString const*) <null> (JavaScriptCore:x86_64+0x9cd20)
    #9 WebCore::CaptionUserPreferencesMediaAF::platformPreferredLanguages() <null> (WebCore:x86_64+0x2de4689)
    #10 WebKit::WebProcessPool::setMediaAccessibilityPreferences(WebKit::WebProcessProxy&)::$_3::operator()() const <null> (WebKit:x86_64+0x1098876)
    #11 invocation function for block in WebKit::WebProcessPool::setMediaAccessibilityPreferences(WebKit::WebProcessProxy&) <null> (WebKit:x86_64+0x109882d)
    #12 __tsan::invoke_and_release_block(void*) <null> (libclang_rt.tsan_osx_dynamic.dylib:x86_64+0x7377b)
    #13 _dispatch_client_callout <null> (libdispatch.dylib:x86_64+0x34ff)

  Thread T3 (tid=12850431, running) is a GCD worker thread

SUMMARY: ThreadSanitizer: data race (WebKitBuild/WebKit.framework/Versions/A/WebKit:x86_64+0x7aca) in WTF::StringImpl::deref()+0x1a

[David Kilzer (:ddkilzer)]

Created attachment 435225 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Comment on attachment 435225 [details]
Patch v1

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:192
> -        callOnMainRunLoop([weakProcess, captionDisplayMode, preferredLanguages] {
> +        callOnMainRunLoop([weakProcess, captionDisplayMode, preferredLanguages = preferredLanguages.isolatedCopy()] {

I opted to make an isolatedCopy() here because the other use of WebCore::CaptionUserPreferencesMediaAF::platformPreferredLanguages() in Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm didn't involve separate threads:

#if HAVE(MEDIA_ACCESSIBILITY_FRAMEWORK)
void WebProcessPool::mediaAccessibilityPreferencesChangedCallback(CFNotificationCenterRef, void *observer, CFStringRef name, const void *, CFDictionaryRef userInfo)
{
    auto* pool = reinterpret_cast<WebProcessPool*>(observer);
    auto captionDisplayMode = WebCore::CaptionUserPreferencesMediaAF::platformCaptionDisplayMode();
    auto preferredLanguages = WebCore::CaptionUserPreferencesMediaAF::platformPreferredLanguages();
    pool->sendToAllProcesses(Messages::WebProcess::SetMediaAccessibilityPreferences(captionDisplayMode, preferredLanguages));
}
#endif

[Radar WebKit Bug Importer]

<rdar://problem/81718915>

[Chris Dumez]

Comment on attachment 435225 [details]
Patch v1

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

r=me

>> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:192
>> +        callOnMainRunLoop([weakProcess, captionDisplayMode, preferredLanguages = preferredLanguages.isolatedCopy()] {
> 
> I opted to make an isolatedCopy() here because the other use of WebCore::CaptionUserPreferencesMediaAF::platformPreferredLanguages() in Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm didn't involve separate threads:
> 
> #if HAVE(MEDIA_ACCESSIBILITY_FRAMEWORK)
> void WebProcessPool::mediaAccessibilityPreferencesChangedCallback(CFNotificationCenterRef, void *observer, CFStringRef name, const void *, CFDictionaryRef userInfo)
> {
>     auto* pool = reinterpret_cast<WebProcessPool*>(observer);
>     auto captionDisplayMode = WebCore::CaptionUserPreferencesMediaAF::platformCaptionDisplayMode();
>     auto preferredLanguages = WebCore::CaptionUserPreferencesMediaAF::platformPreferredLanguages();
>     pool->sendToAllProcesses(Messages::WebProcess::SetMediaAccessibilityPreferences(captionDisplayMode, preferredLanguages));
> }
> #endif

nit: WTFMove(preferredLanguages).isolatedCopy() may be more efficient in some cases because of this version of isolatedCopy():
template<typename U = T> Vector<U> isolatedCopy() &&;

[David Kilzer (:ddkilzer)]

Created attachment 435227 [details]
Patch v2 for landing

[David Kilzer (:ddkilzer)]

Regressed in r279591:

    Collect Accessibility preferences on a background queue
    https://bugs.webkit.org/show_bug.cgi?id=227617
    https://trac.webkit.org/changeset/279591/webkit

[EWS]

Committed r280841 (240392@main): <https://commits.webkit.org/240392@main>

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