From a3e6bec23aea397b5922a1a08845d0c3ed46a600 Mon Sep 17 00:00:00 2001
From: "henrike@webrtc.org"
 <henrike@webrtc.org@4adac7df-926f-26a2-2b94-8c16560cd09d>
Date: Fri, 18 Jan 2013 16:39:21 +0000
Subject: [PATCH] Posix Thread: Removes the setting of the run function to NULL
 which could cause data race.

BUG=http://code.google.com/p/chromium/issues/detail?id=103711
TESTED=Code analysis (no tools)

Review URL: https://webrtc-codereview.appspot.com/1008006

git-svn-id: http://webrtc.googlecode.com/svn/trunk@3388 4adac7df-926f-26a2-2b94-8c16560cd09d
---
 .../interface/thread_wrapper.h                |  4 +-
 webrtc/system_wrappers/source/thread_posix.cc | 38 +++++++------------
 .../system_wrappers/source/thread_unittest.cc |  9 +----
 3 files changed, 17 insertions(+), 34 deletions(-)

diff --git a/webrtc/system_wrappers/interface/thread_wrapper.h b/webrtc/system_wrappers/interface/thread_wrapper.h
index 11b7894575..e832363698 100644
--- a/webrtc/system_wrappers/interface/thread_wrapper.h
+++ b/webrtc/system_wrappers/interface/thread_wrapper.h
@@ -52,8 +52,8 @@ class ThreadWrapper {
   // prio        Thread priority. May require root/admin rights.
   // thread_name  NULL terminated thread name, will be visable in the Windows
   //             debugger.
-  static ThreadWrapper* CreateThread(ThreadRunFunction func = 0,
-                                     ThreadObj obj = 0,
+  static ThreadWrapper* CreateThread(ThreadRunFunction func,
+                                     ThreadObj obj,
                                      ThreadPriority prio = kNormalPriority,
                                      const char* thread_name = 0);
 
diff --git a/webrtc/system_wrappers/source/thread_posix.cc b/webrtc/system_wrappers/source/thread_posix.cc
index 7245b0b976..c821931973 100644
--- a/webrtc/system_wrappers/source/thread_posix.cc
+++ b/webrtc/system_wrappers/source/thread_posix.cc
@@ -97,8 +97,7 @@ extern "C"
   }
 }
 
-ThreadWrapper* ThreadPosix::Create(ThreadRunFunction func,
-                                   ThreadObj obj,
+ThreadWrapper* ThreadPosix::Create(ThreadRunFunction func, ThreadObj obj,
                                    ThreadPriority prio,
                                    const char* thread_name) {
   ThreadPosix* ptr = new ThreadPosix(func, obj, prio, thread_name);
@@ -174,9 +173,6 @@ ThreadPosix::~ThreadPosix() {
 
 bool ThreadPosix::Start(unsigned int& thread_id)
 {
-  if (!run_function_) {
-    return false;
-  }
   int result = pthread_attr_setdetachstate(&attr_, PTHREAD_CREATE_DETACHED);
   // Set the stack stack size to 1M.
   result |= pthread_attr_setstacksize(&attr_, 1024 * 1024);
@@ -195,15 +191,17 @@ bool ThreadPosix::Start(unsigned int& thread_id)
   if (result != 0) {
     return false;
   }
+  {
+    CriticalSectionScoped cs(crit_state_);
+    dead_ = false;
+  }
 
   // Wait up to 10 seconds for the OS to call the callback function. Prevents
   // race condition if Stop() is called too quickly after start.
   if (kEventSignaled != event_->Wait(WEBRTC_EVENT_10_SEC)) {
     WEBRTC_TRACE(kTraceError, kTraceUtility, -1,
                  "posix thread event never triggered");
-
     // Timed out. Something went wrong.
-    run_function_ = NULL;
     return true;
   }
 
@@ -291,7 +289,7 @@ bool ThreadPosix::Stop() {
 
   // TODO(hellner) why not use an event here?
   // Wait up to 10 seconds for the thread to terminate
-  for (int i = 0; i < 1000 && !dead; i++) {
+  for (int i = 0; i < 1000 && !dead; ++i) {
     timespec t;
     t.tv_sec = 0;
     t.tv_nsec = 10 * 1000 * 1000;
@@ -312,7 +310,6 @@ void ThreadPosix::Run() {
   {
     CriticalSectionScoped cs(crit_state_);
     alive_ = true;
-    dead_  = false;
   }
 #if (defined(WEBRTC_LINUX) || defined(WEBRTC_ANDROID))
   pid_ = GetThreadId();
@@ -331,22 +328,15 @@ void ThreadPosix::Run() {
                  "Thread without name started");
   }
   bool alive = true;
-  do {
-    if (run_function_) {
-      if (!run_function_(obj_)) {
-        alive = false;
-      }
-    } else {
-      alive = false;
+  bool run = true;
+  while (alive) {
+    run = run_function_(obj_);
+    CriticalSectionScoped cs(crit_state_);
+    if (!run) {
+      alive_ = false;
     }
-    {
-      CriticalSectionScoped cs(crit_state_);
-      if (!alive) {
-        alive_ = false;
-      }
-      alive = alive_;
-    }
-  } while (alive);
+    alive = alive_;
+  }
 
   if (set_thread_name_) {
     // Don't set the name for the trace thread because it may cause a
diff --git a/webrtc/system_wrappers/source/thread_unittest.cc b/webrtc/system_wrappers/source/thread_unittest.cc
index 6b55fb0bf7..60302d2bd5 100644
--- a/webrtc/system_wrappers/source/thread_unittest.cc
+++ b/webrtc/system_wrappers/source/thread_unittest.cc
@@ -15,20 +15,13 @@
 
 namespace webrtc {
 
-TEST(ThreadTest, NullFunctionPointer) {
-  webrtc::scoped_ptr<ThreadWrapper> thread(
-    webrtc::ThreadWrapper::CreateThread());
-  unsigned int id = 42;
-  EXPECT_FALSE(thread->Start(id));
-}
-
 // Function that does nothing, and reports success.
 bool NullRunFunction(void* obj) {
   return true;
 }
 
 TEST(ThreadTest, StartStop) {
-  ThreadWrapper* thread = ThreadWrapper::CreateThread(&NullRunFunction);
+  ThreadWrapper* thread = ThreadWrapper::CreateThread(&NullRunFunction, NULL);
   unsigned int id = 42;
   ASSERT_TRUE(thread->Start(id));
   EXPECT_TRUE(thread->Stop());