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

chrome/chrome_watcher/chrome_watcher_main.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 <windows.h>
 
 #include "base/at_exit.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
@@ skipping 23 matching lines @@
         { 0x80, 0xc1, 0x52, 0x7f, 0xea, 0x23, 0xe3, 0xa7 } };
 
 // Takes care of monitoring a browser. This class watches for a browser's exit
 // code, as well as listening for WM_ENDSESSION messages. Events are recorded
 // in an exit funnel, for reporting the next time Chrome runs.
 class BrowserMonitor {
  public:
   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();
 
   // True if BrowserExited has run.
   bool browser_exited_;
 
   // The funnel used to record events for this browser.
   browser_watcher::ExitFunnel exit_funnel_;
 
@@ skipping 17 matching lines @@
     end_session_watcher_window_(
         base::Bind(&BrowserMonitor::OnEndSession, base::Unretained(this))),
     background_thread_("BrowserWatcherThread"),
     run_loop_(run_loop),
     main_thread_(base::MessageLoopProxy::current()) {
 }
 
 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;
 
   if (!exit_funnel_.Init(registry_path,
                         exit_code_watcher_.process().Handle())) {
     return false;
   }
 
   if (!background_thread_.StartWithOptions(
         base::Thread::Options(base::MessageLoop::TYPE_IO, 0))) {
     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;
   }
 
   return true;
 }
 
 void BrowserMonitor::OnEndSession(LPARAM lparam) {
   DCHECK_EQ(main_thread_, base::MessageLoopProxy::current());
 
   exit_funnel_.RecordEvent(L"WatcherLogoff");
   if (lparam & ENDSESSION_CLOSEAPP)
     exit_funnel_.RecordEvent(L"ES_CloseApp");
   if (lparam & ENDSESSION_CRITICAL)
     exit_funnel_.RecordEvent(L"ES_Critical");
   if (lparam & ENDSESSION_LOGOFF)
     exit_funnel_.RecordEvent(L"ES_Logoff");
   const LPARAM kKnownBits =
       ENDSESSION_CLOSEAPP | ENDSESSION_CRITICAL | ENDSESSION_LOGOFF;
   if (lparam & ~kKnownBits)
     exit_funnel_.RecordEvent(L"ES_Other");
 
   // Belt-and-suspenders; make sure our message loop exits ASAP.
   if (browser_exited_)
     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");
 
   main_thread_->PostTask(FROM_HERE,
       base::Bind(&BrowserMonitor::BrowserExited, base::Unretained(this)));
 }
 
 void BrowserMonitor::BrowserExited() {
   // This runs in the main thread.
   DCHECK_EQ(main_thread_, base::MessageLoopProxy::current());
@@ skipping 16 matching lines @@
                                   run_loop_->QuitClosure(),
                                   base::TimeDelta::FromSeconds(30));
   }
 }
 
 }  // namespace
 
 // 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;
   // Initialize the commandline singleton from the environment.
   base::CommandLine::Init(0, nullptr);
 
   logging::LogEventProvider::Initialize(kChromeWatcherTraceProviderName);
 
   // Arrange to be shut down as late as possible, as we want to outlive
   // chrome.exe in order to report its exit status.
   ::SetProcessShutdownParameters(0x100, SHUTDOWN_NORETRY);
 
   // Run a UI message loop on the main thread.
   base::MessageLoop msg_loop(base::MessageLoop::TYPE_UI);
   msg_loop.set_thread_name("WatcherMainThread");
 
   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();
 
   // Wind logging down.
   logging::LogEventProvider::Uninitialize();
 
   return 0;
 }
 
 static_assert(
     base::is_same<decltype(&WatcherMain), ChromeWatcherMainFunction>::value,
     "WatcherMain() has wrong type");