Always drop a tombstone

Always drop a tombstone

If the DoneCallback returns false, breakpad should reinstall the old
signal handlers. Specifically, it will reinstall the Android tombstone
handler. This change ensure that on Android we always return false from
these callbacks. When this happens, the minidump should still be
created/uploaded as normal.

Also, just prior to returning we log a Chrome build fingerprint. This
should appear in the log just above the tombstone.

Requires breakpad DEPS roll to >=1025
(http://breakpad.appspot.com/440002/) to work correctly, but fails
safely without it.

BUG=


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

[Lei Zhang, 2012-09-06 19:13:36]

+yfriedman for a second look.

https://chromiumcodereview.appspot.com/10908115/diff/9001/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/10908115/diff/9001/build/common.gypi#n...
build/common.gypi:2534: # Gyp defines by the buildbots that actually archive
symbols.
s/Gyp defines/GYP_DEFINES/ ?

https://chromiumcodereview.appspot.com/10908115/diff/9001/chrome/app/breakpad...
File chrome/app/breakpad_linux.cc (right):

https://chromiumcodereview.appspot.com/10908115/diff/9001/chrome/app/breakpad...
chrome/app/breakpad_linux.cc:619: // CrashDoneNonBrowserAndroid.
nit: CrashDoneNonBrowserAndroid()

https://chromiumcodereview.appspot.com/10908115/diff/9001/chrome/app/breakpad...
chrome/app/breakpad_linux.cc:635: CrashDoneNonBrowserAndroid,
Would it be easier to avoid duplicating the code by having MinidumpCallback
variable that's set appropriately depending on the platform?

[cjhopman, 2012-09-07 18:09:20]

http://codereview.chromium.org/10908115/diff/9001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10908115/diff/9001/build/common.gypi#newcode2534
build/common.gypi:2534: # Gyp defines by the buildbots that actually archive
symbols.
On 2012/09/06 19:13:36, Lei Zhang wrote:
> s/Gyp defines/GYP_DEFINES/ ?

Done.

http://codereview.chromium.org/10908115/diff/9001/chrome/app/breakpad_linux.cc
File chrome/app/breakpad_linux.cc (right):

http://codereview.chromium.org/10908115/diff/9001/chrome/app/breakpad_linux.c...
chrome/app/breakpad_linux.cc:619: // CrashDoneNonBrowserAndroid.
On 2012/09/06 19:13:36, Lei Zhang wrote:
> nit: CrashDoneNonBrowserAndroid()

Done.

http://codereview.chromium.org/10908115/diff/9001/chrome/app/breakpad_linux.c...
chrome/app/breakpad_linux.cc:635: CrashDoneNonBrowserAndroid,
On 2012/09/06 19:13:36, Lei Zhang wrote:
> Would it be easier to avoid duplicating the code by having MinidumpCallback
> variable that's set appropriately depending on the platform?

Done.

[Lei Zhang, 2012-09-07 18:12:55]

lgtm with Yaron's approval.

[Yaron, 2012-09-07 20:15:17]

lgtm - fine with me but wondering about Mark's thoughts on this. I understand
that for Android we want to include this extra little bit in the tombstone, but
beyond that, shouldn't the behaviour be the same?

http://codereview.chromium.org/10908115/diff/10001/chrome/app/breakpad_linux.cc
File chrome/app/breakpad_linux.cc (right):

http://codereview.chromium.org/10908115/diff/10001/chrome/app/breakpad_linux....
chrome/app/breakpad_linux.cc:622: return true;
Just wondering why other ports return true here as opposed to false like us.

[Mark Mentovai, 2012-09-07 20:22:35]

http://codereview.chromium.org/10908115/diff/10001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10908115/diff/10001/build/common.gypi#newcode2537
build/common.gypi:2537: 'chrome_symbols_location%': '',
What’s your intention for this?

meaning:

What will this be set to? “Location” is totally ambiguous.

http://codereview.chromium.org/10908115/diff/10001/chrome/app/breakpad_linux.cc
File chrome/app/breakpad_linux.cc (right):

http://codereview.chromium.org/10908115/diff/10001/chrome/app/breakpad_linux....
chrome/app/breakpad_linux.cc:396: android_build_info->package_version_code());
Isn’t the name and version number of an official build enough?

[cjhopman, 2012-09-07 20:30:56]

On 2012/09/07 20:22:35, Mark Mentovai wrote:
> http://codereview.chromium.org/10908115/diff/10001/build/common.gypi
> File build/common.gypi (right):
> 
>
http://codereview.chromium.org/10908115/diff/10001/build/common.gypi#newcode2537
> build/common.gypi:2537: 'chrome_symbols_location%': '',
> What’s your intention for this?
> 
> meaning:
> 
> What will this be set to? “Location” is totally ambiguous.
> 
>
http://codereview.chromium.org/10908115/diff/10001/chrome/app/breakpad_linux.cc
> File chrome/app/breakpad_linux.cc (right):
> 
>
http://codereview.chromium.org/10908115/diff/10001/chrome/app/breakpad_linux....
> chrome/app/breakpad_linux.cc:396: android_build_info->package_version_code());
> Isn’t the name and version number of an official build enough?

chrome_symbols_location is set to the path on Google Cloud Storage that symbols
are archived to. There is not a simple mapping from version_name+version_code to
this path.

[Mark Mentovai, 2012-09-07 20:33:04]

Is it publicly accessible? Is it OK to ship the links to the world?

[cjhopman1, 2012-09-07 20:45:46]

The archive buckets are not publicly accessible.

Currently, the actual path is a variation of:
<bucket-name>/<builder-name>/<datestamp>/<version_code>.
chrome_symbols_location either doesn't include <bucket_name> or includes a
numeric id for the bucket-name. There should be no issues with this being
publicly available.


On Fri, Sep 7, 2012 at 1:33 PM, <mark@chromium.org> wrote:

> Is it publicly accessible? Is it OK to ship the links to the world?
>
>
http://codereview.chromium.**org/10908115/<http://codereview.chromium.org/109...
>

[cjhopman, 2012-09-10 16:21:48]

http://codereview.chromium.org/10908115/diff/10001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10908115/diff/10001/build/common.gypi#newcode2537
build/common.gypi:2537: 'chrome_symbols_location%': '',
On 2012/09/07 20:22:36, Mark Mentovai wrote:
> What’s your intention for this?
> 
> meaning:
> 
> What will this be set to? “Location” is totally ambiguous.

Done as discussed.

[Mark Mentovai, 2012-09-10 16:23:52]

LGTM

[commit-bot: I haz the power, 2012-09-11 16:18:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/10908115/8006

[commit-bot: I haz the power, 2012-09-11 18:19:05]

Change committed as 156063