Call RegisterCrashKeys for chrome.exe in static build.

Call RegisterCrashKeys for chrome.exe in static build.

In component builds the single invocation from child_process_logging_win is sufficient (as there is only a single copy of base).

BUG=460512
Committed: https://crrev.com/0bfeabec41ac37bc9e8c0b62316a11269db156e5
Cr-Commit-Position: refs/heads/master@{#320967}

Committed: https://crrev.com/b3fc718aa98116bddd7d6e34952dc45a7e414e0a
Cr-Commit-Position: refs/heads/master@{#321033}

[erikwright (departed), 2015-03-12 19:22:44]

erikwright@chromium.org changed reviewers:
+ grt@chromium.org, rsesek@chromium.org

[erikwright (departed), 2015-03-12 19:27:58]

grt, rsesek: PTAL.

I reverted the previous CL. I'm still verifying locally, but I believe that this
failed to launch under the following precise scenario:

DCHECK enabled in a component build.

I had tested with a static build.

The reason is that, in a component build, there is a single base.dll shared by
the exe and the rest of the modules. So the initialization in
child_process_logging_win.cc is sufficient to initialize that. In a static
build, however, both chrome.dll and chrome.exe have separate instances of base,
needing separate initialization.

I am currently building a component build (and verifying that it crashes with
the old patch and not with the new) and will then proceed to build a static
debug build (and verify that it does not crash but _does_ correctly configure
the crash keys).

I'll complete those tests before landing.

[erikwright (departed), 2015-03-12 19:29:51]

+kbr

[Ken Russell (switch to Gerrit), 2015-03-12 20:23:05]

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

[Ken Russell (switch to Gerrit), 2015-03-12 20:23:06]

Argh.

This seems fine but I wonder whether future changes to Chromium's build (for
example, splitting off base into its own DLL) might break it.

Should a runtime check be used rather than the #ifdef?

[erikwright (departed), 2015-03-13 14:08:01]

On 2015/03/12 20:23:06, Ken Russell wrote:
> Argh.
> 
> This seems fine but I wonder whether future changes to Chromium's build (for
> example, splitting off base into its own DLL) might break it.
> 
> Should a runtime check be used rather than the #ifdef?

I don't mind adding a base::debug::AreCrashKeysRegistered method, or similar,
but I don't think it's according to prevailing style in Chrome.

I don't think there's a single instance of an initialization API in Chrome where
we rely on the API to indicate to the client whether it's already initialized.

[erikwright (departed), 2015-03-13 14:10:24]

I have performed the following tests:

(1) Build the old patchset in Debug, Component. Verified that it crashes.
(2) Build the new patchset, in Debug, Component. Verified that it does not
crash.
(3) Build the new patchset, in Release, Official, Chrome Branding, SyzyASAN,
Kasko. Verified that the patch has the desired result (correctly configuring
support for setting crash keys in chrome.exe).

[erikwright (departed), 2015-03-13 14:10:38]

rsesek, grt: PTAL.

[grt (UTC plus 2), 2015-03-13 14:30:12]

On 2015/03/13 14:08:01, erikwright wrote:
> On 2015/03/12 20:23:06, Ken Russell wrote:
> > Argh.
> > 
> > This seems fine but I wonder whether future changes to Chromium's build (for
> > example, splitting off base into its own DLL) might break it.
> > 
> > Should a runtime check be used rather than the #ifdef?
> 
> I don't mind adding a base::debug::AreCrashKeysRegistered method, or similar,
> but I don't think it's according to prevailing style in Chrome.
> 
> I don't think there's a single instance of an initialization API in Chrome
where
> we rely on the API to indicate to the client whether it's already initialized.

CommandLine, for example, returns false if it has already been initialized. Can
RegisterCrashKeys be made to be idempotent so that it's safe to call more than
once?

[erikwright (departed), 2015-03-13 14:35:20]

On 2015/03/13 14:30:12, grt wrote:
> On 2015/03/13 14:08:01, erikwright wrote:
> > On 2015/03/12 20:23:06, Ken Russell wrote:
> > > Argh.
> > > 
> > > This seems fine but I wonder whether future changes to Chromium's build
(for
> > > example, splitting off base into its own DLL) might break it.
> > > 
> > > Should a runtime check be used rather than the #ifdef?
> > 
> > I don't mind adding a base::debug::AreCrashKeysRegistered method, or
similar,
> > but I don't think it's according to prevailing style in Chrome.
> > 
> > I don't think there's a single instance of an initialization API in Chrome
> where
> > we rely on the API to indicate to the client whether it's already
initialized.
> 
> CommandLine, for example, returns false if it has already been initialized.
Can
> RegisterCrashKeys be made to be idempotent so that it's safe to call more than
> once?

Color me corrected.

Yes, RegisterChromeCrashKeys could be made to be idempotent. Not
base::debug::InitCrashKeys, though, as there would be no reasonable way to deal
with consecutive invocations with distinct arguments.

I'll upload that patchset momentarily.

[erikwright (departed), 2015-03-13 17:10:34]

Patchset #3 (id:40001) has been deleted

[erikwright (departed), 2015-03-13 17:11:48]

grt, rsesek: PTAL.

[grt (UTC plus 2), 2015-03-13 18:12:21]

lgtm

[Robert Sesek, 2015-03-13 20:23:28]

LGTM

[erikwright (departed), 2015-03-17 02:44:11]

In fact, the second patchset doesn't work:

crash_keys.cc is compiled separately into the exe and chrome.dll, so the static
protection against double-invocation doesn't protect anything (there are two
copies of the static).

It is inappropriate, in my opinion, to make base::debug::InitCrashKeys
idempotent, since it could be called twice with distinct parameters.

It's also inappropriate to have it just return false when invoked a second time
and have the caller assume all is good, because a later attempt to use a crash
key that is not registered will cause a DCHECK, so the caller really needs to
know that the initialization happened with the same parameters.

So I think the original patchset is the most correct.

[erikwright (departed), 2015-03-17 02:44:30]

rsesek, grt: PTAL again.

[grt (UTC plus 2), 2015-03-17 02:59:31]

Sigh. Sometimes it just ain't easy.

https://codereview.chromium.org/1008523002/diff/60001/chrome/app/chrome_crash...
File chrome/app/chrome_crash_reporter_client.cc (right):

https://codereview.chromium.org/1008523002/diff/60001/chrome/app/chrome_crash...
chrome/app/chrome_crash_reporter_client.cc:329: return
crash_keys::RegisterChromeCrashKeys();
for safety's sake, should this be
#if defined(COMPONENT_BUILD)
  CHECK(false);
#else
  return crash_keys::RegisterChromeCrashKeys();
#endif

[grt (UTC plus 2), 2015-03-17 13:10:17]

On 2015/03/17 02:59:31, grt wrote:
> Sigh. Sometimes it just ain't easy.

I meant to also add LGTM. I trust your judgement for whether or not the CHECK is
called for. Apologies if my previous advice sent you down a rabbit hole.

[erikwright (departed), 2015-03-17 14:39:17]

Patchset #4 (id:80001) has been deleted

[erikwright (departed), 2015-03-17 14:40:03]

rsesek: I'll wait for your re-review.

Added a NOTREACHED based on grt's suggestion.

https://codereview.chromium.org/1008523002/diff/60001/chrome/app/chrome_crash...
File chrome/app/chrome_crash_reporter_client.cc (right):

https://codereview.chromium.org/1008523002/diff/60001/chrome/app/chrome_crash...
chrome/app/chrome_crash_reporter_client.cc:329: return
crash_keys::RegisterChromeCrashKeys();
On 2015/03/17 02:59:31, grt wrote:
> for safety's sake, should this be
> #if defined(COMPONENT_BUILD)
>   CHECK(false);
> #else
>   return crash_keys::RegisterChromeCrashKeys();
> #endif

I think a NOTREACHED is more appropriate?

The guaranteed outcome of double-registration is a CHECK in
base::debug::InitCrashKeys.

The outcome of non-registration is a DCHECK in debug and missing crash key
values in release.

So I don't see any reason to hard-terminate the process in a release build here,
but I agree that a NOTREACHED is a good way to catch an error.

[erikwright (departed), 2015-03-17 18:15:19]

Patchset #5 (id:120001) has been deleted

[erikwright (departed), 2015-03-17 18:16:02]

I have verified the latest patchset in Debug/Component and Release/Static
configurations and it works as intended in both cases.

[Robert Sesek, 2015-03-17 18:56:01]

https://codereview.chromium.org/1008523002/diff/100001/components/crash/app/b...
File components/crash/app/breakpad_win.cc (right):

https://codereview.chromium.org/1008523002/diff/100001/components/crash/app/b...
components/crash/app/breakpad_win.cc:546: // configuration.
Copy over the comment about where the DLL initializes this?

[erikwright (departed), 2015-03-17 19:01:35]

PTAL.

https://codereview.chromium.org/1008523002/diff/100001/components/crash/app/b...
File components/crash/app/breakpad_win.cc (right):

https://codereview.chromium.org/1008523002/diff/100001/components/crash/app/b...
components/crash/app/breakpad_win.cc:546: // configuration.
On 2015/03/17 18:56:01, Robert Sesek wrote:
> Copy over the comment about where the DLL initializes this?

Done.

[Robert Sesek, 2015-03-17 19:02:15]

LGTM

[erikwright (departed), 2015-03-17 19:02:31]

The CQ bit was checked by erikwright@chromium.org

[erikwright (departed), 2015-03-17 19:02:31]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1008523002/#ps140001
(title: "Add commentary.")

[commit-bot: I haz the power, 2015-03-17 19:02:52]

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

[commit-bot: I haz the power, 2015-03-17 20:52:17]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-03-17 20:52:55]

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

[erikwright (departed), 2015-03-17 22:21:15]

A revert of this CL (patchset #5 id:140001) has been created in
https://codereview.chromium.org/1016933002/ by erikwright@chromium.org.

The reason for reverting is: Test failures..

[erikwright (departed), 2015-03-17 22:24:04]

The ifdef should only apply to Windows. I'm relanding the CL using previous
reviews.

[erikwright (departed), 2015-03-17 22:24:10]

The CQ bit was checked by erikwright@chromium.org

[erikwright (departed), 2015-03-17 22:24:10]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1008523002/#ps160001
(title: "Fix ifdef to only apply to Windows.")

[commit-bot: I haz the power, 2015-03-17 22:25:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1008523002/160001

[commit-bot: I haz the power, 2015-03-18 00:28:33]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2015-03-18 00:29:48]

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