Ensure that SyncWaiter (base/waitable_event_posix.cc) condition variables

cannot be destroyed (pthread_cond_destroy) during a broadcast
(pthread_cond_broadcast).  SyncWaiter::Fire now holds the lock until the
broadcast is complete.  Given the lock ordering, this guarantees that the
condition variable cannot be destroyed until it is safe to do so.  Holding
the lock through the broadcast is harmless, as pthread_cond_wait and
pthread_cond_timedwait will simply block (if needed) until able to take the
lock prior to returning.

Ownership of the lock and condition variable is moved to SyncWaiter for better
self-documentation.  The only true functional change here, however, is the
duration that SyncWaiter::Fire holds the lock.

This bug caused hangs in optimized official release builds on the Mac when
DCHECKS were optimized away.  The DCHECK optimization was initially covered in
bug 16512 r20497 and was backed out due to bug 16871 r20889.  The optimization
changed the timings just enough that we wound up hitting this case.  On the
Mac, pthread_cond_broadcast wound up blocking indefinitely (with a trashed
stack) when its condition variable was destroyed beneath it; this caused other
locks held by its thread to never be released, resulting in other threads
becoming deadlocked and made the bug particularly difficult to diagnose.

BUG=19710
TEST=Application works with no unexplained deadlocks, hanging, or crashing
     base_unittests (specifically WaitableEvent*)
     ipc_tests (especially IPCSyncChannelTest.ChattyServer, which would hang
                previously on the Mac with DCHECKs optimized away)
Review URL: http://codereview.chromium.org/173059

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

1 files changed, 42 insertions, 37 deletions

diff --git a/base/waitable_event_posix.cc b/base/waitable_event_posix.cc
index 690c755..793f635 100644
--- a/base/waitable_event_posix.cc
+++ b/base/waitable_event_posix.cc
@@ -76,36 +76,37 @@ bool WaitableEvent::IsSignaled() {
 // Synchronous waits
 
 // -----------------------------------------------------------------------------
-// This is an synchronous waiter. The thread is waiting on the given condition
+// This is a synchronous waiter. The thread is waiting on the given condition
 // variable and the fired flag in this object.
 // -----------------------------------------------------------------------------
 class SyncWaiter : public WaitableEvent::Waiter {
  public:
-  SyncWaiter(ConditionVariable* cv, Lock* lock)
+  SyncWaiter()
       : fired_(false),
-        cv_(cv),
-        lock_(lock),
-        signaling_event_(NULL) {
+        signaling_event_(NULL),
+        lock_(),
+        cv_(&lock_) {
   }
 
-  bool Fire(WaitableEvent *signaling_event) {
-    lock_->Acquire();
-      const bool previous_value = fired_;
-      fired_ = true;
-      if (!previous_value)
-        signaling_event_ = signaling_event;
-    lock_->Release();
+  bool Fire(WaitableEvent* signaling_event) {
+    AutoLock locked(lock_);
 
-    if (previous_value)
+    if (fired_)
       return false;
 
-    cv_->Broadcast();
+    fired_ = true;
+    signaling_event_ = signaling_event;
+
+    cv_.Broadcast();
+
+    // Unlike AsyncWaiter objects, SyncWaiter objects are stack-allocated on
+    // the blocking thread's stack.  There is no |delete this;| in Fire.  The
+    // SyncWaiter object is destroyed when it goes out of scope.
 
-    // SyncWaiters are stack allocated on the stack of the blocking thread.
     return true;
   }
 
-  WaitableEvent* signaled_event() const {
+  WaitableEvent* signaling_event() const {
     return signaling_event_;
   }
 
@@ -133,11 +134,19 @@ class SyncWaiter : public WaitableEvent::Waiter {
     fired_ = true;
   }
 
+  Lock* lock() {
+    return &lock_;
+  }
+
+  ConditionVariable* cv() {
+    return &cv_;
+  }
+
  private:
   bool fired_;
-  ConditionVariable *const cv_;
-  Lock *const lock_;
   WaitableEvent* signaling_event_;  // The WaitableEvent which woke us
+  Lock lock_;
+  ConditionVariable cv_;
 };
 
 bool WaitableEvent::TimedWait(const TimeDelta& max_time) {
@@ -156,10 +165,8 @@ bool WaitableEvent::TimedWait(const TimeDelta& max_time) {
       return true;
     }
 
-    Lock lock;
-    lock.Acquire();
-    ConditionVariable cv(&lock);
-    SyncWaiter sw(&cv, &lock);
+    SyncWaiter sw;
+    sw.lock()->Acquire();
 
     Enqueue(&sw);
   kernel_->lock_.Release();
@@ -173,13 +180,13 @@ bool WaitableEvent::TimedWait(const TimeDelta& max_time) {
     if (sw.fired() || (finite_time && current_time >= end_time)) {
       const bool return_value = sw.fired();
 
-      // We can't acquire @lock_ before releasing @lock (because of locking
-      // order), however, inbetween the two a signal could be fired and @sw
-      // would accept it, however we will still return false, so the signal
-      // would be lost on an auto-reset WaitableEvent. Thus we call Disable
-      // which makes sw::Fire return false.
+      // We can't acquire @lock_ before releasing the SyncWaiter lock (because
+      // of locking order), however, in between the two a signal could be fired
+      // and @sw would accept it, however we will still return false, so the
+      // signal would be lost on an auto-reset WaitableEvent. Thus we call
+      // Disable which makes sw::Fire return false.
       sw.Disable();
-      lock.Release();
+      sw.lock()->Release();
 
       kernel_->lock_.Acquire();
         kernel_->Dequeue(&sw, &sw);
@@ -190,9 +197,9 @@ bool WaitableEvent::TimedWait(const TimeDelta& max_time) {
 
     if (finite_time) {
       const TimeDelta max_wait(end_time - current_time);
-      cv.TimedWait(max_wait);
+      sw.cv()->TimedWait(max_wait);
     } else {
-      cv.Wait();
+      sw.cv()->Wait();
     }
   }
 }
@@ -237,9 +244,7 @@ size_t WaitableEvent::WaitMany(WaitableEvent** raw_waitables,
     DCHECK(waitables[i].first != waitables[i+1].first);
   }
 
-  Lock lock;
-  ConditionVariable cv(&lock);
-  SyncWaiter sw(&cv, &lock);
+  SyncWaiter sw;
 
   const size_t r = EnqueueMany(&waitables[0], count, &sw);
   if (r) {
@@ -252,7 +257,7 @@ size_t WaitableEvent::WaitMany(WaitableEvent** raw_waitables,
 
   // At this point, we hold the locks on all the WaitableEvents and we have
   // enqueued our waiter in them all.
-  lock.Acquire();
+  sw.lock()->Acquire();
     // Release the WaitableEvent locks in the reverse order
     for (size_t i = 0; i < count; ++i) {
       waitables[count - (1 + i)].first->kernel_->lock_.Release();
@@ -262,12 +267,12 @@ size_t WaitableEvent::WaitMany(WaitableEvent** raw_waitables,
       if (sw.fired())
         break;
 
-      cv.Wait();
+      sw.cv()->Wait();
     }
-  lock.Release();
+  sw.lock()->Release();
 
   // The address of the WaitableEvent which fired is stored in the SyncWaiter.
-  WaitableEvent *const signaled_event = sw.signaled_event();
+  WaitableEvent *const signaled_event = sw.signaling_event();
   // This will store the index of the raw_waitables which fired.
   size_t signaled_index = 0;