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

base/synchronization/waitable_event_posix.cc

Index: base/synchronization/waitable_event_posix.cc
diff --git a/base/synchronization/waitable_event_posix.cc b/base/synchronization/waitable_event_posix.cc
index fccba9d31c67c767b0b8172e10ad127ab2a5cb24..9eaf6b9c6c2d5d10343f17d8bcef32485aab5979 100644
--- a/base/synchronization/waitable_event_posix.cc
+++ b/base/synchronization/waitable_event_posix.cc
@@ -279,7 +279,7 @@ size_t WaitableEvent::WaitMany(WaitableEvent** raw_waitables,
   // 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
+  // Take the locks of each WaitableEvent in turn and
   // remove our SyncWaiter from the wait-list
   for (size_t i = 0; i < count; ++i) {
     if (raw_waitables[i] != signaled_event) {
@@ -290,6 +290,11 @@ size_t WaitableEvent::WaitMany(WaitableEvent** raw_waitables,
         raw_waitables[i]->kernel_->Dequeue(&sw, &sw);
       raw_waitables[i]->kernel_->lock_.Release();
     } else {
+      // We acquire and release the lock in order to ensure that the

[agl, 2014/07/25 17:32:46]

It doesn't seem reasonable that WaitMany has this property, but Wait and
TimedWait don't.

[yhirano, 2014/07/29 06:48:11]

TimedWait has the property because it acquires the lock (see L200 and L202).
Blink WebSocket relies on the property. Now it uses TimedWait and I don't find
any customers that use the property via WaitMany.
Nevertheless, WaitMany with one waitable event should be equivalent to |Wait|, I
think.

In any case, the comment for WaitMany, 

  // You MUST NOT delete any of the WaitableEvent objects while this  wait is
  // happening.

is wrong. We can't delete any of the WaitableEvent objects even when this wait
finishes.
If we don't modify the implementation, we should fix the comment.

[agl, 2014/07/29 21:39:49]

Ah, you have found a bug in the code! TimedWait calls Release even when the
SyncWaiter fired (which is the common case). That call will always fail in that
case, but the return value is ignored so we never noticed.
The only property that an event provides is that the Wait() call return happens
after the Signal() call. I don't think that I'm being crazy believing that. If
you know of code that assumes more than that, then it's only working because of
the above bug. Ideally it would be fixed and then we could fix that bug.
Yes you can: the WaitMany has stopped using them. The fact that you have other
threads using them is why you can't delete them.

[yhirano, 2014/07/30 11:43:45]

Hmm... I can easily find multiple code relying on the property ([1] and [2] for
example).
Your opinion is that the following code is wrong, is that right?

{
  WaitableEvent e;
  PostTask(..., &e);
  e.Wait();
}

1:
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...
2:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr...
(CompletionEvent contains a WaitableEvent)

+      // corresponding |Signal| is done at this point.
+      raw_waitables[i]->kernel_->lock_.Acquire();
+      raw_waitables[i]->kernel_->lock_.Release();
+
       signaled_index = i;
     }
   }