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

chrome/chrome_watcher/chrome_watcher_main.cc

Index: chrome/chrome_watcher/chrome_watcher_main.cc
diff --git a/chrome/chrome_watcher/chrome_watcher_main.cc b/chrome/chrome_watcher/chrome_watcher_main.cc
index 4bd8410b4bb18eb551f06a9d19bc857761aa31b9..d5a6b467f802a4c4d3f4c020a822380d8d4fc0fa 100644
--- a/chrome/chrome_watcher/chrome_watcher_main.cc
+++ b/chrome/chrome_watcher/chrome_watcher_main.cc
@@ -41,16 +41,20 @@ class BrowserMonitor {
   BrowserMonitor(base::RunLoop* run_loop, const base::char16* registry_path);
   ~BrowserMonitor();
 
-  // Starts the monitor, returns true on success.
+  // Starts the monitor, returns true on success. |on_initialized_event| will be
+  // signaled from the background thread immediately before blocking on the exit

[Sigurður Ásgeirsson, 2015/01/29 16:59:57]

nit: the background thread is implementation details - maybe phrase this more
generically?

[erikwright (departed), 2015/01/30 20:39:57]

I have made an attempt. Note that the code as it stands makes no attempt at
encapsulation. The client is required to understand exactly what StartWatching
does, that it does so asynchronously, that it Quits |run_loop| at certain
points, etc.

So it's a bit difficult to extend that behaviour with some semblance of
encapsulation.

That being said, I considered extracting this class and found that it was
trivial enough, mainly consisting of configuring a bunch of disparate and
unrelated components, that the refactoring was without merit.

+  // of |process|.
   bool StartWatching(const base::char16* registry_path,
-                     base::Process process);
+                     base::Process process,
+                     base::win::ScopedHandle on_initialized_event);
 
  private:
   // Called from EndSessionWatcherWindow on a WM_ENDSESSION message.
   void OnEndSession(LPARAM lparam);
 
-  // Blocking function that runs on |background_thread_|.
-  void Watch();
+  // Blocking function that runs on |background_thread_|. Signals
+  // |on_initialized_event| before waiting for the browser process to exit.
+  void Watch(base::win::ScopedHandle on_initialized_event);
 
   // Posted to main thread from Watch when browser exits.
   void BrowserExited();
@@ -88,8 +92,10 @@ BrowserMonitor::BrowserMonitor(base::RunLoop* run_loop,
 BrowserMonitor::~BrowserMonitor() {
 }
 
-bool BrowserMonitor::StartWatching(const base::char16* registry_path,
-                                   base::Process process) {
+bool BrowserMonitor::StartWatching(
+    const base::char16* registry_path,
+    base::Process process,
+    base::win::ScopedHandle on_initialized_event) {
   if (!exit_code_watcher_.Initialize(process.Pass()))
     return false;
 
@@ -103,8 +109,9 @@ bool BrowserMonitor::StartWatching(const base::char16* registry_path,
     return false;
   }
 
-  if (!background_thread_.task_runner()->PostTask(FROM_HERE,
-        base::Bind(&BrowserMonitor::Watch, base::Unretained(this)))) {
+  if (!background_thread_.task_runner()->PostTask(
+          FROM_HERE, base::Bind(&BrowserMonitor::Watch, base::Unretained(this),
+                                base::Passed(&on_initialized_event)))) {

[Sigurður Ásgeirsson, 2015/01/29 16:59:57]

ugh - this syntax irks me, but I guess that's just me ...

[erikwright (departed), 2015/01/30 20:39:57]

You mean base::Passed(&movable)? I replaced it with an alternative syntax that
might be less jarring.

[Sigurður Ásgeirsson, 2015/02/02 16:59:20]

It's probably just me - but base::Passed(&movable) looks to me like passing a
pointer to a local across threads (despite knowing that it doesn't).

[erikwright (departed), 2015/02/03 20:05:34]

Acknowledged.

     background_thread_.Stop();
     return false;
   }
@@ -132,10 +139,15 @@ void BrowserMonitor::OnEndSession(LPARAM lparam) {
     run_loop_->Quit();
 }
 
-void BrowserMonitor::Watch() {
+void BrowserMonitor::Watch(base::win::ScopedHandle on_initialized_event) {
   // This needs to run on an IO thread.
   DCHECK_NE(main_thread_, base::MessageLoopProxy::current());
 
+  // Signal our client now that RPC is initialized and we have cleared all of

[Sigurður Ásgeirsson, 2015/01/29 16:59:57]

RPC?

[erikwright (departed), 2015/01/30 20:39:57]

Done.

+  // the obstacles that might lead to an early exit.
+  ::SetEvent(on_initialized_event.Get());
+  on_initialized_event.Close();
+
   exit_code_watcher_.WaitForExit();
   exit_funnel_.RecordEvent(L"BrowserExit");
 
@@ -172,8 +184,10 @@ void BrowserMonitor::BrowserExited() {
 // The main entry point to the watcher, declared as extern "C" to avoid name
 // mangling.
 extern "C" int WatcherMain(const base::char16* registry_path,
-                           HANDLE process_handle) {
+                           HANDLE process_handle,
+                           HANDLE on_initialized_event_handle) {
   base::Process process(process_handle);
+  base::win::ScopedHandle on_initialized_event(on_initialized_event_handle);
 
   // The exit manager is in charge of calling the dtors of singletons.
   base::AtExitManager exit_manager;
@@ -192,8 +206,10 @@ extern "C" int WatcherMain(const base::char16* registry_path,
 
   base::RunLoop run_loop;
   BrowserMonitor monitor(&run_loop, registry_path);
-  if (!monitor.StartWatching(registry_path, process.Pass()))
+  if (!monitor.StartWatching(registry_path, process.Pass(),
+                             on_initialized_event.Pass())) {
     return 1;
+  }
 
   run_loop.Run();