win: Implement CrashpadClient::StartHandler()

client/crashpad_client_win.cc

Index: client/crashpad_client_win.cc
diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc
index 92529ae18989c002ccb9f507294003daf6799509..1dbda0012064342687ffac13226ebfc875d2e9b6 100644
--- a/client/crashpad_client_win.cc
+++ b/client/crashpad_client_win.cc
@@ -19,10 +19,13 @@
 
 #include "base/atomicops.h"
 #include "base/logging.h"
+#include "base/rand_util.h"
 #include "base/strings/string16.h"
+#include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/lock.h"
 #include "util/file/file_io.h"
+#include "util/win/command_line.h"
 #include "util/win/critical_section_with_debug_info.h"
 #include "util/win/registration_protocol_win.h"
 #include "util/win/scoped_handle.h"
@@ -102,11 +105,17 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) {
   return EXCEPTION_CONTINUE_SEARCH;
 }
 
+std::wstring FormatArgumentString(const std::string& name,
+                                  const std::wstring& value) {
+  return std::wstring(L"--") + base::UTF8ToUTF16(name) + L"=" + value;
+}
+
 }  // namespace
 
 namespace crashpad {
 
-CrashpadClient::CrashpadClient() {
+CrashpadClient::CrashpadClient()
+    : ipc_port_() {
 }
 
 CrashpadClient::~CrashpadClient() {
@@ -118,11 +127,81 @@ bool CrashpadClient::StartHandler(
     const std::string& url,
     const std::map<std::string, std::string>& annotations,
     const std::vector<std::string>& arguments) {
-  LOG(FATAL) << "SetHandler should be used on Windows";
-  return false;
+  DCHECK(ipc_port_.empty());
+
+  ipc_port_ =
+      base::StringPrintf("\\\\.\\pipe\\crashpad_%d_", GetCurrentProcessId());
+  for (int index = 0; index < 16; ++index) {
+    ipc_port_.append(1, static_cast<char>(base::RandInt('A', 'Z')));
+  }
+
+  std::wstring command_line;
+  AppendCommandLineArgument(handler.value(), &command_line);
+  for (const std::string& argument : arguments) {
+    AppendCommandLineArgument(base::UTF8ToUTF16(argument), &command_line);
+  }
+  if (!database.value().empty()) {
+    AppendCommandLineArgument(FormatArgumentString("database",
+                                                   database.value()),
+                              &command_line);
+  }
+  if (!url.empty()) {
+    AppendCommandLineArgument(FormatArgumentString("url",
+                                                   base::UTF8ToUTF16(url)),
+                              &command_line);
+  }
+  for (const auto& kv : annotations) {
+    AppendCommandLineArgument(
+        FormatArgumentString("annotation",
+                             base::UTF8ToUTF16(kv.first + '=' + kv.second)),
+        &command_line);
+  }
+  AppendCommandLineArgument(FormatArgumentString("pipe-name",
+                                                 base::UTF8ToUTF16(ipc_port_)),
+                            &command_line);
+
+  STARTUPINFO startup_info = {};
+  startup_info.cb = sizeof(startup_info);
+  startup_info.dwFlags = STARTF_USESTDHANDLES;
+  startup_info.hStdInput = GetStdHandle(STD_INPUT_HANDLE);

[scottmg, 2015/10/30 23:16:31]

This was one reason I hadn't done this yet. In order to use crashpad_handler
directly in Chrome we'd need to make it a windowed app, so then I wasn't sure
what to do with output.

[Mark Mentovai, 2015/10/31 01:22:24]

scottmg wrote:
Can you explain this a little more?

These handles are one of the warts here, I think. I don’t really want to give
the new process a stdin at all, but I wasn’t sure if I could pass
INVALID_HANDLE_VALUE here or if that would do the right thing.

I was also planning on changing this in a follow-up to use
PROC_THREAD_ATTRIBUTE_LIST to limit the inherited handles, when the runtime OS
supports that.

[Mark Mentovai, 2015/10/31 18:34:29]

I wrote:
I guess that question is kinda vague…

What I mean is: was this going to be a problem if we were to run
crashpad_handler (in its current console program form) as a service without
being started by Chrome too?

Would it help to put STARTF_USESHOWWINDOW into startup_info.dwFlags and set
startup_info.wShowWindow to SW_HIDE?

As for the output, is there a handle that we can get that would go to a system
log or something? Chrome uses the same LOG() family and it’s a GUI program, how
does it deal with this?

[scottmg, 2015/10/31 18:34:59]

Not strictly related, but this is where I stopped because I wasn't sure what to
do. In general, we want to have the logs from crashpad_handler go somewhere, but
in order to use crashpad_handler in Chrome it needs to be switched to a
/subsystem:windows. So then I wasn't sure about creating a log file, or creating
a console sometimes, or attaching to an existing console, or some other option.
I believe INVALID_HANDLE_VALUE is fine as long as we don't actually do a read
from it. That is, it stays truly invalid, it's not a null handle. For that, we
have to open "NUL" and pass that through.

+  startup_info.hStdOutput = GetStdHandle(STD_OUTPUT_HANDLE);
+  startup_info.hStdError = GetStdHandle(STD_ERROR_HANDLE);
+  PROCESS_INFORMATION process_info;
+  BOOL rv = CreateProcess(handler.value().c_str(),
+                          const_cast<wchar_t*>(command_line.c_str()),

[scottmg, 2015/10/30 23:16:31]

Does this actually make it mutable? (In practice I'm sure it doesn't matter, but
I thought it should be &command_line[0]). (CreateProcessW can write to the
argument.)

[Mark Mentovai, 2015/10/31 01:22:24]

scottmg wrote:
I didn’t realize that! Should have read the documentation more closely. I’ll fix
it.

+                          nullptr,
+                          nullptr,
+                          true,
+                          0,
+                          nullptr,
+                          nullptr,
+                          &startup_info,
+                          &process_info);
+  if (!rv) {
+    PLOG(ERROR) << "CreateProcess";
+    return false;
+  }
+
+  rv = CloseHandle(process_info.hThread);
+  PLOG_IF(WARNING, !rv) << "CloseHandle thread";
+
+  rv = CloseHandle(process_info.hProcess);
+  PLOG_IF(WARNING, !rv) << "CloseHandle process";
+
+  return true;
 }
 
 bool CrashpadClient::SetHandler(const std::string& ipc_port) {
+  DCHECK(ipc_port_.empty());
+  DCHECK(!ipc_port.empty());
+
+  ipc_port_ = ipc_port;
+
+  return true;
+}
+
+bool CrashpadClient::UseHandler() {
+  DCHECK(!ipc_port_.empty());
   DCHECK_EQ(g_signal_exception, INVALID_HANDLE_VALUE);
   DCHECK_EQ(g_signal_non_crash_dump, INVALID_HANDLE_VALUE);
   DCHECK_EQ(g_non_crash_dump_done, INVALID_HANDLE_VALUE);
@@ -154,7 +233,7 @@ bool CrashpadClient::SetHandler(const std::string& ipc_port) {
   ServerToClientMessage response = {0};
 
   if (!SendToCrashHandlerServer(
-          base::UTF8ToUTF16(ipc_port), message, &response)) {
+          base::UTF8ToUTF16(ipc_port_), message, &response)) {
     return false;
   }
 
@@ -168,16 +247,6 @@ bool CrashpadClient::SetHandler(const std::string& ipc_port) {
 
   g_non_crash_dump_lock = new base::Lock();
 
-  return true;
-}
-
-bool CrashpadClient::UseHandler() {
-  if (g_signal_exception == INVALID_HANDLE_VALUE ||
-      g_signal_non_crash_dump == INVALID_HANDLE_VALUE ||
-      g_non_crash_dump_done == INVALID_HANDLE_VALUE) {
-    return false;
-  }
-
   // In theory we could store the previous handler but it is not clear what
   // use we have for it.
   SetUnhandledExceptionFilter(&UnhandledExceptionHandler);
@@ -188,7 +257,7 @@ bool CrashpadClient::UseHandler() {
 void CrashpadClient::DumpWithoutCrash(const CONTEXT& context) {
   if (g_signal_non_crash_dump == INVALID_HANDLE_VALUE ||
       g_non_crash_dump_done == INVALID_HANDLE_VALUE) {
-    LOG(ERROR) << "haven't called SetHandler()";
+    LOG(ERROR) << "haven't called UseHandler()";
     return;
   }