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

Review-Url: https://codereview.chromium.org/2475863004
Cr-Commit-Position: refs/heads/master@{#439188}

chrome/app/chrome_exe_main_win.cc

@@ -206,7 +206,6 @@ int main() {
 #endif
   install_static::InstallDetails::InitializeFromPrimaryModule(
       chrome::kChromeElfDllName);
-  SignalInitializeCrashReporting();
 
   // Initialize the CommandLine singleton from the environment.
   base::CommandLine::Init(0, nullptr);

chrome/browser/chrome_browser_main.cc

@@ -212,6 +212,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)
@@ -1402,6 +1403,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"));
+    if (block_until_handler_started) {
+      SCOPED_UMA_HISTOGRAM_TIMER("Startup.BlockForCrashpadHandlerStartupTime");
+      block_until_handler_started();
+    }
+  }
+#endif
+
   SCOPED_UMA_HISTOGRAM_LONG_TIMER("Startup.PreMainMessageLoopRunImplLongTime");
   const base::TimeTicks start_time_step1 = base::TimeTicks::Now();
 

chrome_elf/chrome_elf.def

@@ -11,5 +11,4 @@ EXPORTS
   GetUserDataDirectoryThunk
   IsBlacklistInitialized
   SignalChromeElf
-  SignalInitializeCrashReporting
   SuccessfullyBlocked

chrome_elf/chrome_elf_main.cc

@@ -14,17 +14,6 @@
 #include "chrome_elf/blacklist/blacklist.h"
 #include "chrome_elf/crash/crash_helper.h"
 
-// This function is a temporary workaround for https://crbug.com/655788. We
-// need to come up with a better way to initialize crash reporting that can
-// happen inside DllMain().
-void SignalInitializeCrashReporting() {
-  if (!elf_crash::InitializeCrashReporting()) {
-#ifdef _DEBUG
-    assert(false);
-#endif  // _DEBUG
-  }
-}
-
 void SignalChromeElf() {
   blacklist::ResetBeacon();
 }
@@ -55,6 +44,10 @@ BOOL APIENTRY DllMain(HMODULE module, DWORD reason, LPVOID reserved) {
   if (reason == DLL_PROCESS_ATTACH) {
     install_static::InitializeProductDetailsForPrimaryModule();
 
+    if (!elf_crash::InitializeCrashReporting()) {
+      assert(false);
+    }
+
     // CRT on initialization installs an exception filter which calls
     // TerminateProcess. We need to hook CRT's attempt to set an exception.
     elf_crash::DisableSetUnhandledExceptionFilter();

chrome_elf/chrome_elf_main.h

@@ -5,7 +5,6 @@
 #ifndef CHROME_ELF_CHROME_ELF_MAIN_H_
 #define CHROME_ELF_CHROME_ELF_MAIN_H_
 
-extern "C" void SignalInitializeCrashReporting();
 extern "C" void SignalChromeElf();
 
 #endif  // CHROME_ELF_CHROME_ELF_MAIN_H_

components/crash/content/app/crashpad.cc

@@ -335,6 +335,13 @@ void __declspec(dllexport)
     RequestSingleCrashUploadImpl(const std::string& local_id) {
   crash_reporter::RequestSingleCrashUpload(local_id);
 }
+
+// This helper is invoked by code in chrome.dll to wait for the handler start to
+// complete.
+void __declspec(dllexport) BlockUntilHandlerStartedImpl() {
+  crash_reporter::BlockUntilHandlerStarted();
+}
+
 }  // extern "C"
 
 #endif  // OS_WIN

components/crash/content/app/crashpad.h

@@ -53,6 +53,10 @@ void InitializeCrashpad(bool initial_client, const std::string& process_type);
 // line argument of --type=crashpad-handler.
 void InitializeCrashpadWithEmbeddedHandler(bool initial_client,
                                            const std::string& process_type);
+
+// Waits until the handler has successfully completed startup or failed, and
+// logs an error in that case.
+void BlockUntilHandlerStarted();
 #endif  // OS_WIN
 
 // Enables or disables crash report upload, taking the given consent to upload

components/crash/content/app/crashpad_win.cc

@@ -118,9 +118,15 @@ base::FilePath PlatformCrashpadInitialization(bool initial_client,
       exe_file = exe_dir.Append(FILE_PATH_LITERAL("crashpad_handler.exe"));
     }
 
-    g_crashpad_client.Get().StartHandler(
-        exe_file, database_path, metrics_path, url, process_annotations,
-        arguments, false, false);
+    if (!g_crashpad_client.Get().StartHandler(
+            exe_file, database_path, metrics_path, url, process_annotations,
+            arguments, false, true)) {
+      // This means that CreateThread() failed, so this process is very messed
+      // up. This should be effectively unreachable. It is unlikely that there
+      // is any utility to ever making this non-fatal, however, if this is done,
+      // calls to BlockUntilHandlerStarted() will have to be amended.
+      LOG(FATAL) << "synchronous part of handler startup failed";
+    }
 
     // If we're the browser, push the pipe name into the environment so child
     // processes can connect to it. If we inherited another crashpad_handler's
@@ -208,6 +214,16 @@ MSVC_ENABLE_OPTIMIZE()
 }  // namespace
 
 }  // namespace internal
+
+void BlockUntilHandlerStarted() {
+  // We know that the StartHandler() at least started asynchronous startup if
+  // we're here, as if it doesn't, we abort.
+  const unsigned int kTimeoutMS = 5000;
+  if (!internal::g_crashpad_client.Get().WaitForHandlerStart(kTimeoutMS)) {
+    LOG(ERROR) << "Crashpad handler failed to start, crash reporting disabled";
+  }
+}
+
 }  // namespace crash_reporter
 
 extern "C" {

tools/metrics/histograms/histograms.xml

@@ -63125,6 +63125,14 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="Startup.BlockForCrashpadHandlerStartupTime" units="ms">
+  <owner>crashpad-dev@chromium.org</owner>
+  <summary>
+    The amount of time that elapsed during in
+    crash_report::BlockUntilHandlerStarted().
+  </summary>
+</histogram>
+
 <histogram name="Startup.BringToForegroundReason"
     enum="BooleanBringToForegroundReason">
   <owner>johnme@chromium.org</owner>