[Mac] Log CHECK() file, line and message into breakpad.

[Mac] Log CHECK() file, line and message into breakpad.

This should make it somewhat easier to figure out which FATAL log line
fired, in the face of inlining and branching.

BUG=none
TEST=monitor crashes.


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

[Scott Hess - ex-Googler, 2011-10-26 23:37:40]

Things I did in testing:
 - built a branded official build.
 - log the items using NSLog().
 - Provoke a crash.
 - Nest a CHECK() to see what would happen (still crashed, but later).

[Mark Mentovai, 2011-10-26 23:50:25]

http://codereview.chromium.org/8401018/diff/1/chrome/app/breakpad_mac.mm
File chrome/app/breakpad_mac.mm (right):

http://codereview.chromium.org/8401018/diff/1/chrome/app/breakpad_mac.mm#newc...
chrome/app/breakpad_mac.mm:60: // NOTE(shess): Could look for the shared prefix
between |file| and
If this were to become a fully-cooked idea, it would result in different
treatment of files in, say, chrome and chrome/app compared to elsewhere.

Xcode always builds things so that __FILE__ contains a full path, but make
probably doesn’t, so relying on this to do anything special is probably a bad
idea. Relying on anything other than being able to grab the last component is
fragile.

http://codereview.chromium.org/8401018/diff/1/chrome/app/breakpad_mac.mm#newc...
chrome/app/breakpad_mac.mm:63: const char* slash = rindex(file, '/');
strrchr is the modern name for rindex. Use that.

http://codereview.chromium.org/8401018/diff/1/chrome/app/breakpad_mac.mm#newc...
chrome/app/breakpad_mac.mm:65: file = slash + 1;
If file is an empty string, this is dangerous. Be defensive about that.

http://codereview.chromium.org/8401018/diff/1/chrome/app/breakpad_mac.mm#newc...
chrome/app/breakpad_mac.mm:68: // What happens if we ran out of memory and the
construction does a
You know how I feel about “we.”

http://codereview.chromium.org/8401018/diff/1/chrome/app/breakpad_mac.mm#newc...
chrome/app/breakpad_mac.mm:71: // Solving would maybe need a pre-allocated
mutable string?
You can have a static bool that gets set when you enter this function and
cleared when you leave it. You can gate entry on its being clear.

That’s not thread-safe, though. Thread-safe would be to use a TLS slot instead
of a static.

The mutable-string idea’s interesting too but it still doesn’t protect against
bad things that might happen inside SetCrashKeyValue.

[dmac, 2011-10-27 00:07:11]

http://codereview.chromium.org/8401018/diff/1/chrome/app/breakpad_mac.mm
File chrome/app/breakpad_mac.mm (right):

http://codereview.chromium.org/8401018/diff/1/chrome/app/breakpad_mac.mm#newc...
chrome/app/breakpad_mac.mm:65: file = slash + 1;
On 2011/10/26 23:50:27, Mark Mentovai wrote:
> If file is an empty string, this is dangerous. Be defensive about that.

if file is an empty string, won't slash be NULL? and if file ends with "/" file
will end up as "". Maybe I'm missing something?

[Scott Hess - ex-Googler, 2011-10-27 00:15:09]

thx

http://codereview.chromium.org/8401018/diff/1/chrome/app/breakpad_mac.mm
File chrome/app/breakpad_mac.mm (right):

http://codereview.chromium.org/8401018/diff/1/chrome/app/breakpad_mac.mm#newc...
chrome/app/breakpad_mac.mm:60: // NOTE(shess): Could look for the shared prefix
between |file| and
On 2011/10/26 23:50:27, Mark Mentovai wrote:
> If this were to become a fully-cooked idea, it would result in different
> treatment of files in, say, chrome and chrome/app compared to elsewhere.
> 
> Xcode always builds things so that __FILE__ contains a full path, but make
> probably doesn’t, so relying on this to do anything special is probably a bad
> idea. Relying on anything other than being able to grab the last component is
> fragile.

removed.

http://codereview.chromium.org/8401018/diff/1/chrome/app/breakpad_mac.mm#newc...
chrome/app/breakpad_mac.mm:63: const char* slash = rindex(file, '/');
On 2011/10/26 23:50:27, Mark Mentovai wrote:
> strrchr is the modern name for rindex. Use that.

GET OFF MY LAWN!

http://codereview.chromium.org/8401018/diff/1/chrome/app/breakpad_mac.mm#newc...
chrome/app/breakpad_mac.mm:65: file = slash + 1;
On 2011/10/26 23:50:27, Mark Mentovai wrote:
> If file is an empty string, this is dangerous. Be defensive about that.

If file were empty, slash would be NULL.

http://codereview.chromium.org/8401018/diff/1/chrome/app/breakpad_mac.mm#newc...
chrome/app/breakpad_mac.mm:68: // What happens if we ran out of memory and the
construction does a
On 2011/10/26 23:50:27, Mark Mentovai wrote:
> You know how I feel about “we.”

Bother.

http://codereview.chromium.org/8401018/diff/1/chrome/app/breakpad_mac.mm#newc...
chrome/app/breakpad_mac.mm:71: // Solving would maybe need a pre-allocated
mutable string?
On 2011/10/26 23:50:27, Mark Mentovai wrote:
> You can have a static bool that gets set when you enter this function and
> cleared when you leave it. You can gate entry on its being clear.

I think that's reasonable.  I'll put that up as a change, let me know if you'd
rather go with an explicit lock.

> That’s not thread-safe, though. Thread-safe would be to use a TLS slot instead
> of a static.

Could using the TLS slot hit OOM?

> The mutable-string idea’s interesting too but it still doesn’t protect against
> bad things that might happen inside SetCrashKeyValue.

Honestly, I assume that once I've hit objc_msgSend(), I can kiss any guarantees
goodbye.

The static can be placed after the FATAL check, in which case it only is a
problem if multiple threads are hitting fatal conditions at once.  Worst case,
neither of them sets the value, but the program should still crash.  Or am I not
being paranoid enough?  Is worst case my house burning down?  OMG, my stuff!

[Mark Mentovai, 2011-10-28 18:02:15]

LGTM.

I’d still be interested in removing the potential for allocations here but since
this will all be obsoleted by bpoop, I don’t see much reason to push for that
unless it shows up as a problem in practice. And it probably won’t.

[commit-bot: I haz the power, 2011-10-28 20:46:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/8401018/3001

[commit-bot: I haz the power, 2011-10-28 22:30:20]

Change committed as 107805