Refactor parsing of the Chrome Watcher command line to make the parent handle accessible outside ExitCodeWatcher.

This CL changes the API between chrome/app and chrome/chrome_watcher. Previously, chrome_watcher was responsible for choosing and interpreting a command-line for launching the Watcher. Now the watcher API is a typical typed C++ API and chrome/app is responsible for all command-line handling.

BUG=

Review URL: https://codereview.chromium.org/841553003

Cr-Commit-Position: refs/heads/master@{#310802}

14 files changed, 214 insertions, 177 deletions

diff --git a/chrome/app/chrome_watcher_command_line_unittest_win.cc b/chrome/app/chrome_watcher_command_line_unittest_win.cc
new file mode 100644
index 0000000..0cf6927
--- /dev/null
+++ b/chrome/app/chrome_watcher_command_line_unittest_win.cc
@@ -0,0 +1,20 @@
+// 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 "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/process/process_handle.h"
+#include "base/win/scoped_handle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+TEST(ChromeWatcherCommandLineTest, BasicTest) {
+  base::ProcessHandle current = nullptr;
+  ASSERT_TRUE(base::OpenProcessHandle(base::GetCurrentProcId(), &current));
+  base::CommandLine cmd_line =
+      GenerateChromeWatcherCommandLine(base::FilePath(L"example.exe"), current);
+  base::win::ScopedHandle result = InterpretChromeWatcherCommandLine(cmd_line);
+  ASSERT_EQ(current, result.Get());
+}
diff --git a/chrome/app/chrome_watcher_command_line_win.cc b/chrome/app/chrome_watcher_command_line_win.cc
new file mode 100644
index 0000000..0093863
--- /dev/null
+++ b/chrome/app/chrome_watcher_command_line_win.cc
@@ -0,0 +1,54 @@
+// 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 kParentHandleSwitch[] = "parent-handle";
+}  // namespace
+
+base::CommandLine GenerateChromeWatcherCommandLine(
+    const base::FilePath& chrome_exe,
+    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)));
+  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();
+  }
+
+  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();
+  }
+
+  return base::win::ScopedHandle(parent_process);
+}
diff --git a/chrome/app/chrome_watcher_command_line_win.h b/chrome/app/chrome_watcher_command_line_win.h
new file mode 100644
index 0000000..30ff988
--- /dev/null
+++ b/chrome/app/chrome_watcher_command_line_win.h
@@ -0,0 +1,29 @@
+// 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.
+
+#ifndef CHROME_APP_CHROME_WATCHER_COMMAND_LINE_WIN_H_
+#define CHROME_APP_CHROME_WATCHER_COMMAND_LINE_WIN_H_
+
+#include <windows.h>
+
+#include "base/win/scoped_handle.h"
+
+namespace base {
+class CommandLine;
+class FilePath;
+}  // namespace base
+
+// Generates a CommandLine that will launch |chrome_exe| in Chrome Watcher mode
+// to observe |parent_process|.
+base::CommandLine GenerateChromeWatcherCommandLine(
+    const base::FilePath& chrome_exe,
+    HANDLE parent_process);
+
+// Interprets the Command Line used to launch a Chrome Watcher process and
+// extracts the parent process HANDLE. Verifies that the handle is usable in
+// this process before returning it, and returns NULL in case of a failure.
+base::win::ScopedHandle InterpretChromeWatcherCommandLine(
+    const base::CommandLine& command_line);
+
+#endif  // CHROME_APP_CHROME_WATCHER_COMMAND_LINE_WIN_H_
diff --git a/chrome/app/client_util.cc b/chrome/app/client_util.cc
index f99a11a..86e3e4f 100644
--- a/chrome/app/client_util.cc
+++ b/chrome/app/client_util.cc
@@ -18,8 +18,10 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/version.h"
+#include "base/win/scoped_handle.h"
 #include "base/win/windows_version.h"
 #include "chrome/app/chrome_crash_reporter_client.h"
+#include "chrome/app/chrome_watcher_command_line_win.h"
 #include "chrome/app/client_util.h"
 #include "chrome/app/image_pre_reader_win.h"
 #include "chrome/common/chrome_constants.h"
@@ -177,6 +179,11 @@ int MainDllLoader::Launch(HINSTANCE instance) {
   }
 
   if (process_type_ == "watcher") {
+    base::win::ScopedHandle parent_process =
+        InterpretChromeWatcherCommandLine(cmd_line);
+    if (!parent_process.IsValid())
+      return chrome::RESULT_CODE_UNSUPPORTED_PARAM;
+
     // Intentionally leaked.
     HMODULE watcher_dll = Load(&version, &file);
     if (!watcher_dll)
@@ -186,8 +193,8 @@ int MainDllLoader::Launch(HINSTANCE instance) {
         reinterpret_cast<browser_watcher::WatcherMainFunction>(
             ::GetProcAddress(watcher_dll,
                              browser_watcher::kWatcherDLLEntrypoint));
-
-    return watcher_main(chrome::kBrowserExitCodesRegistryPath);
+    return watcher_main(chrome::kBrowserExitCodesRegistryPath,
+                        parent_process.Take());
   }
 
   // Initialize the sandbox services.
@@ -255,10 +262,8 @@ void ChromeDllLoader::OnBeforeLaunch(const std::string& process_type,
       base::char16 exe_path[MAX_PATH];
       ::GetModuleFileNameW(nullptr, exe_path, arraysize(exe_path));
 
-      base::CommandLine cmd_line = base::CommandLine(base::FilePath(exe_path));
-      cmd_line.AppendSwitchASCII(switches::kProcessType, "watcher");
-      browser_watcher::WatcherClient watcher_client(cmd_line);
-
+      browser_watcher::WatcherClient watcher_client(base::Bind(
+          &GenerateChromeWatcherCommandLine, base::FilePath(exe_path)));
       watcher_client.LaunchWatcher();
     }
   }
diff --git a/chrome/chrome_exe.gypi b/chrome/chrome_exe.gypi
index fe1f92b..0c78a0e 100644
--- a/chrome/chrome_exe.gypi
+++ b/chrome/chrome_exe.gypi
@@ -62,6 +62,8 @@
         'app/chrome_exe_main_mac.cc',
         'app/chrome_exe_main_win.cc',
         'app/chrome_exe_resource.h',
+        'app/chrome_watcher_command_line_win.cc',
+        'app/chrome_watcher_command_line_win.h',
         'app/client_util.cc',
         'app/client_util.h',
         'app/signature_validator_win.cc',
diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi
index 7ffd1ad..2e319fe 100644
--- a/chrome/chrome_tests_unit.gypi
+++ b/chrome/chrome_tests_unit.gypi
@@ -2719,6 +2719,8 @@
         '..',
       ],
       'sources': [
+        'app/chrome_watcher_command_line_unittest_win.cc',
+        'app/chrome_watcher_command_line_win.cc',
         'app/delay_load_hook_win.cc',
         'app/delay_load_hook_win.h',
         'app/delay_load_hook_unittest_win.cc',
diff --git a/chrome/chrome_watcher/chrome_watcher_main.cc b/chrome/chrome_watcher/chrome_watcher_main.cc
index 0eb644c..8dab91a 100644
--- a/chrome/chrome_watcher/chrome_watcher_main.cc
+++ b/chrome/chrome_watcher/chrome_watcher_main.cc
@@ -54,7 +54,9 @@ BOOL CALLBACK ConsoleCtrlHandler(DWORD ctl_type) {
 
 // The main entry point to the watcher, declared as extern "C" to avoid name
 // mangling.
-extern "C" int WatcherMain(const base::char16* registry_path) {
+extern "C" int WatcherMain(const base::char16* registry_path,
+                           HANDLE process_handle) {
+  base::Process process(process_handle);
   // The exit manager is in charge of calling the dtors of singletons.
   base::AtExitManager exit_manager;
   // Initialize the commandline singleton from the environment.
@@ -71,12 +73,11 @@ extern "C" int WatcherMain(const base::char16* registry_path) {
   browser_watcher::ExitCodeWatcher exit_code_watcher(registry_path);
   int ret = 1;
   // Attempt to wait on our parent process, and record its exit status.
-  if (exit_code_watcher.ParseArguments(
-        *base::CommandLine::ForCurrentProcess())) {
+  if (exit_code_watcher.Initialize(process.Duplicate())) {
     // Set up a console control handler, and provide it the data it needs
     // to record into the browser's exit funnel.
     HandlerData data;
-    data.process = exit_code_watcher.process().Handle();
+    data.process = process.Handle();
     data.registry_path = registry_path;
     g_handler_data = &data;
     ::SetConsoleCtrlHandler(&ConsoleCtrlHandler, TRUE /* Add */);
@@ -85,7 +86,7 @@ extern "C" int WatcherMain(const base::char16* registry_path) {
     exit_code_watcher.WaitForExit();
 
     browser_watcher::ExitFunnel funnel;
-    funnel.Init(registry_path, exit_code_watcher.process().Handle());
+    funnel.Init(registry_path, process.Handle());
     funnel.RecordEvent(L"BrowserExit");
 
     // Chrome/content exit codes are currently in the range of 0-28.
diff --git a/components/browser_watcher/exit_code_watcher_win.cc b/components/browser_watcher/exit_code_watcher_win.cc
index 0c9e7df..218978c 100644
--- a/components/browser_watcher/exit_code_watcher_win.cc
+++ b/components/browser_watcher/exit_code_watcher_win.cc
@@ -4,10 +4,8 @@
 
 #include "components/browser_watcher/exit_code_watcher_win.h"
 
-#include "base/command_line.h"
 #include "base/logging.h"
 #include "base/process/kill.h"
-#include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/win/registry.h"
 
@@ -24,8 +22,6 @@ base::string16 GetValueName(const base::Time creation_time,
 
 }  // namespace
 
-const char ExitCodeWatcher::kParenthHandleSwitch[] = "parent-handle";
-
 ExitCodeWatcher::ExitCodeWatcher(const base::char16* registry_path) :
     registry_path_(registry_path), exit_code_(STILL_ACTIVE) {
 }
@@ -33,36 +29,23 @@ ExitCodeWatcher::ExitCodeWatcher(const base::char16* registry_path) :
 ExitCodeWatcher::~ExitCodeWatcher() {
 }
 
-bool ExitCodeWatcher::ParseArguments(const base::CommandLine& cmd_line) {
-  std::string process_handle_str =
-      cmd_line.GetSwitchValueASCII(kParenthHandleSwitch);
-  unsigned process_handle_uint = 0;
-  if (process_handle_str.empty() ||
-      !base::StringToUint(process_handle_str, &process_handle_uint)) {
-    LOG(ERROR) << "Missing or invalid parent-handle argument.";
-    return false;
-  }
-
-  HANDLE process_handle = reinterpret_cast<HANDLE>(process_handle_uint);
-  // Initial test of the handle, a zero PID indicates invalid handle, or not
-  // a process handle. In this case, bail immediately and avoid closing the
-  // handle.
-  DWORD process_pid = ::GetProcessId(process_handle);
+bool ExitCodeWatcher::Initialize(base::Process process) {
+  DWORD process_pid = process.pid();
   if (process_pid == 0) {
-    LOG(ERROR) << "Invalid parent-handle, can't get parent PID.";
+    LOG(ERROR) << "Invalid parent handle, can't get parent process ID.";
     return false;
   }
 
   FILETIME creation_time = {};
   FILETIME dummy = {};
-  if (!::GetProcessTimes(process_handle, &creation_time,
-                         &dummy, &dummy, &dummy)) {
-    LOG(ERROR) << "Invalid parent handle, can't get parent process times.";
+  if (!::GetProcessTimes(process.Handle(), &creation_time, &dummy, &dummy,
+                         &dummy)) {
+    PLOG(ERROR) << "Invalid parent handle, can't get parent process times.";
     return false;
   }
 
   // Success, take ownership of the process handle.
-  process_ = base::Process(process_handle);
+  process_ = process.Pass();
   process_creation_time_ = base::Time::FromFileTime(creation_time);
 
   // Start by writing the value STILL_ACTIVE to registry, to allow detection
diff --git a/components/browser_watcher/exit_code_watcher_win.h b/components/browser_watcher/exit_code_watcher_win.h
index 672a527..644b2a5 100644
--- a/components/browser_watcher/exit_code_watcher_win.h
+++ b/components/browser_watcher/exit_code_watcher_win.h
@@ -10,31 +10,23 @@
 #include "base/time/time.h"
 #include "base/win/scoped_handle.h"
 
-namespace base {
-class CommandLine;
-}
-
 namespace browser_watcher {
 
 // Watches for the exit code of a process and records it in a given registry
 // location.
 class ExitCodeWatcher {
  public:
-  // Name of the switch used for the parent process handle.
-  static const char kParenthHandleSwitch[];
-
   // Initialize the watcher with a registry path.
   explicit ExitCodeWatcher(const base::char16* registry_path);
   ~ExitCodeWatcher();
 
   // Initializes from arguments on |cmd_line|, returns true on success.
-  // This function expects the process handle indicated by kParentHandleSwitch
-  // in |cmd_line| to be open with sufficient privilege to wait and retrieve
-  // the process exit code.
+  // This function expects |process| to be open with sufficient privilege to
+  // wait and retrieve the process exit code.
   // It checks the handle for validity and takes ownership of it.
   // The intent is for this handle to be inherited into the watcher process
   // hosting the instance of this class.
-  bool ParseArguments(const base::CommandLine& cmd_line);
+  bool Initialize(base::Process process);
 
   // Waits for the process to exit and records its exit code in registry.
   // This is a blocking call.
diff --git a/components/browser_watcher/exit_code_watcher_win_unittest.cc b/components/browser_watcher/exit_code_watcher_win_unittest.cc
index b226700..dc6a0e1 100644
--- a/components/browser_watcher/exit_code_watcher_win_unittest.cc
+++ b/components/browser_watcher/exit_code_watcher_win_unittest.cc
@@ -22,7 +22,7 @@ namespace browser_watcher {
 
 namespace {
 
-const base::char16 kRegistryPath[] = L"Software\\BrowserWatcherTest";
+const base::char16 kRegistryPath[] = L"Software\\ExitCodeWatcherTest";
 
 MULTIPROCESS_TEST_MAIN(Sleeper) {
   // Sleep forever - the test harness will kill this process to give it an
@@ -65,18 +65,6 @@ class ScopedSleeperProcess {
     is_killed_ = true;
   }
 
-  void GetNewHandle(base::ProcessHandle* output) {
-    ASSERT_TRUE(process_.IsValid());
-
-    ASSERT_TRUE(DuplicateHandle(::GetCurrentProcess(),
-                                process_.Handle(),
-                                ::GetCurrentProcess(),
-                                output,
-                                0,
-                                FALSE,
-                                DUPLICATE_SAME_ACCESS));
-  }
-
   const base::Process& process() const { return process_; }
 
  private:
@@ -84,13 +72,13 @@ class ScopedSleeperProcess {
   bool is_killed_;
 };
 
-class BrowserWatcherTest : public testing::Test {
+class ExitCodeWatcherTest : public testing::Test {
  public:
   typedef testing::Test Super;
 
   static const int kExitCode = 0xCAFEBABE;
 
-  BrowserWatcherTest() :
+  ExitCodeWatcherTest() :
       cmd_line_(base::CommandLine::NO_PROGRAM),
       process_(base::kNullProcessHandle) {
   }
@@ -110,10 +98,11 @@ class BrowserWatcherTest : public testing::Test {
     Super::TearDown();
   }
 
-  void OpenSelfWithAccess(uint32 access) {
-    ASSERT_EQ(base::kNullProcessHandle, process_);
-    ASSERT_TRUE(base::OpenProcessHandleWithAccess(
-        base::GetCurrentProcId(), access, &process_));
+  base::Process OpenSelfWithAccess(uint32 access) {
+    HANDLE self = nullptr;
+    EXPECT_TRUE(base::OpenProcessHandleWithAccess(base::GetCurrentProcId(),
+                                                  access, &self));
+    return base::Process(self);
   }
 
   void VerifyWroteExitCode(base::ProcessId proc_id, int exit_code) {
@@ -142,76 +131,46 @@ class BrowserWatcherTest : public testing::Test {
 
 }  // namespace
 
-TEST_F(BrowserWatcherTest, ExitCodeWatcherInvalidCmdLineFailsInit) {
-  ExitCodeWatcher watcher(kRegistryPath);
-
-  // An empty command line should fail.
-  EXPECT_FALSE(watcher.ParseArguments(cmd_line_));
-
-  // A non-numeric parent-handle argument should fail.
-  cmd_line_.AppendSwitchASCII(ExitCodeWatcher::kParenthHandleSwitch, "asdf");
-  EXPECT_FALSE(watcher.ParseArguments(cmd_line_));
-}
-
-TEST_F(BrowserWatcherTest, ExitCodeWatcherInvalidHandleFailsInit) {
+TEST_F(ExitCodeWatcherTest, ExitCodeWatcherInvalidHandleFailsInit) {
   ExitCodeWatcher watcher(kRegistryPath);
 
   // A waitable event has a non process-handle.
-  base::WaitableEvent event(false, false);
+  base::Process event(::CreateEvent(NULL, false, false, NULL));
 
   // A non-process handle should fail.
-  cmd_line_.AppendSwitchASCII(ExitCodeWatcher::kParenthHandleSwitch,
-                              base::StringPrintf("%d", event.handle()));
-  EXPECT_FALSE(watcher.ParseArguments(cmd_line_));
+  EXPECT_FALSE(watcher.Initialize(event.Pass()));
 }
 
-TEST_F(BrowserWatcherTest, ExitCodeWatcherNoAccessHandleFailsInit) {
+TEST_F(ExitCodeWatcherTest, ExitCodeWatcherNoAccessHandleFailsInit) {
   ExitCodeWatcher watcher(kRegistryPath);
 
   // Open a SYNCHRONIZE-only handle to this process.
-  ASSERT_NO_FATAL_FAILURE(OpenSelfWithAccess(SYNCHRONIZE));
+  base::Process self = OpenSelfWithAccess(SYNCHRONIZE);
+  ASSERT_TRUE(self.IsValid());
 
   // A process handle with insufficient access should fail.
-  cmd_line_.AppendSwitchASCII(ExitCodeWatcher::kParenthHandleSwitch,
-                              base::StringPrintf("%d", process_));
-  EXPECT_FALSE(watcher.ParseArguments(cmd_line_));
+  EXPECT_FALSE(watcher.Initialize(self.Pass()));
 }
 
-TEST_F(BrowserWatcherTest, ExitCodeWatcherSucceedsInit) {
+TEST_F(ExitCodeWatcherTest, ExitCodeWatcherSucceedsInit) {
   ExitCodeWatcher watcher(kRegistryPath);
 
   // Open a handle to this process with sufficient access for the watcher.
-  ASSERT_NO_FATAL_FAILURE(
-      OpenSelfWithAccess(SYNCHRONIZE | PROCESS_QUERY_INFORMATION));
+  base::Process self =
+      OpenSelfWithAccess(SYNCHRONIZE | PROCESS_QUERY_INFORMATION);
+  ASSERT_TRUE(self.IsValid());
 
   // A process handle with sufficient access should succeed init.
-  cmd_line_.AppendSwitchASCII(ExitCodeWatcher::kParenthHandleSwitch,
-                              base::StringPrintf("%d", process_));
-  EXPECT_TRUE(watcher.ParseArguments(cmd_line_));
-
-  ASSERT_EQ(process_, watcher.process().Handle());
-
-  // The watcher takes ownership of the handle, make sure it's not
-  // double-closed.
-  process_ = base::kNullProcessHandle;
+  EXPECT_TRUE(watcher.Initialize(self.Pass()));
 }
 
-TEST_F(BrowserWatcherTest, ExitCodeWatcherOnExitedProcess) {
+TEST_F(ExitCodeWatcherTest, ExitCodeWatcherOnExitedProcess) {
   ScopedSleeperProcess sleeper;
   ASSERT_NO_FATAL_FAILURE(sleeper.Launch());
 
-  // Create a new handle to the sleeper process. This handle will leak in
-  // the case this test fails. A ScopedHandle cannot be used here, as the
-  // ownership would momentarily be held by two of them, which is disallowed.
-  base::ProcessHandle sleeper_handle = base::kNullProcessHandle;
-  sleeper.GetNewHandle(&sleeper_handle);
-
   ExitCodeWatcher watcher(kRegistryPath);
 
-  cmd_line_.AppendSwitchASCII(ExitCodeWatcher::kParenthHandleSwitch,
-                              base::StringPrintf("%d", sleeper_handle));
-  EXPECT_TRUE(watcher.ParseArguments(cmd_line_));
-  ASSERT_EQ(sleeper_handle, watcher.process().Handle());
+  EXPECT_TRUE(watcher.Initialize(sleeper.process().Duplicate()));
 
   // Verify that the watcher wrote a sentinel for the process.
   VerifyWroteExitCode(sleeper.process().pid(), STILL_ACTIVE);
diff --git a/components/browser_watcher/watcher_client_win.cc b/components/browser_watcher/watcher_client_win.cc
index ee654e7..e055278 100644
--- a/components/browser_watcher/watcher_client_win.cc
+++ b/components/browser_watcher/watcher_client_win.cc
@@ -6,11 +6,10 @@
 
 #include <windows.h>
 
+#include "base/command_line.h"
 #include "base/logging.h"
 #include "base/process/launch.h"
-#include "base/strings/stringprintf.h"
 #include "base/win/windows_version.h"
-#include "components/browser_watcher/exit_code_watcher_win.h"
 
 namespace browser_watcher {
 
@@ -23,21 +22,22 @@ base::Process OpenOwnProcessInheritable() {
                     base::GetCurrentProcId()));
 }
 
-void AddHandleArgument(base::ProcessHandle handle,
-                       base::CommandLine* cmd_line) {
-  cmd_line->AppendSwitchASCII(ExitCodeWatcher::kParenthHandleSwitch,
-                              base::StringPrintf("%d", handle));
-}
-
 }  // namespace
 
-WatcherClient::WatcherClient(const base::CommandLine& base_command_line) :
-    use_legacy_launch_(base::win::GetVersion() < base::win::VERSION_VISTA),
-    base_command_line_(base_command_line) {
+WatcherClient::WatcherClient(const CommandLineGenerator& command_line_generator)
+    : use_legacy_launch_(base::win::GetVersion() < base::win::VERSION_VISTA),
+      command_line_generator_(command_line_generator),
+      process_(base::kNullProcessHandle) {
 }
 
-void WatcherClient::LaunchWatcherProcess(const base::CommandLine& cmd_line,
-                                         base::Process self) {
+void WatcherClient::LaunchWatcher() {
+  DCHECK(!process_.IsValid());
+
+  // Build the command line for the watcher process.
+  base::Process self = OpenOwnProcessInheritable();
+  DCHECK(self.IsValid());
+  base::CommandLine cmd_line(command_line_generator_.Run(self.Handle()));
+
   base::HandlesToInheritVector to_inherit;
   base::LaunchOptions options;
   options.start_hidden = true;
@@ -45,7 +45,7 @@ void WatcherClient::LaunchWatcherProcess(const base::CommandLine& cmd_line,
     // Launch the child process inheriting all handles on XP.
     options.inherit_handles = true;
   } else {
-    // Launch the child process inheriting only |handle| on
+    // Launch the child process inheriting only |self| on
     // Vista and better.
     to_inherit.push_back(self.Handle());
     options.handles_to_inherit = &to_inherit;
@@ -56,17 +56,4 @@ void WatcherClient::LaunchWatcherProcess(const base::CommandLine& cmd_line,
     LOG(ERROR) << "Failed to launch browser watcher.";
 }
 
-void WatcherClient::LaunchWatcher() {
-  DCHECK(!process_.IsValid());
-
-  // Build the command line for the watcher process.
-  base::Process self = OpenOwnProcessInheritable();
-  DCHECK(self.IsValid());
-  base::CommandLine cmd_line(base_command_line_);
-  AddHandleArgument(self.Handle(), &cmd_line);
-
-  // Launch it.
-  LaunchWatcherProcess(cmd_line, self.Pass());
-}
-
 }  // namespace browser_watcher
diff --git a/components/browser_watcher/watcher_client_win.h b/components/browser_watcher/watcher_client_win.h
index 11caed8..5ae3c96 100644
--- a/components/browser_watcher/watcher_client_win.h
+++ b/components/browser_watcher/watcher_client_win.h
@@ -5,22 +5,31 @@
 #ifndef COMPONENTS_BROWSER_WATCHER_WATCHER_CLIENT_WIN_H_
 #define COMPONENTS_BROWSER_WATCHER_WATCHER_CLIENT_WIN_H_
 
-#include "base/command_line.h"
+#include "base/callback.h"
 #include "base/macros.h"
 #include "base/process/process.h"
 
+namespace base {
+class CommandLine;
+}  // namespace base
+
 namespace browser_watcher {
 
 // An interface class to take care of the details in launching a browser
 // watch process.
 class WatcherClient {
  public:
-  // Constructs a watcher client with a |base_command_line|, which should be
-  // a command line sufficient to invoke the watcher process. This command
-  // line will be augmented with the value of the inherited handle.
-  explicit WatcherClient(const base::CommandLine& base_command_line);
+  // A CommandLineGenerator generates command lines that will launch a separate
+  // process and pass the supplied HANDLE to ExitCodeWatcher in that process.
+  typedef base::Callback<base::CommandLine(HANDLE)> CommandLineGenerator;
+
+  // Constructs a watcher client that launches its watcher process using the
+  // command line generated by |command_line_generator|.
+  explicit WatcherClient(const CommandLineGenerator& command_line_generator);
 
-  // Launches the watcher process.
+  // Launches the watcher process such that the child process is able to inherit
+  // a handle to the current process. If use_legacy_launch() is true, this uses
+  // a non-threadsafe legacy launch mode that's compatible with Windows XP.
   void LaunchWatcher();
 
   // Accessors, exposed only for testing.
@@ -31,20 +40,13 @@ class WatcherClient {
   base::ProcessHandle process() const { return process_.Handle(); }
 
  private:
-  // Launches |cmd_line| such that the child process is able to inherit
-  // |handle|. If |use_legacy_launch_| is true, this uses a non-threadsafe
-  // legacy launch mode that's compatible with Windows XP.
-  // Returns a handle to the child process.
-  void LaunchWatcherProcess(const base::CommandLine& cmd_line,
-                            base::Process self);
-
   // If true, the watcher process will be launched with XP legacy handle
   // inheritance. This is not thread safe and can leak random handles into the
   // child process, but it's the best we can do on XP.
   bool use_legacy_launch_;
 
-  // The base command line passed to the constructor.
-  base::CommandLine base_command_line_;
+  // The CommandLineGenerator passed to the constructor.
+  CommandLineGenerator command_line_generator_;
 
   // A handle to the launched watcher process. Valid after a successful
   // LaunchWatcher() call.
diff --git a/components/browser_watcher/watcher_client_win_unittest.cc b/components/browser_watcher/watcher_client_win_unittest.cc
index ec6efbf..bfce358 100644
--- a/components/browser_watcher/watcher_client_win_unittest.cc
+++ b/components/browser_watcher/watcher_client_win_unittest.cc
@@ -7,6 +7,7 @@
 #include <string>
 
 #include "base/base_switches.h"
+#include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/process/process_handle.h"
@@ -26,7 +27,7 @@ namespace browser_watcher {
 namespace {
 
 // Command line switches used to communiate to the child test.
-const char kParentPid[] = "parent-pid";
+const char kParentHandle[] = "parent-handle";
 const char kLeakHandle[] = "leak-handle";
 const char kNoLeakHandle[] = "no-leak-handle";
 
@@ -39,12 +40,6 @@ bool IsValidParentProcessHandle(base::CommandLine& cmd_line,
   if (!base::StringToUint(str_handle, &int_handle))
     return false;
 
-  int parent_pid = 0;
-  if (!base::StringToInt(cmd_line.GetSwitchValueASCII(kParentPid),
-                         &parent_pid)) {
-    return false;
-  }
-
   base::ProcessHandle handle =
       reinterpret_cast<base::ProcessHandle>(int_handle);
   // Verify that we can get the associated process id.
@@ -68,8 +63,7 @@ MULTIPROCESS_TEST_MAIN(VerifyParentHandle) {
   base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess();
 
   // Make sure we got a valid parent process handle from the watcher client.
-  if (!IsValidParentProcessHandle(*cmd_line,
-                                  ExitCodeWatcher::kParenthHandleSwitch)) {
+  if (!IsValidParentProcessHandle(*cmd_line, kParentHandle)) {
     LOG(ERROR) << "Invalid or missing parent-handle.";
     return 1;
   }
@@ -94,7 +88,7 @@ MULTIPROCESS_TEST_MAIN(VerifyParentHandle) {
   return 0;
 }
 
-class BrowserWatcherClientTest : public base::MultiProcessTest {
+class WatcherClientTest : public base::MultiProcessTest {
  public:
   virtual void SetUp() {
     // Open an inheritable handle on our own process to test handle leakage.
@@ -111,22 +105,21 @@ class BrowserWatcherClientTest : public base::MultiProcessTest {
   };
 
   // Get a base command line to launch back into this test fixture.
-  base::CommandLine GetBaseCommandLine(HandlePolicy handle_policy) {
+  base::CommandLine GetBaseCommandLine(HandlePolicy handle_policy,
+                                       HANDLE parent_handle) {
     base::CommandLine ret = base::GetMultiProcessTestChildBaseCommandLine();
 
     ret.AppendSwitchASCII(switches::kTestChildProcess, "VerifyParentHandle");
-    ret.AppendSwitchASCII(kParentPid,
-                          base::StringPrintf("%d", base::GetCurrentProcId()));
+    ret.AppendSwitchASCII(kParentHandle,
+                          base::StringPrintf("%d", parent_handle));
 
     switch (handle_policy) {
       case LEAK_HANDLE:
-        ret.AppendSwitchASCII(kLeakHandle,
-                              base::StringPrintf("%d", self_.Get()));
+        ret.AppendSwitchASCII(kLeakHandle, base::StringPrintf("%d", self_));
         break;
 
       case NO_LEAK_HANDLE:
-        ret.AppendSwitchASCII(kNoLeakHandle,
-                              base::StringPrintf("%d", self_.Get()));
+        ret.AppendSwitchASCII(kNoLeakHandle, base::StringPrintf("%d", self_));
         break;
 
       default:
@@ -136,6 +129,12 @@ class BrowserWatcherClientTest : public base::MultiProcessTest {
     return ret;
   }
 
+  WatcherClient::CommandLineGenerator GetBaseCommandLineGenerator(
+      HandlePolicy handle_policy) {
+    return base::Bind(&WatcherClientTest::GetBaseCommandLine,
+                      base::Unretained(this), handle_policy);
+  }
+
   void AssertSuccessfulExitCode(base::ProcessHandle handle) {
     ASSERT_NE(base::kNullProcessHandle, handle);
 
@@ -166,12 +165,12 @@ class BrowserWatcherClientTest : public base::MultiProcessTest {
 
 // TODO(siggi): More testing - test WatcherClient base implementation.
 
-TEST_F(BrowserWatcherClientTest, LaunchWatcherSucceeds) {
+TEST_F(WatcherClientTest, LaunchWatcherSucceeds) {
   // We can only use the non-legacy launch method on Windows Vista or better.
   if (base::win::GetVersion() < base::win::VERSION_VISTA)
     return;
 
-  WatcherClient client(GetBaseCommandLine(NO_LEAK_HANDLE));
+  WatcherClient client(GetBaseCommandLineGenerator(NO_LEAK_HANDLE));
   ASSERT_FALSE(client.use_legacy_launch());
 
   client.LaunchWatcher();
@@ -179,10 +178,10 @@ TEST_F(BrowserWatcherClientTest, LaunchWatcherSucceeds) {
   ASSERT_NO_FATAL_FAILURE(AssertSuccessfulExitCode(client.process()));
 }
 
-TEST_F(BrowserWatcherClientTest, LaunchWatcherLegacyModeSucceeds) {
+TEST_F(WatcherClientTest, LaunchWatcherLegacyModeSucceeds) {
   // Test the XP-compatible legacy launch mode. This is expected to leak
   // a handle to the child process.
-  WatcherClient client(GetBaseCommandLine(LEAK_HANDLE));
+  WatcherClient client(GetBaseCommandLineGenerator(LEAK_HANDLE));
 
   // Use the legacy launch mode.
   client.set_use_legacy_launch(true);
@@ -192,14 +191,14 @@ TEST_F(BrowserWatcherClientTest, LaunchWatcherLegacyModeSucceeds) {
   ASSERT_NO_FATAL_FAILURE(AssertSuccessfulExitCode(client.process()));
 }
 
-TEST_F(BrowserWatcherClientTest, LegacyModeDetectedOnXP) {
+TEST_F(WatcherClientTest, LegacyModeDetectedOnXP) {
   // This test only works on Windows XP.
   if (base::win::GetVersion() > base::win::VERSION_XP)
     return;
 
   // Test that the client detects the need to use legacy launch mode, and that
   // it works on Windows XP.
-  WatcherClient client(GetBaseCommandLine(LEAK_HANDLE));
+  WatcherClient client(GetBaseCommandLineGenerator(LEAK_HANDLE));
   ASSERT_TRUE(client.use_legacy_launch());
 
   client.LaunchWatcher();
diff --git a/components/browser_watcher/watcher_main_api_win.h b/components/browser_watcher/watcher_main_api_win.h
index 4313982..b0a829f 100644
--- a/components/browser_watcher/watcher_main_api_win.h
+++ b/components/browser_watcher/watcher_main_api_win.h
@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_BROWSER_WATCHER_WATCHER_MAIN_API_WIN_H_
 #define COMPONENTS_BROWSER_WATCHER_WATCHER_MAIN_API_WIN_H_
 
+#include <Windows.h>
 #include "base/files/file_path.h"
 #include "base/strings/string16.h"
 
@@ -16,9 +17,10 @@ extern const base::FilePath::CharType kWatcherDll[];
 extern const char kWatcherDLLEntrypoint[];
 
 // The type of the watcher DLL's main entry point.
-// The |registry_path| parameter is the path under HKCU where the exit
-// codes will be written.
-typedef int (*WatcherMainFunction)(const base::char16* registry_path);
+// Watches |parent_process| and records its exit code under |registry_path| in
+// HKCU. Takes ownership of |parent_process|.
+typedef int (*WatcherMainFunction)(const base::char16* registry_path,
+                                   HANDLE parent_process);
 
 }  // namespace browser_watcher