1506812 - Data race in URLPreloader

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P3)

Description

[Yaron Tausky [:ytausky]]

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.

Comment 1

[Kris Maglione [:kmag]]

Pretty sure this is a false positive.

Comment 2

[Yaron Tausky [:ytausky]]

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

[Kris Maglione [:kmag]]

(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

[Christian Holler (:decoder)]

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

Comment 6

[Aria Beingessner [:Gankra]]

Attachment: tsan-backtrace.txt

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

Comment 7

[Aria Beingessner [:Gankra]]

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

whoops, bad copy-paste.

Comment 8

[Aria Beingessner [:Gankra]]

Attachment: tsan-backtrace.txt

Comment 9

[Aria Beingessner [:Gankra]]

Attachment: tsan-backtrace.txt

oh boy gonna be one of those mornings huh

Comment 10

[Aria Beingessner [:Gankra]]

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?

Comment 11

[Aria Beingessner [:Gankra]]

Nika has recommend just removing the "initial task" convenience as a footgun. I'm going to move forward with that solution unless someone objects.

Comment 12

[Aria Beingessner [:Gankra]]

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

Comment 13

[Aria Beingessner [:Gankra]]

Attachment: Bug 1506812 - Wrap all accesses to URLPreloader's mReaderThread in a mutex. r?kmag

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)

Comment 14

[Aria Beingessner [:Gankra]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c4d6da06deb
Wrap all accesses to URLPreloader's mReaderThread in a mutex. r=decoder,nika

Comment 16

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/6c4d6da06deb