Two fixes for mojo JS bindings

Makes WaitingCallback cancel a wait if there is a wait pending and
it's deleted.
Makes MessagePumpMojo deal with MOJO_RESULT_CANCELLED.

BUG=371755
TEST=see bug
R=darin@chromium.org, mpcomplete@chromium.org

Review URL: https://codereview.chromium.org/276093002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@269663 0039d316-1c4b-4281-b951-d872f2087c98

mojo/bindings/js/support.cc

@@ -25,30 +25,11 @@ namespace {
 WaitingCallback* AsyncWait(const gin::Arguments& args, mojo::Handle handle,
                            MojoWaitFlags flags,
                            v8::Handle<v8::Function> callback) {
-  gin::Handle<WaitingCallback> waiting_callback =
-      WaitingCallback::Create(args.isolate(), callback);
-
-  MojoAsyncWaiter* waiter = GetDefaultAsyncWaiter();
-  MojoAsyncWaitID wait_id = waiter->AsyncWait(
-      waiter,
-      handle.value(),
-      flags,
-      MOJO_DEADLINE_INDEFINITE,
-      &WaitingCallback::CallOnHandleReady,
-      waiting_callback.get());
-
-  waiting_callback->set_wait_id(wait_id);
-
-  return waiting_callback.get();
+  return WaitingCallback::Create(args.isolate(), callback, handle, flags).get();
 }
 
 void CancelWait(WaitingCallback* waiting_callback) {
-  if (!waiting_callback->wait_id())
-    return;
-
-  MojoAsyncWaiter* waiter = GetDefaultAsyncWaiter();
-  waiter->CancelWait(waiter, waiting_callback->wait_id());
-  waiting_callback->set_wait_id(0);
+  waiting_callback->Cancel();
 }
 
 gin::WrapperInfo g_wrapper_info = { gin::kEmbedderNativeGin };

mojo/bindings/js/waiting_callback.cc

@@ -5,6 +5,7 @@
 #include "mojo/bindings/js/waiting_callback.h"
 
 #include "gin/per_context_data.h"
+#include "mojo/public/cpp/environment/default_async_waiter.h"
 
 namespace mojo {
 namespace js {
@@ -19,6 +20,34 @@ v8::Handle<v8::String> GetHiddenPropertyName(v8::Isolate* isolate) {
 
 gin::WrapperInfo WaitingCallback::kWrapperInfo = { gin::kEmbedderNativeGin };
 
+// static
+gin::Handle<WaitingCallback> WaitingCallback::Create(
+    v8::Isolate* isolate,
+    v8::Handle<v8::Function> callback,
+    mojo::Handle handle,
+    MojoWaitFlags flags) {
+  gin::Handle<WaitingCallback> waiting_callback =
+      gin::CreateHandle(isolate, new WaitingCallback(isolate, callback));
+  MojoAsyncWaiter* waiter = GetDefaultAsyncWaiter();
+  waiting_callback->wait_id_ = waiter->AsyncWait(
+      waiter,
+      handle.value(),
+      flags,
+      MOJO_DEADLINE_INDEFINITE,
+      &WaitingCallback::CallOnHandleReady,
+      waiting_callback.get());
+  return waiting_callback;
+}
+
+void WaitingCallback::Cancel() {
+  if (!wait_id_)
+    return;
+
+  MojoAsyncWaiter* waiter = GetDefaultAsyncWaiter();
+  waiter->CancelWait(waiter, wait_id_);
+  wait_id_ = 0;
+}
+
 WaitingCallback::WaitingCallback(v8::Isolate* isolate,
                                  v8::Handle<v8::Function> callback)
       : wait_id_() {
@@ -28,12 +57,7 @@ WaitingCallback::WaitingCallback(v8::Isolate* isolate,
 }
 
 WaitingCallback::~WaitingCallback() {
-  DCHECK(!wait_id_) << "Waiting callback was destroyed before being cancelled.";
-}
-
-gin::Handle<WaitingCallback> WaitingCallback::Create(
-    v8::Isolate* isolate, v8::Handle<v8::Function> callback) {
-  return gin::CreateHandle(isolate, new WaitingCallback(isolate, callback));
+  Cancel();
 }
 
 // static

mojo/bindings/js/waiting_callback.h

@@ -9,6 +9,7 @@
 #include "gin/runner.h"
 #include "gin/wrappable.h"
 #include "mojo/public/c/environment/async_waiter.h"
+#include "mojo/public/cpp/system/core.h"
 
 namespace mojo {
 namespace js {
@@ -17,24 +18,26 @@ class WaitingCallback : public gin::Wrappable<WaitingCallback> {
  public:
   static gin::WrapperInfo kWrapperInfo;
 
+  // Creates a new WaitingCallback.
   static gin::Handle<WaitingCallback> Create(
-      v8::Isolate* isolate, v8::Handle<v8::Function> callback);
+      v8::Isolate* isolate,
+      v8::Handle<v8::Function> callback,
+      mojo::Handle handle,
+      MojoWaitFlags flags);
 
-  MojoAsyncWaitID wait_id() const {
-    return wait_id_;
-  }
-
-  void set_wait_id(MojoAsyncWaitID wait_id) {
-    wait_id_ = wait_id;
-  }
-
-  // MojoAsyncWaitCallback
-  static void CallOnHandleReady(void* closure, MojoResult result);
+  // Cancels the callback. Does nothing if a callback is not pending. This is
+  // implicitly invoked from the destructor but can be explicitly invoked as
+  // necessary.
+  void Cancel();
 
  private:
   WaitingCallback(v8::Isolate* isolate, v8::Handle<v8::Function> callback);
   virtual ~WaitingCallback();
 
+  // Callback from MojoAsyncWaiter. |closure| is the WaitingCallback.
+  static void CallOnHandleReady(void* closure, MojoResult result);
+
+  // Invoked from CallOnHandleReady() (CallOnHandleReady() must be static).
   void OnHandleReady(MojoResult result);
 
   base::WeakPtr<gin::Runner> runner_;

mojo/common/message_pump_mojo.cc

@@ -7,6 +7,7 @@
 #include <algorithm>
 #include <vector>
 
+#include "base/debug/alias.h"
 #include "base/logging.h"
 #include "base/time/time.h"
 #include "mojo/common/message_pump_mojo_handler.h"
@@ -154,14 +155,17 @@ void MessagePumpMojo::DoInternalWork(const RunState& run_state, bool block) {
         wait_state.handles[index]);
   } else {
     switch (result) {
-      case MOJO_RESULT_INVALID_ARGUMENT:
+      case MOJO_RESULT_CANCELLED:
       case MOJO_RESULT_FAILED_PRECONDITION:
+      case MOJO_RESULT_INVALID_ARGUMENT:
         RemoveFirstInvalidHandle(wait_state);
         break;
       case MOJO_RESULT_DEADLINE_EXCEEDED:
         break;
       default:
-        NOTREACHED();
+        base::debug::Alias(&result);
+        // Unexpected result is likely fatal, crash so we can determine cause.
+        CHECK(false);
     }
   }
 
@@ -187,7 +191,8 @@ void MessagePumpMojo::RemoveFirstInvalidHandle(const WaitState& wait_state) {
     const MojoResult result =
         Wait(wait_state.handles[i], wait_state.wait_flags[i], 0);
     if (result == MOJO_RESULT_INVALID_ARGUMENT ||
-        result == MOJO_RESULT_FAILED_PRECONDITION) {
+        result == MOJO_RESULT_FAILED_PRECONDITION ||
+        result == MOJO_RESULT_CANCELLED) {
       // Remove the handle first, this way if OnHandleError() tries to remove
       // the handle our iterator isn't invalidated.
       DCHECK(handlers_.find(wait_state.handles[i]) != handlers_.end());