Make Crashpad start asynchronous, and move back to chrome_elf

chrome/browser/chrome_browser_main.cc

Index: chrome/browser/chrome_browser_main.cc
diff --git a/chrome/browser/chrome_browser_main.cc b/chrome/browser/chrome_browser_main.cc
index 19a377e00806b21c43b962e24f91e4baf94a9442..478a41cf691e66e8c73037156dd11ded9bd76676 100644
--- a/chrome/browser/chrome_browser_main.cc
+++ b/chrome/browser/chrome_browser_main.cc
@@ -213,6 +213,7 @@
 #include "chrome/installer/util/helper.h"
 #include "chrome/installer/util/install_util.h"
 #include "chrome/installer/util/shell_util.h"
+#include "components/crash/content/app/crashpad.h"
 #include "ui/base/l10n/l10n_util_win.h"
 #include "ui/shell_dialogs/select_file_dialog.h"
 #endif  // defined(OS_WIN)
@@ -1411,6 +1412,18 @@ void ChromeBrowserMainParts::PostBrowserStart() {
 int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   TRACE_EVENT0("startup", "ChromeBrowserMainParts::PreMainMessageLoopRunImpl");
 
+#if defined(OS_WIN)
+  HMODULE chrome_elf = GetModuleHandle(chrome::kChromeElfDllName);
+  if (chrome_elf) {
+    auto block_until_handler_started = reinterpret_cast<void (*)()>(
+        GetProcAddress(chrome_elf, "BlockUntilHandlerStartedImpl"));

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

This isn't in elf's .def file. Should it be?

[scottmg, 2016/12/09 20:23:14]

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]

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. 

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

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.

+    if (block_until_handler_started) {
+      SCOPED_UMA_HISTOGRAM_TIMER("Startup.BlockForCrashpadHandlerStartupTime");
+      block_until_handler_started();

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

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:46:59]

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.

+    }
+  }
+#endif
+
   SCOPED_UMA_HISTOGRAM_LONG_TIMER("Startup.PreMainMessageLoopRunImplLongTime");
   const base::TimeTicks start_time_step1 = base::TimeTicks::Now();