[bgmode] Log the unpin stack trace to Crash Keys

[bgmode] Log the unpin stack trace to Crash Keys

BUG=625646
Committed: https://crrev.com/24c2c8cda1e14b7523b6fb841bec1e810633a26e
Cr-Commit-Position: refs/heads/master@{#439176}

[dgn, 2016-12-13 17:19:25]

dgn@chromium.org changed reviewers:
+ bauerb@chromium.org

[dgn, 2016-12-13 17:19:26]

PTAL

[Bernhard Bauer, 2016-12-14 09:34:08]

lgtm

[dgn, 2016-12-14 10:01:31]

The CQ bit was checked by dgn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 10:01:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgn, 2016-12-14 10:38:39]

dgn@chromium.org changed reviewers:
+ sky@chromium.org

[dgn, 2016-12-14 10:38:40]

sky@: PTAL. This should allow us to get more info while tracking the crashes.

[commit-bot: I haz the power, 2016-12-14 11:04:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 11:04:59]

Dry run: This issue passed the CQ dry run.

[sky, 2016-12-14 16:34:03]

sky@chromium.org changed reviewers:
+ eroman@chromium.org

[sky, 2016-12-14 16:34:03]

+eroman as he knows more about crashes than I.
From my perspective, LGTM

https://codereview.chromium.org/2569953003/diff/1/chrome/common/crash_keys.cc
File chrome/common/crash_keys.cc (right):

https://codereview.chromium.org/2569953003/diff/1/chrome/common/crash_keys.cc...
chrome/common/crash_keys.cc:36: const char kBrowserUnpinTrace[] =
"browser-unpin-trace";
keep sorted?

[eroman, 2016-12-14 20:40:24]

https://codereview.chromium.org/2569953003/diff/1/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/2569953003/diff/1/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:1318:
base::debug::SetCrashKeyToStackTrace(crash_keys::kBrowserUnpinTrace,
Have you tested that this works?

I am pretty sure crash keys are not dynamic -- you need to register them in
advance (see RegisterChromeCrashKeys()).

https://codereview.chromium.org/2569953003/diff/1/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:1320: CHECK(false);
I imagine there should be something like:

// TODO(crbug.com/625646): Temporary instrumentation

[dgn, 2016-12-16 15:43:48]

The CQ bit was checked by dgn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 15:44:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 16:02:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 16:02:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dgn, 2016-12-16 16:12:19]

The CQ bit was checked by dgn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 16:12:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 17:00:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 17:00:40]

Dry run: This issue passed the CQ dry run.

[dgn, 2016-12-16 19:10:42]

PTAL

https://codereview.chromium.org/2569953003/diff/1/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/2569953003/diff/1/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:1318:
base::debug::SetCrashKeyToStackTrace(crash_keys::kBrowserUnpinTrace,
On 2016/12/14 20:40:24, eroman (slow) wrote:
> Have you tested that this works?
> 
> I am pretty sure crash keys are not dynamic -- you need to register them in
> advance (see RegisterChromeCrashKeys()).

Thanks! Sadly we don't have a reliable repro for the bug and because of the
CHECK/crash below it's not testable. So I'm not sure how to exercise that path.

https://codereview.chromium.org/2569953003/diff/1/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:1320: CHECK(false);
On 2016/12/14 20:40:24, eroman (slow) wrote:
> I imagine there should be something like:
> 
> // TODO(crbug.com/625646): Temporary instrumentation

Not sure if temporary... you added that almost 5 years ago :)
https://codereview.chromium.org/9375037

[dgn, 2016-12-16 19:11:20]

dgn@chromium.org changed reviewers:
+ rsesek@chromium.org

[dgn, 2016-12-16 19:11:21]

rsesek@chromium.org: PTAL, I added a crash key and had to also put it in //blimp

[Robert Sesek, 2016-12-16 19:17:29]

LGTM

[eroman, 2016-12-16 19:28:16]

lgtm

https://codereview.chromium.org/2569953003/diff/1/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/2569953003/diff/1/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:1320: CHECK(false);
On 2016/12/16 19:10:42, dgn wrote:
> On 2016/12/14 20:40:24, eroman (slow) wrote:
> > I imagine there should be something like:
> > 
> > // TODO(crbug.com/625646): Temporary instrumentation
> 
> Not sure if temporary... you added that almost 5 years ago :)
> https://codereview.chromium.org/9375037

Heh, nothing quite as durable as a "temporary" hack :)

[dgn, 2016-12-16 19:55:02]

Thanks! :)

[dgn, 2016-12-16 19:55:08]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-12-16 19:55:08]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2569953003/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-12-16 19:57:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 20:45:05]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481918108427760,
"parent_rev": "bbaaaee06199cca51ad294298080b625bf50f793", "commit_rev":
"1178a92f065b9791b6a4ce9689da2b960567c7b6"}

[commit-bot: I haz the power, 2016-12-16 20:45:33]

Description was changed from

==========
[bgmode] Log the unpin stack trace to Crash Keys

BUG=625646
==========

to

==========
[bgmode] Log the unpin stack trace to Crash Keys

BUG=625646

Review-Url: https://codereview.chromium.org/2569953003
==========

[commit-bot: I haz the power, 2016-12-16 20:45:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-16 20:48:28]

Description was changed from

==========
[bgmode] Log the unpin stack trace to Crash Keys

BUG=625646

Review-Url: https://codereview.chromium.org/2569953003
==========

to

==========
[bgmode] Log the unpin stack trace to Crash Keys

BUG=625646

Committed: https://crrev.com/24c2c8cda1e14b7523b6fb841bec1e810633a26e
Cr-Commit-Position: refs/heads/master@{#439176}
==========

[commit-bot: I haz the power, 2016-12-16 20:48:29]

Patchset 3 (id:??) landed as
https://crrev.com/24c2c8cda1e14b7523b6fb841bec1e810633a26e
Cr-Commit-Position: refs/heads/master@{#439176}