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

chrome/app/chrome_watcher_client_win.cc

+// Copyright 2015 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_client_win.h"
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "components/browser_watcher/watcher_client_win.h"
+
+ChromeWatcherClient::ChromeWatcherClient(
+    const CommandLineGenerator& command_line_generator)
+    : command_line_generator_(command_line_generator) {
+}
+
+ChromeWatcherClient::~ChromeWatcherClient() {
+}
+
+bool ChromeWatcherClient::LaunchWatcher() {
+  // Create an inheritable event that the child process will signal when it has
+  // completed initialization.
+  SECURITY_ATTRIBUTES on_initialized_event_attributes = {
+      sizeof(SECURITY_ATTRIBUTES),  // nLength
+      nullptr,                      // lpSecurityDescriptor
+      TRUE                          // bInheritHandle
+  };
+  on_initialized_event_.Set(::CreateEvent(&on_initialized_event_attributes,
+                                          TRUE,  // manual reset
+                                          FALSE, nullptr));
+  if (!on_initialized_event_.IsValid()) {
+    PLOG(ERROR) << "Failed to create an event.";

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

should any of these be debug-only? are they needed in release builds?

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

Good question. I don't know. I googled for a policy on this and couldn't find
anything.

Certainly at least some of them could be useful in diagnosing failure in the
field using Sawbuck.

This one is exceptionally unlikely, so on the one hand it might as well be DPLOG
and on the other hand it's irrelevant since it will presumably never be reached.

WDYT?

[grt (UTC plus 2), 2015/02/04 03:04:47]

Every LOG adds weight. If it will never be reached, it shouldn't be there.
Here's the "never use LOG" thread:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/nAiKeh-zLws/6wljl....
I still consider the installer a special case for the reasons I stated in the
thread. Outside of the installer, I think it's hard to make a case for LOG. On
the one hand, I love logging. I really, really love logging. On the other hand,
if every developer thought that their special snowflake required extensive
logging, Chrome would be huge and the logs would be impenetrable, so we might as
well not have logging and ask for repro cases rather than asking for logs.
You're welcome to disagree, but please scrutinize each LOG and decide whether or
not it'll ever happen, and whether or not you're ever likely to learn something
if it does happen. I'm also of the mind that we don't need verbose messages in
non-debug LOG statements since the emitted message will contain file+line, and
we have the source. Put the description in a comment rather than in a string
that gets built into chrome.dll. It's entirely possible that there is no
Chromium consensus around this, however, so take this as just like my opinion,
man.

[erikwright (departed), 2015/02/04 04:19:18]

Thanks for filling me in on the philosophy. It seems very sensible.

I kept one log message that I thought was key (that the watcher failed to
launch). All of the others I converted to debug-only.

+    return false;
+  }
+
+  // Configure the basic WatcherClient, binding in the initialization event
+  // HANDLE.
+  browser_watcher::WatcherClient watcher_client(
+      base::Bind(command_line_generator_, on_initialized_event_.Get()));
+  // Indicate that the event HANDLE should be inherited.
+  watcher_client.AddInheritedHandle(on_initialized_event_.Get());
+  // Launch the watcher.
+  watcher_client.LaunchWatcher();
+  // Grab a handle to the watcher so that we may later wait on its
+  // initialization.
+  process_ = watcher_client.process().Duplicate();
+  if (!process_.IsValid())
+    on_initialized_event_.Close();
+  return process_.IsValid();
+}
+
+bool ChromeWatcherClient::EnsureInitialized() {
+  if (!process_.IsValid()) {
+    LOG(ERROR) << "EnsureInitialized called after a failed LaunchWatcher "

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

is this documenting the API contract? why not just DCHECK(process_.IsValid());?

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

In fact, it seems reasonable to call this even if it was never launched (i.e. to
save the caller the need to maintain that state separately).

+                  "call, or without calling LaunchWatcher.";
+    return false;
+  }
+
+  DCHECK(on_initialized_event_.IsValid());
+
+  HANDLE handles[] = {on_initialized_event_.Get(), process_.Handle()};
+  DWORD result = ::WaitForMultipleObjects(arraysize(handles), handles,
+                                          FALSE, INFINITE);
+
+  switch (result) {
+    case WAIT_OBJECT_0:
+      return true;
+    case WAIT_OBJECT_0 + 1:
+      LOG(ERROR) << "Chrome watcher process failed to launch.";
+      return false;
+    case WAIT_FAILED:
+      PLOG(ERROR)
+          << "Failure while waiting on Chrome watcher process launch.";
+      return false;
+    default:
+      NOTREACHED() << "Unexpected result while waiting on Chrome watcher "
+                      "process launch: " << result;
+      return false;
+  }
+}
+
+bool ChromeWatcherClient::WaitForExit(int* exit_code) {
+  if (!process_.IsValid()) {
+    LOG(ERROR) << "WaitForExit called after a failed LaunchWatcher call, or "

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

same comment as above

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

Done.

+                  "without calling LaunchWatcher.";
+    return false;
+  }
+
+  return process_.WaitForExit(exit_code);
+}
+
+bool ChromeWatcherClient::WaitForExitWithTimeout(const base::TimeDelta& timeout,
+                                                 int* exit_code) {
+  if (!process_.IsValid()) {
+    LOG(ERROR) << "WaitForExitWithTimeout called after a failed LaunchWatcher "

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

and here

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

Done.

+                  "call, or without calling LaunchWatcher.";
+    return false;
+  }
+
+  return process_.WaitForExitWithTimeout(timeout, exit_code);
+}