Bug 229435

Summary: ThreadSanitizer: data race of WTF::StringImpl in WebCoreNSURLSessionDataTask._metrics instance variable
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebKit Misc.Assignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, katherine_cheney, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 229579    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

David Kilzer (:ddkilzer)
Reported 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
Attachments
Patch (4.38 KB, patch)
2021-08-23 19:43 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (4.85 KB, patch)
2021-08-23 19:50 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2021-08-23 19:40:14 PDT
<rdar://problem/82273277>
David Kilzer (:ddkilzer)
Comment 2 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.
Alex Christensen
Comment 3 2021-08-23 19:43:45 PDT
Created attachment 436258 [details] Patch
David Kilzer (:ddkilzer)
Comment 4 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?
David Kilzer (:ddkilzer)
Comment 5 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
Alex Christensen
Comment 6 2021-08-23 19:50:57 PDT
Created attachment 436259 [details] Patch
David Kilzer (:ddkilzer)
Comment 7 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
EWS
Comment 8 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].
Note You need to log in before you can comment on or make changes to this bug.