Makes HandleWatcher block until no longer waiting on pipe

mojo/common/handle_watcher.cc

Index: mojo/common/handle_watcher.cc
diff --git a/mojo/common/handle_watcher.cc b/mojo/common/handle_watcher.cc
index 84e8b64fb9ad7a93fd0fb5ca1b211178a5f3bb6b..c8c4ac236194c13c711bda62087a7639b7a7c788 100644
--- a/mojo/common/handle_watcher.cc
+++ b/mojo/common/handle_watcher.cc
@@ -13,6 +13,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_loop_proxy.h"
+#include "base/synchronization/condition_variable.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread.h"
 #include "base/time/time.h"
@@ -57,6 +58,20 @@ struct WatchData {
   scoped_refptr<base::MessageLoopProxy> message_loop;
 };
 
+// WaitState -------------------------------------------------------------------
+
+// Used when removing a handle. The main thread blocks until the background
+// thread successfully removes the handle from the set of handles being watched.
+// When the background thread removes the handle it sets |removed| and signals
+// |condition|.
+struct WaitState {

[darin (slow to review), 2014/07/22 17:42:56]

nit: You could also just use base::WaitableEvent here as that is essentially
just a lock+cvar+bool. It would simplify this CL.

[sky, 2014/07/22 18:20:50]

I thought there was something like that. Done!

+  WaitState() : condition(&lock), removed(false) {}
+
+  base::Lock lock;
+  base::ConditionVariable condition;
+  bool removed;
+};
+
 // WatcherBackend --------------------------------------------------------------
 
 // WatcherBackend is responsible for managing the requests and interacting with
@@ -68,7 +83,10 @@ class WatcherBackend : public MessagePumpMojoHandler {
   virtual ~WatcherBackend();
 
   void StartWatching(const WatchData& data);
-  void StopWatching(WatcherID watcher_id);
+
+  // Cancels a previously schedule request to start a watch. When done sets
+  // |wait_state->removed| and signals the condition.
+  void StopWatching(WatcherID watcher_id, WaitState* wait_state);
 
  private:
   typedef std::map<Handle, WatchData> HandleToWatchDataMap;
@@ -107,15 +125,19 @@ void WatcherBackend::StartWatching(const WatchData& data) {
                                 data.deadline);
 }
 
-void WatcherBackend::StopWatching(WatcherID watcher_id) {
+void WatcherBackend::StopWatching(WatcherID watcher_id, WaitState* wait_state) {
   // Because of the thread hop it is entirely possible to get here and not
   // have a valid handle registered for |watcher_id|.
   Handle handle;
-  if (!GetMojoHandleByWatcherID(watcher_id, &handle))
-    return;
-
-  handle_to_data_.erase(handle);
-  message_pump_mojo->RemoveHandler(handle);
+  if (GetMojoHandleByWatcherID(watcher_id, &handle)) {
+    handle_to_data_.erase(handle);
+    message_pump_mojo->RemoveHandler(handle);
+  }
+  {
+    base::AutoLock auto_lock(wait_state->lock);
+    wait_state->removed = true;
+    wait_state->condition.Signal();
+  }
 }
 
 void WatcherBackend::RemoveAndNotify(const Handle& handle,
@@ -208,7 +230,7 @@ WatcherID WatcherThreadManager::StartWatching(
   data.message_loop = base::MessageLoopProxy::current();
   DCHECK_NE(static_cast<base::MessageLoopProxy*>(NULL),
             data.message_loop.get());
-  // We outlive |thread_|, so it's safe to use Unretained() here.
+  // We own |thread_|, so it's safe to use Unretained() here.
   thread_.message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&WatcherBackend::StartWatching,
@@ -218,12 +240,21 @@ WatcherID WatcherThreadManager::StartWatching(
 }
 
 void WatcherThreadManager::StopWatching(WatcherID watcher_id) {
-  // We outlive |thread_|, so it's safe to use Unretained() here.
+  // We own |thread_|, so it's safe to use Unretained() here.
+  WaitState wait_state;
   thread_.message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&WatcherBackend::StopWatching,
                  base::Unretained(&backend_),
-                 watcher_id));
+                 watcher_id,
+                 &wait_state));
+
+  // We need to block until the handle is actually removed.
+  {
+    base::AutoLock auto_lock(wait_state.lock);
+    while (!wait_state.removed)
+      wait_state.condition.Wait();
+  }
 }
 
 WatcherThreadManager::WatcherThreadManager()