|
|
Created:
5 years, 3 months ago by erikchen Modified:
5 years, 3 months ago Reviewers:
Mark Mentovai CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmac: 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}
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Created: 5 years, 3 months ago
Messages
Total messages: 12 (2 generated)
erikchen@chromium.org changed reviewers: + mark@chromium.org
mark: Please review.
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.
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.
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?
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.
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.
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.
The CQ bit was checked by erikchen@chromium.org
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
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9c98c005f4ea59dd9425a4ee5d4e278f1c00ad8c Cr-Commit-Position: refs/heads/master@{#347736} |