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

1 files changed, 23 insertions, 32 deletions

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_