Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 347559 ThreadSanitizer v2 reports data races in third_party/WebKit/Source/heap/ThreadState.cpp
Starred by 1 user Project Member Reported by glider@chromium.org, Feb 27 2014 Back to list
Status: Fixed
Owner: vegorov@chromium.org
Closed: Mar 2014
Cc: dvyukov@chromium.org, wibling@chromium.org, kcc@chromium.org, ager@chromium.org
Components:
OS: Linux
Pri: 2
Type: Bug


Sign in to add a comment
(see the full log attached)
TSAN_OPTIONS="history_size=7 external_symbolizer_path=third_party/llvm-build/Release+Asserts/bin/llvm-symbolizer suppressions=tools/valgrind/tsan_v2/suppressions.txt report_signal_unsafe=0 report_thread_leaks=0 print_suppressions=1" xvfb-run --server-args="-screen 0 1024x768x24" out/Release/content_browsertests --gtest_filter=WorkerTest.SingleSharedWorker --no-sandbox --child-clean-exit

[ RUN      ] WorkerTest.SingleSharedWorker
Xlib:  extension "RANDR" missing on display ":99".
[5099:5116:0227/185552:283406145610:WARNING:proxy_service.cc(903)] PAC support disabled because there is no system implementation
[5126:5126:0227/185552:283406307409:ERROR:renderer_main.cc(224)] Running without renderer sandbox
[       OK ] WorkerTest.SingleSharedWorker (2165 ms)

==================
WARNING: ThreadSanitizer: data race (pid=5135)
  Atomic write of size 4 at 0x7d240000d5c0 by thread T7 (mutexes: write M605, write M603):
    #0 __tsan_atomic32_store /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cc:440 (content_browsertests+0x000000621bb8)
    #1 size third_party/WebKit/Source/wtf/HashTable.h:301 (content_browsertests+0x000001dc2203)
    #2 size third_party/WebKit/Source/wtf/HashSet.h:147 (content_browsertests+0x000001dc2203)
    #3 WebCore::SafePointBarrier::parkOthers() third_party/WebKit/Source/heap/ThreadState.cpp:117 (content_browsertests+0x000001dc2203)
    #4 WebCore::ThreadState::stopThreads() third_party/WebKit/Source/heap/ThreadState.cpp:561 (content_browsertests+0x000001dc20b4)
    #5 WebCore::GCScope::GCScope(WebCore::ThreadState::StackState) third_party/WebKit/Source/heap/Heap.cpp:273 (content_browsertests+0x000001dc02ad)
    #6 WebCore::Heap::collectGarbage(WebCore::ThreadState::StackState, WebCore::Heap::GCType) third_party/WebKit/Source/heap/Heap.cpp:1284 (content_browsertests+0x000001dbe5c2)
    #7 WebCore::ThreadState::cleanup() third_party/WebKit/Source/heap/ThreadState.cpp:301 (content_browsertests+0x000001dc140d)
    #8 WebCore::WorkerThread::workerThread() third_party/WebKit/Source/core/workers/WorkerThread.cpp:148 (content_browsertests+0x000002b9fbc8)
    #9 WebCore::WorkerThread::workerThreadStart(void*) third_party/WebKit/Source/core/workers/WorkerThread.cpp:104 (content_browsertests+0x000002b9f5e9)
    #10 WTF::threadEntryPoint(void*) third_party/WebKit/Source/wtf/Threading.cpp:69 (content_browsertests+0x00000459f69f)
    #11 WTF::wtfThreadEntryPoint(void*) third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:175 (content_browsertests+0x000001c5866d)

  Previous read of size 4 at 0x7d240000d5c0 by main thread:
    #0 checkAndPark third_party/WebKit/Source/heap/ThreadState.cpp:175 (content_browsertests+0x000001dc282a)
    #1 WebCore::ThreadState::safePoint(WebCore::ThreadState::StackState) third_party/WebKit/Source/heap/ThreadState.cpp:575 (content_browsertests+0x000001dc282a)
    #2 WebCore::PendingGCRunner::didProcessTask() third_party/WebKit/Source/heap/glue/PendingGCRunner.h:62 (content_browsertests+0x000001bdce68)
    #3 content::WebThreadBase::TaskObserverAdapter::DidProcessTask(base::PendingTask const&) content/child/webthread_impl.cc:32 (content_browsertests+0x000003a5322f)
    #4 base::MessageLoop::RunTask(base::PendingTask const&) base/message_loop/message_loop.cc:448 (content_browsertests+0x000000ed9b7b)
...
==================
WARNING: ThreadSanitizer: data race (pid=5135)
  Read of size 4 at 0x7d240000d5c4 by thread T7 (mutexes: write M605, write M603):
    #0 WebCore::SafePointBarrier::parkOthers() third_party/WebKit/Source/heap/ThreadState.cpp:130 (content_browsertests+0x000001dc23f4)
    #1 WebCore::ThreadState::stopThreads() third_party/WebKit/Source/heap/ThreadState.cpp:561 (content_browsertests+0x000001dc20b4)
    #2 WebCore::GCScope::GCScope(WebCore::ThreadState::StackState) third_party/WebKit/Source/heap/Heap.cpp:273 (content_browsertests+0x000001dc02ad)
    #3 WebCore::Heap::collectGarbage(WebCore::ThreadState::StackState, WebCore::Heap::GCType) third_party/WebKit/Source/heap/Heap.cpp:1284 (content_browsertests+0x000001dbe5c2)
    #4 WebCore::ThreadState::cleanup() third_party/WebKit/Source/heap/ThreadState.cpp:301 (content_browsertests+0x000001dc140d)
    #5 WebCore::WorkerThread::workerThread() third_party/WebKit/Source/core/workers/WorkerThread.cpp:148 (content_browsertests+0x000002b9fbc8)
    #6 WebCore::WorkerThread::workerThreadStart(void*) third_party/WebKit/Source/core/workers/WorkerThread.cpp:104 (content_browsertests+0x000002b9f5e9)
    #7 WTF::threadEntryPoint(void*) third_party/WebKit/Source/wtf/Threading.cpp:69 (content_browsertests+0x00000459f69f)
    #8 WTF::wtfThreadEntryPoint(void*) third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:175 (content_browsertests+0x000001c5866d)
...
  Previous atomic write of size 4 at 0x7d240000d5c4 by main thread:
    #0 __tsan_atomic32_fetch_sub /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cc:521 (content_browsertests+0x000000624557)
    #1 doEnterSafePoint third_party/WebKit/Source/heap/ThreadState.cpp:192:14 (content_browsertests+0x000001dc414b)
    #2 WebCore::SafePointBarrier::enterSafePointAfterPushRegisters(WebCore::SafePointBarrier*, WebCore::ThreadState*, long*) third_party/WebKit/Source/heap/ThreadState.cpp:220 (content_browsertests+0x000001dc414b)
    #3 <null> <null>:0 (content_browsertests+0x000001dc4372)
    #4 WebCore::ThreadState::removeInterruptor(WebCore::ThreadState::Interruptor*) third_party/WebKit/Source/heap/ThreadState.cpp:681 (content_browsertests+0x000001dc31f2)
    #5 blink::shutdown() third_party/WebKit/Source/web/WebKit.cpp:205 (content_browsertests+0x000001bdca27)
    #6 content::WorkerThread::Shutdown() content/worker/worker_thread.cc:88 (content_browsertests+0x000004a3901d)
...
==================
WARNING: ThreadSanitizer: data race (pid=5135)
  Read of size 4 at 0x7f8267b29570 by thread T7 (mutexes: write M605):
    #0 trace third_party/WebKit/Source/heap/ThreadState.cpp:374 (content_browsertests+0x000001dc1ce1)
    #1 WebCore::ThreadState::visitRoots(WebCore::Visitor*) third_party/WebKit/Source/heap/ThreadState.cpp:340 (content_browsertests+0x000001dc1ce1)
    #2 WebCore::Heap::collectGarbage(WebCore::ThreadState::StackState, WebCore::Heap::GCType) third_party/WebKit/Source/heap/Heap.cpp:1291 (content_browsertests+0x000001dbe746)
    #3 WebCore::ThreadState::cleanup() third_party/WebKit/Source/heap/ThreadState.cpp:301 (content_browsertests+0x000001dc140d)
    #4 WebCore::WorkerThread::workerThread() third_party/WebKit/Source/core/workers/WorkerThread.cpp:148 (content_browsertests+0x000002b9fbc8)
    #5 WebCore::WorkerThread::workerThreadStart(void*) third_party/WebKit/Source/core/workers/WorkerThread.cpp:104 (content_browsertests+0x000002b9f5e9)
    #6 WTF::threadEntryPoint(void*) third_party/WebKit/Source/wtf/Threading.cpp:69 (content_browsertests+0x00000459f69f)
    #7 WTF::wtfThreadEntryPoint(void*) third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:175 (content_browsertests+0x000001c5866d)

  Previous write of size 4 at 0x7f8267b29570 by main thread:
    #0 enterSafePoint third_party/WebKit/Source/heap/ThreadState.cpp:616 (content_browsertests+0x000001dc3165)
    #1 WebCore::ThreadState::SafePointScope::SafePointScope(WebCore::ThreadState::StackState, WebCore::ThreadState::SafePointScope::ScopeNesting) third_party/WebKit/Source/heap/ThreadState.h:367 (content_browsertests+0x000001dc3165)
    #2 WebCore::ThreadState::removeInterruptor(WebCore::ThreadState::Interruptor*) third_party/WebKit/Source/heap/ThreadState.cpp:681 (content_browsertests+0x000001dc31f2)
    #3 blink::shutdown() third_party/WebKit/Source/web/WebKit.cpp:205 (content_browsertests+0x000001bdca27)
    #4 content::WorkerThread::Shutdown() content/worker/worker_thread.cc:88 (content_browsertests+0x000004a3901d)
    #5 content::ChildProcess::~ChildProcess() content/child/child_process.cc:67 (content_browsertests+0x0000039e069d)
    #6 content::WorkerMain(content::MainFunctionParams const&) content/worker/worker_main.cc:71 (content_browsertests+0x000004a386e1)
    #7 content::RunZygote(content::MainFunctionParams const&, content::ContentMainDelegate*) content/app/content_main_runner.cc:397 (content_browsertests+0x000004a27689)
    #8 content::RunNamedProcessTypeMain(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) content/app/content_main_runner.cc:480 (content_browsertests+0x000004a28193)
    #9 content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:792 (content_browsertests+0x000004a29043)
    #10 content::ContentMain(int, char const**, content::ContentMainDelegate*) content/app/content_main.cc:35 (content_browsertests+0x000004a2700e)
    #11 RunContentMain content/public/test/test_launcher.cc:453 (content_browsertests+0x00000428f082)
    #12 content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) content/public/test/test_launcher.cc:491 (content_browsertests+0x00000428f082)
    #13 main content/test/content_test_launcher.cc:137 (content_browsertests+0x00000083646e)

  Location is global 'WebCore::ThreadState::s_mainThreadStateStorage' of size 200 at 0x7f8267b29560 (content_browsertests+0x00000677e570)
...
==================
WARNING: ThreadSanitizer: data race (pid=5135)
  Read of size 8 at 0x7f8267b29588 by thread T7 (mutexes: write M605):
    #0 visitStack third_party/WebKit/Source/heap/ThreadState.cpp:353 (content_browsertests+0x000001dc1d0b)
    #1 trace third_party/WebKit/Source/heap/ThreadState.cpp:375 (content_browsertests+0x000001dc1d0b)
    #2 WebCore::ThreadState::visitRoots(WebCore::Visitor*) third_party/WebKit/Source/heap/ThreadState.cpp:340 (content_browsertests+0x000001dc1d0b)
    #3 WebCore::Heap::collectGarbage(WebCore::ThreadState::StackState, WebCore::Heap::GCType) third_party/WebKit/Source/heap/Heap.cpp:1291 (content_browsertests+0x000001dbe746)
    #4 WebCore::ThreadState::cleanup() third_party/WebKit/Source/heap/ThreadState.cpp:301 (content_browsertests+0x000001dc140d)
    #5 WebCore::WorkerThread::workerThread() third_party/WebKit/Source/core/workers/WorkerThread.cpp:148 (content_browsertests+0x000002b9fbc8)
    #6 WebCore::WorkerThread::workerThreadStart(void*) third_party/WebKit/Source/core/workers/WorkerThread.cpp:104 (content_browsertests+0x000002b9f5e9)
    #7 WTF::threadEntryPoint(void*) third_party/WebKit/Source/wtf/Threading.cpp:69 (content_browsertests+0x00000459f69f)
    #8 WTF::wtfThreadEntryPoint(void*) third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:175 (content_browsertests+0x000001c5866d)

  Previous write of size 8 at 0x7f8267b29588 by main thread:
    #0 enterSafePoint third_party/WebKit/Source/heap/ThreadState.cpp:617 (content_browsertests+0x000001dc3174)
    #1 WebCore::ThreadState::SafePointScope::SafePointScope(WebCore::ThreadState::StackState, WebCore::ThreadState::SafePointScope::ScopeNesting) third_party/WebKit/Source/heap/ThreadState.h:367 (content_browsertests+0x000001dc3174)
    #2 WebCore::ThreadState::removeInterruptor(WebCore::ThreadState::Interruptor*) third_party/WebKit/Source/heap/ThreadState.cpp:681 (content_browsertests+0x000001dc31f2)
    #3 blink::shutdown() third_party/WebKit/Source/web/WebKit.cpp:205 (content_browsertests+0x000001bdca27)
    #4 content::WorkerThread::Shutdown() content/worker/worker_thread.cc:88 (content_browsertests+0x000004a3901d)
    #5 content::ChildProcess::~ChildProcess() content/child/child_process.cc:67 (content_browsertests+0x0000039e069d)
    #6 content::WorkerMain(content::MainFunctionParams const&) content/worker/worker_main.cc:71 (content_browsertests+0x000004a386e1)
    #7 content::RunZygote(content::MainFunctionParams const&, content::ContentMainDelegate*) content/app/content_main_runner.cc:397 (content_browsertests+0x000004a27689)
    #8 content::RunNamedProcessTypeMain(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) content/app/content_main_runner.cc:480 (content_browsertests+0x000004a28193)
    #9 content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:792 (content_browsertests+0x000004a29043)
    #10 content::ContentMain(int, char const**, content::ContentMainDelegate*) content/app/content_main.cc:35 (content_browsertests+0x000004a2700e)
    #11 RunContentMain content/public/test/test_launcher.cc:453 (content_browsertests+0x00000428f082)
    #12 content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) content/public/test/test_launcher.cc:491 (content_browsertests+0x00000428f082)
    #13 main content/test/content_test_launcher.cc:137 (content_browsertests+0x00000083646e)

  Location is global 'WebCore::ThreadState::s_mainThreadStateStorage' of size 200 at 0x7f8267b29560 (content_browsertests+0x00000677e588)
...
==================
WARNING: ThreadSanitizer: data race (pid=5135)
  Read of size 8 at 0x7fff81b56170 by thread T7 (mutexes: write M605):
    #0 visitStack third_party/WebKit/Source/heap/ThreadState.cpp:361 (content_browsertests+0x000001dc1d37)
    #1 trace third_party/WebKit/Source/heap/ThreadState.cpp:375 (content_browsertests+0x000001dc1d37)
    #2 WebCore::ThreadState::visitRoots(WebCore::Visitor*) third_party/WebKit/Source/heap/ThreadState.cpp:340 (content_browsertests+0x000001dc1d37)
    #3 WebCore::Heap::collectGarbage(WebCore::ThreadState::StackState, WebCore::Heap::GCType) third_party/WebKit/Source/heap/Heap.cpp:1291 (content_browsertests+0x000001dbe746)
    #4 WebCore::ThreadState::cleanup() third_party/WebKit/Source/heap/ThreadState.cpp:301 (content_browsertests+0x000001dc140d)
    #5 WebCore::WorkerThread::workerThread() third_party/WebKit/Source/core/workers/WorkerThread.cpp:148 (content_browsertests+0x000002b9fbc8)
    #6 WebCore::WorkerThread::workerThreadStart(void*) third_party/WebKit/Source/core/workers/WorkerThread.cpp:104 (content_browsertests+0x000002b9f5e9)
    #7 WTF::threadEntryPoint(void*) third_party/WebKit/Source/wtf/Threading.cpp:69 (content_browsertests+0x00000459f69f)
    #8 WTF::wtfThreadEntryPoint(void*) third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:175 (content_browsertests+0x000001c5866d)

  Previous write of size 8 at 0x7fff81b56170 by main thread:
    #0 WebCore::ThreadState::SafePointScope::SafePointScope(WebCore::ThreadState::StackState, WebCore::ThreadState::SafePointScope::ScopeNesting) third_party/WebKit/Source/heap/ThreadState.h:359 (content_browsertests+0x000001dc3100)
    #1 WebCore::ThreadState::removeInterruptor(WebCore::ThreadState::Interruptor*) third_party/WebKit/Source/heap/ThreadState.cpp:681 (content_browsertests+0x000001dc31f2)
    #2 blink::shutdown() third_party/WebKit/Source/web/WebKit.cpp:205 (content_browsertests+0x000001bdca27)
    #3 content::WorkerThread::Shutdown() content/worker/worker_thread.cc:88 (content_browsertests+0x000004a3901d)
    #4 content::ChildProcess::~ChildProcess() content/child/child_process.cc:67 (content_browsertests+0x0000039e069d)
    #5 content::WorkerMain(content::MainFunctionParams const&) content/worker/worker_main.cc:71 (content_browsertests+0x000004a386e1)
    #6 content::RunZygote(content::MainFunctionParams const&, content::ContentMainDelegate*) content/app/content_main_runner.cc:397 (content_browsertests+0x000004a27689)
    #7 content::RunNamedProcessTypeMain(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) content/app/content_main_runner.cc:480 (content_browsertests+0x000004a28193)
    #8 content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:792 (content_browsertests+0x000004a29043)
    #9 content::ContentMain(int, char const**, content::ContentMainDelegate*) content/app/content_main.cc:35 (content_browsertests+0x000004a2700e)
    #10 RunContentMain content/public/test/test_launcher.cc:453 (content_browsertests+0x00000428f082)
    #11 content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) content/public/test/test_launcher.cc:491 (content_browsertests+0x00000428f082)
    #12 main content/test/content_test_launcher.cc:137 (content_browsertests+0x00000083646e)

  Location is stack of main thread.
...

 
log.txt
285 KB View Download
Comment 1 by glider@chromium.org, Feb 27 2014
In addition to other races there are two variables, m_canResume and m_unparkedThreadCount, declared as volatile int and treated as atomic variables in certain places.
This is by no means thread-safe. A better solution is to adopt some atomicops implementation, e.g. base/atomicops.h from Chromium.
The code was actually written using base/atomicops.h originally but for various reasons we could not bring it in as-is into Blink. There are also questions to the quality of atomicops.h implementation on some platforms. I'll see what we can do given that we are confined to the limited set of atomics available in Blink.

As for the TSAN report above I have yet to go through this stuff but some of these don't look right, e.g. for

Read of size 8 at 0x7f8267b29588 by thread T7 (mutexes: write M605):
    #0 visitStack third_party/WebKit/Source/heap/ThreadState.cpp:353 (content_browsertests+0x000001dc1d0b)

Location will never be read unless respective thread is at safe-point and there are full barriers in right places (e.g. doEnterSafePoint does atomicDecrement in doEnterSafePoint which is a full barrier to the best of my knowledge).

Why does ASAN thinks there is data-race here? 

Comment 3 by glider@chromium.org, Feb 27 2014
According to the race report above, TSan correctly understood that doEnterSafePoint executes an atomic decrement (look for '__tsan_atomic32_fetch_sub')
But there's another place where m_unparkedThreadCount is being read without an acquire barrier:

130        while (m_unparkedThreadCount > 0)
131            m_parked.wait(m_mutex);

Dima suggested to 'fix' it with __sync_fetch_and_add(&m_unparkedThreadCount, 0) to just check whether the race reports go away. This removed one of the race reports, but not the other ones, since there are nonatomic accesses to other variables like m_canResume.

PS: If you want to experiment with TSan yourself, there're build instructions at http://dev.chromium.org/developers/testing/threadsanitizer-tsan-v2
@glider: the report I referred to is not about m_unparkedThreadCount, but about m_safePointScopeMarker from the looks of it.

Also I don't entirely understand why this load you mention would require acquire semantics. I think it should be just a NoBarrier_Load in terms on base/atomics.h. Is my reasoning not correct in that matter?
Comment 5 by dvyukov@google.com, Feb 27 2014
> Location will never be read unless respective thread is at safe-point and there are full barriers in right place

How does the thread that reads understands that the other thread is at safepoint?


> How does the thread that reads understands that the other thread is at safepoint?

Precisely with the code from above:

130        while (m_unparkedThreadCount > 0)
131            m_parked.wait(m_mutex);
Comment 7 by dvyukov@google.com, Feb 27 2014
Then load of m_unparkedThreadCount has to be acquire. Otherwise this thread is not synchronized with the parked thread and cannot read any of it's data.

@7: I still don't compeltely understand the reasoning for using Acquire semantics. My reasoning goes as follows:

Decrementing m_unparkedThreadCount is a full barrier thus nothing can be reordered past it and if 0 is observed by a requesting thread that means all other writes done prior to decrementing can be observed as well. 

Any reason why this would not be true, in other words when can it happen that we can read 0 from m_unparkedThreadCount but later not observe any rights the decrementing thread did prior to decrement? 
Comment 9 by dvyukov@google.com, Feb 27 2014
The C/C++ memory model does not take control dependencies into consideration. So reads of thread data are not ordered with reading of m_unparkedThreadCount==0. A compiler can well emit speculative loads of thread data before the read of m_unparkedThreadCount, and then use the loaded data later.
As a general rule of thumb: a single barrier cannot ever make any sense. It's always a game of two. One thread must do release, and another thread must do acquire. Then these accesses synchronize.

Ok, I am starting to see your reasoning for Acquire_Load() here instead of NoBarrier_Load.

Though in a practical sense I really don't see any compiler moving m_safePointScopeMarker load from far-far down the control flow --- up and past 

130        while (m_unparkedThreadCount > 0)
131            m_parked.wait(m_mutex);

especially with conditional m_parked.wait(m_mutex) in between, which I expect is a full barrier (of course speculative load can be duplicated on both code paths). 

For logical correctness I will put Acquire_Load here or just issue a full memory barrier at the end of the whole function and check other similar places where my reasoning was following similar trail.
Another question while we are on the topic: does pthread_mutex_unlock have acquire/release/full barrier semantics or nothing is guaranteed?
> For logical correctness I will put Acquire_Load here or just issue a full memory barrier at the end of the whole function and check other similar places where my reasoning was following similar trail.

Unfortunately TSan doesn't know how to deal with a full memory barrier, because it's hard to infer the happens-before relation from that.
In order to still let TSan reason about this code (and check the similar places), can we have an Acquire_Load here?
> Another question while we are on the topic: does pthread_mutex_unlock have acquire/release/full barrier semantics or nothing is guaranteed?

pthread_mutex_unlock always happens before the next pthread_mutex_lock on the same mutex.
Comment 14 by dvyukov@google.com, Feb 27 2014
I agree that it's an unlikely optimization. However, compilers only become smarter over time. They can do global analysis, they know semantics of some library functions (e.g. malloc/free already today).

On x86 the acquire barrier is free. On ARM it is not free, but it is strictly necessary at the same time. Consider that this thread reads m_unparkedThreadCount=0 right after another thread has decremented it to 0. Thus this thread returns w/o executing m_parked.wait(m_mutex), but it does not necessary acquire visibility over other thread's data yet.

POSIX does not operate with acqurie/release terms. It just [vaguely] specifies sequential consistency for race-free programs (all shared access are protected with mutexes). But from practical point of view, reasonable implementations of pthread_mutex_unlock must have a release barrier associated with some word in mutex state.

The problem with Acquire_Load is that we don't have access to atomicops.h. I will have to port bits and pieces including TSAN specific Acquire_Load implementation into the Blink.
> Consider that this thread reads m_unparkedThreadCount=0 right after another thread has decremented it to 0.
> Thus this thread returns w/o executing m_parked.wait(m_mutex), but it does not necessary acquire visibility over other thread's data yet.

This was exactly my question from #8. Does ARM memory model allow such thing to happen, i.e. partial visibility like that? Any documentation on this I can read?


Comment 17 by dvyukov@google.com, Feb 27 2014
> The problem with Acquire_Load is that we don't have access to atomicops.h. I will have to port bits and pieces including TSAN specific Acquire_Load implementation into the Blink.

Investing into flexible, efficient and tsan-supported atomics for a project that is performance-critical and relies heavily on fine-grained atomic looks like a good investment to me.

> This was exactly my question from #8. Does ARM memory model allow such thing to happen, i.e. partial visibility like that? Any documentation on this I can read?

I don't know where it is documented. ARM respects data-dependencies. For anything else it requires explicit ordering with DMB instruction.


Yet another report from http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28TSan%20v2%29%282%29/builds/2065/steps/content_browsertests/logs/stdio:

[ RUN      ] WorkerTest.WorkerClose
Xlib:  extension "RANDR" missing on display ":9".
[15984:16020:0311/054220:1673534671:WARNING:proxy_service.cc(903)] PAC support disabled because there is no system implementation
[16065:16065:0311/054221:1673993404:ERROR:renderer_main.cc(227)] Running without renderer sandbox
==================
WARNING: ThreadSanitizer: data race (pid=16065)
  Atomic write of size 4 at 0x7d240000d380 by thread T9 (mutexes: write M2585):
    #0 __tsan_atomic32_exchange /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cc:467 (content_browsertests+0x000000640508)
    #1 size third_party/WebKit/Source/wtf/HashTable.h:303 (content_browsertests+0x000001d1689e)
    #2 size third_party/WebKit/Source/wtf/HashSet.h:147 (content_browsertests+0x000001d1689e)
    #3 WebCore::SafePointBarrier::resumeOthers() third_party/WebKit/Source/heap/ThreadState.cpp:137 (content_browsertests+0x000001d1689e)
    #4 WebCore::ThreadState::resumeThreads() third_party/WebKit/Source/heap/ThreadState.cpp:581 (content_browsertests+0x000001d167d4)
    #5 ~NoAllocationScope third_party/WebKit/Source/heap/Heap.cpp:281 (content_browsertests+0x000001d1265b)
    #6 WebCore::Heap::collectGarbage(WebCore::ThreadState::StackState, WebCore::Heap::GCType) third_party/WebKit/Source/heap/Heap.cpp:1319 (content_browsertests+0x000001d1265b)
    #7 WebCore::Heap::collectAllGarbage(WebCore::ThreadState::StackState, WebCore::Heap::GCType) third_party/WebKit/Source/heap/Heap.cpp:1329 (content_browsertests+0x000001d1422c)
    #8 WebCore::ThreadState::cleanup() third_party/WebKit/Source/heap/ThreadState.cpp:302 (content_browsertests+0x000001d154bd)
    #9 WebCore::ThreadState::detach() third_party/WebKit/Source/heap/ThreadState.cpp:317 (content_browsertests+0x000001d1572d)
    #10 WebCore::WorkerThread::workerThread() third_party/WebKit/Source/core/workers/WorkerThread.cpp:150 (content_browsertests+0x000002b296a4)
    #11 WebCore::WorkerThread::workerThreadStart(void*) third_party/WebKit/Source/core/workers/WorkerThread.cpp:104 (content_browsertests+0x000002b29159)
    #12 WTF::threadEntryPoint(void*) third_party/WebKit/Source/wtf/Threading.cpp:69 (content_browsertests+0x00000445270f)
    #13 WTF::wtfThreadEntryPoint(void*) third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:175 (content_browsertests+0x000001badc0d)

  Previous read of size 4 at 0x7d240000d380 by main thread (mutexes: write M2583):
    #0 doPark third_party/WebKit/Source/heap/ThreadState.cpp:167 (content_browsertests+0x000001d18042)
    #1 WebCore::SafePointBarrier::parkAfterPushRegisters(WebCore::SafePointBarrier*, WebCore::ThreadState*, long*) third_party/WebKit/Source/heap/ThreadState.cpp:215 (content_browsertests+0x000001d18042)
    #2 <null> <null>:0 (content_browsertests+0x000001d181a4)
    #3 WebCore::PendingGCRunner::didProcessTask() third_party/WebKit/Source/heap/glue/PendingGCRunner.h:62 (content_browsertests+0x000001b34928)
    #4 content::WebThreadBase::TaskObserverAdapter::DidProcessTask(base::PendingTask const&) content/child/webthread_impl.cc:32 (content_browsertests+0x00000399986f)
    #5 base::MessageLoop::RunTask(base::PendingTask const&) base/message_loop/message_loop.cc:450 (content_browsertests+0x000000ec8f4f)
    #6 DeferOrRunPendingTask base/message_loop/message_loop.cc:461 (content_browsertests+0x000000ec9da2)
    #7 base::MessageLoop::DoWork() base/message_loop/message_loop.cc:575 (content_browsertests+0x000000ec9da2)
    #8 base::MessagePumpDefault::Run(base::MessagePump::Delegate*) base/message_loop/message_pump_default.cc:32 (content_browsertests+0x000000eccb20)
    #9 base::MessageLoop::RunHandler() base/message_loop/message_loop.cc:399 (content_browsertests+0x000000ec8532)
    #10 base::RunLoop::Run() base/run_loop.cc:49 (content_browsertests+0x000000ee434f)
    #11 base::MessageLoop::Run() base/message_loop/message_loop.cc:292 (content_browsertests+0x000000ec7d75)
    #12 content::RendererMain(content::MainFunctionParams const&) content/renderer/renderer_main.cc:252 (content_browsertests+0x0000048b70e6)
    #13 content::RunZygote(content::MainFunctionParams const&, content::ContentMainDelegate*) content/app/content_main_runner.cc:391 (content_browsertests+0x0000048696ba)
    #14 content::RunNamedProcessTypeMain(std::string const&, content::MainFunctionParams const&, content::ContentMainDelegate*) content/app/content_main_runner.cc:474 (content_browsertests+0x000004869e60)
    #15 content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:779 (content_browsertests+0x00000486a8d0)
    #16 content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:19 (content_browsertests+0x00000486932e)
    #17 RunContentMain content/public/test/test_launcher.cc:455 (content_browsertests+0x000004111f30)
    #18 content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) content/public/test/test_launcher.cc:491 (content_browsertests+0x000004111f30)
    #19 main content/test/content_test_launcher.cc:137 (content_browsertests+0x000000898f1e)

  Location is heap block of size 144 at 0x7d240000d380 allocated by main thread:
    #0 operator new(unsigned long) /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:549 (content_browsertests+0x000000604f6d)
    #1 WebCore::ThreadState::init() third_party/WebKit/Source/heap/ThreadState.cpp:273 (content_browsertests+0x000001d150f3)
    #2 WebCore::Heap::init() third_party/WebKit/Source/heap/Heap.cpp:1219 (content_browsertests+0x000001d13cf4)
    #3 blink::initializeWithoutV8(blink::Platform*) third_party/WebKit/Source/web/WebKit.cpp:164 (content_browsertests+0x000001b3400e)
    #4 blink::initialize(blink::Platform*) third_party/WebKit/Source/web/WebKit.cpp:105 (content_browsertests+0x000001b33e11)
    #5 content::RenderThreadImpl::EnsureWebKitInitialized() content/renderer/render_thread_impl.cc:693 (content_browsertests+0x0000009cad80)
    #6 OnCreateNewView content/renderer/render_thread_impl.cc:1215 (content_browsertests+0x0000009ceec1)
    #7 DispatchToMethod<content::RenderThreadImpl, void (content::RenderThreadImpl::*)(const ViewMsg_New_Params &), ViewMsg_New_Params> base/tuple.h:548 (content_browsertests+0x0000009ceec1)
    #8 Dispatch<content::RenderThreadImpl, content::RenderThreadImpl, void (content::RenderThreadImpl::*)(const ViewMsg_New_Params &)> content/common/view_messages.h:590 (content_browsertests+0x0000009ceec1)
    #9 content::RenderThreadImpl::OnControlMessageReceived(IPC::Message const&) content/renderer/render_thread_impl.cc:1198 (content_browsertests+0x0000009ceec1)
    #10 non-virtual thunk to content::RenderThreadImpl::OnControlMessageReceived(IPC::Message const&) content/renderer/render_thread_impl.cc:1212 (content_browsertests+0x0000009cf6d4)
    #11 content::ChildThread::OnMessageReceived(IPC::Message const&) content/child/child_thread.cc:441 (content_browsertests+0x0000039241f5)
    #12 IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message const&) ipc/ipc_channel_proxy.cc:372 (content_browsertests+0x000000f3cca5)
    #13 Run base/bind_internal.h:190 (content_browsertests+0x000000f3ffad)
    #14 MakeItSo base/bind_internal.h:898 (content_browsertests+0x000000f3ffad)
    #15 base::internal::Invoker<2, base::internal::BindState<base::internal::RunnableAdapter<void (IPC::ChannelProxy::Context::*)(IPC::Message const&)>, void (IPC::ChannelProxy::Context*, IPC::Message const&), void (IPC::ChannelProxy::Context*, IPC::Message)>, void (IPC::ChannelProxy::Context*, IPC::Message const&)>::Run(base::internal::BindStateBase*) base/bind_internal.h:1248 (content_browsertests+0x000000f3ffad)
    #16 Run base/callback.h:401 (content_browsertests+0x000000ec8cd3)
    #17 base::MessageLoop::RunTask(base::PendingTask const&) base/message_loop/message_loop.cc:449 (content_browsertests+0x000000ec8cd3)
    #18 DeferOrRunPendingTask base/message_loop/message_loop.cc:461 (content_browsertests+0x000000ec9da2)
    #19 base::MessageLoop::DoWork() base/message_loop/message_loop.cc:575 (content_browsertests+0x000000ec9da2)
    #20 base::MessagePumpDefault::Run(base::MessagePump::Delegate*) base/message_loop/message_pump_default.cc:32 (content_browsertests+0x000000eccb20)
    #21 base::MessageLoop::RunHandler() base/message_loop/message_loop.cc:399 (content_browsertests+0x000000ec8532)
    #22 base::RunLoop::Run() base/run_loop.cc:49 (content_browsertests+0x000000ee434f)
    #23 base::MessageLoop::Run() base/message_loop/message_loop.cc:292 (content_browsertests+0x000000ec7d75)
    #24 content::RendererMain(content::MainFunctionParams const&) content/renderer/renderer_main.cc:252 (content_browsertests+0x0000048b70e6)
    #25 content::RunZygote(content::MainFunctionParams const&, content::ContentMainDelegate*) content/app/content_main_runner.cc:391 (content_browsertests+0x0000048696ba)
    #26 content::RunNamedProcessTypeMain(std::string const&, content::MainFunctionParams const&, content::ContentMainDelegate*) content/app/content_main_runner.cc:474 (content_browsertests+0x000004869e60)
    #27 content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:779 (content_browsertests+0x00000486a8d0)
    #28 content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:19 (content_browsertests+0x00000486932e)
    #29 RunContentMain content/public/test/test_launcher.cc:455 (content_browsertests+0x000004111f30)
    #30 content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) content/public/test/test_launcher.cc:491 (content_browsertests+0x000004111f30)
    #31 main content/test/content_test_launcher.cc:137 (content_browsertests+0x000000898f1e)


Project Member Comment 19 by bugdroid1@chromium.org, Mar 11 2014
------------------------------------------------------------------------
r256230 | glider@chromium.org | 2014-03-11T15:03:55.437411Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/tools/valgrind/tsan_v2/suppressions.txt?r1=256230&r2=256229&pathrev=256230

More TSan v2 suppressions:
 -- races in third_party/WebKit/Source/heap/ThreadState.cpp (issue 347559)

BUG= 347559 
TBR=timurrrr@chromium.org
NOTRY=true

Review URL: https://codereview.chromium.org/195173002
------------------------------------------------------------------------
Project Member Comment 20 by bugdroid1@chromium.org, Mar 20 2014
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=169469

------------------------------------------------------------------
r169469 | vegorov@chromium.org | 2014-03-18T20:17:21.401130Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/wtf/Atomics.h?r1=169469&r2=169468&pathrev=169469
   M http://src.chromium.org/viewvc/blink/trunk/Source/heap/ThreadState.cpp?r1=169469&r2=169468&pathrev=169469

Introduce WTF::releaseStore and WTF::acquireLoad.

Use them to avoid data races in the SafePointBarrier discovered by TSAN and guarantee that compiler does not move any memory accesses around m_unparkedThreadCount and m_canResume loads.

This change is likely to affect only ARM with its weakly ordered memory model.

BUG= 347559 

Review URL: https://codereview.chromium.org/199413012
-----------------------------------------------------------------
Project Member Comment 21 by bugdroid1@chromium.org, Mar 20 2014
------------------------------------------------------------------
r258305 | vegorov@chromium.org | 2014-03-20T15:09:12.021856Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/tools/valgrind/tsan_v2/suppressions.txt?r1=258305&r2=258304&pathrev=258305

Remove SafePointBarrier related TSAN suppressions.

BUG= 347559 
NOTRY=true

Review URL: https://codereview.chromium.org/205893007
-----------------------------------------------------------------
Status: Fixed
Sign in to add a comment