Initial addition of Blimp engine crash client code (take 2)

Initial addition of Blimp engine crash client code (take 2)

This introduces a simple crash client for the engine, which includes a client
that will always allow and always upload crash reports with the
"Chrome_Blimp_Engine" name. Crash reporting is enabled for all processes.
wget is added to the Docker container as it's necessary for crash uploads.

This is based on http://crrev.com/1783053002, with the addition of
missing //content crash keys (without which you'd check-fail), and a
note to people who modify the list in //chrome/common/crash_keys.cc
to also kindly update our list if they're changing things. This should
get better when http://crbug.com/598854 is resolved.

BUG=597454
Committed: https://crrev.com/ffc513054fd32c30c67c4e6d6798c2f198974aec
Cr-Commit-Position: refs/heads/master@{#391577}

[marcinjb, 2016-04-28 20:56:47]

marcinjb@chromium.org changed reviewers:
+ kmarshall@chromium.org, rsesek@chromium.org

[marcinjb, 2016-04-28 20:56:47]

rsesek@chromium.org: Please review changes in //chrome/common/crash_keys.cc, as
well as the added dependency on //components/crash

kmarshall@chromium.org: This is basically the same as
http://crrev.com/1783053002.

NB: While I do have a refactor of the chromium crash keys in progress, this is
to unstick blimp engine crash reporting until that lands.

[Robert Sesek, 2016-04-28 21:37:31]

https://codereview.chromium.org/1925993004/diff/1/blimp/engine/app/blimp_engi...
File blimp/engine/app/blimp_engine_crash_keys.cc (right):

https://codereview.chromium.org/1925993004/diff/1/blimp/engine/app/blimp_engi...
blimp/engine/app/blimp_engine_crash_keys.cc:23: { ::crash_keys::kClientId,
::crash_keys::kSmallSize },
Do you need to prefix everything with :: ?

[marcinjb, 2016-04-28 22:00:34]

PTAL

https://codereview.chromium.org/1925993004/diff/1/blimp/engine/app/blimp_engi...
File blimp/engine/app/blimp_engine_crash_keys.cc (right):

https://codereview.chromium.org/1925993004/diff/1/blimp/engine/app/blimp_engi...
blimp/engine/app/blimp_engine_crash_keys.cc:23: { ::crash_keys::kClientId,
::crash_keys::kSmallSize },
On 2016/04/28 21:37:30, Robert Sesek wrote:
> Do you need to prefix everything with :: ?

Hmm, I think I originally had an additional nested crash_keys namespace here,
but that disappeared somewhere along the way (if that were the case, I'd
strongly have preferred the leading ::'s to remove any ambiguity).

Removed.

[Robert Sesek, 2016-04-28 22:02:50]

LGTM

https://codereview.chromium.org/1925993004/diff/1/blimp/engine/app/blimp_engi...
File blimp/engine/app/blimp_engine_crash_keys.cc (right):

https://codereview.chromium.org/1925993004/diff/1/blimp/engine/app/blimp_engi...
blimp/engine/app/blimp_engine_crash_keys.cc:23: { ::crash_keys::kClientId,
::crash_keys::kSmallSize },
On 2016/04/28 22:00:34, marcinjb wrote:
> On 2016/04/28 21:37:30, Robert Sesek wrote:
> > Do you need to prefix everything with :: ?
> 
> Hmm, I think I originally had an additional nested crash_keys namespace here,
> but that disappeared somewhere along the way (if that were the case, I'd
> strongly have preferred the leading ::'s to remove any ambiguity).
> 
> Removed.

Right, and the compiler probably would want them to remove ambiguity too.

[Kevin M, 2016-05-02 22:05:32]

https://codereview.chromium.org/1925993004/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_content_main_delegate.cc (right):

https://codereview.chromium.org/1925993004/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_main_delegate.cc:73: const std::string
process_type =
No need to bind a variable, just use the switch getter inline.

https://codereview.chromium.org/1925993004/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_engine_crash_keys.cc (right):

https://codereview.chromium.org/1925993004/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_engine_crash_keys.cc:22: const
std::vector<base::debug::CrashKey> engine_keys = {
Oh noes, brace initialization isn't allowed yet :-O

I think you can do this instead:

const base::debug::CrashKey[] engine_keys = { { crash_keys::blah,
crash_keys::foo }, ...}

and then you can pass |engine_keys| and arraysize(engine_keys) into
InitCrashKeys.

https://codereview.chromium.org/1925993004/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_engine_crash_keys.cc:34: { "channel_error_bt",
crash_keys::kMediumSize },
Let's keep each block of pairs alphabetized

https://codereview.chromium.org/1925993004/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_engine_crash_reporter_client.h (right):

https://codereview.chromium.org/1925993004/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_engine_crash_reporter_client.h:28: void
SetCrashReporterClientIdFromGUID(
private:
// CrashReporterClient implementation.

https://codereview.chromium.org/1925993004/diff/20001/chrome/common/crash_key...
File chrome/common/crash_keys.cc (right):

https://codereview.chromium.org/1925993004/diff/20001/chrome/common/crash_key...
chrome/common/crash_keys.cc:92: // If you're adding keys here, please also add
them to the list in
Maintaining two parallel key lists seems risky, and burdensome for contributors.

I recommend factoring out fixed_keys into a global array (kChromeCrashKeys?)
that we can reference and build off of in Blimp.

[Robert Sesek, 2016-05-02 22:07:00]

https://codereview.chromium.org/1925993004/diff/20001/chrome/common/crash_key...
File chrome/common/crash_keys.cc (right):

https://codereview.chromium.org/1925993004/diff/20001/chrome/common/crash_key...
chrome/common/crash_keys.cc:92: // If you're adding keys here, please also add
them to the list in
On 2016/05/02 22:05:32, Kevin M wrote:
> Maintaining two parallel key lists seems risky, and burdensome for
contributors.
> 
> I recommend factoring out fixed_keys into a global array (kChromeCrashKeys?)
> that we can reference and build off of in Blimp.

The CL description mentions a bug to fix this issue.

[marcinjb, 2016-05-03 18:06:59]

PTAL

https://codereview.chromium.org/1925993004/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_content_main_delegate.cc (right):

https://codereview.chromium.org/1925993004/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_main_delegate.cc:73: const std::string
process_type =
On 2016/05/02 22:05:31, Kevin M wrote:
> No need to bind a variable, just use the switch getter inline.

Done.

https://codereview.chromium.org/1925993004/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_engine_crash_keys.cc (right):

https://codereview.chromium.org/1925993004/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_engine_crash_keys.cc:22: const
std::vector<base::debug::CrashKey> engine_keys = {
On 2016/05/02 22:05:31, Kevin M wrote:
> Oh noes, brace initialization isn't allowed yet :-O
> 
> I think you can do this instead:
> 
> const base::debug::CrashKey[] engine_keys = { { crash_keys::blah,
> crash_keys::foo }, ...}
> 
> and then you can pass |engine_keys| and arraysize(engine_keys) into
> InitCrashKeys.

I saw some people breaking the rules and thought that I'd be one of the cool
kids and do the same.  Way to take all the fun out of initializing vectors, man.

Done.

https://codereview.chromium.org/1925993004/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_engine_crash_keys.cc:34: { "channel_error_bt",
crash_keys::kMediumSize },
On 2016/05/02 22:05:31, Kevin M wrote:
> Let's keep each block of pairs alphabetized

This was mostly a copy and paste job from //chrome/common/crash_keys.cc, but
done anyway.

https://codereview.chromium.org/1925993004/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_engine_crash_reporter_client.h (right):

https://codereview.chromium.org/1925993004/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_engine_crash_reporter_client.h:28: void
SetCrashReporterClientIdFromGUID(
On 2016/05/02 22:05:32, Kevin M wrote:
> private:
> // CrashReporterClient implementation.
> 

Done.

https://codereview.chromium.org/1925993004/diff/20001/chrome/common/crash_key...
File chrome/common/crash_keys.cc (right):

https://codereview.chromium.org/1925993004/diff/20001/chrome/common/crash_key...
chrome/common/crash_keys.cc:92: // If you're adding keys here, please also add
them to the list in
On 2016/05/02 22:07:00, Robert Sesek wrote:
> On 2016/05/02 22:05:32, Kevin M wrote:
> > Maintaining two parallel key lists seems risky, and burdensome for
> contributors.
> > 
> > I recommend factoring out fixed_keys into a global array (kChromeCrashKeys?)
> > that we can reference and build off of in Blimp.
> 
> The CL description mentions a bug to fix this issue.

Yeah, I've got something going in http://crrev.com/1849413003, but that's going
to take some time finish and to land.  This CL is a way to get us crash
reporting sooner rather than later.

In parallel, the testing team is getting a buildbot to hopefully test some basic
interactions, so if someone introduces a breaking change, it'll get caught in
the CQ.

[Kevin M, 2016-05-03 20:06:13]

lgtm

https://codereview.chromium.org/1925993004/diff/60001/blimp/engine/Dockerfile
File blimp/engine/Dockerfile (right):

https://codereview.chromium.org/1925993004/diff/60001/blimp/engine/Dockerfile...
blimp/engine/Dockerfile:10: RUN apt-get update && apt-get install -yq stunnel4
wget
Is this Dockerfile change related to the crash code? Can you add an entry to the
CL description for this?

https://codereview.chromium.org/1925993004/diff/60001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_engine_crash_reporter_client.cc (right):

https://codereview.chromium.org/1925993004/diff/60001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_engine_crash_reporter_client.cc:26: *product_name =
"Chrome_Blimp_Engine";
Make this a kConstant

[marcinjb, 2016-05-03 20:20:02]

Description was changed from

==========
Initial addition of Blimp engine crash client code (take 2)

This introduces a simple crash client for the engine, which includes a client
that will always allow and always upload crash reports with the
"Chrome_Blimp_Engine" name. Crash reporting is enabled for all processes.

This is based on http://crrev.com/1783053002, with the addition of
missing //content crash keys (without which you'd check-fail), and a
note to people who modify the list in //chrome/common/crash_keys.cc
to also kindly update our list if they're changing things. This should
get better when http://crbug.com/598854 is resolved.

BUG=597454
==========

to

==========
Initial addition of Blimp engine crash client code (take 2)

This introduces a simple crash client for the engine, which includes a client
that will always allow and always upload crash reports with the
"Chrome_Blimp_Engine" name. Crash reporting is enabled for all processes.
wget is added to the Docker container as it's necessary for crash uploads.

This is based on http://crrev.com/1783053002, with the addition of
missing //content crash keys (without which you'd check-fail), and a
note to people who modify the list in //chrome/common/crash_keys.cc
to also kindly update our list if they're changing things. This should
get better when http://crbug.com/598854 is resolved.

BUG=597454
==========

[marcinjb, 2016-05-03 22:06:31]

Submitting after the trybots pass

https://codereview.chromium.org/1925993004/diff/60001/blimp/engine/Dockerfile
File blimp/engine/Dockerfile (right):

https://codereview.chromium.org/1925993004/diff/60001/blimp/engine/Dockerfile...
blimp/engine/Dockerfile:10: RUN apt-get update && apt-get install -yq stunnel4
wget
On 2016/05/03 20:06:13, Kevin M wrote:
> Is this Dockerfile change related to the crash code? Can you add an entry to
the
> CL description for this?

Added to the CL description.

https://codereview.chromium.org/1925993004/diff/60001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_engine_crash_reporter_client.cc (right):

https://codereview.chromium.org/1925993004/diff/60001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_engine_crash_reporter_client.cc:26: *product_name =
"Chrome_Blimp_Engine";
On 2016/05/03 20:06:13, Kevin M wrote:
> Make this a kConstant

Done.

[Robert Sesek, 2016-05-03 22:07:47]

FYI you're racing https://codereview.chromium.org/1855383002/

[marcinjb, 2016-05-03 22:49:03]

Dang, what are the odds?

I'll hold off mine until http://crrev.com/1855383002 lands as that's already in
the CQ.  Afterwards I'll merge and update.

[ncarter (slow), 2016-05-03 23:05:51]

On 2016/05/03 22:49:03, marcinjb wrote:
> Dang, what are the odds?
> 
> I'll hold off mine until http://crrev.com/1855383002 lands as that's already
in
> the CQ.  Afterwards I'll merge and update.

Thanks. I'll try to remember to delete the keys from the second location when
reverting my patch in a couple days.

Do we have a plan to avoid the duplication? Manually keeping them in sync sucks.
Could content could provide its list in a header file?

[Robert Sesek, 2016-05-03 23:06:24]

On 2016/05/03 23:05:51, ncarter wrote:
> On 2016/05/03 22:49:03, marcinjb wrote:
> > Dang, what are the odds?
> > 
> > I'll hold off mine until http://crrev.com/1855383002 lands as that's already
> in
> > the CQ.  Afterwards I'll merge and update.
> 
> Thanks. I'll try to remember to delete the keys from the second location when
> reverting my patch in a couple days.
> 
> Do we have a plan to avoid the duplication? Manually keeping them in sync
sucks.
> Could content could provide its list in a header file?

There's a bug to fix this referenced in the CL description.

[marcinjb, 2016-05-04 17:02:31]

The CQ bit was checked by marcinjb@chromium.org

[marcinjb, 2016-05-04 17:02:31]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1925993004/#ps120001
(title: "Add crash keys related to https://crbug.com/600441")

[commit-bot: I haz the power, 2016-05-04 17:02:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1925993004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1925993004/120001

[commit-bot: I haz the power, 2016-05-04 17:10:37]

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

[commit-bot: I haz the power, 2016-05-04 17:10:38]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[marcinjb, 2016-05-04 17:20:56]

The CQ bit was checked by marcinjb@chromium.org

[marcinjb, 2016-05-04 17:20:56]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1925993004/#ps140001
(title: "Merge with head and resolve conflicts")

[commit-bot: I haz the power, 2016-05-04 17:21:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1925993004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1925993004/140001

[commit-bot: I haz the power, 2016-05-04 18:35:38]

Description was changed from

==========
Initial addition of Blimp engine crash client code (take 2)

This introduces a simple crash client for the engine, which includes a client
that will always allow and always upload crash reports with the
"Chrome_Blimp_Engine" name. Crash reporting is enabled for all processes.
wget is added to the Docker container as it's necessary for crash uploads.

This is based on http://crrev.com/1783053002, with the addition of
missing //content crash keys (without which you'd check-fail), and a
note to people who modify the list in //chrome/common/crash_keys.cc
to also kindly update our list if they're changing things. This should
get better when http://crbug.com/598854 is resolved.

BUG=597454
==========

to

==========
Initial addition of Blimp engine crash client code (take 2)

This introduces a simple crash client for the engine, which includes a client
that will always allow and always upload crash reports with the
"Chrome_Blimp_Engine" name. Crash reporting is enabled for all processes.
wget is added to the Docker container as it's necessary for crash uploads.

This is based on http://crrev.com/1783053002, with the addition of
missing //content crash keys (without which you'd check-fail), and a
note to people who modify the list in //chrome/common/crash_keys.cc
to also kindly update our list if they're changing things. This should
get better when http://crbug.com/598854 is resolved.

BUG=597454
==========

[commit-bot: I haz the power, 2016-05-04 18:35:40]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-04 18:37:53]

Description was changed from

==========
Initial addition of Blimp engine crash client code (take 2)

This introduces a simple crash client for the engine, which includes a client
that will always allow and always upload crash reports with the
"Chrome_Blimp_Engine" name. Crash reporting is enabled for all processes.
wget is added to the Docker container as it's necessary for crash uploads.

This is based on http://crrev.com/1783053002, with the addition of
missing //content crash keys (without which you'd check-fail), and a
note to people who modify the list in //chrome/common/crash_keys.cc
to also kindly update our list if they're changing things. This should
get better when http://crbug.com/598854 is resolved.

BUG=597454
==========

to

==========
Initial addition of Blimp engine crash client code (take 2)

This introduces a simple crash client for the engine, which includes a client
that will always allow and always upload crash reports with the
"Chrome_Blimp_Engine" name. Crash reporting is enabled for all processes.
wget is added to the Docker container as it's necessary for crash uploads.

This is based on http://crrev.com/1783053002, with the addition of
missing //content crash keys (without which you'd check-fail), and a
note to people who modify the list in //chrome/common/crash_keys.cc
to also kindly update our list if they're changing things. This should
get better when http://crbug.com/598854 is resolved.

BUG=597454

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

[commit-bot: I haz the power, 2016-05-04 18:37:54]

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