Change thread check to race check

webrtc/base/race_checker.cc

Index: webrtc/base/race_checker.cc
diff --git a/webrtc/base/race_checker.cc b/webrtc/base/race_checker.cc
index 9f0946ca76f87e1386219b21f8073676f3497a00..78d6a259b325d8599e1cbf0d2ed139e543d66300 100644
--- a/webrtc/base/race_checker.cc
+++ b/webrtc/base/race_checker.cc
@@ -14,6 +14,10 @@ namespace rtc {
 
 RaceChecker::RaceChecker() {}
 
+// Note that the implementation here is in itself racy, but we pretend it does
+// not matter. If there is indeed a race (two threads both think access_count_
+// is zero) access_count_ will likely only be incremented once, but decremented
+// twice, causing the IsThreadRefEqual() check to return false at a later point.

[kwiberg-webrtc, 2016/09/20 19:29:07]

It may be incremented once instead of twice, possibly followed by both threads
writing to accessing_thread_. A similar race may occur if two threads try to
leave the critical section at the same time, or if one leaves at the same time
as one enters. The only possible bad effect of a race should be that we fail to
detect a race.

However, this assumes that all reads of accessing_thread_ retrieve a value that
was previously written. This is not guaranteed to be the case! E.g. if
PlatformThreadRef is 64 bits and we have a 32-bit system, the write may happen
in two parts.

I'd be more comfortable if RaceChecker used atomics so that it itself was race
free, or if it was only enabled in debug builds. I don't expect that problematic
behavior is very likely, though, so fixing this isn't a high priority.

[pbos-webrtc, 2016/09/20 19:36:02]

If two threads use it, and anyone at any point writes garbage into access_count_
(or generate -1 by double-decrementing or whatever) then the thread ref check
will fail for either thread A or B in the future since they won't set
accessing_thread_ even though they use the critical section.

Atomics cause synchronization and could also potentially make a data race less
likely to be found, plus it adds non-zero overhead, especially on ARM platforms
etc.

The idea was that this should be used in release as well, and the cost being
that this is more probabilistic than deterministic in detection. Though I doubt
it has a very high false negative rate, unless a compiler manages to completely
miscompile it since it's not supposed to be racy.

[kwiberg-webrtc, 2016/09/20 20:01:31]

Yes, garbage in access_count_ shouldn't be a problem. At most, it'll cause us to
fail to detect a race. It's garbage in accessing_thread_ that could be bad. On
Windows, we end up comparing such values as plain integers, which is safe even
if they contain garbage, but where we use pthreads we compare them with
pthread_equal(). If that function e.g. interprets (parts of) its two pthread_t
arguments as indexes and uses those indexes to look stuff up in memory, we have
a problem.
Yes on both counts, although I wouldn't suspect that atomic instructions would
cause data races to be all that much less likely to occur. Did you try it and
find a nonnegligible effect?

Implementing this with proper atomics would make it exactly equivalent to a
mutex that explodes whenever there is contention (i.e. in exactly the
circumstances where a real mutex would busy-wait or sleep). That seems like it
ought to work just fine.
OK. That may serve as a convincing argument for why using a mutex or even
atomics might be too expensive.
The compiler doesn't need to miscompile this code---if you have a race, that's
undefined behavior, so the compiler is fully within its rights to produce code
that behaves very badly if a race occurs. This code relies on compiler
implementations and hardware to be nice. As a result, you have to know a bunch
of low-level details about the various processors, OSes, and compilers that we
use in order to assess if it's safe.

 bool RaceChecker::Acquire() const {
   const PlatformThreadRef current_thread = CurrentThreadRef();
   // Set new accessing thread if this is a new use.