Initial implementation of new AssertAcquired() functionality for Posix.

Hoisted the windows LockImpl funcitonality up into Lock and added
material to ConditionVariable to adjust those shadow variables when
needed.  Also got rid of os_lock() primitive in Lock class by piggybacking
on friend decl needed for accessing shadow variables.

BUG=44091
TEST=Try bot run on Windows, Linux, Mac.  Will land during low traffic time and revert on any problems or perf degradation.

Review URL: http://codereview.chromium.org/2196001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@49648 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 107 insertions, 74 deletions

diff --git a/base/condition_variable.h b/base/condition_variable.h
index 430f57c..da87bfc 100644
--- a/base/condition_variable.h
+++ b/base/condition_variable.h
@@ -174,6 +174,9 @@ class ConditionVariable {
 
   pthread_cond_t condition_;
   pthread_mutex_t* user_mutex_;
+#if !defined(NDEBUG)
+  Lock* user_lock_;     // Needed to adjust shadow lock state on wait.
+#endif
 
 #endif
 
diff --git a/base/condition_variable_posix.cc b/base/condition_variable_posix.cc
index 8c5a5c2..9d08f18 100644
--- a/base/condition_variable_posix.cc
+++ b/base/condition_variable_posix.cc
@@ -16,7 +16,7 @@ using base::Time;
 using base::TimeDelta;
 
 ConditionVariable::ConditionVariable(Lock* user_lock)
-    : user_mutex_(user_lock->lock_impl()->os_lock()) {
+  : user_mutex_(user_lock->lock_.os_lock()), user_lock_(user_lock) {
   int rv = pthread_cond_init(&condition_, NULL);
   DCHECK(rv == 0);
 }
@@ -27,8 +27,14 @@ ConditionVariable::~ConditionVariable() {
 }
 
 void ConditionVariable::Wait() {
+#if !defined(NDEBUG)
+  user_lock_->CheckHeldAndUnmark();
+#endif
   int rv = pthread_cond_wait(&condition_, user_mutex_);
   DCHECK(rv == 0);
+#if !defined(NDEBUG)
+  user_lock_->CheckUnheldAndMark();
+#endif
 }
 
 void ConditionVariable::TimedWait(const TimeDelta& max_time) {
@@ -46,8 +52,14 @@ void ConditionVariable::TimedWait(const TimeDelta& max_time) {
   abstime.tv_nsec %= Time::kNanosecondsPerSecond;
   DCHECK(abstime.tv_sec >= now.tv_sec);  // Overflow paranoia
 
+#if !defined(NDEBUG)
+  user_lock_->CheckHeldAndUnmark();
+#endif
   int rv = pthread_cond_timedwait(&condition_, user_mutex_, &abstime);
   DCHECK(rv == 0 || rv == ETIMEDOUT);
+#if !defined(NDEBUG)
+  user_lock_->CheckUnheldAndMark();
+#endif
 }
 
 void ConditionVariable::Broadcast() {
diff --git a/base/lock.cc b/base/lock.cc
index 9ff963b..c73a458 100644
--- a/base/lock.cc
+++ b/base/lock.cc
@@ -1,7 +1,37 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Lock class.
+// This file is used for debugging assertion support.  The Lock class
+// is functionally a wrapper around the LockImpl class, so the only
+// real intelligence in the class is in the debugging logic.
 
-// Depricated file.  See lock_impl_*.cc for platform specific versions.
+#if !defined(NDEBUG)
+
+#include "base/lock.h"
+#include "base/logging.h"
+
+Lock::Lock() : lock_() {
+  owned_by_thread_ = false;
+  owning_thread_id_ = static_cast<PlatformThreadId>(0);
+}
+
+void Lock::AssertAcquired() const {
+  DCHECK(owned_by_thread_);
+  DCHECK_EQ(owning_thread_id_, PlatformThread::CurrentId());
+}
+
+void Lock::CheckHeldAndUnmark() {
+  DCHECK(owned_by_thread_);
+  DCHECK_EQ(owning_thread_id_, PlatformThread::CurrentId());
+  owned_by_thread_ = false;
+  owning_thread_id_ = static_cast<PlatformThreadId>(0);
+}
+
+void Lock::CheckUnheldAndMark() {
+  DCHECK(!owned_by_thread_);
+  owned_by_thread_ = true;
+  owning_thread_id_ = PlatformThread::CurrentId();
+}
+
+#endif  // NDEBUG
diff --git a/base/lock.h b/base/lock.h
index 31ad9a0..64b8f74 100644
--- a/base/lock.h
+++ b/base/lock.h
@@ -7,30 +7,78 @@
 
 #include "base/lock_impl.h"
 
-// A convenient wrapper for an OS specific critical section.
+// A convenient wrapper for an OS specific critical section.  The only real
+// intelligence in this class is in debug mode for the support for the
+// AssertAcquired() method.
 
 class Lock {
  public:
+#if defined(NDEBUG)             // Optimized wrapper implementation
   Lock() : lock_() {}
   ~Lock() {}
   void Acquire() { lock_.Lock(); }
   void Release() { lock_.Unlock(); }
+
   // If the lock is not held, take it and return true. If the lock is already
   // held by another thread, immediately return false.
   bool Try() { return lock_.Try(); }
 
-  // In debug builds this method checks that the lock has been acquired by the
-  // calling thread.  If the lock has not been acquired, then the method
-  // will DCHECK().  In non-debug builds, the LockImpl's implementation of
-  // AssertAcquired() is an empty inline method.
-  void AssertAcquired() const { return lock_.AssertAcquired(); }
+  // Null implementation if not debug.
+  void AssertAcquired() const {}
+#else
+  Lock();
+  ~Lock() {}
+
+  // NOTE: Although windows critical sections support recursive locks, we do not
+  // allow this, and we will commonly fire a DCHECK() if a thread attempts to
+  // acquire the lock a second time (while already holding it).
+  void Acquire() {
+    lock_.Lock();
+    CheckUnheldAndMark();
+  }
+  void Release() {
+    CheckHeldAndUnmark();
+    lock_.Unlock();
+  }
 
-  // Return the underlying lock implementation.
-  // TODO(awalker): refactor lock and condition variables so that this is
-  // unnecessary.
-  LockImpl* lock_impl() { return &lock_; }
+  bool Try() {
+    bool rv = lock_.Try();
+    if (rv) {
+      CheckUnheldAndMark();
+    }
+    return rv;
+  }
+
+  void AssertAcquired() const;
+#endif                          // NDEBUG
+
+#if defined(OS_POSIX)
+  // The posix implementation of ConditionVariable needs to be able
+  // to see our lock and tweak our debugging counters, as it releases
+  // and acquires locks inside of pthread_cond_{timed,}wait.
+  // Windows doesn't need to do this as it calls the Lock::* methods.
+  friend class ConditionVariable;
+#endif
 
  private:
+#if !defined(NDEBUG)
+  // Members and routines taking care of locks assertions.
+  // Note that this checks for recursive locks and allows them
+  // if the variable is set.  This is allowed by the underlying implementation
+  // on windows but not on Posix, so we're doing unneeded checks on Posix.
+  // It's worth it to share the code.
+  void CheckHeldAndUnmark();
+  void CheckUnheldAndMark();
+
+  // All private data is implicitly protected by lock_.
+  // Be VERY careful to only access members under that lock.
+
+  // Determines validity of owning_thread_id_.  Needed as we don't have
+  // a null owning_thread_id_ value.
+  bool owned_by_thread_;
+  PlatformThreadId owning_thread_id_;
+#endif  // NDEBUG
+
   LockImpl lock_;  // Platform specific underlying lock implementation.
 
   DISALLOW_COPY_AND_ASSIGN(Lock);
diff --git a/base/lock_impl.h b/base/lock_impl.h
index 2d4a921..5e323a6 100644
--- a/base/lock_impl.h
+++ b/base/lock_impl.h
@@ -41,17 +41,6 @@ class LockImpl {
   // a successful call to Try, or a call to Lock.
   void Unlock();
 
-  // Debug-only method that will DCHECK() if the lock is not acquired by the
-  // current thread.  In non-debug builds, no check is performed.
-  // Because linux and mac condition variables modify the underlyning lock
-  // through the os_lock() method, runtime assertions can not be done on those
-  // builds.
-#if defined(NDEBUG) || !defined(OS_WIN)
-  void AssertAcquired() const {}
-#else
-  void AssertAcquired() const;
-#endif
-
   // Return the native underlying lock.  Not supported for Windows builds.
   // TODO(awalker): refactor lock and condition variables so that this is
   // unnecessary.
@@ -62,14 +51,6 @@ class LockImpl {
  private:
   OSLockType os_lock_;
 
-#if !defined(NDEBUG) && defined(OS_WIN)
-  // All private data is implicitly protected by lock_.
-  // Be VERY careful to only access members under that lock.
-  PlatformThreadId owning_thread_id_;
-  int32 recursion_count_shadow_;
-  bool recursion_used_;      // Allow debugging to continued after a DCHECK().
-#endif  // NDEBUG
-
   DISALLOW_COPY_AND_ASSIGN(LockImpl);
 };
 
diff --git a/base/lock_impl_win.cc b/base/lock_impl_win.cc
index 0d1ac93..14b76f8 100644
--- a/base/lock_impl_win.cc
+++ b/base/lock_impl_win.cc
@@ -5,16 +5,7 @@
 #include "base/lock_impl.h"
 #include "base/logging.h"
 
-// NOTE: Although windows critical sections support recursive locks, we do not
-// allow this, and we will commonly fire a DCHECK() if a thread attempts to
-// acquire the lock a second time (while already holding it).
-
 LockImpl::LockImpl() {
-#ifndef NDEBUG
-  recursion_count_shadow_ = 0;
-  recursion_used_ = false;
-  owning_thread_id_ = 0;
-#endif  // NDEBUG
   // The second parameter is the spin count, for short-held locks it avoid the
   // contending thread from going to sleep which helps performance greatly.
   ::InitializeCriticalSectionAndSpinCount(&os_lock_, 2000);
@@ -26,16 +17,6 @@ LockImpl::~LockImpl() {
 
 bool LockImpl::Try() {
   if (::TryEnterCriticalSection(&os_lock_) != FALSE) {
-#ifndef NDEBUG
-    // ONLY access data after locking.
-    owning_thread_id_ = PlatformThread::CurrentId();
-    DCHECK_NE(owning_thread_id_, 0u);
-    recursion_count_shadow_++;
-    if (2 == recursion_count_shadow_ && !recursion_used_) {
-      recursion_used_ = true;
-      DCHECK(false);  // Catch accidental redundant lock acquisition.
-    }
-#endif
     return true;
   }
   return false;
@@ -43,31 +24,9 @@ bool LockImpl::Try() {
 
 void LockImpl::Lock() {
   ::EnterCriticalSection(&os_lock_);
-#ifndef NDEBUG
-  // ONLY access data after locking.
-  owning_thread_id_ = PlatformThread::CurrentId();
-  DCHECK_NE(owning_thread_id_, 0u);
-  recursion_count_shadow_++;
-  if (2 == recursion_count_shadow_ && !recursion_used_) {
-    recursion_used_ = true;
-    DCHECK(false);  // Catch accidental redundant lock acquisition.
-  }
-#endif  // NDEBUG
 }
 
 void LockImpl::Unlock() {
-#ifndef NDEBUG
-  --recursion_count_shadow_;  // ONLY access while lock is still held.
-  DCHECK(0 <= recursion_count_shadow_);
-  owning_thread_id_ = 0;
-#endif  // NDEBUG
   ::LeaveCriticalSection(&os_lock_);
 }
 
-// In non-debug builds, this method is declared as an empty inline method.
-#ifndef NDEBUG
-void LockImpl::AssertAcquired() const {
-  DCHECK(recursion_count_shadow_ > 0);
-  DCHECK_EQ(owning_thread_id_, PlatformThread::CurrentId());
-}
-#endif