Introduce the ability to wait for the watcher process to initialize.

chrome/app/chrome_watcher_command_line_win.cc

Index: chrome/app/chrome_watcher_command_line_win.cc
diff --git a/chrome/app/chrome_watcher_command_line_win.cc b/chrome/app/chrome_watcher_command_line_win.cc
index 00938631eba5d67f676676e68ca842fcc19c4b46..e88a21f0553aacb4fd25ca4d4f2486ac871d7234 100644
--- a/chrome/app/chrome_watcher_command_line_win.cc
+++ b/chrome/app/chrome_watcher_command_line_win.cc
@@ -14,41 +14,86 @@
 #include "chrome/common/chrome_switches.h"
 
 namespace {
+
+const char kOnIninitializedEventHandleSwitch[] = "on-initialized-event-handle";
 const char kParentHandleSwitch[] = "parent-handle";
+
+void AppendHandleSwitch(base::CommandLine* command_line,

[grt (UTC plus 2), 2015/02/03 21:20:45]

base::CommandLine* command_line should be last since it's an in/out param

[erikwright (departed), 2015/02/03 21:56:47]

Done.

+                        const std::string& switch_name,
+                        HANDLE handle) {
+  command_line->AppendSwitchASCII(
+      switch_name, base::UintToString(reinterpret_cast<unsigned int>(handle)));
+}
+
+HANDLE ReadHandleFromSwitch(const base::CommandLine& command_line,
+                            const std::string& switch_name) {
+  std::string switch_string = command_line.GetSwitchValueASCII(switch_name);
+  unsigned switch_uint = 0;

[grt (UTC plus 2), 2015/02/03 21:20:45]

"unsigned" -> "unsigned int"

[erikwright (departed), 2015/02/03 21:56:47]

Done.

+  if (switch_string.empty() ||
+      !base::StringToUint(switch_string, &switch_uint)) {
+    LOG(ERROR) << "Missing or invalid " << switch_name << " argument.";
+    return NULL;

[grt (UTC plus 2), 2015/02/03 21:20:45]

nullptr

[erikwright (departed), 2015/02/03 21:56:47]

Done.

+  }
+  return reinterpret_cast<HANDLE>(switch_uint);
+}
+
 }  // namespace
 
 base::CommandLine GenerateChromeWatcherCommandLine(
     const base::FilePath& chrome_exe,
+    HANDLE on_initialized_event,
     HANDLE parent_process) {
   base::CommandLine command_line(chrome_exe);
   command_line.AppendSwitchASCII(switches::kProcessType, "watcher");
-  command_line.AppendSwitchASCII(
-      kParentHandleSwitch,
-      base::UintToString(reinterpret_cast<unsigned int>(parent_process)));
+  AppendHandleSwitch(&command_line, kOnIninitializedEventHandleSwitch,
+                     on_initialized_event);
+  AppendHandleSwitch(&command_line, kParentHandleSwitch, parent_process);
   return command_line;
 }
 
-base::win::ScopedHandle InterpretChromeWatcherCommandLine(
-    const base::CommandLine& command_line) {
-  std::string parent_handle_str =
-      command_line.GetSwitchValueASCII(kParentHandleSwitch);
-  unsigned parent_handle_uint = 0;
-  if (parent_handle_str.empty() ||
-      !base::StringToUint(parent_handle_str, &parent_handle_uint)) {
-    LOG(ERROR) << "Missing or invalid " << kParentHandleSwitch << " argument.";
-    return base::win::ScopedHandle();
+bool InterpretChromeWatcherCommandLine(
+    const base::CommandLine& command_line,
+    base::win::ScopedHandle* on_initialized_event,
+    base::win::ScopedHandle* parent_process) {
+  DCHECK(on_initialized_event);
+  DCHECK(parent_process);
+
+  HANDLE parent_handle =
+      ReadHandleFromSwitch(command_line, kParentHandleSwitch);
+  HANDLE on_initialized_event_handle =
+      ReadHandleFromSwitch(command_line, kOnIninitializedEventHandleSwitch);
+
+  if (parent_handle) {
+    // Initial test of the handle, a zero PID indicates invalid handle, or not
+    // a process handle. In this case, parsing fails and we avoid closing the
+    // handle.
+    DWORD process_pid = ::GetProcessId(parent_handle);
+    if (process_pid == 0) {
+      LOG(ERROR) << "Invalid " << kParentHandleSwitch
+                 << " argument. Can't get parent PID.";
+    } else {
+      parent_process->Set(parent_handle);
+    }
+  }
+
+  if (on_initialized_event_handle) {
+    DWORD result = ::WaitForSingleObject(on_initialized_event_handle, 0);
+    if (result == WAIT_FAILED) {
+      PLOG(ERROR) << "Unexpected error while testing the initialization event.";
+    } else if (result != WAIT_TIMEOUT) {
+      LOG(ERROR) << "Unexpected result while testing the initialization event "
+                    "with WaitForSingleObject: " << result;
+    } else {
+      on_initialized_event->Set(on_initialized_event_handle);
+    }
   }
 
-  HANDLE parent_process = reinterpret_cast<HANDLE>(parent_handle_uint);
-  // Initial test of the handle, a zero PID indicates invalid handle, or not
-  // a process handle. In this case, parsing fails and we avoid closing the
-  // handle.
-  DWORD process_pid = ::GetProcessId(parent_process);
-  if (process_pid == 0) {
-    LOG(ERROR) << "Invalid " << kParentHandleSwitch
-               << " argument. Can't get parent PID.";
-    return base::win::ScopedHandle();
+  if (!on_initialized_event->IsValid() || !parent_process->IsValid()) {

[grt (UTC plus 2), 2015/02/03 21:20:45]

this function doesn't ensure that the two ScopedHandles provided by the caller
are not valid upon entry, so it isn't safe to rely on them being valid only if
set on line 75 and 87.

[erikwright (departed), 2015/02/03 21:56:47]

Added explicit Close calls as an expedient and consistent solution.

+    // If one was valid and not the other, free the valid one.
+    on_initialized_event->Close();
+    parent_process->Close();
+    return false;
   }
 
-  return base::win::ScopedHandle(parent_process);
+  return true;
 }