mac: Add a hack to condition_variable_posix.cc to avoid a crash.

mac: Add a hack to condition_variable_posix.cc to avoid a crash.

The Darwin pthreads subsystem has a bug that can cause the program to crash if a
conditional variable is destroyed after it has been signalled, but before it has
been waited on. This hack is a solution proposed by Apple in its own developer
forums.

BUG=517681
Committed: https://crrev.com/9c98c005f4ea59dd9425a4ee5d4e278f1c00ad8c
Cr-Commit-Position: refs/heads/master@{#347736}

[erikchen, 2015-09-03 22:22:40]

erikchen@chromium.org changed reviewers:
+ mark@chromium.org

[erikchen, 2015-09-03 22:22:41]

mark: Please review.

[Mark Mentovai, 2015-09-04 12:30:56]

https://codereview.chromium.org/1323293005/diff/20001/base/synchronization/co...
File base/synchronization/condition_variable_posix.cc (right):

https://codereview.chromium.org/1323293005/diff/20001/base/synchronization/co...
base/synchronization/condition_variable_posix.cc:48: {
The bug says 10.10 and 10.11. Does this happen on earlier releases? If not,
would it be beneficial to avoid this on earlier releases?

https://codereview.chromium.org/1323293005/diff/20001/base/synchronization/co...
base/synchronization/condition_variable_posix.cc:49: base::Lock lock;
How do we know that the memory used for this mutex has never been used for a
condition variable? (Some condition variable beyond our control without this
workaround, that is.) The forum post said that mutex allocation would fail.

[erikchen, 2015-09-04 17:25:37]

https://codereview.chromium.org/1323293005/diff/20001/base/synchronization/co...
File base/synchronization/condition_variable_posix.cc (right):

https://codereview.chromium.org/1323293005/diff/20001/base/synchronization/co...
base/synchronization/condition_variable_posix.cc:48: {
On 2015/09/04 12:30:56, Mark Mentovai - August is over wrote:
> The bug says 10.10 and 10.11. Does this happen on earlier releases? If not,
> would it be beneficial to avoid this on earlier releases?

It does - the comment was looking only at the most recent crashes.
Unfortunately, it's hard to look at the full history since there have been other
bugs with similar signatures and the signatures have changed slightly over time.

The bug report in the apple forums was produced on OSX 10.9, so we know it goes
back at least that far. When looking at the source for 10.8.5, the radar comment
was not present in the source. I don't know if the bug didn't exist, or it just
hadn't been discovered yet. I think it's safer to assume the latter.

https://codereview.chromium.org/1323293005/diff/20001/base/synchronization/co...
base/synchronization/condition_variable_posix.cc:49: base::Lock lock;
On 2015/09/04 12:30:56, Mark Mentovai - August is over wrote:
> How do we know that the memory used for this mutex has never been used for a
> condition variable? (Some condition variable beyond our control without this
> workaround, that is.) The forum post said that mutex allocation would fail.

We could, soon after each new thread is created, make a lock and reuse it only
on the thread, only for this purpose. But even that wouldn't fully fix the
problem, since we have no guarantees about which memory has or has not been used
for condition variables if the thread is created late in the life cycle of the
application. I believe that this solution solves the problem sufficiently well,
and if it doesn't, we can take another look.

[Mark Mentovai, 2015-09-04 18:22:02]

I just had a chance to look at the system code more closely. I believe that the
comment:

217                  // <rdar://problem/13782056> Need to clear preposts.

refers to the fix for the bug described at
https://devforums.apple.com/message/945082#945082.

Furthermore, https://devforums.apple.com/message/1128455#1128455 states that
this bug was fixed, definitely in iOS 7.1, and probably in OS X 10.10.

Do you still want to try this?

[erikchen, 2015-09-05 00:57:06]

On 2015/09/04 18:22:02, Mark Mentovai - August is over wrote:
> I just had a chance to look at the system code more closely. I believe that
the
> comment:
> 
> 217                  // <rdar://problem/13782056> Need to clear preposts.
> 
> refers to the fix for the bug described at
> https://devforums.apple.com/message/945082#945082.
> 
> Furthermore, https://devforums.apple.com/message/1128455#1128455 states that
> this bug was fixed, definitely in iOS 7.1, and probably in OS X 10.10.
> 
> Do you still want to try this?

The symptoms that we're seeing certainly resemble the symptoms of the bug that
Apple claims to have fixed. Based on my limited understanding, it seems like
we're still seeing a bug in pthread library.  

I'm inclined to try this solution, which we could easily revert if the crashes
keep coming in. But I will defer to your judgement, since you are much more
experienced in this area.

[Mark Mentovai, 2015-09-08 14:46:47]

LGTM, because I can’t say that this definitely won’t help without trying to
mentally diff Apple code that’s (uncharacteristically!) been refactored. This
won’t hurt too much, but let’s watch and back it out if it doesn’t help at all.

[erikchen, 2015-09-08 16:32:41]

sgtm

On Tue, Sep 8, 2015 at 7:46 AM, <mark@chromium.org> wrote:

> LGTM, because I can’t say that this definitely won’t help without trying to
> mentally diff Apple code that’s (uncharacteristically!) been refactored.
> This
> won’t hurt too much, but let’s watch and back it out if it doesn’t help at
> all.
>
> https://codereview.chromium.org/1323293005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[erikchen, 2015-09-08 16:32:50]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-09-08 16:32:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1323293005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1323293005/20001

[commit-bot: I haz the power, 2015-09-08 17:50:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-08 17:51:30]

Patchset 2 (id:??) landed as
https://crrev.com/9c98c005f4ea59dd9425a4ee5d4e278f1c00ad8c
Cr-Commit-Position: refs/heads/master@{#347736}