Data race in URLPreloader
Categories
(Core :: XPConnect, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox84 | --- | fixed |
People
(Reporter: ytausky, Assigned: Gankra)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
I ran TSan on some tests and got this race: 0:11.66 pid:21091 ================== 0:11.66 pid:21091 WARNING: ThreadSanitizer: data race (pid=21091) 0:11.66 pid:21091 Read of size 8 at 0x7b3800001398 by thread T18: 0:11.66 pid:21091 #0 RefPtr<nsIThread>::get() const /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h:307:27 (libxul.so+0x23aebdd) 0:11.66 pid:21091 #1 RefPtr<nsIThread>::operator nsIThread*() const & /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h:320 (libxul.so+0x23aebdd) 0:11.66 pid:21091 #2 mozilla::detail::RunnableMethodImpl<RefPtr<nsIThread>, nsresult (nsIThread::*)(), true, (mozilla::RunnableKind)0>::RunnableMethodImpl<RefPtr<nsIThread>&>(char const*, RefPtr<nsIThread>&, nsresult (nsIThread::*)()) /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:1230 (libxul.so+0x23aebdd) 0:11.66 pid:21091 #3 already_AddRefed<nsRunnableMethodTraits<mozilla::RemoveReference<RefPtr<nsIThread>&>::Type, nsresult (nsIThread::*)(), true, (mozilla::RunnableKind)0>::base_type> mozilla::NewRunnableMethod<RefPtr<nsIThread>&, nsresult (nsIThread::*)()>(char const*, RefPtr<nsIThread>&, nsresult (nsIThread::*)()) /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:1441 (libxul.so+0x23aebdd) 0:11.66 pid:21091 #4 mozilla::URLPreloader::BackgroundReadFiles()::$_7::operator()() const /home/ytausky/dev/mozilla-central/js/xpconnect/loader/URLPreloader.cpp:349 (libxul.so+0x23aebdd) 0:11.66 pid:21091 #5 mozilla::ScopeExit<mozilla::URLPreloader::BackgroundReadFiles()::$_7>::~ScopeExit() /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ScopeExit.h:113 (libxul.so+0x23aebdd) 0:11.66 pid:21091 #6 mozilla::URLPreloader::BackgroundReadFiles() /home/ytausky/dev/mozilla-central/js/xpconnect/loader/URLPreloader.cpp:447 (libxul.so+0x23aebdd) 0:11.66 pid:21091 #7 decltype (((*{parm#1}).*{parm#2})()) mozilla::detail::RunnableMethodArguments<>::applyImpl<mozilla::URLPreloader, void (mozilla::URLPreloader::*)()>(mozilla::URLPreloader*, void (mozilla::URLPreloader::*)(), mozilla::Tuple<>&, std::integer_sequence<unsigned long>) /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:1191:12 (libxul.so+0x23b85aa) 0:11.66 pid:21091 #8 _ZN7mozilla6detail23RunnableMethodArgumentsIJEE5applyINS_12URLPreloaderEMS4_FvvEEEDTcl9applyImplfp_fp0_dtdefpT10mArgumentstlSt16integer_sequenceImJEEEEEPT_T0_ /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:1197 (libxul.so+0x23b85aa) 0:11.66 pid:21091 #9 mozilla::detail::RunnableMethodImpl<mozilla::URLPreloader*, void (mozilla::URLPreloader::*)(), true, (mozilla::RunnableKind)0>::Run() /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:1242 (libxul.so+0x23b85aa) 0:11.66 pid:21091 #10 nsThread::ProcessNextEvent(bool, bool*) /home/ytausky/dev/mozilla-central/xpcom/threads/nsThread.cpp:1246:14 (libxul.so+0x14935d9) 0:11.67 pid:21091 #11 NS_ProcessNextEvent(nsIThread*, bool) /home/ytausky/dev/mozilla-central/xpcom/threads/nsThreadUtils.cpp:530:10 (libxul.so+0x1496575) 0:11.67 pid:21091 #12 mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /home/ytausky/dev/mozilla-central/ipc/glue/MessagePump.cpp:334:20 (libxul.so+0x1daa8ce) 0:11.67 pid:21091 #13 MessageLoop::RunInternal() /home/ytausky/dev/mozilla-central/ipc/chromium/src/base/message_loop.cc:325:10 (libxul.so+0x1cfc27f) 0:11.67 pid:21091 #14 MessageLoop::RunHandler() /home/ytausky/dev/mozilla-central/ipc/chromium/src/base/message_loop.cc:318 (libxul.so+0x1cfc27f) 0:11.67 pid:21091 #15 MessageLoop::Run() /home/ytausky/dev/mozilla-central/ipc/chromium/src/base/message_loop.cc:298 (libxul.so+0x1cfc27f) 0:11.67 pid:21091 #16 nsThread::ThreadFunc(void*) /home/ytausky/dev/mozilla-central/xpcom/threads/nsThread.cpp:505:11 (libxul.so+0x14901b2) 0:11.67 pid:21091 #17 _pt_root /home/ytausky/dev/mozilla-central/nsprpub/pr/src/pthreads/ptthread.c:201:5 (libnspr4.so+0x428c8) 0:11.67 pid:21091 Previous write of size 8 at 0x7b3800001398 by main thread: 0:11.67 pid:21091 #0 nsCOMPtr<nsIThread>::swap(nsIThread*&) /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h:887:10 (libxul.so+0x1497b18) 0:11.67 pid:21091 #1 NS_NewNamedThread(nsTSubstring<char> const&, nsIThread**, nsIRunnable*, unsigned int) /home/ytausky/dev/mozilla-central/xpcom/threads/nsThreadUtils.cpp:167 (libxul.so+0x1497b18) 0:11.67 pid:21091 #2 nsresult NS_NewNamedThread<11ul>(char const (&) [11ul], nsIThread**, nsIRunnable*, unsigned int) /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:75:10 (libxul.so+0x23af08f) 0:11.67 pid:21091 #3 mozilla::URLPreloader::BeginBackgroundRead() /home/ytausky/dev/mozilla-central/js/xpconnect/loader/URLPreloader.cpp:458 (libxul.so+0x23af08f) 0:11.67 pid:21091 #4 mozilla::URLPreloader::AutoBeginReading::AutoBeginReading() /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/URLPreloader.h:120:28 (libxul.so+0x23a706d) 0:11.67 pid:21091 #5 mozilla::ScriptPreloader::InitCache(nsTSubstring<char16_t> const&) /home/ytausky/dev/mozilla-central/js/xpconnect/loader/ScriptPreloader.cpp:509 (libxul.so+0x23a706d) 0:11.67 pid:21091 #6 mozilla::ScriptPreloader::GetChildSingleton() /home/ytausky/dev/mozilla-central/js/xpconnect/loader/ScriptPreloader.cpp:167:34 (libxul.so+0x23a5396) 0:11.67 pid:21091 #7 mozilla::ScriptPreloader::GetSingleton() /home/ytausky/dev/mozilla-central/js/xpconnect/loader/ScriptPreloader.cpp:124:39 (libxul.so+0x23a6c5d) 0:11.67 pid:21091 #8 NS_InitXPCOM2 /home/ytausky/dev/mozilla-central/xpcom/build/XPCOMInit.cpp:712:3 (libxul.so+0x14c58d8) 0:11.67 pid:21091 #9 ScopedXPCOMStartup::Initialize() /home/ytausky/dev/mozilla-central/toolkit/xre/nsAppRunner.cpp:1483:8 (libxul.so+0x731127e) 0:11.67 pid:21091 #10 XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/ytausky/dev/mozilla-central/toolkit/xre/nsAppRunner.cpp:4930 (libxul.so+0x731127e) 0:11.67 pid:21091 #11 XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/ytausky/dev/mozilla-central/toolkit/xre/nsAppRunner.cpp:5026:21 (libxul.so+0x73118b6) 0:11.67 pid:21091 #12 mozilla::BootstrapImpl::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/ytausky/dev/mozilla-central/toolkit/xre/Bootstrap.cpp:49:12 (libxul.so+0x731f627) 0:11.67 pid:21091 #13 do_main(int, char**, char**) /home/ytausky/dev/mozilla-central/browser/app/nsBrowserApp.cpp:233:22 (firefox+0xc3c86) 0:11.67 pid:21091 #14 main /home/ytausky/dev/mozilla-central/browser/app/nsBrowserApp.cpp:315 (firefox+0xc3c86) 0:11.67 pid:21091 Location is heap block of size 224 at 0x7b3800001340 allocated by main thread: 0:11.67 pid:21091 #0 malloc <null> (firefox+0x4c75c) 0:11.67 pid:21091 #1 moz_xmalloc /home/ytausky/dev/mozilla-central/memory/mozalloc/mozalloc.cpp:70:17 (firefox+0xc455a) 0:11.67 pid:21091 #2 operator new(unsigned long) /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/mozalloc.h:139:12 (libxul.so+0x23ab04a) 0:11.67 pid:21091 #3 mozilla::URLPreloader::GetSingleton() /home/ytausky/dev/mozilla-central/js/xpconnect/loader/URLPreloader.cpp:90 (libxul.so+0x23ab04a) 0:11.67 pid:21091 #4 mozilla::URLPreloader::AutoBeginReading::AutoBeginReading() /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/URLPreloader.h:120:13 (libxul.so+0x23a7065) 0:11.67 pid:21091 #5 mozilla::ScriptPreloader::InitCache(nsTSubstring<char16_t> const&) /home/ytausky/dev/mozilla-central/js/xpconnect/loader/ScriptPreloader.cpp:509 (libxul.so+0x23a7065) 0:11.67 pid:21091 #6 mozilla::ScriptPreloader::GetChildSingleton() /home/ytausky/dev/mozilla-central/js/xpconnect/loader/ScriptPreloader.cpp:167:34 (libxul.so+0x23a5396) 0:11.67 pid:21091 #7 mozilla::ScriptPreloader::GetSingleton() /home/ytausky/dev/mozilla-central/js/xpconnect/loader/ScriptPreloader.cpp:124:39 (libxul.so+0x23a6c5d) 0:11.67 pid:21091 #8 NS_InitXPCOM2 /home/ytausky/dev/mozilla-central/xpcom/build/XPCOMInit.cpp:712:3 (libxul.so+0x14c58d8) 0:11.67 pid:21091 #9 ScopedXPCOMStartup::Initialize() /home/ytausky/dev/mozilla-central/toolkit/xre/nsAppRunner.cpp:1483:8 (libxul.so+0x731127e) 0:11.67 pid:21091 #10 XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/ytausky/dev/mozilla-central/toolkit/xre/nsAppRunner.cpp:4930 (libxul.so+0x731127e) 0:11.67 pid:21091 #11 XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/ytausky/dev/mozilla-central/toolkit/xre/nsAppRunner.cpp:5026:21 (libxul.so+0x73118b6) 0:11.67 pid:21091 #12 mozilla::BootstrapImpl::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/ytausky/dev/mozilla-central/toolkit/xre/Bootstrap.cpp:49:12 (libxul.so+0x731f627) 0:11.67 pid:21091 #13 do_main(int, char**, char**) /home/ytausky/dev/mozilla-central/browser/app/nsBrowserApp.cpp:233:22 (firefox+0xc3c86) 0:11.67 pid:21091 #14 main /home/ytausky/dev/mozilla-central/browser/app/nsBrowserApp.cpp:315 (firefox+0xc3c86) 0:11.67 pid:21091 Thread T18 'BGReadURLs' (tid=21122, running) created by main thread at: 0:11.67 pid:21091 #0 pthread_create <null> (firefox+0x2f296) 0:11.67 pid:21091 #1 _PR_CreateThread /home/ytausky/dev/mozilla-central/nsprpub/pr/src/pthreads/ptthread.c:433:14 (libnspr4.so+0x40707) 0:11.67 pid:21091 #2 PR_CreateThread /home/ytausky/dev/mozilla-central/nsprpub/pr/src/pthreads/ptthread.c:518:12 (libnspr4.so+0x40447) 0:11.67 pid:21091 #3 nsThread::Init(nsTSubstring<char> const&) /home/ytausky/dev/mozilla-central/xpcom/threads/nsThread.cpp:719:8 (libxul.so+0x1491608) 0:11.67 pid:21091 #4 nsThreadManager::NewNamedThread(nsTSubstring<char> const&, unsigned int, nsIThread**) /home/ytausky/dev/mozilla-central/xpcom/threads/nsThreadManager.cpp:485:22 (libxul.so+0x1495d58) 0:11.67 pid:21091 #5 NS_NewNamedThread(nsTSubstring<char> const&, nsIThread**, nsIRunnable*, unsigned int) /home/ytausky/dev/mozilla-central/xpcom/threads/nsThreadUtils.cpp:143:45 (libxul.so+0x1497ac7) 0:11.67 pid:21091 #6 nsresult NS_NewNamedThread<11ul>(char const (&) [11ul], nsIThread**, nsIRunnable*, unsigned int) /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:75:10 (libxul.so+0x23af08f) 0:11.67 pid:21091 #7 mozilla::URLPreloader::BeginBackgroundRead() /home/ytausky/dev/mozilla-central/js/xpconnect/loader/URLPreloader.cpp:458 (libxul.so+0x23af08f) 0:11.67 pid:21091 #8 mozilla::URLPreloader::AutoBeginReading::AutoBeginReading() /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/URLPreloader.h:120:28 (libxul.so+0x23a706d) 0:11.67 pid:21091 #9 mozilla::ScriptPreloader::InitCache(nsTSubstring<char16_t> const&) /home/ytausky/dev/mozilla-central/js/xpconnect/loader/ScriptPreloader.cpp:509 (libxul.so+0x23a706d) 0:11.67 pid:21091 #10 mozilla::ScriptPreloader::GetChildSingleton() /home/ytausky/dev/mozilla-central/js/xpconnect/loader/ScriptPreloader.cpp:167:34 (libxul.so+0x23a5396) 0:11.67 pid:21091 #11 mozilla::ScriptPreloader::GetSingleton() /home/ytausky/dev/mozilla-central/js/xpconnect/loader/ScriptPreloader.cpp:124:39 (libxul.so+0x23a6c5d) 0:11.67 pid:21091 #12 NS_InitXPCOM2 /home/ytausky/dev/mozilla-central/xpcom/build/XPCOMInit.cpp:712:3 (libxul.so+0x14c58d8) 0:11.67 pid:21091 #13 ScopedXPCOMStartup::Initialize() /home/ytausky/dev/mozilla-central/toolkit/xre/nsAppRunner.cpp:1483:8 (libxul.so+0x731127e) 0:11.67 pid:21091 #14 XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/ytausky/dev/mozilla-central/toolkit/xre/nsAppRunner.cpp:4930 (libxul.so+0x731127e) 0:11.67 pid:21091 #15 XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/ytausky/dev/mozilla-central/toolkit/xre/nsAppRunner.cpp:5026:21 (libxul.so+0x73118b6) 0:11.67 pid:21091 #16 mozilla::BootstrapImpl::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/ytausky/dev/mozilla-central/toolkit/xre/Bootstrap.cpp:49:12 (libxul.so+0x731f627) 0:11.67 pid:21091 #17 do_main(int, char**, char**) /home/ytausky/dev/mozilla-central/browser/app/nsBrowserApp.cpp:233:22 (firefox+0xc3c86) 0:11.67 pid:21091 #18 main /home/ytausky/dev/mozilla-central/browser/app/nsBrowserApp.cpp:315 (firefox+0xc3c86) 0:11.67 pid:21091 SUMMARY: ThreadSanitizer: data race /home/ytausky/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h:307:27 in RefPtr<nsIThread>::get() const 0:11.67 pid:21091 ================== It looks like mReaderThread needs to be atomic with acquire-release semantics.
Updated•5 years ago
|
Reporter | ||
Comment 2•5 years ago
|
||
I don't think so. NS_NewNamedThread first launches a new thread, then sets the result. The newly created thread reads that result, so if right after launching the new thread the main thread is preempted, then the new thread runs its task to completion, it would read the previous value. Could you please have a second look?
Comment 3•5 years ago
|
||
(In reply to Yaron Tausky [:ytausky] from comment #2) > I don't think so. NS_NewNamedThread first launches a new thread, then sets > the result. The newly created thread reads that result, so if right after > launching the new thread the main thread is preempted, then the new thread > runs its task to completion, it would read the previous value. Could you > please have a second look? It looks like you're right. Ish. Launching the thread isn't the issue. The issue is that it dispatches the runnable before setting the result, which is the opposite of what I thought it did, and is somewhat annoying.
Comment 4•5 years ago
|
||
This is still an issue, I will add this to the runtime suppressions until it is fixed.
Assignee | ||
Comment 6•4 years ago
|
||
Turned this off in a try run to see if it's still a problem. It is. Modern stack trace attached.
Assignee | ||
Comment 7•4 years ago
•
|
||
Comment on attachment 9182071 [details]
tsan-backtrace.txt
whoops, bad copy-paste.
Assignee | ||
Comment 8•4 years ago
|
||
Assignee | ||
Comment 9•4 years ago
|
||
oh boy gonna be one of those mornings huh
Assignee | ||
Comment 10•4 years ago
•
|
||
Oh interesting -- this trace is actually slightly different.
The previous trace: NS_NewNamedThread is still writing[0] mReaderThread while the task's (BackgroundReadFiles) cleanup code is reading[1] that value to shut itself down.
The new trace: A new call to BeginBackgroundRead is reading[2] mReaderThread while the task's (BackgroundReadFiles) cleanup code is nulling[1] that value out to indicate it has shut down.
So although the issue with NS_NewNamedThread having a weird initialization order is still an issue, this code is still wrong, because it doesn't synchronize between the task's shutdown and the thing that checks if the task is already running.
It looks like ScriptPreloader might have the same problem, although its self-nulling runs on the main thread[3], which might be sufficient for the new trace, but would still be wrong for the old trace?
I think it would be reasonable to change NS_NewNamedThread to optimistically write the result before kicking off the task, and then have it revert the write if starting the task fails. wdyt?
Assignee | ||
Comment 11•4 years ago
|
||
Nika has recommend just removing the "initial task" convenience as a footgun. I'm going to move forward with that solution unless someone objects.
Assignee | ||
Comment 12•4 years ago
•
|
||
Kris has volunteered to go on an Editing 60 Call Sites Adventure.
Assignee | ||
Comment 13•4 years ago
|
||
The shutdown code of BackgroundReadFiles races with BeginBackgroundRead.
This is paritally obfuscated by the usage of the initialEvent convenience
of NS_NewNamedThread, so this change also removes that in favour of explicit
dispatch. (xpcom devs want to remove the convenience anyway)
Assignee | ||
Comment 14•4 years ago
|
||
Realized the aforementioned "Editing 60 Call Sites" isn't needed to fix this single usage, so here's the fix.
Comment 15•3 years ago
|
||
Pushed by abeingessner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6c4d6da06deb Wrap all accesses to URLPreloader's mReaderThread in a mutex. r=decoder,nika
Comment 16•3 years ago
|
||
bugherder |
Assignee | ||
Updated•3 years ago
|
Description
•