From 2eb166079130628f908f07538333130e6885c12d Mon Sep 17 00:00:00 2001 From: "tommi@webrtc.org" Date: Sat, 7 Feb 2015 19:17:47 +0000 Subject: [PATCH] Switch ThreadCheckerImpl over to using PlatformThreadRef. Like PlatformThreadId, this type is borrowed from Chromium. The difference between the two is that PlatformThreadRef is pthread_t on posix platforms. On Windows PlatformThreadRef and PlatformThreadId are the same thing. The reason for this switch is pretty crazy. On Chromium's "Mac 10.9 dbg" bot, we have been seeing the following code: ThreadCheckerImpl::ThreadCheckerImpl() : valid_thread_(CurrentThreadId()) { fprintf(stderr, "*** valid=%d\n", valid_thread_); valid_thread_ = CurrentThreadId(); fprintf(stderr, "*** valid after=%d\n", valid_thread_); } print this: *** valid=946872320 *** valid after=5647 This is for the same thread checker instance. What's worse is that printing out what CurrentThreadId was returning, yielded that it was always returning 5647. After switching over to pthread_t on Mac, this stopped happening. So, to remove the current hack, reinstate the class on Mac and take a look at the next problem, I'm switching to pthread_t. Really looking forward to truly getting to the bottom of this. Tbr-ing since the build is essentially broken (we can't roll). TBR=pbos@webrtc.org Review URL: https://webrtc-codereview.appspot.com/37199004 Cr-Commit-Position: refs/heads/master@{#8283} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8283 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/base/thread_checker_impl.cc | 36 ++++++++++++++------------ webrtc/base/thread_checker_impl.h | 8 +++++- webrtc/base/thread_checker_unittest.cc | 18 ++----------- 3 files changed, 28 insertions(+), 34 deletions(-) diff --git a/webrtc/base/thread_checker_impl.cc b/webrtc/base/thread_checker_impl.cc index 5e4aab8e32..5d59c99c10 100644 --- a/webrtc/base/thread_checker_impl.cc +++ b/webrtc/base/thread_checker_impl.cc @@ -25,8 +25,6 @@ PlatformThreadId CurrentThreadId() { #if defined(WEBRTC_WIN) ret = GetCurrentThreadId(); #elif defined(WEBRTC_POSIX) - // Pthreads doesn't have the concept of a thread ID, so we have to reach down - // into the kernel. #if defined(WEBRTC_MAC) || defined(WEBRTC_IOS) ret = pthread_mach_thread_np(pthread_self()); #elif defined(WEBRTC_LINUX) @@ -42,30 +40,34 @@ PlatformThreadId CurrentThreadId() { return ret; } -ThreadCheckerImpl::ThreadCheckerImpl() : valid_thread_(CurrentThreadId()) { +PlatformThreadRef CurrentThreadRef() { +#if defined(WEBRTC_WIN) + return GetCurrentThreadId(); +#elif defined(WEBRTC_POSIX) + return pthread_self(); +#endif +} + +bool IsThreadRefEqual(const PlatformThreadRef& a, const PlatformThreadRef& b) { +#if defined(WEBRTC_WIN) + return a == b; +#elif defined(WEBRTC_POSIX) + return pthread_equal(a, b); +#endif +} + +ThreadCheckerImpl::ThreadCheckerImpl() : valid_thread_(CurrentThreadRef()) { } ThreadCheckerImpl::~ThreadCheckerImpl() { } bool ThreadCheckerImpl::CalledOnValidThread() const { - const PlatformThreadId current_thread = CurrentThreadId(); + const PlatformThreadRef current_thread = CurrentThreadRef(); CritScope scoped_lock(&lock_); if (!valid_thread_) // Set if previously detached. valid_thread_ = current_thread; -#if defined(WEBRTC_MAC) || defined(WEBRTC_IOS) - // TODO(tommi): Remove this hack after we've figured out the roll issue - // with chromium's Mac 10.9 debug bot. - if (valid_thread_ != current_thread) { - // At the moment, this file cannot use logging from either webrtc or - // libjingle. :( - fprintf(stderr, "*** WRONG THREAD *** current=%i vs valid=%i\n", - current_thread, valid_thread_); - } - return true; // le sigh. -#else - return valid_thread_ == current_thread; -#endif + return IsThreadRefEqual(valid_thread_, current_thread); } void ThreadCheckerImpl::DetachFromThread() { diff --git a/webrtc/base/thread_checker_impl.h b/webrtc/base/thread_checker_impl.h index 67adc6ebdf..5ad15bb515 100644 --- a/webrtc/base/thread_checker_impl.h +++ b/webrtc/base/thread_checker_impl.h @@ -25,12 +25,18 @@ namespace rtc { // Used for identifying the current thread. Always an integer value. #if defined(WEBRTC_WIN) typedef DWORD PlatformThreadId; +typedef DWORD PlatformThreadRef; #elif defined(WEBRTC_POSIX) typedef pid_t PlatformThreadId; +typedef pthread_t PlatformThreadRef; #endif // TODO(tommi): This+PlatformThreadId belongs in a common thread related header. PlatformThreadId CurrentThreadId(); +PlatformThreadRef CurrentThreadRef(); + +// Compares two thread identifiers for equality. +bool IsThreadRefEqual(const PlatformThreadRef& a, const PlatformThreadRef& b); // Real implementation of ThreadChecker, for use in debug mode, or // for temporary use in release mode (e.g. to CHECK on a threading issue @@ -54,7 +60,7 @@ class ThreadCheckerImpl { mutable CriticalSection lock_; // This is mutable so that CalledOnValidThread can set it. // It's guarded by |lock_|. - mutable PlatformThreadId valid_thread_; + mutable PlatformThreadRef valid_thread_; }; } // namespace rtc diff --git a/webrtc/base/thread_checker_unittest.cc b/webrtc/base/thread_checker_unittest.cc index 762fad20c0..03053ad02e 100644 --- a/webrtc/base/thread_checker_unittest.cc +++ b/webrtc/base/thread_checker_unittest.cc @@ -157,15 +157,7 @@ void ThreadCheckerClass::MethodOnDifferentThreadImpl() { } #if ENABLE_THREAD_CHECKER -// TODO(tommi): Re-enable tests after figuring out issue with Chromium try bots. -#if defined(WEBRTC_MAC) || defined(WEBRTC_IOS) -#define MAYBE_MethodNotAllowedOnDifferentThreadInDebug \ - DISABLED_MethodNotAllowedOnDifferentThreadInDebug -#else -#define MAYBE_MethodNotAllowedOnDifferentThreadInDebug \ - MethodNotAllowedOnDifferentThreadInDebug -#endif -TEST(ThreadCheckerDeathTest, MAYBE_MethodNotAllowedOnDifferentThreadInDebug) { +TEST(ThreadCheckerDeathTest, MethodNotAllowedOnDifferentThreadInDebug) { ASSERT_DEATH({ ThreadCheckerClass::MethodOnDifferentThreadImpl(); }, ""); @@ -194,13 +186,7 @@ void ThreadCheckerClass::DetachThenCallFromDifferentThreadImpl() { } #if ENABLE_THREAD_CHECKER -// TODO(tommi): Re-enable tests after figuring out issue with Chromium try bots. -#if defined(WEBRTC_MAC) || defined(WEBRTC_IOS) -#define MAYBE_DetachFromThreadInDebug DISABLED_DetachFromThreadInDebug -#else -#define MAYBE_DetachFromThreadInDebug DetachFromThreadInDebug -#endif -TEST(ThreadCheckerDeathTest, MAYBE_DetachFromThreadInDebug) { +TEST(ThreadCheckerDeathTest, DetachFromThreadInDebug) { ASSERT_DEATH({ ThreadCheckerClass::DetachThenCallFromDifferentThreadImpl(); }, "");