Mojo: Add some CHECKs to debug watcher crashes

Also changes an incorrect DCHECK to be a safe runtime check in the event
of racy usage by callers.

BUG=740044
TBR=jcivelli@chromium.org

Change-Id: I8725648035eda70ac5bf9fb046073f21557727bc
Reviewed-on: https://chromium-review.googlesource.com/575521Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487327}

mojo/edk/system/core.cc

@@ -438,7 +438,8 @@ MojoResult Core::Watch(MojoHandle watcher_handle,
   scoped_refptr<Dispatcher> dispatcher = GetDispatcher(handle);
   if (!dispatcher)
     return MOJO_RESULT_INVALID_ARGUMENT;
-  return watcher->WatchDispatcher(dispatcher, signals, condition, context);
+  return watcher->WatchDispatcher(std::move(dispatcher), signals, condition,
+                                  context);
 }
 
 MojoResult Core::CancelWatch(MojoHandle watcher_handle, uintptr_t context) {

mojo/edk/system/watch.cc

@@ -25,6 +25,9 @@ bool Watch::NotifyState(const HandleSignalsState& state,
                         bool allowed_to_call_callback) {
   AssertWatcherLockAcquired();
 
+  // TODO(crbug.com/740044): Remove this CHECK.
+  CHECK(this);
+
   // NOTE: This method must NEVER call into |dispatcher_| directly, because it
   // may be called while |dispatcher_| holds a lock.
 
@@ -56,6 +59,9 @@ bool Watch::NotifyState(const HandleSignalsState& state,
 }
 
 void Watch::Cancel() {
+  // TODO(crbug.com/740044): Remove this CHECK.
+  CHECK(this);
+
   RequestContext::current()->AddWatchCancelFinalizer(this);
 }
 

mojo/edk/system/watcher_dispatcher.cc

@@ -25,7 +25,7 @@ void WatcherDispatcher::NotifyHandleState(Dispatcher* dispatcher,
   if (it == watched_handles_.end())
     return;
 
-  // Maybe fire a notification to the watch assoicated with this dispatcher,
+  // Maybe fire a notification to the watch associated with this dispatcher,
   // provided we're armed and it cares about the new state.
   if (it->second->NotifyState(state, armed_)) {
     ready_watches_.insert(it->second.get());
@@ -47,6 +47,9 @@ void WatcherDispatcher::NotifyHandleClosed(Dispatcher* dispatcher) {
 
     watch = std::move(it->second);
 
+    // TODO(crbug.com/740044): Remove this CHECK.
+    CHECK(watch);
+
     // Wipe out all state associated with the closed dispatcher.
     watches_.erase(watch->context());
     ready_watches_.erase(watch.get());
@@ -119,6 +122,10 @@ MojoResult WatcherDispatcher::WatchDispatcher(
   // after we've updated all our own relevant state and released |lock_|.
   {
     base::AutoLock lock(lock_);
+
+    // TODO(crbug.com/740044): Remove this CHECK.
+    CHECK(!closed_);
+
     if (watches_.count(context) || watched_handles_.count(dispatcher.get()))
       return MOJO_RESULT_ALREADY_EXISTS;
 
@@ -156,6 +163,11 @@ MojoResult WatcherDispatcher::CancelWatch(uintptr_t context) {
     watches_.erase(it);
   }
 
+  // TODO(crbug.com/740044): Remove these CHECKs.
+  CHECK(watch);
+  CHECK(watch->dispatcher());
+  CHECK(this);
+
   // Mark the watch as cancelled so no further notifications get through.
   watch->Cancel();
 
@@ -167,7 +179,12 @@ MojoResult WatcherDispatcher::CancelWatch(uintptr_t context) {
   {
     base::AutoLock lock(lock_);
     auto handle_it = watched_handles_.find(watch->dispatcher().get());
-    DCHECK(handle_it != watched_handles_.end());
+
+    // If another thread races to close this watcher handler, |watched_handles_|
+    // may have been cleared by the time we reach this section.
+    if (handle_it == watched_handles_.end())
+      return MOJO_RESULT_OK;
+
     ready_watches_.erase(handle_it->second.get());
     watched_handles_.erase(handle_it);
   }
@@ -229,7 +246,7 @@ MojoResult WatcherDispatcher::Arm(
 }
 
 WatcherDispatcher::~WatcherDispatcher() {
-  // TODO(crbug.com/74044): Remove this.
+  // TODO(crbug.com/740044): Remove this.
   CHECK(closed_);
 }