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

chrome/app/chrome_watcher_command_line_win.cc

 // Copyright (c) 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/app/chrome_watcher_command_line_win.h"
 
 #include <string>
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #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);
+    }
   }
 
-  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_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);
+    }
   }
 
-  return base::win::ScopedHandle(parent_process);
+  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 true;
 }