Make Crashpad start asynchronous, and move back to chrome_elf

Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass. This allows for error reporting, and prevents log spew from
each renderer that starts up and tries to connect to a nonexistent
handler. Also added is a UMA timer to see how long we're blocking
startup for by joining with the background thread.

This includes an effective revert of
https://chromium.googlesource.com/chromium/src/+/f3a5670dd8d42b045d31625dde4a2561b471138f
.

BUG=655788, 656800, 565063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

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

[scottmg, 2016-11-04 18:52:05]

Description was changed from

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass, and allows for error reporting. Also added is a UMA timer to
see how long we're blocking startup for by joining with the background
thread.

BUG=655788,656800,565063,656800
==========

to

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass, and allows for error reporting. Also added is a UMA timer to
see how long we're blocking startup for by joining with the background
thread.

BUG=655788,656800,565063,656800
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[scottmg, 2016-11-04 21:36:11]

Description was changed from

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass, and allows for error reporting. Also added is a UMA timer to
see how long we're blocking startup for by joining with the background
thread.

BUG=655788,656800,565063,656800
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass, and allows for error reporting. Also added is a UMA timer to
see how long we're blocking startup for by joining with the background
thread.

This includes an effective revert of
https://chromium.googlesource.com/chromium/src/+/f3a5670dd8d42b045d31625dde4a...
.

BUG=655788,656800,565063,656800
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[scottmg, 2016-11-04 21:37:29]

scottmg@chromium.org changed reviewers:
+ ananta@chromium.org, mark@chromium.org, pennymac@chromium.org

[scottmg, 2016-11-04 21:38:59]

mark: general review
pennymac/ananta: chrome_elf/fyi

[Mark Mentovai, 2016-11-04 21:47:56]

LGTM otherwise

https://codereview.chromium.org/2475863004/diff/20001/chrome/app/chrome_exe_m...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/2475863004/diff/20001/chrome/app/chrome_exe_m...
chrome/app/chrome_exe_main_win.cc:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
CL description: it allows for error reporting and prevents log spew from each
renderer that starts up and tries to connect to a nonexistent crashpad_handler.

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

https://codereview.chromium.org/2475863004/diff/20001/components/crash/conten...
components/crash/content/app/crashpad_win.cc:128: env->SetVar(kPipeNameVar,
I thought that we’d be able to stick the wait in the first call into a getter in
here that grabs the pipe to give to a child, but I see that this isn’t how we’re
organized, we just dump the pipe name into the environment as soon as we can.

https://codereview.chromium.org/2475863004/diff/20001/components/crash/conten...
components/crash/content/app/crashpad_win.cc:195: if
(!internal::g_crashpad_client.Get().WaitForHandlerStart()) {
This isn’t valid unless StartHandler() returned true. There’s even a DCHECK()
that this will trip over.

[scottmg, 2016-11-04 22:05:36]

Description was changed from

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass, and allows for error reporting. Also added is a UMA timer to
see how long we're blocking startup for by joining with the background
thread.

This includes an effective revert of
https://chromium.googlesource.com/chromium/src/+/f3a5670dd8d42b045d31625dde4a...
.

BUG=655788,656800,565063,656800
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass. This allows for error reporting, and prevents log speq from
each renderer that starts up and tries to connect to a nonexistent
handler. Also added is a UMA timer to see how long we're blocking
startup for by joining with the background thread.

This includes an effective revert of
https://chromium.googlesource.com/chromium/src/+/f3a5670dd8d42b045d31625dde4a...
.

BUG=655788,656800,565063,656800
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[scottmg, 2016-11-04 22:13:30]

Description was changed from

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass. This allows for error reporting, and prevents log speq from
each renderer that starts up and tries to connect to a nonexistent
handler. Also added is a UMA timer to see how long we're blocking
startup for by joining with the background thread.

This includes an effective revert of
https://chromium.googlesource.com/chromium/src/+/f3a5670dd8d42b045d31625dde4a...
.

BUG=655788,656800,565063,656800
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass. This allows for error reporting, and prevents log speq from
each renderer that starts up and tries to connect to a nonexistent
handler. Also added is a UMA timer to see how long we're blocking
startup for by joining with the background thread.

This includes an effective revert of
https://chromium.googlesource.com/chromium/src/+/f3a5670dd8d42b045d31625dde4a...
.

BUG=655788,656800,565063,656800
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[scottmg, 2016-11-04 22:13:30]

scottmg@chromium.org changed reviewers:
+ asvitkine@chromium.org, thestig@chromium.org

[scottmg, 2016-11-04 22:13:42]

https://codereview.chromium.org/2475863004/diff/20001/chrome/app/chrome_exe_m...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/2475863004/diff/20001/chrome/app/chrome_exe_m...
chrome/app/chrome_exe_main_win.cc:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2016/11/04 21:47:56, Mark Mentovai wrote:
> CL description: it allows for error reporting and prevents log spew from each
> renderer that starts up and tries to connect to a nonexistent
crashpad_handler.

Done.

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

https://codereview.chromium.org/2475863004/diff/20001/components/crash/conten...
components/crash/content/app/crashpad_win.cc:128: env->SetVar(kPipeNameVar,
On 2016/11/04 21:47:56, Mark Mentovai wrote:
> I thought that we’d be able to stick the wait in the first call into a getter
in
> here that grabs the pipe to give to a child, but I see that this isn’t how
we’re
> organized, we just dump the pipe name into the environment as soon as we can.

Yeah, we could make the call location of BlockUntilHandlerStarted() a get/set in
the environment, but I guess it amounts to about the same thing.

https://codereview.chromium.org/2475863004/diff/20001/components/crash/conten...
components/crash/content/app/crashpad_win.cc:195: if
(!internal::g_crashpad_client.Get().WaitForHandlerStart()) {
On 2016/11/04 21:47:56, Mark Mentovai wrote:
> This isn’t valid unless StartHandler() returned true. There’s even a DCHECK()
> that this will trip over.

Ah good point, I was only considering the async part. I'm going to burn things
down rather than try to set bools and continue if the _synchronous_ part of
StartHandler() fails. That means that a simple call to CreateThread() very early
in the process lifetime failed, implying that the process is totally screwed, so
continuing probably isn't worthwhile. So I LOG(FATAL)d above at StartHandler(),
and noted the dependency in comments.

[scottmg, 2016-11-04 22:14:01]

+thestig: chrome/
+asvitkine: histograms.xml

[Alexei Svitkine (slow), 2016-11-04 22:14:50]

lgtm

[Mark Mentovai, 2016-11-04 22:17:35]

Works for me. LGTM.

[Lei Zhang, 2016-11-04 22:23:53]

lgtm

[scottmg, 2016-11-04 22:48:02]

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

[commit-bot: I haz the power, 2016-11-04 22:49:44]

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-11-05 00:23:18]

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

[commit-bot: I haz the power, 2016-11-05 00:23:19]

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_...)

[scottmg, 2016-11-29 05:45:29]

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

[commit-bot: I haz the power, 2016-11-29 05:45:45]

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-11-29 05:48:28]

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

[commit-bot: I haz the power, 2016-11-29 05:48:30]

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

[scottmg, 2016-11-29 17:14:00]

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

[commit-bot: I haz the power, 2016-11-29 17:14:18]

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-11-29 17:49:47]

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

[commit-bot: I haz the power, 2016-11-29 17:49:48]

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

[scottmg, 2016-11-29 17:52:58]

Patchset #4 (id:60001) has been deleted

[scottmg, 2016-11-29 17:53:42]

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

[commit-bot: I haz the power, 2016-11-29 17:54:00]

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-11-29 20:55:40]

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

[commit-bot: I haz the power, 2016-11-29 20:55:41]

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_...)

[scottmg, 2016-11-29 23:25:15]

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

[scottmg, 2016-11-29 23:25:37]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-11-29 23:26:33]

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-11-30 01:30:01]

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

[commit-bot: I haz the power, 2016-11-30 01:30:02]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[scottmg, 2016-12-03 00:40:29]

Description was changed from

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass. This allows for error reporting, and prevents log speq from
each renderer that starts up and tries to connect to a nonexistent
handler. Also added is a UMA timer to see how long we're blocking
startup for by joining with the background thread.

This includes an effective revert of
https://chromium.googlesource.com/chromium/src/+/f3a5670dd8d42b045d31625dde4a...
.

BUG=655788,656800,565063,656800
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass. This allows for error reporting, and prevents log spew from
each renderer that starts up and tries to connect to a nonexistent
handler. Also added is a UMA timer to see how long we're blocking
startup for by joining with the background thread.

This includes an effective revert of
https://chromium.googlesource.com/chromium/src/+/f3a5670dd8d42b045d31625dde4a...
.

BUG=655788,656800,565063,656800
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[scottmg, 2016-12-03 00:41:06]

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

[commit-bot: I haz the power, 2016-12-03 00:41: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-03 02:06:27]

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

[commit-bot: I haz the power, 2016-12-03 02:06:28]

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

[scottmg, 2016-12-05 18:39:04]

Patchset #5 (id:120001) has been deleted

[scottmg, 2016-12-05 18:45:50]

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

[commit-bot: I haz the power, 2016-12-05 18:46:20]

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

[scottmg, 2016-12-05 19:17:31]

After shaving a few yaks deep, this should finally be able to land, so will CQ
if bots look happy.

[Mark Mentovai, 2016-12-05 20:03:01]

LGTM

[commit-bot: I haz the power, 2016-12-05 20:17:09]

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

[commit-bot: I haz the power, 2016-12-05 20:17:10]

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_...)

[scottmg, 2016-12-05 20:34:22]

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

[commit-bot: I haz the power, 2016-12-05 20:35: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-12-06 00:22:40]

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

[commit-bot: I haz the power, 2016-12-06 00:22:41]

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_...)

[scottmg, 2016-12-06 01:07:40]

Patchset #6 (id:160001) has been deleted

[scottmg, 2016-12-06 17:41:01]

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

[commit-bot: I haz the power, 2016-12-06 17:41:29]

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

[scottmg, 2016-12-06 18:27:33]

Patchset #6 (id:180001) has been deleted

[commit-bot: I haz the power, 2016-12-06 18:28:02]

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_...)

[scottmg, 2016-12-06 18:31:32]

On 2016/12/05 19:17:31, scottmg wrote:
> After shaving a few yaks deep, this should finally be able to land, so will CQ
> if bots look happy.

As it turns out there's another yak named
ConvertStringSecurityDescriptorToSecurityDescriptor().

It has long hair, and lives in advapi32.dll where we can't call it from
chrome_elf during initialization. We need a SECURITY_DESCRIPTOR for
"S:(ML;;;;;S-1-16-0)" to create the pipe that's inherited to the handler (see
CreateNamedPipeInstance()). Unfortunately, the pipe is currently created during
the synchronous part of initialization (i.e. inside DllMain).

Allegedly creating a SECURITY_DESCRIPTOR without that yak is possible, but it
seems very non-trivial. I'm going to look into that, but we might also need to
consider alternate initialization methods.

[scottmg, 2016-12-06 20:22:46]

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

[commit-bot: I haz the power, 2016-12-06 20:23:47]

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

[scottmg, 2016-12-06 20:30:55]

Description was changed from

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass. This allows for error reporting, and prevents log spew from
each renderer that starts up and tries to connect to a nonexistent
handler. Also added is a UMA timer to see how long we're blocking
startup for by joining with the background thread.

This includes an effective revert of
https://chromium.googlesource.com/chromium/src/+/f3a5670dd8d42b045d31625dde4a...
.

BUG=655788,656800,565063,656800
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass. This allows for error reporting, and prevents log spew from
each renderer that starts up and tries to connect to a nonexistent
handler. Also added is a UMA timer to see how long we're blocking
startup for by joining with the background thread.

This includes an effective revert of
https://chromium.googlesource.com/chromium/src/+/f3a5670dd8d42b045d31625dde4a...
.

BUG=655788,656800,565063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[commit-bot: I haz the power, 2016-12-06 21:30:05]

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

[commit-bot: I haz the power, 2016-12-06 21:30:07]

Dry run: This issue passed the CQ dry run.

[scottmg, 2016-12-08 23:35:35]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2016-12-08 23:35:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, thestig@chromium.org, mark@chromium.org
Link to the patchset: https://codereview.chromium.org/2475863004/#ps220001
(title: "rebase part 3")

[commit-bot: I haz the power, 2016-12-08 23:36: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-12-08 23:44:51]

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

[commit-bot: I haz the power, 2016-12-08 23:44:52]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[scottmg, 2016-12-08 23:49:46]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2016-12-08 23:50:21]

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-08 23:54:12]

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

[commit-bot: I haz the power, 2016-12-08 23:54:13]

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

[scottmg, 2016-12-09 00:18:19]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2016-12-09 00:18:49]

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-09 01:05:42]

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

[commit-bot: I haz the power, 2016-12-09 01:05:46]

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

[scottmg, 2016-12-09 01:30:05]

scottmg@chromium.org changed reviewers:
+ robertshield@chromium.org

[scottmg, 2016-12-09 01:31:04]

+robertshield for chrome_elf/OWNERS

[robertshield, 2016-12-09 01:56:44]

https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1419: GetProcAddress(chrome_elf,
"BlockUntilHandlerStartedImpl"));
This isn't in elf's .def file. Should it be?

[scottmg, 2016-12-09 20:23:15]

https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1419: GetProcAddress(chrome_elf,
"BlockUntilHandlerStartedImpl"));
On 2016/12/09 01:56:44, robertshield_slow_reviews wrote:
> This isn't in elf's .def file. Should it be?

We seem to have a mix of __declspec(dllexport) and .def export. I was following
the others in the file where BlockUntilHandlerStartedImpl was added, which are
in components/... rather than chrome_elf/

I did confirm that it is indeed in the /exports of chrome_elf.dll though.

[robertshield, 2016-12-09 21:11:47]

https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1419: GetProcAddress(chrome_elf,
"BlockUntilHandlerStartedImpl"));
On 2016/12/09 20:23:14, scottmg wrote:
> On 2016/12/09 01:56:44, robertshield_slow_reviews wrote:
> > This isn't in elf's .def file. Should it be?
> 
> We seem to have a mix of __declspec(dllexport) and .def export. I was
following
> the others in the file where BlockUntilHandlerStartedImpl was added, which are
> in components/... rather than chrome_elf/
> 
> I did confirm that it is indeed in the /exports of chrome_elf.dll though.

No worries, just thought it was a bit weird that we have both ways of declaring
exports. Might be nice to just use one (would probably pick the declspec
approach).

FWIW it's also a little odd that a number of the dll exports are called FooImpl
but maybe that's fine since chrome.exe and chrome_elf.dll are so tightly
coupled. 

Anyway, this comment feels like nits, feel free to ignore entirely if you'd
like.

https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1422: block_until_handler_started();
This is a WaitForSingleObject(.., INFINITE) at Chrome startup. If something goes
wrong here does Chrome hang forever or will a watchdog step in and crash the
process / send us a minidump? If the former, would it be worth using a
non-INFINITE wait instead?

I can't see an obvious case in which the wait wouldn't return immediately, but
there might be an unobvious case (some 3rd party module's DLL_THREAD_ATTACH
taking forever or some such). 

Maybe this is fine, but figured I'd ask.

[scottmg, 2016-12-13 18:37:37]

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

[commit-bot: I haz the power, 2016-12-13 18:38:02]

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

[scottmg, 2016-12-13 18:46:59]

https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1419: GetProcAddress(chrome_elf,
"BlockUntilHandlerStartedImpl"));
On 2016/12/09 21:11:47, robertshield_slow_reviews wrote:
> On 2016/12/09 20:23:14, scottmg wrote:
> > On 2016/12/09 01:56:44, robertshield_slow_reviews wrote:
> > > This isn't in elf's .def file. Should it be?
> > 
> > We seem to have a mix of __declspec(dllexport) and .def export. I was
> following
> > the others in the file where BlockUntilHandlerStartedImpl was added, which
are
> > in components/... rather than chrome_elf/
> > 
> > I did confirm that it is indeed in the /exports of chrome_elf.dll though.
> 
> No worries, just thought it was a bit weird that we have both ways of
declaring
> exports. Might be nice to just use one (would probably pick the declspec
> approach).
> 
> FWIW it's also a little odd that a number of the dll exports are called
FooImpl
> but maybe that's fine since chrome.exe and chrome_elf.dll are so tightly
> coupled. 
> 
> Anyway, this comment feels like nits, feel free to ignore entirely if you'd
> like. 

Yeah, it's a bit messy. I think "Impl" has been used to just mean "Thunk" or
"Helper" or something. We should clean all those up, but I'm going to go with
deferring that to a later CL for the purposes of this change.

https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1422: block_until_handler_started();
On 2016/12/09 21:11:47, robertshield_slow_reviews wrote:
> This is a WaitForSingleObject(.., INFINITE) at Chrome startup. If something
goes
> wrong here does Chrome hang forever or will a watchdog step in and crash the
> process / send us a minidump? If the former, would it be worth using a
> non-INFINITE wait instead?
> 
> I can't see an obvious case in which the wait wouldn't return immediately, but
> there might be an unobvious case (some 3rd party module's DLL_THREAD_ATTACH
> taking forever or some such). 
> 
> Maybe this is fine, but figured I'd ask. 

Updated so that it waits a maximum of 5s. There's probably still some case where
the act of WFSO could be bad if the process is really screwed, but at least a
temporary wait is better than an INFINITE wait.

[commit-bot: I haz the power, 2016-12-13 20:01:10]

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

[commit-bot: I haz the power, 2016-12-13 20:01:12]

Dry run: This issue passed the CQ dry run.

[scottmg, 2016-12-14 17:53:33]

On 2016/12/13 18:46:59, scottmg wrote:
>
https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_...
> File chrome/browser/chrome_browser_main.cc (right):
> 
>
https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_...
> chrome/browser/chrome_browser_main.cc:1419: GetProcAddress(chrome_elf,
> "BlockUntilHandlerStartedImpl"));
> On 2016/12/09 21:11:47, robertshield_slow_reviews wrote:
> > On 2016/12/09 20:23:14, scottmg wrote:
> > > On 2016/12/09 01:56:44, robertshield_slow_reviews wrote:
> > > > This isn't in elf's .def file. Should it be?
> > > 
> > > We seem to have a mix of __declspec(dllexport) and .def export. I was
> > following
> > > the others in the file where BlockUntilHandlerStartedImpl was added, which
> are
> > > in components/... rather than chrome_elf/
> > > 
> > > I did confirm that it is indeed in the /exports of chrome_elf.dll though.
> > 
> > No worries, just thought it was a bit weird that we have both ways of
> declaring
> > exports. Might be nice to just use one (would probably pick the declspec
> > approach).
> > 
> > FWIW it's also a little odd that a number of the dll exports are called
> FooImpl
> > but maybe that's fine since chrome.exe and chrome_elf.dll are so tightly
> > coupled. 
> > 
> > Anyway, this comment feels like nits, feel free to ignore entirely if you'd
> > like. 
> 
> Yeah, it's a bit messy. I think "Impl" has been used to just mean "Thunk" or
> "Helper" or something. We should clean all those up, but I'm going to go with
> deferring that to a later CL for the purposes of this change.
> 
>
https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_...
> chrome/browser/chrome_browser_main.cc:1422: block_until_handler_started();
> On 2016/12/09 21:11:47, robertshield_slow_reviews wrote:
> > This is a WaitForSingleObject(.., INFINITE) at Chrome startup. If something
> goes
> > wrong here does Chrome hang forever or will a watchdog step in and crash the
> > process / send us a minidump? If the former, would it be worth using a
> > non-INFINITE wait instead?
> > 
> > I can't see an obvious case in which the wait wouldn't return immediately,
but
> > there might be an unobvious case (some 3rd party module's DLL_THREAD_ATTACH
> > taking forever or some such). 
> > 
> > Maybe this is fine, but figured I'd ask. 
> 
> Updated so that it waits a maximum of 5s. There's probably still some case
where
> the act of WFSO could be bad if the process is really screwed, but at least a
> temporary wait is better than an INFINITE wait.

Robert, ping?

[robertshield, 2016-12-15 20:06:30]

LGTM, w/ one comment update suggestion

https://codereview.chromium.org/2475863004/diff/240001/components/crash/conte...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/2475863004/diff/240001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:126: // converted to non-fatal,
calls to BlockUntilHandlerStarted() will have
nit: consider updating the comment about "if this is converted to non-fatal". As
discussed we probably don't ever want to do this conversion.

[scottmg, 2016-12-16 17:16:51]

https://codereview.chromium.org/2475863004/diff/240001/components/crash/conte...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/2475863004/diff/240001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:126: // converted to non-fatal,
calls to BlockUntilHandlerStarted() will have
On 2016/12/15 20:06:30, robertshield_slow_reviews wrote:
> nit: consider updating the comment about "if this is converted to non-fatal".
As
> discussed we probably don't ever want to do this conversion.

Done.

[scottmg, 2016-12-16 17:17:42]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2016-12-16 17:17:42]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
thestig@chromium.org, asvitkine@chromium.org, robertshield@chromium.org
Link to the patchset: https://codereview.chromium.org/2475863004/#ps260001
(title: "comment")

[commit-bot: I haz the power, 2016-12-16 17:18:27]

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 19:55:00]

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

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

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[scottmg, 2016-12-16 20:00:14]

The CQ bit was checked by scottmg@chromium.org

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

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 21:01:05]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1481918414192480,
"parent_rev": "38692e762529c219ec05fec22eac287c08039591", "commit_rev":
"1adfa4247a8dab1f850588442258ac1286d12ef7"}

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

Description was changed from

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass. This allows for error reporting, and prevents log spew from
each renderer that starts up and tries to connect to a nonexistent
handler. Also added is a UMA timer to see how long we're blocking
startup for by joining with the background thread.

This includes an effective revert of
https://chromium.googlesource.com/chromium/src/+/f3a5670dd8d42b045d31625dde4a...
.

BUG=655788,656800,565063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass. This allows for error reporting, and prevents log spew from
each renderer that starts up and tries to connect to a nonexistent
handler. Also added is a UMA timer to see how long we're blocking
startup for by joining with the background thread.

This includes an effective revert of
https://chromium.googlesource.com/chromium/src/+/f3a5670dd8d42b045d31625dde4a...
.

BUG=655788,656800,565063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

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

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

Committed patchset #9 (id:260001)

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

Description was changed from

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass. This allows for error reporting, and prevents log spew from
each renderer that starts up and tries to connect to a nonexistent
handler. Also added is a UMA timer to see how long we're blocking
startup for by joining with the background thread.

This includes an effective revert of
https://chromium.googlesource.com/chromium/src/+/f3a5670dd8d42b045d31625dde4a...
.

BUG=655788,656800,565063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

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

to

==========
Make Crashpad start asynchronous, and move back to chrome_elf

Crashpad initialization has been reworked to support an asynchronous
mode where StartHandler() only calls CreateThread() and does not
synchronize with that thread, making it safe to use in DllMain().

So, we can now move it back into chrome_elf to catch earlier crashes.

We block much later now in browser startup, before a renderer (i.e.
another client that would connect to the handler) will be started. This
should not be strictly necessary, but is slightly more conservative as a
first pass. This allows for error reporting, and prevents log spew from
each renderer that starts up and tries to connect to a nonexistent
handler. Also added is a UMA timer to see how long we're blocking
startup for by joining with the background thread.

This includes an effective revert of
https://chromium.googlesource.com/chromium/src/+/f3a5670dd8d42b045d31625dde4a...
.

BUG=655788,656800,565063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

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

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

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