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}

7 files changed, 86 insertions, 11 deletions

diff --git a/mojo/bindings/js/BUILD.gn b/mojo/bindings/js/BUILD.gn
index 9b3550f..73f008c 100644
--- a/mojo/bindings/js/BUILD.gn
+++ b/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",
diff --git a/mojo/bindings/js/handle.cc b/mojo/bindings/js/handle.cc
index e1d0fb0..a25a911 100644
--- a/mojo/bindings/js/handle.cc
+++ b/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,
diff --git a/mojo/bindings/js/handle.h b/mojo/bindings/js/handle.h
index 35202b0..e0f00dd 100644
--- a/mojo/bindings/js/handle.h
+++ b/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
diff --git a/mojo/bindings/js/handle_close_observer.h b/mojo/bindings/js/handle_close_observer.h
new file mode 100644
index 0000000..854603e
--- /dev/null
+++ b/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_
diff --git a/mojo/bindings/js/support.cc b/mojo/bindings/js/support.cc
index dc97865..1c82f17 100644
--- a/mojo/bindings/js/support.cc
+++ b/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)
diff --git a/mojo/bindings/js/waiting_callback.cc b/mojo/bindings/js/waiting_callback.cc
index 692551b..a88f956 100644
--- a/mojo/bindings/js/waiting_callback.cc
+++ b/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
diff --git a/mojo/bindings/js/waiting_callback.h b/mojo/bindings/js/waiting_callback.h
index 973a500..1871d93 100644
--- a/mojo/bindings/js/waiting_callback.h
+++ b/mojo/bindings/js/waiting_callback.h
@@ -8,13 +8,16 @@
 #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;
 
@@ -22,7 +25,7 @@ class WaitingCallback : public gin::Wrappable<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);
 };