Ensure that SyncWaiter condition variables cannot be destroyed during a broadcast

base/waitable_event_posix.cc

 // Copyright (c) 2006-2008 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.
 
 #include "base/waitable_event.h"
 
 #include "base/condition_variable.h"
 #include "base/lock.h"
 #include "base/message_loop.h"
 
@@ skipping 58 matching lines @@
   const bool result = kernel_->signaled_;
   if (result && !kernel_->manual_reset_)
     kernel_->signaled_ = false;
   return result;
 }
 
 // -----------------------------------------------------------------------------
 // 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_(),

[agl, 2009/08/19 17:35:35]

Isn't this line a no-op?

+        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;
 
-    // SyncWaiters are stack allocated on the stack of the blocking thread.
+    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.
+
     return true;
   }
 
-  WaitableEvent* signaled_event() const {
+  WaitableEvent* signaling_event() const {
     return signaling_event_;
   }
 
   // ---------------------------------------------------------------------------
   // These waiters are always stack allocated and don't delete themselves. Thus
   // there's no problem and the ABA tag is the same as the object pointer.
   // ---------------------------------------------------------------------------
   bool Compare(void* tag) {
     return this == tag;
   }
 
   // ---------------------------------------------------------------------------
   // Called with lock held.
   // ---------------------------------------------------------------------------
   bool fired() const {
     return fired_;
   }
 
   // ---------------------------------------------------------------------------
   // During a TimedWait, we need a way to make sure that an auto-reset
   // WaitableEvent doesn't think that this event has been signaled between
   // unlocking it and removing it from the wait-list. Called with lock held.
   // ---------------------------------------------------------------------------
   void Disable() {
     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) {
   const Time end_time(Time::Now() + max_time);
   const bool finite_time = max_time.ToInternalValue() >= 0;
 
   kernel_->lock_.Acquire();
     if (kernel_->signaled_) {
       if (!kernel_->manual_reset_) {
         // In this case we were signaled when we had no waiters. Now that
         // someone has waited upon us, we can automatically reset.
         kernel_->signaled_ = false;
       }
 
       kernel_->lock_.Release();
       return true;
     }
 
-    Lock lock;
-    lock.Acquire();
-    ConditionVariable cv(&lock);
-    SyncWaiter sw(&cv, &lock);
+    SyncWaiter sw;
+    sw.lock()->Acquire();
 
     Enqueue(&sw);
   kernel_->lock_.Release();
   // We are violating locking order here by holding the SyncWaiter lock but not
   // the WaitableEvent lock. However, this is safe because we don't lock @lock_
   // again before unlocking it.
 
   for (;;) {
     const Time current_time(Time::Now());
 
     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);
       kernel_->lock_.Release();
 
       return return_value;
     }
 
     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();
     }
   }
 }
 
 bool WaitableEvent::Wait() {
   return TimedWait(TimeDelta::FromSeconds(-1));
 }
 
 // -----------------------------------------------------------------------------
 
@@ skipping 24 matching lines @@
 
   sort(waitables.begin(), waitables.end(), cmp_fst_addr);
 
   // The set of waitables must be distinct. Since we have just sorted by
   // address, we can check this cheaply by comparing pairs of consecutive
   // elements.
   for (size_t i = 0; i < waitables.size() - 1; ++i) {
     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) {
     // One of the events is already signaled. The SyncWaiter has not been
     // enqueued anywhere. EnqueueMany returns the count of remaining waitables
     // when the signaled one was seen, so the index of the signaled event is
     // @count - @r.
     return waitables[count - r].second;
   }
 
   // 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();
     }
 
     for (;;) {
       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;
 
   // Take the locks of each WaitableEvent in turn (except the signaled one) and
   // remove our SyncWaiter from the wait-list
   for (size_t i = 0; i < count; ++i) {
     if (raw_waitables[i] != signaled_event) {
       raw_waitables[i]->kernel_->lock_.Acquire();
         // There's no possible ABA issue with the address of the SyncWaiter here
         // because it lives on the stack. Thus the tag value is just the pointer
@@ skipping 100 matching lines @@
       return true;
     }
   }
 
   return false;
 }
 
 // -----------------------------------------------------------------------------
 
 }  // namespace base