Added a debug method AssertAcquired() that compiles to nothing in non-debug builds. In debug builds, the method will DCHECK if the

current thread does not hold the lock.

I also removed a class that was not being used (AutoLockImpl) which seems like someone just copied the Lock.h file and blindly added
an Impl for the AutoLock class.

Thre are places in my code where I want to use the AutoUnlock class.  I can't see why it was restricted to use by the condition variable
class only, so I just made it into a regular class with no restrictions.

I noticed that JamesR posted a CL about a month ago that made AutoUnlock a public class, but it was never committed, so I added him to this CL for review too.
Review URL: http://codereview.chromium.org/48109

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

3 files changed, 49 insertions, 28 deletions

diff --git a/base/lock.h b/base/lock.h
index 8a4441a..f3653cf 100644
--- a/base/lock.h
+++ b/base/lock.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2006-2009 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.
 
@@ -19,6 +19,12 @@ class Lock {
   // 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() { return lock_.AssertAcquired(); }
+
   // Return the underlying lock implementation.
   // TODO(awalker): refactor lock and condition variables so that this is
   // unnecessary.
@@ -38,6 +44,7 @@ class AutoLock {
   }
 
   ~AutoLock() {
+    lock_.AssertAcquired();
     lock_.Release();
   }
 
@@ -46,23 +53,23 @@ class AutoLock {
   DISALLOW_COPY_AND_ASSIGN(AutoLock);
 };
 
-// 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;
+// AutoUnlock is a helper that will Release() the |lock| argument in the
+// constructor, and re-Acquire() it in the destructor.
 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) : lock_(&lock) {
+ public:
+  explicit AutoUnlock(Lock& lock) : lock_(lock) {
     // We require our caller to have the lock.
-    lock_->Release();
+    lock_.AssertAcquired();
+    lock_.Release();
   }
 
   ~AutoUnlock() {
-    lock_->Acquire();
+    lock_.Acquire();
   }
 
-  Lock* lock_;
+ private:
+  Lock& lock_;
+  DISALLOW_COPY_AND_ASSIGN(AutoUnlock);
 };
 
 #endif  // BASE_LOCK_H_
diff --git a/base/lock_impl.h b/base/lock_impl.h
index 1c29075..d3fcc19 100644
--- a/base/lock_impl.h
+++ b/base/lock_impl.h
@@ -14,6 +14,7 @@
 #endif
 
 #include "base/basictypes.h"
+#include "base/platform_thread.h"
 
 // This class implements the underlying platform-specific spin-lock mechanism
 // used for the Lock class.  Most users should not use LockImpl directly, but
@@ -40,10 +41,23 @@ class LockImpl {
   // a successful call to Try, or a call to Lock.
   void Unlock();
 
-  // Return the native underlying lock.
+  // 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() {}
+#else
+  void AssertAcquired();
+#endif
+
+  // Return the native underlying lock.  Not supported for Windows builds.
   // TODO(awalker): refactor lock and condition variables so that this is
   // unnecessary.
+#if !defined(OS_WIN)
   OSLockType* os_lock() { return &os_lock_; }
+#endif
 
  private:
   OSLockType os_lock_;
@@ -51,6 +65,7 @@ class LockImpl {
 #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
@@ -58,21 +73,5 @@ class LockImpl {
   DISALLOW_COPY_AND_ASSIGN(LockImpl);
 };
 
-class AutoLockImpl {
- public:
-  AutoLockImpl(LockImpl* lock_impl)
-      : lock_impl_(lock_impl) {
-    lock_impl_->Lock();
-  }
-
-  ~AutoLockImpl() {
-    lock_impl_->Unlock();
-  }
-
- private:
-  DISALLOW_EVIL_CONSTRUCTORS(AutoLockImpl);
-
-  LockImpl* lock_impl_;
-};
 
 #endif  // BASE_LOCK_IMPL_H_
diff --git a/base/lock_impl_win.cc b/base/lock_impl_win.cc
index 8bb1943..503f105 100644
--- a/base/lock_impl_win.cc
+++ b/base/lock_impl_win.cc
@@ -13,6 +13,7 @@ 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.
@@ -26,6 +27,9 @@ 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_, 0);
     recursion_count_shadow_++;
     if (2 == recursion_count_shadow_ && !recursion_used_) {
       recursion_used_ = true;
@@ -41,6 +45,8 @@ void LockImpl::Lock() {
   ::EnterCriticalSection(&os_lock_);
 #ifndef NDEBUG
   // ONLY access data after locking.
+  owning_thread_id_ = PlatformThread::CurrentId();
+  DCHECK_NE(owning_thread_id_, 0);
   recursion_count_shadow_++;
   if (2 == recursion_count_shadow_ && !recursion_used_) {
     recursion_used_ = true;
@@ -53,6 +59,15 @@ 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() {
+  DCHECK(recursion_count_shadow_ > 0);
+  DCHECK_EQ(owning_thread_id_, PlatformThread::CurrentId());
+}
+#endif