admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Switch ThreadCheckerImpl over to using PlatformThreadRef.

Browse Source 
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
This commit is contained in:
tommi@webrtc.org 2015-02-07 19:17:47 +00:00
parent 2bf0e90c9d
commit 2eb1660791
3 changed files with 28 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
webrtc/base/thread_checker_impl.cc
View File

@ -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() {

8
webrtc/base/thread_checker_impl.h
View File

@ -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

18
webrtc/base/thread_checker_unittest.cc
View File

@ -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();
}, "");