Monitor crashpad_handler for crashes [sometimes]

Monitor crashpad_handler for crashes [sometimes]

When Crashpad self-monitoring is enabled, crashes in crashpad_handler
will be written to the crash report database and uploaded according to
the user's preference. Self-monitoring dedicates an additional
crashpad_handler process to serve as the handler for the original
instance. Having a dedicated process is somewhat expensive, so
self-monitoring is not enabled at all times.

An existing mechanism, the "fallback crash handler," is already in place
on Windows. The fallback crash handler does not create any additional
processes until a crashpad_handler crash occurs. The fallback handler
remains effective in cases where Crashpad's own self-monitoring is
disabled. The fallback handler and Crashpad self-monitoring have
different attributes, so there are no plans to move Windows entirely
onto Crashpad self-monitoring.

For branded Chrome, the dev channel gets Crashpad self-monitoring in 25%
of browser starts, and the beta channel gets it in 10% of browser
starts. On macOS, the stable channel gets self-monitoring in 1% of
browser starts, and the canary and Chromium builds get it all the time.
On Windows, the Chrome stable channel never gets self-monitoring because
the fallback crash handler is sufficient, and the canary and Chromium
builds get it 50% of the time, allowing for some use of the fallback
crash handler in those cases.

While reports will be collected for crashes that occur while Crashpad is
attempting to upload another report, it's unlikley that these reports
will be uploaded automatically under the current one-attempt-per-hour,
no-retry rate limiting strategy. See
https://crashpad.chromium.org/bug/23.

BUG=crashpad:143
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2799013002
Cr-Commit-Position: refs/heads/master@{#463352}
Committed: https://chromium.googlesource.com/chromium/src/+/dde55ab75d6af6c19e99372ef7678c38915be7f6

[Mark Mentovai, 2017-04-05 18:33:35]

Description was changed from

==========
Monitor crashpad_handler for crashes [sometimes]

When Crashpad self-monitoring is enabled, crashes in crashpad_handler
will be written to the crash report database and uploaded according to
the user's preference. Self-monitoring dedicates an additional
crashpad_handler process to serve as the handler for the original
instance. Having a dedicated process is somewhat expensive, so
self-monitoring is not enabled at all times.

An existing mechanism, the "fallback crash handler," is already in place
on Windows. The fallback crash handler does not create any additional
processes until a crashpad_handler crash occurs. The fallback handler
remains effective in cases where Crashpad's own self-monitoring is
disabled. The fallback handler and Crashpad self-monitoring have
different attributes, so there are no plans to move Windows entirely
onto Crashpad self-monitoring.

For branded Chrome, the dev channel gets Crashpad self-monitoring in 25%
of browser starts, and the beta channel gets it in 10% of browser
starts. On macOS, the stable channel gets self-monitoring in 1% of
browser starts, and the canary and Chromium builds get it all the time.
On Windows, the Chrome stable channel never gets self-monitoring because
the fallback crash handler is sufficient, and the canary and Chromium
builds get it 50% of the time, allowing for some use of the fallback
crash handler in those cases.

While reports will be collected for crashes that occur while Crashpad is
attempting to upload another report, it's unlikley that these reports
will be uploaded automatically under the current one-attempt-per-hour,
no-retry rate limiting strategy. See
https://crashpad.chromium.org/bug/23.

BUG=crashpad:143
==========

to

==========
Monitor crashpad_handler for crashes [sometimes]

When Crashpad self-monitoring is enabled, crashes in crashpad_handler
will be written to the crash report database and uploaded according to
the user's preference. Self-monitoring dedicates an additional
crashpad_handler process to serve as the handler for the original
instance. Having a dedicated process is somewhat expensive, so
self-monitoring is not enabled at all times.

An existing mechanism, the "fallback crash handler," is already in place
on Windows. The fallback crash handler does not create any additional
processes until a crashpad_handler crash occurs. The fallback handler
remains effective in cases where Crashpad's own self-monitoring is
disabled. The fallback handler and Crashpad self-monitoring have
different attributes, so there are no plans to move Windows entirely
onto Crashpad self-monitoring.

For branded Chrome, the dev channel gets Crashpad self-monitoring in 25%
of browser starts, and the beta channel gets it in 10% of browser
starts. On macOS, the stable channel gets self-monitoring in 1% of
browser starts, and the canary and Chromium builds get it all the time.
On Windows, the Chrome stable channel never gets self-monitoring because
the fallback crash handler is sufficient, and the canary and Chromium
builds get it 50% of the time, allowing for some use of the fallback
crash handler in those cases.

While reports will be collected for crashes that occur while Crashpad is
attempting to upload another report, it's unlikley that these reports
will be uploaded automatically under the current one-attempt-per-hour,
no-retry rate limiting strategy. See
https://crashpad.chromium.org/bug/23.

BUG=crashpad:143
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[Mark Mentovai, 2017-04-05 18:38:36]

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

[commit-bot: I haz the power, 2017-04-05 18:39: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, 2017-04-05 18:45:37]

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

[commit-bot: I haz the power, 2017-04-05 18:45:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mark Mentovai, 2017-04-05 18:47:39]

mark@chromium.org changed reviewers:
+ grt@chromium.org, robertshield@chromium.org, scottmg@chromium.org,
thestig@chromium.org

[Mark Mentovai, 2017-04-05 18:47:39]

Who’s who:

grt: chrome/app,  chrome/install_static
scottmg: components/crash
thestig: chrome/common, components/version_info
robertshield: chrome_elf

Much of this is just hoisting the Windows implementation of chrome::GetChannel()
into chrome/install_static. I had to do this because the Windows
ChromeCrashReporterClient::ShouldMonitorCrashHandlerExpensively() needs the
channel (not the channel name), and (surprise!)
chrome/app/chrome_crash_reporter_client_win.cc isn’t part of the chrome/app
target at all, it’s actually part of chrome_elf, which intentionally doesn’t
have access to gigantic swaths of the codebase.

The real meat of what’s happening is the stuff in chrome/app and
components/crash.

[robertshield, 2017-04-05 20:27:41]

chrome_elf/BUILD.gn rubber stamp lgtm

[grt (UTC plus 2), 2017-04-06 10:12:23]

lgtm, although i think moving GetChannel out of InstallDetails would be a good
thing.

https://codereview.chromium.org/2799013002/diff/1/chrome/install_static/insta...
File chrome/install_static/install_details.cc (right):

https://codereview.chromium.org/2799013002/diff/1/chrome/install_static/insta...
chrome/install_static/install_details.cc:30: version_info::Channel
InstallDetails::GetChannel() const {
i think this interpretation of the channel string would fit nicely in
install_util.{cc,h}, which can take care of calling
InstallDetails::Get().channel() so that callers need only call
"install_static::GetChannel()".

[Mark Mentovai, 2017-04-06 15:51:36]

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

[commit-bot: I haz the power, 2017-04-06 15:52:01]

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

[Mark Mentovai, 2017-04-06 15:53:19]

Thanks, Greg, I agree that this works out better too. I also switched other
direct users of install_details to install_util where possible in the files I’m
touching. Updated.

[commit-bot: I haz the power, 2017-04-06 18:52:41]

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

[commit-bot: I haz the power, 2017-04-06 18:52:42]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2017-04-06 20:42:09]

lgtm

[scottmg, 2017-04-10 17:09:31]

components/crash lgtm, sorry for the delay.

https://codereview.chromium.org/2799013002/diff/20001/components/crash/conten...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/2799013002/diff/20001/components/crash/conten...
components/crash/content/app/crashpad_win.cc:130: // that minidumps produced by
generate_dump will contain these annotations.
A little questionable here since there's no generate_dump on Windows. (It's fine
to keep it if you prefer symmetry, but just update the comment).

https://codereview.chromium.org/2799013002/diff/20001/components/crash/conten...
File components/crash/content/app/run_as_crashpad_handler_win.cc (right):

https://codereview.chromium.org/2799013002/diff/20001/components/crash/conten...
components/crash/content/app/run_as_crashpad_handler_win.cc:41:
crashpad::CrashpadInfo::GetCrashpadInfo();
I'd like to DCHECK that crashpad_info->simple_annotations() is nullptr here.

[Mark Mentovai, 2017-04-10 17:19:02]

Thanks, welcome back! Updated.

https://codereview.chromium.org/2799013002/diff/20001/components/crash/conten...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/2799013002/diff/20001/components/crash/conten...
components/crash/content/app/crashpad_win.cc:130: // that minidumps produced by
generate_dump will contain these annotations.
scottmg wrote:
> A little questionable here since there's no generate_dump on Windows.

Sure there is!

We don’t ship it in Chrome on any platform, and maybe that’s what’s confusing. I
revised the comment (in both files) to call it “Crashpad’s generate_dump tool.”

https://codereview.chromium.org/2799013002/diff/20001/components/crash/conten...
File components/crash/content/app/run_as_crashpad_handler_win.cc (right):

https://codereview.chromium.org/2799013002/diff/20001/components/crash/conten...
components/crash/content/app/run_as_crashpad_handler_win.cc:41:
crashpad::CrashpadInfo::GetCrashpadInfo();
scottmg wrote:
> I'd like to DCHECK that crashpad_info->simple_annotations() is nullptr here.

Done.

[Mark Mentovai, 2017-04-10 17:19:19]

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

[commit-bot: I haz the power, 2017-04-10 17:20:02]

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

[scottmg, 2017-04-10 18:05:45]

https://codereview.chromium.org/2799013002/diff/20001/components/crash/conten...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/2799013002/diff/20001/components/crash/conten...
components/crash/content/app/crashpad_win.cc:130: // that minidumps produced by
generate_dump will contain these annotations.
On 2017/04/10 17:19:02, Mark Mentovai wrote:
> scottmg wrote:
> > A little questionable here since there's no generate_dump on Windows.
> 
> Sure there is!
> 
> We don’t ship it in Chrome on any platform, and maybe that’s what’s confusing.
I
> revised the comment (in both files) to call it “Crashpad’s generate_dump
tool.”

Huh! I didn't remember porting it. For some reason I was thinking it would
require registration. But I guess there's no reason it does.

[Mark Mentovai, 2017-04-10 18:09:10]

The CQ bit was unchecked by mark@chromium.org

[Mark Mentovai, 2017-04-10 18:09:13]

The CQ bit was checked by mark@chromium.org

[Mark Mentovai, 2017-04-10 18:09:13]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org,
robertshield@chromium.org, thestig@chromium.org, scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/2799013002/#ps40001
(title: "Address review feedback (scottmg)")

[commit-bot: I haz the power, 2017-04-10 18:09:41]

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, 2017-04-10 19:15:05]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491847753050690,
"parent_rev": "67f5982276daedc4bcfe25fbc21d861613625fb8", "commit_rev":
"dde55ab75d6af6c19e99372ef7678c38915be7f6"}

[commit-bot: I haz the power, 2017-04-10 19:15:57]

Description was changed from

==========
Monitor crashpad_handler for crashes [sometimes]

When Crashpad self-monitoring is enabled, crashes in crashpad_handler
will be written to the crash report database and uploaded according to
the user's preference. Self-monitoring dedicates an additional
crashpad_handler process to serve as the handler for the original
instance. Having a dedicated process is somewhat expensive, so
self-monitoring is not enabled at all times.

An existing mechanism, the "fallback crash handler," is already in place
on Windows. The fallback crash handler does not create any additional
processes until a crashpad_handler crash occurs. The fallback handler
remains effective in cases where Crashpad's own self-monitoring is
disabled. The fallback handler and Crashpad self-monitoring have
different attributes, so there are no plans to move Windows entirely
onto Crashpad self-monitoring.

For branded Chrome, the dev channel gets Crashpad self-monitoring in 25%
of browser starts, and the beta channel gets it in 10% of browser
starts. On macOS, the stable channel gets self-monitoring in 1% of
browser starts, and the canary and Chromium builds get it all the time.
On Windows, the Chrome stable channel never gets self-monitoring because
the fallback crash handler is sufficient, and the canary and Chromium
builds get it 50% of the time, allowing for some use of the fallback
crash handler in those cases.

While reports will be collected for crashes that occur while Crashpad is
attempting to upload another report, it's unlikley that these reports
will be uploaded automatically under the current one-attempt-per-hour,
no-retry rate limiting strategy. See
https://crashpad.chromium.org/bug/23.

BUG=crashpad:143
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Monitor crashpad_handler for crashes [sometimes]

When Crashpad self-monitoring is enabled, crashes in crashpad_handler
will be written to the crash report database and uploaded according to
the user's preference. Self-monitoring dedicates an additional
crashpad_handler process to serve as the handler for the original
instance. Having a dedicated process is somewhat expensive, so
self-monitoring is not enabled at all times.

An existing mechanism, the "fallback crash handler," is already in place
on Windows. The fallback crash handler does not create any additional
processes until a crashpad_handler crash occurs. The fallback handler
remains effective in cases where Crashpad's own self-monitoring is
disabled. The fallback handler and Crashpad self-monitoring have
different attributes, so there are no plans to move Windows entirely
onto Crashpad self-monitoring.

For branded Chrome, the dev channel gets Crashpad self-monitoring in 25%
of browser starts, and the beta channel gets it in 10% of browser
starts. On macOS, the stable channel gets self-monitoring in 1% of
browser starts, and the canary and Chromium builds get it all the time.
On Windows, the Chrome stable channel never gets self-monitoring because
the fallback crash handler is sufficient, and the canary and Chromium
builds get it 50% of the time, allowing for some use of the fallback
crash handler in those cases.

While reports will be collected for crashes that occur while Crashpad is
attempting to upload another report, it's unlikley that these reports
will be uploaded automatically under the current one-attempt-per-hour,
no-retry rate limiting strategy. See
https://crashpad.chromium.org/bug/23.

BUG=crashpad:143
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2799013002
Cr-Commit-Position: refs/heads/master@{#463352}
Committed:
https://chromium.googlesource.com/chromium/src/+/dde55ab75d6af6c19e99372ef767...
==========

[commit-bot: I haz the power, 2017-04-10 19:15:59]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/dde55ab75d6af6c19e99372ef767...

[grt (UTC plus 2), 2017-04-19 14:08:37]

lgtm, thanks.