From ba737cba1aa6607911b1ca10460423b4c3e51fb9 Mon Sep 17 00:00:00 2001 From: "jiayl@webrtc.org" Date: Thu, 18 Sep 2014 16:45:21 +0000 Subject: [PATCH] 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 | 63 ++++++++++++++++++++-------------- webrtc/base/thread.h | 26 ++++++++------ webrtc/base/thread_unittest.cc | 10 ++++-- 3 files changed, 61 insertions(+), 38 deletions(-) diff --git a/webrtc/base/thread.cc b/webrtc/base/thread.cc index 6da9a7fbb..9d2917d9a 100644 --- a/webrtc/base/thread.cc +++ b/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); diff --git a/webrtc/base/thread.h b/webrtc/base/thread.h index 742ba6dc0..25b0f569f 100644 --- a/webrtc/base/thread.h +++ b/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); } diff --git a/webrtc/base/thread_unittest.cc b/webrtc/base/thread_unittest.cc index 6a54ac7b3..6a6875745 100644 --- a/webrtc/base/thread_unittest.cc +++ b/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) {