Mojo: Cancel WaitingCallbacks when their HandleWrappers are closed.

Currently, if a JS connector is left to be garbage collected, the handle
and the WaitingCallback both become ready to be collected at the same
time. If the handle is collected first, this results in an asynchronous
wait on a closed handle. With this change, WaitingCallback registers
itself as an observer to be notified when the handle it's watching is
closing and cancels the wait if the handle closes while the wait is in
progress.

BUG=406487

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

Cr-Commit-Position: refs/heads/master@{#294775}

Author: sammc

mojo/apps/js/test/BUILD.gn

@@ -17,6 +17,7 @@ test("mojo_apps_js_unittests") {
   ]
 
   sources = [
+    "handle_unittest.cc",
     "js_to_cpp_unittest.cc",
     "run_apps_js_tests.cc",
   ]

mojo/apps/js/test/handle_unittest.cc

@@ -0,0 +1,89 @@
+// Copyright 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 "mojo/bindings/js/handle.h"
+#include "mojo/bindings/js/handle_close_observer.h"
+#include "mojo/public/cpp/system/core.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace mojo {
+namespace js {
+
+class HandleWrapperTest : public testing::Test,
+                          public gin::HandleCloseObserver {
+ public:
+  HandleWrapperTest() : closes_observed_(0) {}
+
+  virtual void OnWillCloseHandle() OVERRIDE { closes_observed_++; }
+
+ protected:
+  int closes_observed_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(HandleWrapperTest);
+};
+
+class TestHandleWrapper : public gin::HandleWrapper {
+ public:
+  explicit TestHandleWrapper(MojoHandle handle) : HandleWrapper(handle) {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestHandleWrapper);
+};
+
+// Test that calling Close() on a HandleWrapper for an invalid handle does not
+// notify observers.
+TEST_F(HandleWrapperTest, CloseWithInvalidHandle) {
+  {
+    TestHandleWrapper wrapper(MOJO_HANDLE_INVALID);
+    wrapper.AddCloseObserver(this);
+    ASSERT_EQ(0, closes_observed_);
+    wrapper.Close();
+    EXPECT_EQ(0, closes_observed_);
+  }
+  EXPECT_EQ(0, closes_observed_);
+}
+
+// Test that destroying a HandleWrapper for an invalid handle does not notify
+// observers.
+TEST_F(HandleWrapperTest, DestroyWithInvalidHandle) {
+  {
+    TestHandleWrapper wrapper(MOJO_HANDLE_INVALID);
+    wrapper.AddCloseObserver(this);
+    ASSERT_EQ(0, closes_observed_);
+  }
+  EXPECT_EQ(0, closes_observed_);
+}
+
+// Test that calling Close on a HandleWrapper for a valid handle notifies
+// observers once.
+TEST_F(HandleWrapperTest, CloseWithValidHandle) {
+  {
+    mojo::MessagePipe pipe;
+    TestHandleWrapper wrapper(pipe.handle0.release().value());
+    wrapper.AddCloseObserver(this);
+    ASSERT_EQ(0, closes_observed_);
+    wrapper.Close();
+    EXPECT_EQ(1, closes_observed_);
+    // Check that calling close again doesn't notify observers.
+    wrapper.Close();
+    EXPECT_EQ(1, closes_observed_);
+  }
+  // Check that destroying a closed HandleWrapper doesn't notify observers.
+  EXPECT_EQ(1, closes_observed_);
+}
+
+// Test that destroying a HandleWrapper for a valid handle notifies observers.
+TEST_F(HandleWrapperTest, DestroyWithValidHandle) {
+  {
+    mojo::MessagePipe pipe;
+    TestHandleWrapper wrapper(pipe.handle0.release().value());
+    wrapper.AddCloseObserver(this);
+    ASSERT_EQ(0, closes_observed_);
+  }
+  EXPECT_EQ(1, closes_observed_);
+}
+
+}  // namespace js
+}  // namespace mojo

mojo/bindings/js/BUILD.gn

@@ -9,6 +9,7 @@ source_set("js") {
     "core.h",
     "handle.cc",
     "handle.h",
+    "handle_close_observer.h",
     "support.cc",
     "support.h",
     "waiting_callback.cc",

mojo/bindings/js/handle.cc

@@ -4,6 +4,8 @@
 
 #include "mojo/bindings/js/handle.h"
 
+#include "mojo/bindings/js/handle_close_observer.h"
+
 namespace gin {
 
 gin::WrapperInfo HandleWrapper::kWrapperInfo = { gin::kEmbedderNativeGin };
@@ -13,6 +15,27 @@ HandleWrapper::HandleWrapper(MojoHandle handle)
 }
 
 HandleWrapper::~HandleWrapper() {
+  NotifyCloseObservers();
+}
+
+void HandleWrapper::Close() {
+  NotifyCloseObservers();
+  handle_.reset();
+}
+
+void HandleWrapper::AddCloseObserver(HandleCloseObserver* observer) {
+  close_observers_.AddObserver(observer);
+}
+
+void HandleWrapper::RemoveCloseObserver(HandleCloseObserver* observer) {
+  close_observers_.RemoveObserver(observer);
+}
+
+void HandleWrapper::NotifyCloseObservers() {
+  if (!handle_.is_valid())
+    return;
+
+  FOR_EACH_OBSERVER(HandleCloseObserver, close_observers_, OnWillCloseHandle());
 }
 
 v8::Handle<v8::Value> Converter<mojo::Handle>::ToV8(v8::Isolate* isolate,

mojo/bindings/js/handle.h

@@ -5,12 +5,14 @@
 #ifndef MOJO_BINDINGS_JS_HANDLE_H_
 #define MOJO_BINDINGS_JS_HANDLE_H_
 
+#include "base/observer_list.h"
 #include "gin/converter.h"
 #include "gin/handle.h"
 #include "gin/wrappable.h"
 #include "mojo/public/cpp/system/core.h"
 
 namespace gin {
+class HandleCloseObserver;
 
 // Wrapper for mojo Handles exposed to JavaScript. This ensures the Handle
 // is Closed when its JS object is garbage collected.
@@ -25,12 +27,18 @@ class HandleWrapper : public gin::Wrappable<HandleWrapper> {
 
   mojo::Handle get() const { return handle_.get(); }
   mojo::Handle release() { return handle_.release(); }
-  void Close() { handle_.reset(); }
+  void Close();
+
+  void AddCloseObserver(HandleCloseObserver* observer);
+  void RemoveCloseObserver(HandleCloseObserver* observer);
 
  protected:
   HandleWrapper(MojoHandle handle);
   virtual ~HandleWrapper();
+  void NotifyCloseObservers();
+
   mojo::ScopedHandle handle_;
+  ObserverList<HandleCloseObserver> close_observers_;
 };
 
 // Note: It's important to use this converter rather than the one for

mojo/bindings/js/handle_close_observer.h

@@ -0,0 +1,20 @@
+// Copyright 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.
+
+#ifndef MOJO_BINDINGS_JS_HANDLE_CLOSE_OBSERVER_H_
+#define MOJO_BINDINGS_JS_HANDLE_CLOSE_OBSERVER_H_
+
+namespace gin {
+
+class HandleCloseObserver {
+ public:
+  virtual void OnWillCloseHandle() = 0;
+
+ protected:
+  virtual ~HandleCloseObserver() {}
+};
+
+}  // namespace gin
+
+#endif  // MOJO_BINDINGS_JS_HANDLE_CLOSE_OBSERVER_H_

mojo/bindings/js/support.cc

@@ -21,7 +21,8 @@ namespace js {
 
 namespace {
 
-WaitingCallback* AsyncWait(const gin::Arguments& args, mojo::Handle handle,
+WaitingCallback* AsyncWait(const gin::Arguments& args,
+                           gin::Handle<gin::HandleWrapper> handle,
                            MojoHandleSignals signals,
                            v8::Handle<v8::Function> callback) {
   return WaitingCallback::Create(args.isolate(), callback, handle, signals)

mojo/bindings/js/waiting_callback.cc

@@ -24,12 +24,12 @@ gin::WrapperInfo WaitingCallback::kWrapperInfo = { gin::kEmbedderNativeGin };
 gin::Handle<WaitingCallback> WaitingCallback::Create(
     v8::Isolate* isolate,
     v8::Handle<v8::Function> callback,
-    mojo::Handle handle,
+    gin::Handle<gin::HandleWrapper> handle_wrapper,
     MojoHandleSignals signals) {
-  gin::Handle<WaitingCallback> waiting_callback =
-      gin::CreateHandle(isolate, new WaitingCallback(isolate, callback));
+  gin::Handle<WaitingCallback> waiting_callback = gin::CreateHandle(
+      isolate, new WaitingCallback(isolate, callback, handle_wrapper));
   waiting_callback->wait_id_ = Environment::GetDefaultAsyncWaiter()->AsyncWait(
-      handle.value(),
+      handle_wrapper->get().value(),
       signals,
       MOJO_DEADLINE_INDEFINITE,
       &WaitingCallback::CallOnHandleReady,
@@ -41,13 +41,17 @@ void WaitingCallback::Cancel() {
   if (!wait_id_)
     return;
 
+  handle_wrapper_->RemoveCloseObserver(this);
+  handle_wrapper_ = NULL;
   Environment::GetDefaultAsyncWaiter()->CancelWait(wait_id_);
   wait_id_ = 0;
 }
 
 WaitingCallback::WaitingCallback(v8::Isolate* isolate,
-                                 v8::Handle<v8::Function> callback)
-      : wait_id_() {
+                                 v8::Handle<v8::Function> callback,
+                                 gin::Handle<gin::HandleWrapper> handle_wrapper)
+    : wait_id_(0), handle_wrapper_(handle_wrapper.get()) {
+  handle_wrapper_->AddCloseObserver(this);
   v8::Handle<v8::Context> context = isolate->GetCurrentContext();
   runner_ = gin::PerContextData::From(context)->runner()->GetWeakPtr();
   GetWrapper(isolate)->SetHiddenValue(GetHiddenPropertyName(isolate), callback);
@@ -64,6 +68,8 @@ void WaitingCallback::CallOnHandleReady(void* closure, MojoResult result) {
 
 void WaitingCallback::OnHandleReady(MojoResult result) {
   wait_id_ = 0;
+  handle_wrapper_->RemoveCloseObserver(this);
+  handle_wrapper_ = NULL;
 
   if (!runner_)
     return;
@@ -80,5 +86,10 @@ void WaitingCallback::OnHandleReady(MojoResult result) {
   runner_->Call(callback, runner_->global(), 1, args);
 }
 
+void WaitingCallback::OnWillCloseHandle() {
+  Environment::GetDefaultAsyncWaiter()->CancelWait(wait_id_);
+  OnHandleReady(MOJO_RESULT_INVALID_ARGUMENT);
+}
+
 }  // namespace js
 }  // namespace mojo

mojo/bindings/js/waiting_callback.h

@@ -8,21 +8,24 @@
 #include "gin/handle.h"
 #include "gin/runner.h"
 #include "gin/wrappable.h"
+#include "mojo/bindings/js/handle.h"
+#include "mojo/bindings/js/handle_close_observer.h"
 #include "mojo/public/c/environment/async_waiter.h"
 #include "mojo/public/cpp/system/core.h"
 
 namespace mojo {
 namespace js {
 
-class WaitingCallback : public gin::Wrappable<WaitingCallback> {
+class WaitingCallback : public gin::Wrappable<WaitingCallback>,
+                        public gin::HandleCloseObserver {
  public:
   static gin::WrapperInfo kWrapperInfo;
 
   // Creates a new WaitingCallback.
   static gin::Handle<WaitingCallback> Create(
       v8::Isolate* isolate,
       v8::Handle<v8::Function> callback,
-      mojo::Handle handle,
+      gin::Handle<gin::HandleWrapper> handle_wrapper,
       MojoHandleSignals signals);
 
   // Cancels the callback. Does nothing if a callback is not pending. This is
@@ -31,7 +34,9 @@ class WaitingCallback : public gin::Wrappable<WaitingCallback> {
   void Cancel();
 
  private:
-  WaitingCallback(v8::Isolate* isolate, v8::Handle<v8::Function> callback);
+  WaitingCallback(v8::Isolate* isolate,
+                  v8::Handle<v8::Function> callback,
+                  gin::Handle<gin::HandleWrapper> handle_wrapper);
   virtual ~WaitingCallback();
 
   // Callback from MojoAsyncWaiter. |closure| is the WaitingCallback.
@@ -40,9 +45,15 @@ class WaitingCallback : public gin::Wrappable<WaitingCallback> {
   // Invoked from CallOnHandleReady() (CallOnHandleReady() must be static).
   void OnHandleReady(MojoResult result);
 
+  // Invoked by the HandleWrapper if the handle is closed while this wait is
+  // still in progress.
+  virtual void OnWillCloseHandle() OVERRIDE;
+
   base::WeakPtr<gin::Runner> runner_;
   MojoAsyncWaitID wait_id_;
 
+  gin::HandleWrapper* handle_wrapper_;
+
   DISALLOW_COPY_AND_ASSIGN(WaitingCallback);
 };
 

mojo/mojo_apps.gypi

@@ -73,6 +73,7 @@
         'mojo_js_lib',
       ],
       'sources': [
+        'apps/js/test/handle_unittest.cc',
         'apps/js/test/js_to_cpp_unittest.cc',
         'apps/js/test/run_apps_js_tests.cc',
       ],

mojo/mojo_base.gyp

@@ -506,6 +506,7 @@
         'bindings/js/core.h',
         'bindings/js/handle.cc',
         'bindings/js/handle.h',
+        'bindings/js/handle_close_observer.h',
         'bindings/js/support.cc',
         'bindings/js/support.h',
         'bindings/js/waiting_callback.cc',