Bug 229435 - ThreadSanitizer: data race of WTF::StringImpl in WebCoreNSURLSessionDataTask._metrics instance variable
Summary: ThreadSanitizer: data race of WTF::StringImpl in WebCoreNSURLSessionDataTask....
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on: 229579
Blocks:
  Show dependency treegraph
 
Reported: 2021-08-23 19:39 PDT by David Kilzer (:ddkilzer)
Modified: 2021-08-26 12:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.38 KB, patch)
2021-08-23 19:43 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (4.85 KB, patch)
2021-08-23 19:50 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2021-08-23 19:39:48 PDT
ThreadSanitizer: data race of WTF::StringImpl in WebCoreNSURLSessionDataTask._metrics instance variable.

The issue appears to be that -[WebCoreNSURLSessionTaskTransactionMetrics _initWithMetrics:] (which is used by -[WebCoreNSURLSessionTaskMetrics _initWithMetrics:]) does not WTFMove() or make an isolated copy of the `const WebCore::NetworkLoadMetrics&` argument passed into it, so a non-isolated copy is made through its default copy constructor for WebCore::NetworkLoadMetrics on a background thread, which causes problems later because the Objective-C object is released on the main thread.

WARNING: ThreadSanitizer: data race (pid=17141)
  Read of size 4 at 0x7b080000d5e0 by main thread:
    #0 WTF::StringImpl::deref() <null> (WebCore:x86_64+0x245d6)
    #1 WebCore::NetworkLoadMetricsWithoutNonTimingData::~NetworkLoadMetricsWithoutNonTimingData() <null> (WebCore:x86_64+0xa4dc36)
    #2 WebCore::NetworkLoadMetrics::~NetworkLoadMetrics() <null> (WebCore:x86_64+0xa4dbdc)
    #3 WebCore::NetworkLoadMetrics::~NetworkLoadMetrics() <null> (WebCore:x86_64+0xa4dad9)
    #4 -[WebCoreNSURLSessionDataTask _resource:loadFinishedWithError:metrics:]::$_18::~$_18() <null> (WebCore:x86_64+0x3d4dd2a)
    #5 -[WebCoreNSURLSessionDataTask _resource:loadFinishedWithError:metrics:]::$_18::~$_18() <null> (WebCore:x86_64+0x3d4c419)
    #6 WTF::Detail::CallableWrapper<-[WebCoreNSURLSessionDataTask _resource:loadFinishedWithError:metrics:]::$_18, void>::~CallableWrapper() <null> (WebCore:x86_64+0x3d5e810)
    #7 WTF::Detail::CallableWrapper<-[WebCoreNSURLSessionDataTask _resource:loadFinishedWithError:metrics:]::$_18, void>::~CallableWrapper() <null> (WebCore:x86_64+0x3d5e6b9)
    #8 WTF::Detail::CallableWrapper<-[WebCoreNSURLSessionDataTask _resource:loadFinishedWithError:metrics:]::$_18, void>::~CallableWrapper() <null> (WebCore:x86_64+0x3d5e6e9)
    #9 std::__1::default_delete<WTF::Detail::CallableWrapperBase<void> >::operator()(WTF::Detail::CallableWrapperBase<void>*) const <null> (WebCore:x86_64+0x1fe07)
    #10 std::__1::unique_ptr<WTF::Detail::CallableWrapperBase<void>, std::__1::default_delete<WTF::Detail::CallableWrapperBase<void> > >::reset(WTF::Detail::CallableWrapperBase<void>*) <null> (WebCore:x86_64+0x1fdcd)
    #11 std::__1::unique_ptr<WTF::Detail::CallableWrapperBase<void>, std::__1::default_delete<WTF::Detail::CallableWrapperBase<void> > >::~unique_ptr() <null> (WebCore:x86_64+0x1fd5b)
    #12 std::__1::unique_ptr<WTF::Detail::CallableWrapperBase<void>, std::__1::default_delete<WTF::Detail::CallableWrapperBase<void> > >::~unique_ptr() <null> (WebCore:x86_64+0x1fd29)
    #13 WTF::Function<void ()>::~Function() <null> (WebCore:x86_64+0x1fcf9)
    #14 WTF::Function<void ()>::~Function() <null> (WebCore:x86_64+0x1fcc9)
    #15 WTF::BlockPtr<void ()> WTF::BlockPtr<void ()>::fromCallable<WTF::Function<void ()> >(WTF::Function<void ()>)::'lambda'(void const*)::operator()(void const*) const <null> (WebCore:x86_64+0x3d4d1ba)
    #16 WTF::BlockPtr<void ()> WTF::BlockPtr<void ()>::fromCallable<WTF::Function<void ()> >(WTF::Function<void ()>)::'lambda'(void const*)::__invoke(void const*) <null> (WebCore:x86_64+0x3d4d139)
    #17 _Block_release <null> (libsystem_blocks.dylib:x86_64+0x1650)
    #18 WTF::AutodrainedPool::~AutodrainedPool() <null> (JavaScriptCore:x86_64+0x143c9)
    #19 WTF::RunLoop::performWork(void*) <null> (JavaScriptCore:x86_64+0x93122)
    #20 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ <null> (CoreFoundation:x86_64+0x81354)
    #21 WKXPCServiceMain <null> (WebKit:x86_64+0x225da4e)
    #22 main <null> (com.apple.WebKit.GPU.Development:x86_64+0x100003e3e)

  Previous write of size 4 at 0x7b080000d5e0 by thread T3:
    #0 WTF::StringImpl::ref() <null> (WebCore:x86_64+0x2449d)
    #1 WTF::RefPtr<WTF::StringImpl, WTF::RawPtrTraits<WTF::StringImpl>, WTF::DefaultRefDerefTraits<WTF::StringImpl> >::operator=(WTF::RefPtr<WTF::StringImpl, WTF::RawPtrTraits<WTF::StringImpl>, WTF::DefaultRefDerefTraits<WTF::StringImpl> > const&) <null> (WebCore:x86_64+0x243ae)
    #2 WTF::String::operator=(WTF::String const&) <null> (WebCore:x86_64+0x24230)
    #3 WebCore::NetworkLoadMetricsWithoutNonTimingData::operator=(WebCore::NetworkLoadMetricsWithoutNonTimingData const&) <null> (WebCore:x86_64+0x2b96ec2)
    #4 WebCore::NetworkLoadMetrics::operator=(WebCore::NetworkLoadMetrics const&) <null> (WebCore:x86_64+0x2b90ac4)
    #5 -[WebCoreNSURLSessionTaskTransactionMetrics _initWithMetrics:] <null> (WebCore:x86_64+0x3d46b77)
    #6 -[WebCoreNSURLSessionTaskMetrics _initWithMetrics:] <null> (WebCore:x86_64+0x3d47214)
    #7 -[WebCoreNSURLSessionDataTask _resource:loadFinishedWithError:metrics:]::$_18::operator()() const <null> (WebCore:x86_64+0x3d5e8fa)
    #8 WTF::Detail::CallableWrapper<-[WebCoreNSURLSessionDataTask _resource:loadFinishedWithError:metrics:]::$_18, void>::call() <null> (WebCore:x86_64+0x3d5e71d)
    #9 WTF::Function<void ()>::operator()() const <null> (WebCore:x86_64+0x1f7cd)
    #10 WTF::BlockPtr<void ()> WTF::BlockPtr<void ()>::fromCallable<WTF::Function<void ()> >(WTF::Function<void ()>)::'lambda'(void*)::operator()(void*) const <null> (WebCore:x86_64+0x3d4d22a)
    #11 WTF::BlockPtr<void ()> WTF::BlockPtr<void ()>::fromCallable<WTF::Function<void ()> >(WTF::Function<void ()>)::'lambda'(void*)::__invoke(void*) <null> (WebCore:x86_64+0x3d4d1e9)
    #12 __NSBLOCKOPERATION_IS_CALLING_OUT_TO_A_BLOCK__ <null> (Foundation:x86_64+0x418a1)
    #13 _dispatch_client_callout <null> (libdispatch.dylib:x86_64+0x34ff)

  Location is heap block of size 28 at 0x7b080000d5e0 allocated by main thread:
    #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+0x122e50)
    #3 bmalloc::Cache::allocate(bmalloc::HeapKind, unsigned long) <null> (JavaScriptCore:x86_64+0x394c9)
    #4 WTF::fastMalloc(unsigned long) <null> (JavaScriptCore:x86_64+0x38cfb)
    #5 WTF::FastMalloc::malloc(unsigned long) <null> (JavaScriptCore:x86_64+0x1b12b99)
    #6 WTF::Ref<WTF::StringImpl, WTF::RawPtrTraits<WTF::StringImpl> > WTF::StringImpl::createUninitializedInternalNonEmpty<unsigned char>(unsigned int, unsigned char*&) <null> (JavaScriptCore:x86_64+0xa0a3b)
    #7 WTF::Ref<WTF::StringImpl, WTF::RawPtrTraits<WTF::StringImpl> > WTF::StringImpl::createInternal<unsigned char>(unsigned char const*, unsigned int) <null> (JavaScriptCore:x86_64+0xa080f)
    #8 WTF::StringImpl::create(unsigned char const*, unsigned int) <null> (JavaScriptCore:x86_64+0xa07b9)
    #9 WTF::StringImpl::isolatedCopy() const <null> (JavaScriptCore:x86_64+0x11b528)
    #10 WTF::String::isolatedCopy() const & <null> (JavaScriptCore:x86_64+0x11b47e)
    #11 WebCore::NetworkLoadMetrics::isolatedCopy() const <null> (WebCore:x86_64+0x29aecaa)
    #12 -[WebCoreNSURLSessionDataTask _resource:loadFinishedWithError:metrics:] <null> (WebCore:x86_64+0x3d4c2c1)
    #13 -[WebCoreNSURLSessionDataTask resourceFinished:metrics:] <null> (WebCore:x86_64+0x3d4c5cf)
    #14 WebCore::WebCoreNSURLSessionDataTaskClient::loadFinished(WebCore::PlatformMediaResource&, WebCore::NetworkLoadMetrics const&) <null> (WebCore:x86_64+0x3d4a898)
    #15 WebKit::RemoteMediaResource::loadFinished(WebCore::NetworkLoadMetrics const&) <null> (WebKit:x86_64+0x985acb)
    #16 WebKit::RemoteMediaResourceManager::loadFinished(WTF::ObjectIdentifier<WebKit::RemoteMediaResourceIdentifierType>, WebCore::NetworkLoadMetrics const&) <null> (WebKit:x86_64+0x9866de)
    #17 void IPC::callMemberFunctionImpl<WebKit::RemoteMediaResourceManager, void (WebKit::RemoteMediaResourceManager::*)(WTF::ObjectIdentifier<WebKit::RemoteMediaResourceIdentifierType>, WebCore::NetworkLoadMetrics const&), std::__1::tuple<WTF::ObjectIdentifier<WebKit::RemoteMediaResourceIdentifierType>, WebCore::NetworkLoadMetrics>, 0ul, 1ul>(WebKit::RemoteMediaResourceManager*, void (WebKit::RemoteMediaResourceManager::*)(WTF::ObjectIdentifier<WebKit::RemoteMediaResourceIdentifierType>, WebCore::NetworkLoadMetrics const&), std::__1::tuple<WTF::ObjectIdentifier<WebKit::RemoteMediaResourceIdentifierType>, WebCore::NetworkLoadMetrics>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>) <null> (WebKit:x86_64+0x57b003)
    #18 void IPC::callMemberFunction<WebKit::RemoteMediaResourceManager, void (WebKit::RemoteMediaResourceManager::*)(WTF::ObjectIdentifier<WebKit::RemoteMediaResourceIdentifierType>, WebCore::NetworkLoadMetrics const&), std::__1::tuple<WTF::ObjectIdentifier<WebKit::RemoteMediaResourceIdentifierType>, WebCore::NetworkLoadMetrics>, std::__1::integer_sequence<unsigned long, 0ul, 1ul> >(std::__1::tuple<WTF::ObjectIdentifier<WebKit::RemoteMediaResourceIdentifierType>, WebCore::NetworkLoadMetrics>&&, WebKit::RemoteMediaResourceManager*, void (WebKit::RemoteMediaResourceManager::*)(WTF::ObjectIdentifier<WebKit::RemoteMediaResourceIdentifierType>, WebCore::NetworkLoadMetrics const&)) <null> (WebKit:x86_64+0x579698)
    #19 void IPC::handleMessage<Messages::RemoteMediaResourceManager::LoadFinished, WebKit::RemoteMediaResourceManager, void (WebKit::RemoteMediaResourceManager::*)(WTF::ObjectIdentifier<WebKit::RemoteMediaResourceIdentifierType>, WebCore::NetworkLoadMetrics const&)>(IPC::Decoder&, WebKit::RemoteMediaResourceManager*, void (WebKit::RemoteMediaResourceManager::*)(WTF::ObjectIdentifier<WebKit::RemoteMediaResourceIdentifierType>, WebCore::NetworkLoadMetrics const&)) <null> (WebKit:x86_64+0x56fb6b)
    #20 WebKit::RemoteMediaResourceManager::didReceiveMessage(IPC::Connection&, IPC::Decoder&) <null> (WebKit:x86_64+0x56f5b1)
    #21 WebKit::GPUConnectionToWebProcess::dispatchMessage(IPC::Connection&, IPC::Decoder&) <null> (WebKit:x86_64+0x8a9249)
    #22 WebKit::GPUConnectionToWebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&) <null> (WebKit:x86_64+0x767717)
    #23 non-virtual thunk to WebKit::GPUConnectionToWebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&) <null> (WebKit:x86_64+0x76809d)
    #24 IPC::Connection::dispatchMessage(IPC::Decoder&) <null> (WebKit:x86_64+0x938cc)
    #25 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) <null> (WebKit:x86_64+0x93ccb)
    #26 IPC::Connection::dispatchOneIncomingMessage() <null> (WebKit:x86_64+0x941d2)
    #27 IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_11::operator()() <null> (WebKit:x86_64+0xae091)
    #28 WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_11, void>::call() <null> (WebKit:x86_64+0xadfbd)
    #29 WTF::Function<void ()>::operator()() const <null> (JavaScriptCore:x86_64+0x2805d)
    #30 WTF::RunLoop::performWork() <null> (JavaScriptCore:x86_64+0x90392)
    #31 WTF::RunLoop::performWork(void*) <null> (JavaScriptCore:x86_64+0x9311a)
    #32 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ <null> (CoreFoundation:x86_64+0x81354)
    #33 WKXPCServiceMain <null> (WebKit:x86_64+0x225da4e)
    #34 main <null> (com.apple.WebKit.GPU.Development:x86_64+0x100003e3e)

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

SUMMARY: ThreadSanitizer: data race (/private/var/build/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x245d6) in WTF::StringImpl::deref()+0x26
Comment 1 Radar WebKit Bug Importer 2021-08-23 19:40:14 PDT
<rdar://problem/82273277>
Comment 2 David Kilzer (:ddkilzer) 2021-08-23 19:40:26 PDT
One way to fix this is for -[WebCoreNSURLSessionTaskTransactionMetrics _initWithMetrics:] to make a defensive isolatedCopy() of its argument every time.

Another potential fix would be to make -[WebCoreNSURLSessionTaskTransactionMetrics _initWithMetrics:] and -[WebCoreNSURLSessionTaskMetrics _initWithMetrics:] take a WTFMove()-able `WebCore::NetworkLoadMetrics&&` argument type, but this relies on callers knowing that they need to make an isolatedCopy() first.
Comment 3 Alex Christensen 2021-08-23 19:43:45 PDT
Created attachment 436258 [details]
Patch
Comment 4 David Kilzer (:ddkilzer) 2021-08-23 19:47:45 PDT
Comment on attachment 436258 [details]
Patch

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

r=me but consider the comment.

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:948
> +            [delegate URLSession:(NSURLSession *)strongSession.get() task:(NSURLSessionDataTask *)strongSelf.get() didFinishCollectingMetrics:(NSURLSessionTaskMetrics *)adoptNS([[WebCoreNSURLSessionTaskMetrics alloc] _initWithMetrics:metrics.isolatedCopy()]).get()];

Line 944 already calls `metrics = metrics.isolatedCopy()`.  Can we just call WTFMove(metrics) here?
Comment 5 David Kilzer (:ddkilzer) 2021-08-23 19:50:20 PDT
Hit with the following layout tests:

    http/tests/media/video-play-progress.html
    http/tests/media/video-redirect.html
Comment 6 Alex Christensen 2021-08-23 19:50:57 PDT
Created attachment 436259 [details]
Patch
Comment 7 David Kilzer (:ddkilzer) 2021-08-23 20:04:36 PDT
(In reply to David Kilzer (:ddkilzer) from comment #5)
> Hit with the following layout tests:
> 
>     http/tests/media/video-play-progress.html
>     http/tests/media/video-redirect.html

And likely this one:

    http/tests/media/modern-media-controls/pip-support/pip-support-live-broadcast.html
Comment 8 EWS 2021-08-23 20:38:12 PDT
Committed r281486 (240860@main): <https://commits.webkit.org/240860@main>

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