Ensure that SyncWaiter condition variables cannot be destroyed during a broadcast

Ensure that SyncWaiter (base/waitable_event_posix.cc) condition variables
cannot be destroyed (pthread_cond_destroy) during a broadcast
(pthread_cond_broadcast).  SyncWaiter::Fire now holds the lock until the
broadcast is complete.  Given the lock ordering, this guarantees that the
condition variable cannot be destroyed until it is safe to do so.  Holding
the lock through the broadcast is harmless, as pthread_cond_wait and
pthread_cond_timedwait will simply block (if needed) until able to take the
lock prior to returning.

Ownership of the lock and condition variable is moved to SyncWaiter for better
self-documentation.  The only true functional change here, however, is the
duration that SyncWaiter::Fire holds the lock.

This bug caused hangs in optimized official release builds on the Mac when
DCHECKS were optimized away.  The DCHECK optimization was initially covered in
bug 16512 r20497 and was backed out due to bug 16871 r20889.  The optimization
changed the timings just enough that we wound up hitting this case.  On the
Mac, pthread_cond_broadcast wound up blocking indefinitely (with a trashed
stack) when its condition variable was destroyed beneath it; this caused other
locks held by its thread to never be released, resulting in other threads
becoming deadlocked and made the bug particularly difficult to diagnose.

BUG=19710
TEST=Application works with no unexplained deadlocks, hanging, or crashing
     base_unittests (specifically WaitableEvent*)
     ipc_tests (especially IPCSyncChannelTest.ChattyServer, which would hang
                previously on the Mac with DCHECKs optimized away)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=23790

[Mark Mentovai, 2009-08-19 16:21:01]

This was really ludicrous to track down.

[agl, 2009-08-19 17:35:34]

LGTM. Sorry about that. OSX appears to have different semantics for locking in
this case and something which was safe on Linux broke for you :(

http://codereview.chromium.org/173059/diff/1/2
File base/waitable_event_posix.cc (right):

http://codereview.chromium.org/173059/diff/1/2#newcode87
Line 87: lock_(),
Isn't this line a no-op?

[Mark Mentovai, 2009-08-19 18:30:48]

agl@chromium.org wrote:
> LGTM. Sorry about that. OSX appears to have different semantics for
> locking in this case and something which was safe on Linux broke for you
> :(

No big deal, it was sort of fun to track the bug down.

> http://codereview.chromium.org/173059/diff/1/2
> File base/waitable_event_posix.cc (right):
>
> http://codereview.chromium.org/173059/diff/1/2#newcode87
> Line 87: lock_(),
> Isn't this line a no-op?

I like to keep it listed explicitly and in the order of construction, because
lock_ is used as the parameter to the ConditionVariable constructor on the next
line.

Mark