Take extra lock in base::WaitableEvent::WaitMany.

Currently WaitMany doesn't ensure that the corresponding |Signal| is done
when the WaitMany call is returned.
That is problematic when we share the event over threads and want to wait
for detaching it in order to delete it after that.

BUG=397435
R=agl@chromium.org

Review URL: https://codereview.chromium.org/419773002

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

base/synchronization/waitable_event.h

@@ -72,12 +72,21 @@ class BASE_EXPORT WaitableEvent {
   // is not a manual reset event, then this test will cause a reset.
   bool IsSignaled();
 
-  // Wait indefinitely for the event to be signaled.
+  // Wait indefinitely for the event to be signaled. Wait's return "happens
+  // after" |Signal| has completed. This means that it's safe for a
+  // WaitableEvent to synchronise its own destruction, like this:
+  //
+  //   WaitableEvent *e = new WaitableEvent;
+  //   SendToOtherThread(e);
+  //   e->Wait();
+  //   delete e;
   void Wait();
 
   // Wait up until max_time has passed for the event to be signaled.  Returns
   // true if the event was signaled.  If this method returns false, then it
   // does not necessarily mean that max_time was exceeded.
+  //
+  // TimedWait can synchronise its own destruction like |Wait|.
   bool TimedWait(const TimeDelta& max_time);
 
 #if defined(OS_WIN)
@@ -91,7 +100,8 @@ class BASE_EXPORT WaitableEvent {
   // returns: the index of a WaitableEvent which has been signaled.
   //
   // You MUST NOT delete any of the WaitableEvent objects while this wait is
-  // happening.
+  // happening, however WaitMany's return "happens after" the |Signal| call
+  // that caused it has completed, like |Wait|.
   static size_t WaitMany(WaitableEvent** waitables, size_t count);
 
   // For asynchronous waiting, see WaitableEventWatcher

base/synchronization/waitable_event_posix.cc

@@ -197,6 +197,11 @@ bool WaitableEvent::TimedWait(const TimeDelta& max_time) {
       sw.Disable();
       sw.lock()->Release();
 
+      // This is a bug that has been enshrined in the interface of
+      // WaitableEvent now: |Dequeue| is called even when |sw.fired()| is true,
+      // even though it'll always return false in that case. However, taking
+      // the lock ensures that |Signal| has completed before we return and
+      // means that a WaitableEvent can synchronise its own destruction.
       kernel_->lock_.Acquire();
       kernel_->Dequeue(&sw, &sw);
       kernel_->lock_.Release();
@@ -290,6 +295,11 @@ size_t WaitableEvent::WaitMany(WaitableEvent** raw_waitables,
         raw_waitables[i]->kernel_->Dequeue(&sw, &sw);
       raw_waitables[i]->kernel_->lock_.Release();
     } else {
+      // By taking this lock here we ensure that |Signal| has completed by the
+      // time we return, because |Signal| holds this lock. This matches the
+      // behaviour of |Wait| and |TimedWait|.
+      raw_waitables[i]->kernel_->lock_.Acquire();
+      raw_waitables[i]->kernel_->lock_.Release();
       signaled_index = i;
     }
   }

base/synchronization/waitable_event_unittest.cc

@@ -79,7 +79,7 @@ class WaitableEventSignaler : public PlatformThread::Delegate {
   }
 
   virtual void ThreadMain() OVERRIDE {
-    PlatformThread::Sleep(TimeDelta::FromSeconds(static_cast<int>(seconds_)));
+    PlatformThread::Sleep(TimeDelta::FromSecondsD(seconds_));
     ev_->Signal();
   }
 
@@ -88,21 +88,43 @@ class WaitableEventSignaler : public PlatformThread::Delegate {
   WaitableEvent *const ev_;
 };
 
+TEST(WaitableEventTest, WaitAndDelete) {
+  // This test tests that if a WaitableEvent can be safely deleted
+  // when |Wait| is done without additional synchrnization.
+  // If this test crashes, it is a bug.
+
+  WaitableEvent* ev = new WaitableEvent(false, false);
+
+  WaitableEventSignaler signaler(0.01, ev);
+  PlatformThreadHandle thread;
+  PlatformThread::Create(0, &signaler, &thread);
+
+  ev->Wait();
+  delete ev;
+
+  PlatformThread::Join(thread);
+}
+
 TEST(WaitableEventTest, WaitMany) {
+  // This test tests that if a WaitableEvent can be safely deleted
+  // when |WaitMany| is done without additional synchrnization.
+  // If this test crashes, it is a bug.
+
   WaitableEvent* ev[5];
   for (unsigned i = 0; i < 5; ++i)
     ev[i] = new WaitableEvent(false, false);
 
-  WaitableEventSignaler signaler(0.1, ev[2]);
+  WaitableEventSignaler signaler(0.01, ev[2]);
   PlatformThreadHandle thread;
   PlatformThread::Create(0, &signaler, &thread);
 
-  EXPECT_EQ(WaitableEvent::WaitMany(ev, 5), 2u);
-
-  PlatformThread::Join(thread);
+  size_t index = WaitableEvent::WaitMany(ev, 5);
 
   for (unsigned i = 0; i < 5; ++i)
     delete ev[i];
+
+  PlatformThread::Join(thread);
+  EXPECT_EQ(2u, index);
 }
 
 }  // namespace base