Revert "POSIX: allow WaitableEvents to be deleted while watching them."

This reverts commit r12459 - it broke the world.


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

5 files changed, 64 insertions, 108 deletions

diff --git a/base/waitable_event.h b/base/waitable_event.h
index d2b0999..73fc7ff5 100644
--- a/base/waitable_event.h
+++ b/base/waitable_event.h
@@ -16,7 +16,6 @@
 #include <utility>
 #include "base/condition_variable.h"
 #include "base/lock.h"
-#include "base/ref_counted.h"
 #endif
 
 #include "base/message_loop.h"
@@ -61,6 +60,8 @@ class WaitableEvent {
   HANDLE Release();
 #endif
 
+  // WARNING: Destroying a WaitableEvent while threads are waiting on it is not
+  // supported.  Doing so will cause crashes or other instability.
   ~WaitableEvent();
 
   // Put the event in the un-signaled state.
@@ -92,9 +93,6 @@ class WaitableEvent {
   //   count: the number of elements in @waitables
   //
   // returns: the index of a WaitableEvent which has been signaled.
-  //
-  // You MUST NOT delete any of the WaitableEvent objects while this wait is
-  // happening.
   static size_t WaitMany(WaitableEvent** waitables, size_t count);
 
   // For asynchronous waiting, see WaitableEventWatcher
@@ -132,35 +130,10 @@ class WaitableEvent {
 #if defined(OS_WIN)
   HANDLE handle_;
 #else
-  // On Windows, one can close a HANDLE which is currently being waited on. The
-  // MSDN documentation says that the resulting behaviour is 'undefined', but
-  // it doesn't crash. However, if we were to include the following members
-  // directly then, on POSIX, one couldn't use WaitableEventWatcher to watch an
-  // event which gets deleted. This mismatch has bitten us several times now,
-  // so we have a kernel of the WaitableEvent, which is reference counted.
-  // WaitableEventWatchers may then take a reference and thus match the Windows
-  // behaviour.
-  struct WaitableEventKernel :
-      public RefCountedThreadSafe<WaitableEventKernel> {
-   public:
-    WaitableEventKernel(bool manual_reset, bool initially_signaled)
-        : manual_reset_(manual_reset),
-          signaled_(initially_signaled) {
-    }
-
-    bool Dequeue(Waiter* waiter, void* tag);
-
-    Lock lock_;
-    bool signaled_;
-    const bool manual_reset_;
-    std::list<Waiter*> waiters_;
-  };
-
-  scoped_refptr<WaitableEventKernel> kernel_;
-
   bool SignalAll();
   bool SignalOne();
   void Enqueue(Waiter* waiter);
+  bool Dequeue(Waiter* waiter, void* tag);
 
   // When dealing with arrays of WaitableEvent*, we want to sort by the address
   // of the WaitableEvent in order to have a globally consistent locking order.
@@ -170,6 +143,11 @@ class WaitableEvent {
   typedef std::pair<WaitableEvent*, size_t> WaiterAndIndex;
   static size_t EnqueueMany(WaiterAndIndex* waitables,
                             size_t count, Waiter* waiter);
+
+  Lock lock_;
+  bool signaled_;
+  const bool manual_reset_;
+  std::list<Waiter*> waiters_;
 #endif
 
   DISALLOW_COPY_AND_ASSIGN(WaitableEvent);
diff --git a/base/waitable_event_posix.cc b/base/waitable_event_posix.cc
index 690c755..09e2868 100644
--- a/base/waitable_event_posix.cc
+++ b/base/waitable_event_posix.cc
@@ -35,40 +35,46 @@ namespace base {
 // This is just an abstract base class for waking the two types of waiters
 // -----------------------------------------------------------------------------
 WaitableEvent::WaitableEvent(bool manual_reset, bool initially_signaled)
-    : kernel_(new WaitableEventKernel(manual_reset, initially_signaled)) {
+    : signaled_(initially_signaled),
+      manual_reset_(manual_reset) {
 }
 
 WaitableEvent::~WaitableEvent() {
+  if (!waiters_.empty()) {
+    LOG(ERROR) << "Destroying a WaitableEvent (" << this << ") with "
+               << waiters_.size() << " waiters";
+    NOTREACHED() << "Aborting.";
+  }
 }
 
 void WaitableEvent::Reset() {
-  AutoLock locked(kernel_->lock_);
-  kernel_->signaled_ = false;
+  AutoLock locked(lock_);
+  signaled_ = false;
 }
 
 void WaitableEvent::Signal() {
-  AutoLock locked(kernel_->lock_);
+  AutoLock locked(lock_);
 
-  if (kernel_->signaled_)
+  if (signaled_)
     return;
 
-  if (kernel_->manual_reset_) {
+  if (manual_reset_) {
     SignalAll();
-    kernel_->signaled_ = true;
+    signaled_ = true;
   } else {
     // In the case of auto reset, if no waiters were woken, we remain
     // signaled.
     if (!SignalOne())
-      kernel_->signaled_ = true;
+      signaled_ = true;
   }
 }
 
 bool WaitableEvent::IsSignaled() {
-  AutoLock locked(kernel_->lock_);
+  AutoLock locked(lock_);
 
-  const bool result = kernel_->signaled_;
-  if (result && !kernel_->manual_reset_)
-    kernel_->signaled_ = false;
+  const bool result = signaled_;
+  if (result && !manual_reset_)
+    signaled_ = false;
   return result;
 }
 
@@ -144,15 +150,15 @@ 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_) {
+  lock_.Acquire();
+    if (signaled_) {
+      if (!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;
+        signaled_ = false;
       }
 
-      kernel_->lock_.Release();
+      lock_.Release();
       return true;
     }
 
@@ -162,7 +168,7 @@ bool WaitableEvent::TimedWait(const TimeDelta& max_time) {
     SyncWaiter sw(&cv, &lock);
 
     Enqueue(&sw);
-  kernel_->lock_.Release();
+  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.
@@ -181,9 +187,9 @@ bool WaitableEvent::TimedWait(const TimeDelta& max_time) {
       sw.Disable();
       lock.Release();
 
-      kernel_->lock_.Acquire();
-        kernel_->Dequeue(&sw, &sw);
-      kernel_->lock_.Release();
+      lock_.Acquire();
+        Dequeue(&sw, &sw);
+      lock_.Release();
 
       return return_value;
     }
@@ -255,7 +261,7 @@ size_t WaitableEvent::WaitMany(WaitableEvent** raw_waitables,
   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();
+      waitables[count - (1 + i)].first->lock_.Release();
     }
 
     for (;;) {
@@ -275,12 +281,12 @@ size_t WaitableEvent::WaitMany(WaitableEvent** raw_waitables,
   // 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();
+      raw_waitables[i]->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
         // value again.
-        raw_waitables[i]->kernel_->Dequeue(&sw, &sw);
-      raw_waitables[i]->kernel_->lock_.Release();
+        raw_waitables[i]->Dequeue(&sw, &sw);
+      raw_waitables[i]->lock_.Release();
     } else {
       signaled_index = i;
     }
@@ -306,17 +312,17 @@ size_t WaitableEvent::EnqueueMany
   if (!count)
     return 0;
 
-  waitables[0].first->kernel_->lock_.Acquire();
-    if (waitables[0].first->kernel_->signaled_) {
-      if (!waitables[0].first->kernel_->manual_reset_)
-        waitables[0].first->kernel_->signaled_ = false;
-      waitables[0].first->kernel_->lock_.Release();
+  waitables[0].first->lock_.Acquire();
+    if (waitables[0].first->signaled_) {
+      if (!waitables[0].first->manual_reset_)
+        waitables[0].first->signaled_ = false;
+      waitables[0].first->lock_.Release();
       return count;
     }
 
     const size_t r = EnqueueMany(waitables + 1, count - 1, waiter);
     if (r) {
-      waitables[0].first->kernel_->lock_.Release();
+      waitables[0].first->lock_.Release();
     } else {
       waitables[0].first->Enqueue(waiter);
     }
@@ -337,12 +343,12 @@ bool WaitableEvent::SignalAll() {
   bool signaled_at_least_one = false;
 
   for (std::list<Waiter*>::iterator
-       i = kernel_->waiters_.begin(); i != kernel_->waiters_.end(); ++i) {
+       i = waiters_.begin(); i != waiters_.end(); ++i) {
     if ((*i)->Fire(this))
       signaled_at_least_one = true;
   }
 
-  kernel_->waiters_.clear();
+  waiters_.clear();
   return signaled_at_least_one;
 }
 
@@ -352,11 +358,11 @@ bool WaitableEvent::SignalAll() {
 // ---------------------------------------------------------------------------
 bool WaitableEvent::SignalOne() {
   for (;;) {
-    if (kernel_->waiters_.empty())
+    if (waiters_.empty())
       return false;
 
-    const bool r = (*kernel_->waiters_.begin())->Fire(this);
-    kernel_->waiters_.pop_front();
+    const bool r = (*waiters_.begin())->Fire(this);
+    waiters_.pop_front();
     if (r)
       return true;
   }
@@ -366,14 +372,14 @@ bool WaitableEvent::SignalOne() {
 // Add a waiter to the list of those waiting. Called with lock held.
 // -----------------------------------------------------------------------------
 void WaitableEvent::Enqueue(Waiter* waiter) {
-  kernel_->waiters_.push_back(waiter);
+  waiters_.push_back(waiter);
 }
 
 // -----------------------------------------------------------------------------
 // Remove a waiter from the list of those waiting. Return true if the waiter was
 // actually removed. Called with lock held.
 // -----------------------------------------------------------------------------
-bool WaitableEvent::WaitableEventKernel::Dequeue(Waiter* waiter, void* tag) {
+bool WaitableEvent::Dequeue(Waiter* waiter, void* tag) {
   for (std::list<Waiter*>::iterator
        i = waiters_.begin(); i != waiters_.end(); ++i) {
     if (*i == waiter && (*i)->Compare(tag)) {
diff --git a/base/waitable_event_watcher.h b/base/waitable_event_watcher.h
index 94c3fdb..1b49944 100644
--- a/base/waitable_event_watcher.h
+++ b/base/waitable_event_watcher.h
@@ -11,11 +11,11 @@
 #include "base/object_watcher.h"
 #else
 #include "base/message_loop.h"
-#include "base/waitable_event.h"
 #endif
 
 namespace base {
 
+class WaitableEvent;
 class Flag;
 class AsyncWaiter;
 class AsyncCallbackTask;
@@ -51,9 +51,6 @@ class AsyncCallbackTask;
 // occurs just before a WaitableEventWatcher is deleted. There is currently no
 // safe way to stop watching an automatic reset WaitableEvent without possibly
 // missing a signal.
-//
-// NOTE: you /are/ allowed to delete the WaitableEvent while still waiting on
-// it with a Watcher. It will act as if the event was never signaled.
 // -----------------------------------------------------------------------------
 
 class WaitableEventWatcher
@@ -143,7 +140,6 @@ class WaitableEventWatcher
   scoped_refptr<Flag> cancel_flag_;
   AsyncWaiter* waiter_;
   AsyncCallbackTask* callback_task_;
-  scoped_refptr<WaitableEvent::WaitableEventKernel> kernel_;
 #endif
 };
 
diff --git a/base/waitable_event_watcher_posix.cc b/base/waitable_event_watcher_posix.cc
index 1910c5f..10d47b7 100644
--- a/base/waitable_event_watcher_posix.cc
+++ b/base/waitable_event_watcher_posix.cc
@@ -155,13 +155,12 @@ bool WaitableEventWatcher::StartWatching
 
   cancel_flag_ = new Flag;
   callback_task_ = new AsyncCallbackTask(cancel_flag_, delegate, event);
-  WaitableEvent::WaitableEventKernel* kernel = event->kernel_.get();
 
-  AutoLock locked(kernel->lock_);
+  AutoLock locked(event->lock_);
 
-  if (kernel->signaled_) {
-    if (!kernel->manual_reset_)
-      kernel->signaled_ = false;
+  if (event->signaled_) {
+    if (!event->manual_reset_)
+      event->signaled_ = false;
 
     // No hairpinning - we can't call the delegate directly here. We have to
     // enqueue a task on the MessageLoop as normal.
@@ -173,7 +172,6 @@ bool WaitableEventWatcher::StartWatching
   current_ml->AddDestructionObserver(this);
 
   event_ = event;
-  kernel_ = kernel;
   waiter_ = new AsyncWaiter(current_ml, callback_task_, cancel_flag_);
   event->Enqueue(waiter_);
 
@@ -196,9 +194,9 @@ void WaitableEventWatcher::StopWatching() {
     return;
   }
 
-  if (!kernel_.get()) {
-    // We have no kernel. This means that we never enqueued a Waiter on an
-    // event because the event was already signaled when StartWatching was
+  if (!event_) {
+    // We have no WaitableEvent. This means that we never enqueued a Waiter on
+    // an event because the event was already signaled when StartWatching was
     // called.
     //
     // In this case, a task was enqueued on the MessageLoop and will run.
@@ -210,9 +208,9 @@ void WaitableEventWatcher::StopWatching() {
     return;
   }
 
-  AutoLock locked(kernel_->lock_);
-  // We have a lock on the kernel. No one else can signal the event while we
-  // have it.
+  AutoLock locked(event_->lock_);
+  // We have a lock on the WaitableEvent. No one else can signal the event while
+  // we have it.
 
   // We have a possible ABA issue here. If Dequeue was to compare only the
   // pointer values then it's possible that the AsyncWaiter could have been
@@ -226,7 +224,7 @@ void WaitableEventWatcher::StopWatching() {
   // have a reference to the Flag, its memory cannot be reused while this object
   // still exists. So if we find a waiter with the correct pointer value, and
   // which shares a Flag pointer, we have a real match.
-  if (kernel_->Dequeue(waiter_, cancel_flag_.get())) {
+  if (event_->Dequeue(waiter_, cancel_flag_.get())) {
     // Case 2: the waiter hasn't been signaled yet; it was still on the wait
     // list. We've removed it, thus we can delete it and the task (which cannot
     // have been enqueued with the MessageLoop because the waiter was never
diff --git a/base/waitable_event_watcher_unittest.cc b/base/waitable_event_watcher_unittest.cc
index 049de38..44b9b89 100644
--- a/base/waitable_event_watcher_unittest.cc
+++ b/base/waitable_event_watcher_unittest.cc
@@ -107,22 +107,6 @@ void RunTest_OutlivesMessageLoop(MessageLoop::Type message_loop_type) {
   }
 }
 
-void RunTest_DeleteUnder(MessageLoop::Type message_loop_type) {
-  // Delete the WaitableEvent out from under the Watcher. This is explictly
-  // allowed by the interface.
-
-  MessageLoop message_loop(message_loop_type);
-
-  {
-    WaitableEventWatcher watcher;
-
-    WaitableEvent* event = new WaitableEvent(false, false);
-    QuitDelegate delegate;
-    watcher.StartWatching(event, &delegate);
-    delete event;
-  }
-}
-
 }  // namespace
 
 //-----------------------------------------------------------------------------
@@ -150,9 +134,3 @@ TEST(WaitableEventWatcherTest, OutlivesMessageLoop) {
   RunTest_OutlivesMessageLoop(MessageLoop::TYPE_IO);
   RunTest_OutlivesMessageLoop(MessageLoop::TYPE_UI);
 }
-
-TEST(WaitableEventWatcherTest, DeleteUnder) {
-  RunTest_DeleteUnder(MessageLoop::TYPE_DEFAULT);
-  RunTest_DeleteUnder(MessageLoop::TYPE_IO);
-  RunTest_DeleteUnder(MessageLoop::TYPE_UI);
-}