Disallow recursive locking via the Lock class

Change contract so that Posix locks (which deadlock on
attempts by a single thread to acquire a mutex recursively)
and windows critical sections can both be used to implement
the Lock() cass, by disallowing recursive locking.


In DEBUG mode only, we watch for (now) illegal use of
recursive locking.

Also remove a pile of cruft that has built up in this file
as various folks have re-re-refactored and moved around
code.

Note that Window's condition variable implementation still
uses the AutoUnlock() helper class, but now the 
implementation (sans nested locking) is very trivial.  
Posix will probably use their own CV implementation.


r=mbelshe
Review URL: http://codereview.chromium.org/5630

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

2 files changed, 46 insertions, 113 deletions

diff --git a/base/lock.cc b/base/lock.cc
index 70fa044..1dd4dc1 100644
--- a/base/lock.cc
+++ b/base/lock.cc
@@ -7,42 +7,31 @@
 // The Lock class is used everywhere, and hence any changes
 // to lock.h tend to require a complete rebuild.  To facilitate
 // profiler development, all the profiling methods are listed
-// here.  Note that they are only instantiated in a debug
-// build, and the header provides all the trivial implementations
-// in a production build.
+// here.  
 
 #include "base/lock.h"
 #include "base/logging.h"
 
+#ifndef NDEBUG
 Lock::Lock()
     : lock_(),
-      recursion_count_shadow_(0) {
-#ifndef NDEBUG
-  recursion_used_ = false;
-  acquisition_count_ = 0;
-  contention_count_ = 0;
-#endif
+      recursion_count_shadow_(0),
+      recursion_used_(false),
+      acquisition_count_(0),
+      contention_count_(0) {
+}
+#else  // NDEBUG
+Lock::Lock()
+    : lock_() {
 }
+#endif  // NDEBUG
 
 Lock::~Lock() {
-#ifndef NDEBUG
-  // There should be no one to contend for the lock,
-  // ...but we need the memory barrier to get a good value.
-  lock_.Lock();
-  int final_recursion_count = recursion_count_shadow_;
-  lock_.Unlock();
-#endif
-
-  // Allow unit test exception only at end of method.
-#ifndef NDEBUG
-  DCHECK(0 == final_recursion_count);
-#endif
 }
 
 void Lock::Acquire() {
 #ifdef NDEBUG
   lock_.Lock();
-  recursion_count_shadow_++;
 #else  // NDEBUG
   if (!lock_.Try()) {
     // We have contention.
@@ -51,82 +40,35 @@ void Lock::Acquire() {
   }
   // ONLY access data after locking.
   recursion_count_shadow_++;
-  if (1 == recursion_count_shadow_)
-    acquisition_count_++;
-  else if (2 == recursion_count_shadow_ && !recursion_used_)
-    // Usage Note: Set a break point to debug.
+  acquisition_count_++;
+  if (2 == recursion_count_shadow_ && !recursion_used_) {
     recursion_used_ = true;
+    // TODO(sky): Uncomment this DCHECK after fixing test cases.
+    // DCHECK(false);  // Catch accidental redundant lock acquisition.
+  }
 #endif  // NDEBUG
 }
 
 void Lock::Release() {
-  --recursion_count_shadow_;  // ONLY access while lock is still held.
 #ifndef NDEBUG
+  --recursion_count_shadow_;  // ONLY access while lock is still held.
   DCHECK(0 <= recursion_count_shadow_);
-#endif
+#endif  // NDEBUG
   lock_.Unlock();
 }
 
 bool Lock::Try() {
   if (lock_.Try()) {
-    recursion_count_shadow_++;
 #ifndef NDEBUG
-    if (1 == recursion_count_shadow_)
-      acquisition_count_++;
-    else if (2 == recursion_count_shadow_ && !recursion_used_)
-      // Usage Note: Set a break point to debug.
+    recursion_count_shadow_++;
+    acquisition_count_++;
+    if (2 == recursion_count_shadow_ && !recursion_used_) {
       recursion_used_ = true;
+      DCHECK(false);  // Catch accidental redundant lock acquisition.
+    }
 #endif
     return true;
   } else {
     return false;
   }
 }
-
-// GetCurrentThreadRecursionCount returns the number of nested Acquire() calls
-// that have been made by the current thread holding this lock.  The calling
-// thread is ***REQUIRED*** to be *currently* holding the lock.   If that
-// calling requirement is violated, the return value is not well defined.
-// Return results are guaranteed correct if the caller has acquired this lock.
-// The return results might be incorrect otherwise.
-// This method is designed to be fast in non-debug mode by co-opting
-// synchronization using lock_ (no additional synchronization is used), but in
-// debug mode it slowly and carefully validates the requirement (and fires a
-// a DCHECK if it was called incorrectly).
-int32 Lock::GetCurrentThreadRecursionCount() {
-#ifndef NDEBUG
-  // If this DCHECK fails, then the most probable cause is:
-  // This method was called by class AutoUnlock during processing of a
-  // Wait() call made into the ConditonVariable class. That call to
-  // Wait() was made (incorrectly) without first Aquiring this Lock
-  // instance.
-  lock_.Lock();
-  int temp = recursion_count_shadow_;
-  lock_.Unlock();
-  // Unit tests catch an exception, so we need to be careful to test
-  // outside the critical section, since the Leave would be skipped!?!
-  DCHECK(temp >= 1);  // Allow unit test exception only at end of method.
-#endif  // DEBUG
-
-  // We hold lock, so this *is* correct value.
-  return recursion_count_shadow_;
-}
-
-
-AutoUnlock::AutoUnlock(Lock& lock) : lock_(&lock), release_count_(0) {
-  // We require our caller have the lock, so we can call for recursion count.
-  // CRITICALLY: Fetch value before we release the lock.
-  int32 count = lock_->GetCurrentThreadRecursionCount();
-  DCHECK(count > 0);  // Make sure we owned the lock.
-  while (count-- > 0) {
-    release_count_++;
-    lock_->Release();
-  }
-}
-
-AutoUnlock::~AutoUnlock() {
-  DCHECK(release_count_ >= 0);
-  while (release_count_-- > 0)
-    lock_->Acquire();
-}
-
diff --git a/base/lock.h b/base/lock.h
index 4e736af..b43e6ef 100644
--- a/base/lock.h
+++ b/base/lock.h
@@ -7,14 +7,16 @@
 
 #include "base/lock_impl.h"
 
-// A convenient wrapper for a critical section.
+// A convenient wrapper for an OS specific critical section.
 //
-// NOTE: A thread may acquire the same lock multiple times, but it must call
-// Release for each call to Acquire in order to finally release the 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).
 //
 // Complication: UnitTest for DeathTests catch DCHECK exceptions, so we need
 // to write code assuming DCHECK will throw.  This means we need to save any
 // assertable value in a local until we can safely throw.
+
 class Lock {
  public:
   Lock();
@@ -22,7 +24,7 @@ class Lock {
   void Acquire();
   void Release();
   // If the lock is not held, take it and return true. If the lock is already
-  // held by something else, immediately return false.
+  // held by another thread, immediately return false.
   bool Try();
 
   // Return the underlying lock implementation.
@@ -33,20 +35,13 @@ class Lock {
  private:
   LockImpl lock_;  // User-supplied underlying lock implementation.
 
-  // All private data is implicitly protected by spin_lock_.
-  // Be VERY careful to only access under that lock.
-  int32 recursion_count_shadow_;
-
-  // Allow access to GetCurrentThreadRecursionCount()
-  friend class AutoUnlock;
-  int32 GetCurrentThreadRecursionCount();
-
 #ifndef NDEBUG
-  // Even in Debug mode, the expensive tallies won't be calculated by default.
-  bool recursion_used_;
-  int32 acquisition_count_;
-
-  int32 contention_count_;
+  // All private data is implicitly protected by lock_.
+  // Be VERY careful to only access members under that lock.
+  int32 recursion_count_shadow_;
+  bool recursion_used_;      // Allow debugging to continued after a DCHECK().
+  int32 acquisition_count_;  // Number of times lock was acquired.
+  int32 contention_count_;   // Number of times there was contention.
 #endif  // NDEBUG
 
   DISALLOW_COPY_AND_ASSIGN(Lock);
@@ -68,27 +63,23 @@ class AutoLock {
   DISALLOW_COPY_AND_ASSIGN(AutoLock);
 };
 
-// AutoUnlock is a helper class for ConditionVariable instances
-// that is analogous to AutoLock.  It provides for nested Releases
-// of a lock for the Wait functionality of a ConditionVariable class.
-// The destructor automatically does the corresponding Acquire
-// calls (to return to the initial nested lock state).
-
-// Instances of AutoUnlock can ***ONLY*** validly be constructed if the
-// caller currently holds the lock provided as the constructor's argument.
-// If that ***REQUIREMENT*** is violated in debug mode, a DCHECK will
-// be generated in the Lock class.  In production (non-debug),
-// the results are undefined (and probably bad) if the caller
-// is not already holding the indicated lock.
+// AutoUnlock is a helper class for ConditionVariable that will Release() the
+// lock argument in the constructor, and re-Acquire() it in the destructor.
 class ConditionVariable;
 class AutoUnlock {
  private:  // Everything is private, so only our friend can use us.
   friend class ConditionVariable;  // The only user of this class.
-  explicit AutoUnlock(Lock& lock);
-  ~AutoUnlock();
+
+  explicit AutoUnlock(Lock& lock) : lock_(&lock) {
+    // We require our caller to have the lock.
+    lock_->Release();
+  }
+
+  ~AutoUnlock() {
+    lock_->Acquire();
+  }
 
   Lock* lock_;
-  int release_count_;
 };
 
 #endif  // BASE_LOCK_H_