Wire in postmortem report collection

Wire in postmortem report collection

Wires in postmortem report collection, which harvests unclean shutdown
debug information to a protocol buffer, wraps this proto inside a
barebone minidump and registers the minidump with CrashPad for upload.
Reports are collected as part of the metrics service client
initialization task chain. Stability file naming is revised so a unique
file name is used every time.

Notes:
- This is experimental work that currently only targets the windows canary
channel (using an experiment).
- CrashPad handles consent checking/uploading/throttling.
- Manufacturing the minidump is a temporary approach. Longer term, Crashpad
should handle it.
- This first version also collects reports for crashes. Longer term, the
reports pertaining to crashes will be bundled with the crash reports.

TEST
- launch chrome with the stability instrumentation (--enable-features=StabilityDebugging)
- crash chrome by visiting chrome://chrome://inducebrowsercrashforrealz
- Validate the existence of a stability file in <user-data-dir>/Stability
- start chrome again
- validate "ActivityTracker.Collect.*" metrics chrome://histograms/ActivityTracker
- validate the presence of a new crash at chrome://crashes

BUG=620813
Committed: https://crrev.com/f232266b5ba25df15843ee2589e795413ee71371
Cr-Commit-Position: refs/heads/master@{#421172}

[manzagop (departed), 2016-09-16 21:39:08]

manzagop@chromium.org changed reviewers:
+ siggi@chromium.org

[manzagop (departed), 2016-09-16 21:39:09]

This is the last part extracted from https://codereview.chromium.org/2128683002/
. Please have a look! 
Thanks,
Pierre

[Sigurður Ásgeirsson, 2016-09-19 14:20:05]

nice!

https://codereview.chromium.org/2344343002/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2344343002/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_field_trials_desktop.cc:72: LOG(ERROR) << "Failed
to create the stability directory.";
in the forest, nobody hears your LOGs.

If you believe this is prone to failure, maybe track with an UMA metric? Maybe
you want generically:
- setup succeeded
- reporting attempted
- reporting failed
or some such

https://codereview.chromium.org/2344343002/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2344343002/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_metrics_service_client.cc:253:
antivirus_metrics_provider_(nullptr),
This init was missing?

https://codereview.chromium.org/2344343002/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_metrics_service_client.cc:633: const base::Closure
on_collected_callback = base::Bind(
I'm pretty lost as to what runs on which thread here, and at which point in
time. Is this the UI thread?

https://codereview.chromium.org/2344343002/diff/1/components/browser_watcher/...
File components/browser_watcher/watcher_metrics_provider_win.cc (right):

https://codereview.chromium.org/2344343002/diff/1/components/browser_watcher/...
components/browser_watcher/watcher_metrics_provider_win.cc:269: LOG(ERROR) <<
"User data directory or crash directory is unknown.";
I would suggest a second UMA metric to count failure for any reason beyond this
point. In the forest, nobody hears your LOGs().

[manzagop (departed), 2016-09-21 22:00:44]

Addressed comments. Another look?

https://codereview.chromium.org/2344343002/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2344343002/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_field_trials_desktop.cc:72: LOG(ERROR) << "Failed
to create the stability directory.";
On 2016/09/19 14:20:05, Sigurður Ásgeirsson wrote:
> in the forest, nobody hears your LOGs.
> 
> If you believe this is prone to failure, maybe track with an UMA metric? Maybe
> you want generically:
> - setup succeeded
> - reporting attempted
> - reporting failed
> or some such

Done.

https://codereview.chromium.org/2344343002/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2344343002/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_metrics_service_client.cc:253:
antivirus_metrics_provider_(nullptr),
On 2016/09/19 14:20:05, Sigurður Ásgeirsson wrote:
> This init was missing?

Correct. Ack.

https://codereview.chromium.org/2344343002/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_metrics_service_client.cc:633: const base::Closure
on_collected_callback = base::Bind(
On 2016/09/19 14:20:05, Sigurður Ásgeirsson wrote:
> I'm pretty lost as to what runs on which thread here, and at which point in
> time. Is this the UI thread?

When the MetricsService calls OpenNewLog in the initialized state, that posts a
delayed MetricsService::StartInitTask on the UI thread's
base::ThreadTaskRunnerHandle::Get() - I think that may actually be the UI thread
itself. Note that on Windows, this is posted 30 seconds in the future.
StartInitTask leads to chain of tasks on that same thread, which eventually ends
up calling MetricsService::FinishedInitTask, which is the upload of the initial
log (that's *not* the initial *stability* log). 

The trick though is that CollectPostmortemReports is an empty shell that does a
PostTaskAndReply (runs the actual work on an io_task_runner_, then calls back to
the original thread).

https://codereview.chromium.org/2344343002/diff/1/components/browser_watcher/...
File components/browser_watcher/watcher_metrics_provider_win.cc (right):

https://codereview.chromium.org/2344343002/diff/1/components/browser_watcher/...
components/browser_watcher/watcher_metrics_provider_win.cc:269: LOG(ERROR) <<
"User data directory or crash directory is unknown.";
On 2016/09/19 14:20:05, Sigurður Ásgeirsson wrote:
> I would suggest a second UMA metric to count failure for any reason beyond
this
> point. In the forest, nobody hears your LOGs().

Done.

[Sigurður Ásgeirsson, 2016-09-22 13:06:12]

nice, lgtm except for a couple of performance nits.

https://codereview.chromium.org/2344343002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2344343002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:634:
&ChromeMetricsServiceClient::OnInitTaskCollectedPostmortemReports,
ubernit: it's pretty wasteful to create a Closure, and then dispatch through it
for !OS_WIN. We don't need cut #1001 :(.

https://codereview.chromium.org/2344343002/diff/20001/components/browser_watc...
File components/browser_watcher/watcher_metrics_provider_win.cc (right):

https://codereview.chromium.org/2344343002/diff/20001/components/browser_watc...
components/browser_watcher/watcher_metrics_provider_win.cc:278:
UMA_HISTOGRAM_ENUMERATION("ActivityTracker.Collect.InitStatus",
It irks me to see these macros used for code that'll only add one count to a
bucket in the program's lifetime. These macros go through all kinds of
contortions to provide caching for reuse. This adds code and data to the
executable - is this really how we swing? Is there anything like
UMA_GET_HISTOGRAM_POINTER()... that would allow you to retrieve the Histogram*
and then use it as appropriate below?

[manzagop (departed), 2016-09-22 15:41:57]

Addressed comments! You may want to have another quick look.

https://codereview.chromium.org/2344343002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2344343002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:634:
&ChromeMetricsServiceClient::OnInitTaskCollectedPostmortemReports,
On 2016/09/22 13:06:11, Sigurður Ásgeirsson wrote:
> ubernit: it's pretty wasteful to create a Closure, and then dispatch through
it
> for !OS_WIN. We don't need cut #1001 :(.

Done.

https://codereview.chromium.org/2344343002/diff/20001/components/browser_watc...
File components/browser_watcher/watcher_metrics_provider_win.cc (right):

https://codereview.chromium.org/2344343002/diff/20001/components/browser_watc...
components/browser_watcher/watcher_metrics_provider_win.cc:278:
UMA_HISTOGRAM_ENUMERATION("ActivityTracker.Collect.InitStatus",
On 2016/09/22 13:06:12, Sigurður Ásgeirsson wrote:
> It irks me to see these macros used for code that'll only add one count to a
> bucket in the program's lifetime. These macros go through all kinds of
> contortions to provide caching for reuse. This adds code and data to the
> executable - is this really how we swing? Is there anything like
> UMA_GET_HISTOGRAM_POINTER()... that would allow you to retrieve the Histogram*
> and then use it as appropriate below?

There doesn't seem to be, so I went with a manual version of it. 

rkaplow@ says they're working on histogram_macros.h this week and might add
something.

[manzagop (departed), 2016-09-22 15:44:24]

manzagop@chromium.org changed reviewers:
+ asvitkine@chromium.org

[manzagop (departed), 2016-09-22 15:44:24]

Hi Alexei,

Here's the collection of the postmortem stability reports we discussed, as part
of the InitTask chain. Please have a look (or punt to another metrics reviewer).

Thanks!

Pierre

[Sigurður Ásgeirsson, 2016-09-22 15:46:14]

lgtm

https://codereview.chromium.org/2344343002/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2344343002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials_desktop.cc:67: base::HistogramBase*
init_status_hist = base::LinearHistogram::FactoryGet(
Ah - I see.
Since this'll at least instantiate only one instance of the code and static
variable, maybe a meet in the middle is to use the UMA macro once here?
This is fine too - whichever way your prefer.

https://codereview.chromium.org/2344343002/diff/40001/components/browser_watc...
File components/browser_watcher/watcher_metrics_provider_win.cc (right):

https://codereview.chromium.org/2344343002/diff/40001/components/browser_watc...
components/browser_watcher/watcher_metrics_provider_win.cc:210: INIT_STATUS_MAX
+ 1, base::HistogramBase::kUmaTargetedHistogramFlag);
same comment here.

[Alexei Svitkine (slow), 2016-09-23 14:13:54]

https://codereview.chromium.org/2344343002/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2344343002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials_desktop.cc:67: base::HistogramBase*
init_status_hist = base::LinearHistogram::FactoryGet(
On 2016/09/22 15:46:14, Sigurður Ásgeirsson wrote:
> Ah - I see.
> Since this'll at least instantiate only one instance of the code and static
> variable, maybe a meet in the middle is to use the UMA macro once here?
> This is fine too - whichever way your prefer.

Yeah, I'd suggest using the macro. Not using the macro will save you a static
var that caches the pointer, but actually that part is not really much savings -
since the Histogram object itself is still cached.

And the macro is more readable. (Once we have the non-caching version of the
macro, it will also be easier to switch to that.)

Also, when you switch back to the macros you can rename the function to omit
"WithoutStaticVar" prefix which should make call sites fit on one line. :)

https://codereview.chromium.org/2344343002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials_desktop.cc:75:
browser_watcher::kStabilityDebuggingFeature))
Nit: {} now that if is multi-line

https://codereview.chromium.org/2344343002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials_desktop.cc:110: #endif
// defined(OS_WIN)

https://codereview.chromium.org/2344343002/diff/40001/components/browser_watc...
File components/browser_watcher/stability_debugging_win.h (right):

https://codereview.chromium.org/2344343002/diff/40001/components/browser_watc...
components/browser_watcher/stability_debugging_win.h:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
Nit: No (c) for new files

https://codereview.chromium.org/2344343002/diff/40001/components/browser_watc...
components/browser_watcher/stability_debugging_win.h:14: class
StabilityDebugging {
Is the extra class wrapper really necessary for these? I'd just making them
free-standing functions.

Style guide says "Prefer grouping functions with a namespace instead of using a
class as if it were a namespace. " - and seems like you already have a
namespace.

https://codereview.chromium.org/2344343002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2344343002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:349: +  <summary>Status of internal
activity tracking initialization.</summary>
Nit: Mention when it's logged. Same for the other one.

[manzagop (departed), 2016-09-23 21:56:39]

Patchset #4 (id:60001) has been deleted

[manzagop (departed), 2016-09-23 21:57:02]

Addressed comments! Please have another look!

https://codereview.chromium.org/2344343002/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2344343002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials_desktop.cc:67: base::HistogramBase*
init_status_hist = base::LinearHistogram::FactoryGet(
On 2016/09/22 15:46:14, Sigurður Ásgeirsson wrote:
> Ah - I see.
> Since this'll at least instantiate only one instance of the code and static
> variable, maybe a meet in the middle is to use the UMA macro once here?
> This is fine too - whichever way your prefer.

Done.

https://codereview.chromium.org/2344343002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials_desktop.cc:67: base::HistogramBase*
init_status_hist = base::LinearHistogram::FactoryGet(
On 2016/09/23 14:13:53, Alexei Svitkine (very slow) wrote:
> On 2016/09/22 15:46:14, Sigurður Ásgeirsson wrote:
> > Ah - I see.
> > Since this'll at least instantiate only one instance of the code and static
> > variable, maybe a meet in the middle is to use the UMA macro once here?
> > This is fine too - whichever way your prefer.
> 
> Yeah, I'd suggest using the macro. Not using the macro will save you a static
> var that caches the pointer, but actually that part is not really much savings
-
> since the Histogram object itself is still cached.
> 
> And the macro is more readable. (Once we have the non-caching version of the
> macro, it will also be easier to switch to that.)
> 
> Also, when you switch back to the macros you can rename the function to omit
> "WithoutStaticVar" prefix which should make call sites fit on one line. :)

Done.

https://codereview.chromium.org/2344343002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials_desktop.cc:75:
browser_watcher::kStabilityDebuggingFeature))
On 2016/09/23 14:13:53, Alexei Svitkine (very slow) wrote:
> Nit: {} now that if is multi-line

Done.

https://codereview.chromium.org/2344343002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials_desktop.cc:110: #endif
On 2016/09/23 14:13:53, Alexei Svitkine (very slow) wrote:
> // defined(OS_WIN)

Done.

https://codereview.chromium.org/2344343002/diff/40001/components/browser_watc...
File components/browser_watcher/stability_debugging_win.h (right):

https://codereview.chromium.org/2344343002/diff/40001/components/browser_watc...
components/browser_watcher/stability_debugging_win.h:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
On 2016/09/23 14:13:53, Alexei Svitkine (very slow) wrote:
> Nit: No (c) for new files
 
Done. Here and elsewhere.

https://codereview.chromium.org/2344343002/diff/40001/components/browser_watc...
components/browser_watcher/stability_debugging_win.h:14: class
StabilityDebugging {
On 2016/09/23 14:13:53, Alexei Svitkine (very slow) wrote:
> Is the extra class wrapper really necessary for these? I'd just making them
> free-standing functions.
> 
> Style guide says "Prefer grouping functions with a namespace instead of using
a
> class as if it were a namespace. " - and seems like you already have a
> namespace.

I added "Stability" to the function names. Done.

https://codereview.chromium.org/2344343002/diff/40001/components/browser_watc...
File components/browser_watcher/watcher_metrics_provider_win.cc (right):

https://codereview.chromium.org/2344343002/diff/40001/components/browser_watc...
components/browser_watcher/watcher_metrics_provider_win.cc:210: INIT_STATUS_MAX
+ 1, base::HistogramBase::kUmaTargetedHistogramFlag);
On 2016/09/22 15:46:14, Sigurður Ásgeirsson wrote:
> same comment here.

Done.

https://codereview.chromium.org/2344343002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2344343002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:349: +  <summary>Status of internal
activity tracking initialization.</summary>
On 2016/09/23 14:13:53, Alexei Svitkine (very slow) wrote:
> Nit: Mention when it's logged. Same for the other one.

Done.

[Sigurður Ásgeirsson, 2016-09-26 15:22:25]

still lgtm

https://codereview.chromium.org/2344343002/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2344343002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials_desktop.cc:67: base::HistogramBase*
init_status_hist = base::LinearHistogram::FactoryGet(
On 2016/09/23 14:13:53, Alexei Svitkine (very slow) wrote:
> On 2016/09/22 15:46:14, Sigurður Ásgeirsson wrote:
> > Ah - I see.
> > Since this'll at least instantiate only one instance of the code and static
> > variable, maybe a meet in the middle is to use the UMA macro once here?
> > This is fine too - whichever way your prefer.
> 
> Yeah, I'd suggest using the macro. Not using the macro will save you a static
> var that caches the pointer, but actually that part is not really much savings
-
> since the Histogram object itself is still cached.

Each of those macro incantations instantiates a fair bit of code, performs at
least two atomic memory accesses, and adds at least one static variable to the
data section. It might be fun to try to fold these incantations into a templated
function or some such, see whether the compiler can scrunch them down. 
I count ~4800 UMA_HISTOGRAM* instances in Chrome, so it might be worth looking
at?

[Alexei Svitkine (slow), 2016-09-26 15:29:49]

lgtm

https://codereview.chromium.org/2344343002/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2344343002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials_desktop.cc:67: base::HistogramBase*
init_status_hist = base::LinearHistogram::FactoryGet(
On 2016/09/26 15:22:25, Sigurður Ásgeirsson wrote:
> On 2016/09/23 14:13:53, Alexei Svitkine (very slow) wrote:
> > On 2016/09/22 15:46:14, Sigurður Ásgeirsson wrote:
> > > Ah - I see.
> > > Since this'll at least instantiate only one instance of the code and
static
> > > variable, maybe a meet in the middle is to use the UMA macro once here?
> > > This is fine too - whichever way your prefer.
> > 
> > Yeah, I'd suggest using the macro. Not using the macro will save you a
static
> > var that caches the pointer, but actually that part is not really much
savings
> -
> > since the Histogram object itself is still cached.
> 
> Each of those macro incantations instantiates a fair bit of code, performs at
> least two atomic memory accesses, and adds at least one static variable to the
> data section. It might be fun to try to fold these incantations into a
templated
> function or some such, see whether the compiler can scrunch them down. 
> I count ~4800 UMA_HISTOGRAM* instances in Chrome, so it might be worth looking
> at?

Agreed.

We've previously done some exploration and improvements on this front here:

https://bugs.chromium.org/p/chromium/issues/detail?id=343946
And we still have a general top-level bug open here:
https://bugs.chromium.org/p/chromium/issues/detail?id=500942

If you have concrete ideas for improvements, feel free to post on later bug
(since it's still open) or file a more specific bug and make it blocked on that.
I definitely support anything we could do to make histograms have less overhead
than they do now. :)

[manzagop (departed), 2016-09-26 15:42:51]

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

[manzagop (departed), 2016-09-26 15:42:57]

Thanks for the reviews!

[commit-bot: I haz the power, 2016-09-26 15:43:22]

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-09-26 17:13:34]

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

[commit-bot: I haz the power, 2016-09-26 17:13:35]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[manzagop (departed), 2016-09-26 18:51:49]

The CQ bit was checked by manzagop@chromium.org

[manzagop (departed), 2016-09-26 18:51:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, siggi@chromium.org
Link to the patchset: https://codereview.chromium.org/2344343002/#ps100001
(title: "Merge")

[commit-bot: I haz the power, 2016-09-26 18:52:22]

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-09-26 19:51:17]

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

[commit-bot: I haz the power, 2016-09-26 19:51:18]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[manzagop (departed), 2016-09-26 20:45:46]

The CQ bit was checked by manzagop@chromium.org

[commit-bot: I haz the power, 2016-09-26 20:46:39]

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-09-26 22:10:38]

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

[commit-bot: I haz the power, 2016-09-26 22:10:39]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[manzagop (departed), 2016-09-27 10:49:56]

The CQ bit was checked by manzagop@chromium.org

[commit-bot: I haz the power, 2016-09-27 10:50:11]

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-09-27 11:40:22]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-09-27 11:42:13]

Description was changed from

==========
Wire in postmortem report collection

Wires in postmortem report collection, which harvests unclean shutdown
debug information to a protocol buffer, wraps this proto inside a
barebone minidump and registers the minidump with CrashPad for upload.
Reports are collected as part of the metrics service client
initialization task chain. Stability file naming is revised so a unique
file name is used every time.

Notes:
- This is experimental work that currently only targets the windows canary
channel (using an experiment).
- CrashPad handles consent checking/uploading/throttling.
- Manufacturing the minidump is a temporary approach. Longer term, Crashpad
should handle it.
- This first version also collects reports for crashes. Longer term, the
reports pertaining to crashes will be bundled with the crash reports.

TEST
- launch chrome with the stability instrumentation
(--enable-features=StabilityDebugging)
- crash chrome by visiting chrome://chrome://inducebrowsercrashforrealz
- Validate the existence of a stability file in <user-data-dir>/Stability
- start chrome again
- validate "ActivityTracker.Collect.*" metrics
chrome://histograms/ActivityTracker
- validate the presence of a new crash at chrome://crashes

BUG=620813
==========

to

==========
Wire in postmortem report collection

Wires in postmortem report collection, which harvests unclean shutdown
debug information to a protocol buffer, wraps this proto inside a
barebone minidump and registers the minidump with CrashPad for upload.
Reports are collected as part of the metrics service client
initialization task chain. Stability file naming is revised so a unique
file name is used every time.

Notes:
- This is experimental work that currently only targets the windows canary
channel (using an experiment).
- CrashPad handles consent checking/uploading/throttling.
- Manufacturing the minidump is a temporary approach. Longer term, Crashpad
should handle it.
- This first version also collects reports for crashes. Longer term, the
reports pertaining to crashes will be bundled with the crash reports.

TEST
- launch chrome with the stability instrumentation
(--enable-features=StabilityDebugging)
- crash chrome by visiting chrome://chrome://inducebrowsercrashforrealz
- Validate the existence of a stability file in <user-data-dir>/Stability
- start chrome again
- validate "ActivityTracker.Collect.*" metrics
chrome://histograms/ActivityTracker
- validate the presence of a new crash at chrome://crashes

BUG=620813

Committed: https://crrev.com/f232266b5ba25df15843ee2589e795413ee71371
Cr-Commit-Position: refs/heads/master@{#421172}
==========

[commit-bot: I haz the power, 2016-09-27 11:42:14]

Patchset 5 (id:??) landed as
https://crrev.com/f232266b5ba25df15843ee2589e795413ee71371
Cr-Commit-Position: refs/heads/master@{#421172}

[Nico, 2017-04-27 15:31:48]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2017-04-27 15:31:49]

You didn't add stability_debugging_win_unittest.cc to any .gn files as far as I
can tell. Want to send a follow-up for that?

[manzagop (departed), 2017-04-27 20:57:58]

On 2017/04/27 15:31:49, Nico wrote:
> You didn't add stability_debugging_win_unittest.cc to any .gn files as far as
I
> can tell. Want to send a follow-up for that?

Thanks. Will do!

[manzagop (departed), 2017-04-28 17:47:46]

On 2017/04/27 20:57:58, manzagop (OOO until April 27) wrote:
> On 2017/04/27 15:31:49, Nico wrote:
> > You didn't add stability_debugging_win_unittest.cc to any .gn files as far
as
> I
> > can tell. Want to send a follow-up for that?
> 
> Thanks. Will do!

For reference, the CL to fix it is out for review: 
https://codereview.chromium.org/2852693003/