Fix a data race in the thread unit tests.

webrtc/base/thread_unittest.cc

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 119 matching lines @@
   }
 
   virtual void Run() {
     // Do nothing.
   }
 
  private:
   Event* event_;
 };
 
+// A bool wrapped in a mutex, to avoid data races. Using a volatile
+// bool should be sufficient for correct code ("eventual consistency"
+// between caches is sufficient), but we can't tell the compiler about
+// that, and then tsan complains about a data race.
+
+// See also discussion at
+// http://stackoverflow.com/questions/7223164/is-mutex-needed-to-synchronize-a-simple-flag-between-pthreads
+
+// Using std::atomic<bool> or std::atomic_flag in C++11 is probably
+// the right thing to do, but those features are not yet allowed. Or
+// rtc::AtomicInt, if/when that is added. Since the use isn't
+// performance critical, use a plain critical section for the time
+// being.
+
+class AtomicBool {
+ public:
+  explicit AtomicBool(bool value = false) : flag_(value) {}
+  AtomicBool& operator=(bool value) {
+    CritScope scoped_lock(&cs_);
+    flag_ = value;
+    return *this;
+  }
+  bool get() const {
+    CritScope scoped_lock(&cs_);
+    return flag_;
+  }
+
+ private:
+  mutable CriticalSection cs_;
+  bool flag_;
+};
+
 // Function objects to test Thread::Invoke.
 struct FunctorA {
   int operator()() { return 42; }
 };
 class FunctorB {
  public:
-  explicit FunctorB(bool* flag) : flag_(flag) {}
+  explicit FunctorB(AtomicBool* flag) : flag_(flag) {}
   void operator()() { if (flag_) *flag_ = true; }
  private:
-  bool* flag_;
+  AtomicBool* flag_;
 };
 struct FunctorC {
   int operator()() {
     Thread::Current()->ProcessMessages(50);
     return 24;
   }
 };
 
 // See: https://code.google.com/p/webrtc/issues/detail?id=2409
 TEST(ThreadTest, DISABLED_Main) {
@@ skipping 99 matching lines @@
   delete cthread;
   current_thread->WrapCurrent();
 }
 
 TEST(ThreadTest, Invoke) {
   // Create and start the thread.
   Thread thread;
   thread.Start();
   // Try calling functors.
   EXPECT_EQ(42, thread.Invoke<int>(FunctorA()));
-  bool called = false;
+  AtomicBool called;
   FunctorB f2(&called);
   thread.Invoke<void>(f2);
-  EXPECT_TRUE(called);
+  EXPECT_TRUE(called.get());
   // Try calling bare functions.
   struct LocalFuncs {
     static int Func1() { return 999; }
     static void Func2() {}
   };
   EXPECT_EQ(999, thread.Invoke<int>(&LocalFuncs::Func1));
   thread.Invoke<void>(&LocalFuncs::Func2);
 }
 
 // Verifies that two threads calling Invoke on each other at the same time does
@@ skipping 118 matching lines @@
   Event invoke_started_;
   Thread* expected_thread_;
 };
 
 TEST_F(AsyncInvokeTest, FireAndForget) {
   AsyncInvoker invoker;
   // Create and start the thread.
   Thread thread;
   thread.Start();
   // Try calling functor.
-  bool called = false;
+  AtomicBool called;
   invoker.AsyncInvoke<void>(&thread, FunctorB(&called));
-  EXPECT_TRUE_WAIT(called, kWaitTimeout);
+  EXPECT_TRUE_WAIT(called.get(), kWaitTimeout);
 }
 
 TEST_F(AsyncInvokeTest, WithCallback) {
   AsyncInvoker invoker;
   // Create and start the thread.
   Thread thread;
   thread.Start();
   // Try calling functor.
   SetExpectedThreadForIntCallback(Thread::Current());
   invoker.AsyncInvoke(&thread, FunctorA(),
@@ skipping 47 matching lines @@
     // Wait for the call to begin.
     ASSERT_TRUE(invoke_started_.Wait(kWaitTimeout));
   }
   // Invoker is destroyed. Function should not execute.
   Thread::Current()->ProcessMessages(kWaitTimeout);
   EXPECT_EQ(0, int_value_);
 }
 
 TEST_F(AsyncInvokeTest, Flush) {
   AsyncInvoker invoker;
-  bool flag1 = false;
-  bool flag2 = false;
+  AtomicBool flag1;
+  AtomicBool flag2;
   // Queue two async calls to the current thread.
   invoker.AsyncInvoke<void>(Thread::Current(),
                             FunctorB(&flag1));
   invoker.AsyncInvoke<void>(Thread::Current(),
                             FunctorB(&flag2));
   // Because we haven't pumped messages, these should not have run yet.
-  EXPECT_FALSE(flag1);
-  EXPECT_FALSE(flag2);
+  EXPECT_FALSE(flag1.get());
+  EXPECT_FALSE(flag2.get());
   // Force them to run now.
   invoker.Flush(Thread::Current());
-  EXPECT_TRUE(flag1);
-  EXPECT_TRUE(flag2);
+  EXPECT_TRUE(flag1.get());
+  EXPECT_TRUE(flag2.get());
 }
 
 TEST_F(AsyncInvokeTest, FlushWithIds) {
   AsyncInvoker invoker;
-  bool flag1 = false;
-  bool flag2 = false;
+  AtomicBool flag1;
+  AtomicBool flag2;
   // Queue two async calls to the current thread, one with a message id.
   invoker.AsyncInvoke<void>(Thread::Current(),
                             FunctorB(&flag1),
                             5);
   invoker.AsyncInvoke<void>(Thread::Current(),
                             FunctorB(&flag2));
   // Because we haven't pumped messages, these should not have run yet.
-  EXPECT_FALSE(flag1);
-  EXPECT_FALSE(flag2);
+  EXPECT_FALSE(flag1.get());
+  EXPECT_FALSE(flag2.get());
   // Execute pending calls with id == 5.
   invoker.Flush(Thread::Current(), 5);
-  EXPECT_TRUE(flag1);
-  EXPECT_FALSE(flag2);
+  EXPECT_TRUE(flag1.get());
+  EXPECT_FALSE(flag2.get());
   flag1 = false;
   // Execute all pending calls. The id == 5 call should not execute again.
   invoker.Flush(Thread::Current());
-  EXPECT_FALSE(flag1);
-  EXPECT_TRUE(flag2);
+  EXPECT_FALSE(flag1.get());
+  EXPECT_TRUE(flag2.get());
 }
 
 class GuardedAsyncInvokeTest : public testing::Test {
  public:
   void IntCallback(int value) {
     EXPECT_EQ(expected_thread_, Thread::Current());
     int_value_ = value;
   }
   void AsyncInvokeIntCallback(GuardedAsyncInvoker* invoker, Thread* thread) {
     expected_thread_ = thread;
@@ skipping 28 matching lines @@
 TEST_F(GuardedAsyncInvokeTest, KillThreadFireAndForget) {
   // Create and start the thread.
   scoped_ptr<Thread> thread(new Thread());
   thread->Start();
   scoped_ptr<GuardedAsyncInvoker> invoker;
   // Create the invoker on |thread|.
   thread->Invoke<void>(CreateInvoker(&invoker));
   // Kill |thread|.
   thread = nullptr;
   // Try calling functor.
-  bool called = false;
+  AtomicBool called;
   EXPECT_FALSE(invoker->AsyncInvoke<void>(FunctorB(&called)));
   // With thread gone, nothing should happen.
-  WAIT(called, kWaitTimeout);
-  EXPECT_FALSE(called);
+  WAIT(called.get(), kWaitTimeout);
+  EXPECT_FALSE(called.get());
 }
 
 // Test that we can call AsyncInvoke with callback after the thread died.
 TEST_F(GuardedAsyncInvokeTest, KillThreadWithCallback) {
   // Create and start the thread.
   scoped_ptr<Thread> thread(new Thread());
   thread->Start();
   scoped_ptr<GuardedAsyncInvoker> invoker;
   // Create the invoker on |thread|.
   thread->Invoke<void>(CreateInvoker(&invoker));
   // Kill |thread|.
   thread = nullptr;
   // Try calling functor.
   EXPECT_FALSE(
       invoker->AsyncInvoke(FunctorC(), &GuardedAsyncInvokeTest::IntCallback,
                            static_cast<GuardedAsyncInvokeTest*>(this)));
   // With thread gone, callback should be cancelled.
   Thread::Current()->ProcessMessages(kWaitTimeout);
   EXPECT_EQ(0, int_value_);
 }
 
 // The remaining tests check that GuardedAsyncInvoker behaves as AsyncInvoker
 // when Thread is still alive.
 TEST_F(GuardedAsyncInvokeTest, FireAndForget) {
   GuardedAsyncInvoker invoker;
   // Try calling functor.
-  bool called = false;
+  AtomicBool called;
   EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&called)));
-  EXPECT_TRUE_WAIT(called, kWaitTimeout);
+  EXPECT_TRUE_WAIT(called.get(), kWaitTimeout);
 }
 
 TEST_F(GuardedAsyncInvokeTest, WithCallback) {
   GuardedAsyncInvoker invoker;
   // Try calling functor.
   SetExpectedThreadForIntCallback(Thread::Current());
   EXPECT_TRUE(invoker.AsyncInvoke(FunctorA(),
                                   &GuardedAsyncInvokeTest::IntCallback,
                                   static_cast<GuardedAsyncInvokeTest*>(this)));
   EXPECT_EQ_WAIT(42, int_value_, kWaitTimeout);
@@ skipping 42 matching lines @@
     // Wait for the call to begin.
     ASSERT_TRUE(invoke_started_.Wait(kWaitTimeout));
   }
   // Invoker is destroyed. Function should not execute.
   Thread::Current()->ProcessMessages(kWaitTimeout);
   EXPECT_EQ(0, int_value_);
 }
 
 TEST_F(GuardedAsyncInvokeTest, Flush) {
   GuardedAsyncInvoker invoker;
-  bool flag1 = false;
-  bool flag2 = false;
+  AtomicBool flag1;
+  AtomicBool flag2;
   // Queue two async calls to the current thread.
   EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag1)));
   EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag2)));
   // Because we haven't pumped messages, these should not have run yet.
-  EXPECT_FALSE(flag1);
-  EXPECT_FALSE(flag2);
+  EXPECT_FALSE(flag1.get());
+  EXPECT_FALSE(flag2.get());
   // Force them to run now.
   EXPECT_TRUE(invoker.Flush());
-  EXPECT_TRUE(flag1);
-  EXPECT_TRUE(flag2);
+  EXPECT_TRUE(flag1.get());
+  EXPECT_TRUE(flag2.get());
 }
 
 TEST_F(GuardedAsyncInvokeTest, FlushWithIds) {
   GuardedAsyncInvoker invoker;
-  bool flag1 = false;
-  bool flag2 = false;
+  AtomicBool flag1;
+  AtomicBool flag2;
   // Queue two async calls to the current thread, one with a message id.
   EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag1), 5));
   EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag2)));
   // Because we haven't pumped messages, these should not have run yet.
-  EXPECT_FALSE(flag1);
-  EXPECT_FALSE(flag2);
+  EXPECT_FALSE(flag1.get());
+  EXPECT_FALSE(flag2.get());
   // Execute pending calls with id == 5.
   EXPECT_TRUE(invoker.Flush(5));
-  EXPECT_TRUE(flag1);
-  EXPECT_FALSE(flag2);
+  EXPECT_TRUE(flag1.get());
+  EXPECT_FALSE(flag2.get());
   flag1 = false;
   // Execute all pending calls. The id == 5 call should not execute again.
   EXPECT_TRUE(invoker.Flush());
-  EXPECT_FALSE(flag1);
-  EXPECT_TRUE(flag2);
+  EXPECT_FALSE(flag1.get());
+  EXPECT_TRUE(flag2.get());
 }
 
 #if defined(WEBRTC_WIN)
 class ComThreadTest : public testing::Test, public MessageHandler {
  public:
   ComThreadTest() : done_(false) {}
  protected:
   virtual void OnMessage(Message* message) {
     HRESULT hr = CoInitializeEx(NULL, COINIT_MULTITHREADED);
     // S_FALSE means the thread was already inited for a multithread apartment.
     EXPECT_EQ(S_FALSE, hr);
     if (SUCCEEDED(hr)) {
       CoUninitialize();
     }
     done_ = true;
   }
   bool done_;
 };
 
 TEST_F(ComThreadTest, ComInited) {
   Thread* thread = new ComThread();
   EXPECT_TRUE(thread->Start());
   thread->Post(this, 0);
   EXPECT_TRUE_WAIT(done_, 1000);
   delete thread;
 }
 #endif