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

base/synchronization/waitable_event_posix.cc

 // Copyright (c) 2012 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 <algorithm>
 #include <vector>
 
 #include "base/logging.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/synchronization/condition_variable.h"
@@ skipping 261 matching lines @@
 
       sw.cv()->Wait();
     }
   sw.lock()->Release();
 
   // The address of the WaitableEvent which fired is stored in the SyncWaiter.
   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
+  // 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) {
       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
         // value again.
         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;
     }
   }
 
   return signaled_index;
 }
 
 // -----------------------------------------------------------------------------
 // If return value == 0:
 //   The locks of the WaitableEvents have been taken in order and the Waiter has
@@ skipping 95 matching lines @@
       return true;
     }
   }
 
   return false;
 }
 
 // -----------------------------------------------------------------------------
 
 }  // namespace base