Do not require synchronization access on the thread if called from rtc::Thread::WrapCurrent.

The synchronization access is unnecessary for rtc::Thread::WrapCurrent (called from JingleThreadWrapper) since JingleThreadWrapper never calls rtc::Thread::Stop or rtc::Thread::Join. Failing to get the access caused crashes in Chrome since rtc::Thread::Current will be NULL when rtc::Thread::WrapCurrent fails.

rtc::ThreadManager::WrapCurrentThread still requires the synchronization access, since I am not sure if the callers (e.g. the plugin) depends on it.

BUG=crbug/413853
R=juberti@webrtc.org, tommi@webrtc.org

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

git-svn-id: http://webrtc.googlecode.com/svn/trunk@7224 4adac7df-926f-26a2-2b94-8c16560cd09d

webrtc/base/thread.cc

@@ -107,7 +107,7 @@ Thread *ThreadManager::WrapCurrentThread() {
   Thread* result = CurrentThread();
   if (NULL == result) {
     result = new Thread();
-    result->WrapCurrentWithThreadManager(this);
+    result->WrapCurrentWithThreadManager(this, true);
   }
   return result;
 }
@@ -188,6 +188,7 @@ bool Thread::SetName(const std::string& name, const void* obj) {
 bool Thread::SetPriority(ThreadPriority priority) {
 #if defined(WEBRTC_WIN)
   if (running()) {
+    ASSERT(thread_ != NULL);
     BOOL ret = FALSE;
     if (priority == PRIORITY_NORMAL) {
       ret = ::SetThreadPriority(thread_, THREAD_PRIORITY_NORMAL);
@@ -288,12 +289,35 @@ bool Thread::Start(Runnable* runnable) {
   return true;
 }
 
+bool Thread::WrapCurrent() {
+  return WrapCurrentWithThreadManager(ThreadManager::Instance(), true);
+}
+
+void Thread::UnwrapCurrent() {
+  // Clears the platform-specific thread-specific storage.
+  ThreadManager::Instance()->SetCurrentThread(NULL);
+#if defined(WEBRTC_WIN)
+  if (thread_ != NULL) {
+    if (!CloseHandle(thread_)) {
+      LOG_GLE(LS_ERROR) << "When unwrapping thread, failed to close handle.";
+    }
+    thread_ = NULL;
+  }
+#endif
+  running_.Reset();
+}
+
+void Thread::SafeWrapCurrent() {
+  WrapCurrentWithThreadManager(ThreadManager::Instance(), false);
+}
+
 void Thread::Join() {
   AssertBlockingIsAllowedOnCurrentThread();
 
   if (running()) {
     ASSERT(!IsCurrent());
 #if defined(WEBRTC_WIN)
+    ASSERT(thread_ != NULL);
     WaitForSingleObject(thread_, INFINITE);
     CloseHandle(thread_);
     thread_ = NULL;
@@ -526,43 +550,32 @@ bool Thread::ProcessMessages(int cmsLoop) {
   }
 }
 
-bool Thread::WrapCurrent() {
-  return WrapCurrentWithThreadManager(ThreadManager::Instance());
-}
-
-bool Thread::WrapCurrentWithThreadManager(ThreadManager* thread_manager) {
+bool Thread::WrapCurrentWithThreadManager(ThreadManager* thread_manager,
+                                          bool need_synchronize_access) {
   if (running())
     return false;
+
 #if defined(WEBRTC_WIN)
-  // We explicitly ask for no rights other than synchronization.
-  // This gives us the best chance of succeeding.
-  thread_ = OpenThread(SYNCHRONIZE, FALSE, GetCurrentThreadId());
-  if (!thread_) {
-    LOG_GLE(LS_ERROR) << "Unable to get handle to thread.";
-    return false;
+  if (need_synchronize_access) {
+    // We explicitly ask for no rights other than synchronization.
+    // This gives us the best chance of succeeding.
+    thread_ = OpenThread(SYNCHRONIZE, FALSE, GetCurrentThreadId());
+    if (!thread_) {
+      LOG_GLE(LS_ERROR) << "Unable to get handle to thread.";
+      return false;
+    }
+    thread_id_ = GetCurrentThreadId();
   }
-  thread_id_ = GetCurrentThreadId();
 #elif defined(WEBRTC_POSIX)
   thread_ = pthread_self();
 #endif
+
   owned_ = false;
   running_.Set();
   thread_manager->SetCurrentThread(this);
   return true;
 }
 
-void Thread::UnwrapCurrent() {
-  // Clears the platform-specific thread-specific storage.
-  ThreadManager::Instance()->SetCurrentThread(NULL);
-#if defined(WEBRTC_WIN)
-  if (!CloseHandle(thread_)) {
-    LOG_GLE(LS_ERROR) << "When unwrapping thread, failed to close handle.";
-  }
-#endif
-  running_.Reset();
-}
-
-
 AutoThread::AutoThread(SocketServer* ss) : Thread(ss) {
   if (!ThreadManager::Instance()->CurrentThread()) {
     ThreadManager::Instance()->SetCurrentThread(this);

webrtc/base/thread.h

@@ -202,15 +202,6 @@ class Thread : public MessageQueue {
   }
 #endif
 
-  // This method should be called when thread is created using non standard
-  // method, like derived implementation of rtc::Thread and it can not be
-  // started by calling Start(). This will set started flag to true and
-  // owned to false. This must be called from the current thread.
-  // NOTE: These methods should be used by the derived classes only, added here
-  // only for testing.
-  bool WrapCurrent();
-  void UnwrapCurrent();
-
   // Expose private method running() for tests.
   //
   // DANGER: this is a terrible public API.  Most callers that might want to
@@ -220,6 +211,18 @@ class Thread : public MessageQueue {
   bool RunningForTest() { return running(); }
 
  protected:
+  // This method should be called when thread is created using non standard
+  // method, like derived implementation of rtc::Thread and it can not be
+  // started by calling Start(). This will set started flag to true and
+  // owned to false. This must be called from the current thread.
+  bool WrapCurrent();
+  void UnwrapCurrent();
+
+  // Same as WrapCurrent except that it never fails as it does not try to
+  // acquire the synchronization access of the thread. The caller should never
+  // call Stop() or Join() on this thread.
+  void SafeWrapCurrent();
+
   // Blocks the calling thread until this thread has terminated.
   void Join();
 
@@ -237,7 +240,10 @@ class Thread : public MessageQueue {
   // ThreadManager calls this instead WrapCurrent() because
   // ThreadManager::Instance() cannot be used while ThreadManager is
   // being created.
-  bool WrapCurrentWithThreadManager(ThreadManager* thread_manager);
+  // The method tries to get synchronization rights of the thread on Windows if
+  // |need_synchronize_access| is true.
+  bool WrapCurrentWithThreadManager(ThreadManager* thread_manager,
+                                    bool need_synchronize_access);
 
   // Return true if the thread was started and hasn't yet stopped.
   bool running() { return running_.Wait(0); }

webrtc/base/thread_unittest.cc

@@ -105,6 +105,13 @@ class CustomThread : public rtc::Thread {
   CustomThread() {}
   virtual ~CustomThread() { Stop(); }
   bool Start() { return false; }
+
+  bool WrapCurrent() {
+    return Thread::WrapCurrent();
+  }
+  void UnwrapCurrent() {
+    Thread::UnwrapCurrent();
+  }
 };
 
 
@@ -240,8 +247,6 @@ TEST(ThreadTest, Priorities) {
 }
 
 TEST(ThreadTest, Wrap) {
-  Thread* current_thread = Thread::Current();
-  current_thread->UnwrapCurrent();
   CustomThread* cthread = new CustomThread();
   EXPECT_TRUE(cthread->WrapCurrent());
   EXPECT_TRUE(cthread->RunningForTest());
@@ -249,7 +254,6 @@ TEST(ThreadTest, Wrap) {
   cthread->UnwrapCurrent();
   EXPECT_FALSE(cthread->RunningForTest());
   delete cthread;
-  current_thread->WrapCurrent();
 }
 
 TEST(ThreadTest, Invoke) {