Closed Bug 1506812 Opened 5 years ago Closed 3 years ago

Data race in URLPreloader

Categories

(Core :: XPConnect, defect, P3)

defect

Tracking

()

RESOLVED FIXED
84 Branch
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.
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P3
Pretty sure this is a false positive.
Flags: needinfo?(kmaglione+bmo)
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?
Flags: needinfo?(kmaglione+bmo)
(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.
Assignee: nobody → kmaglione+bmo
Flags: needinfo?(kmaglione+bmo)

This is still an issue, I will add this to the runtime suppressions until it is fixed.

Blocks: tsan
Attached file tsan-backtrace.txt (obsolete) —

Turned this off in a try run to see if it's still a problem. It is. Modern stack trace attached.

Comment on attachment 9182071 [details]
tsan-backtrace.txt

whoops, bad copy-paste.
Attached file tsan-backtrace.txt (obsolete) —
Attachment #9182071 - Attachment is obsolete: true
Attached file tsan-backtrace.txt

oh boy gonna be one of those mornings huh

Attachment #9182072 - Attachment is obsolete: true

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?

Flags: needinfo?(kmaglione+bmo)

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: kmaglione+bmo → a.beingessner

Kris has volunteered to go on an Editing 60 Call Sites Adventure.

Assignee: a.beingessner → kwright

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)

Realized the aforementioned "Editing 60 Call Sites" isn't needed to fix this single usage, so here's the fix.

Assignee: kwright → a.beingessner
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
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: