From c1e5c788c583e685210dcae48b025c95bf71ca56 Mon Sep 17 00:00:00 2001 From: "darin@chromium.org" Date: Tue, 12 Nov 2013 05:10:07 +0000 Subject: Mojo: Add BindingsSupportImpl on top of HandleWatcher (take 2) Plumb MojoResult to the callback set on HandleWatcher. Revise BindingsSupport interface to make the implementation on top of HandleWatcher trivial. Originally reviewed at https://codereview.chromium.org/62773003/. Includes fix to mojo.gyp to make libmojo_shell link against mojo_common_lib on Android. R=sky@chromium.org Review URL: https://codereview.chromium.org/69633002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@234408 0039d316-1c4b-4281-b951-d872f2087c98 --- mojo/common/bindings_support_impl.cc | 65 ++++++++++++++++++++++++++++ mojo/common/bindings_support_impl.h | 39 +++++++++++++++++ mojo/common/handle_watcher.cc | 26 ++++++----- mojo/common/handle_watcher.h | 4 +- mojo/common/handle_watcher_unittest.cc | 33 ++++++++++++-- mojo/mojo.gyp | 5 +++ mojo/public/bindings/lib/bindings_support.h | 28 +++++++----- mojo/public/bindings/lib/connector.cc | 41 +++++++----------- mojo/public/bindings/lib/connector.h | 7 ++- mojo/public/tests/simple_bindings_support.cc | 47 +++++++++++--------- mojo/public/tests/simple_bindings_support.h | 13 +++--- mojo/shell/context.cc | 2 + mojo/shell/context.h | 2 + 13 files changed, 231 insertions(+), 81 deletions(-) create mode 100644 mojo/common/bindings_support_impl.cc create mode 100644 mojo/common/bindings_support_impl.h diff --git a/mojo/common/bindings_support_impl.cc b/mojo/common/bindings_support_impl.cc new file mode 100644 index 0000000..9cec141 --- /dev/null +++ b/mojo/common/bindings_support_impl.cc @@ -0,0 +1,65 @@ +// Copyright 2013 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/common/bindings_support_impl.h" + +#include "base/atomic_ref_count.h" +#include "base/bind.h" +#include "mojo/common/handle_watcher.h" + +namespace mojo { +namespace common { + +// Context is used to track the number of HandleWatcher objects in use by a +// particular BindingsSupportImpl instance. +class BindingsSupportImpl::Context + : public base::RefCountedThreadSafe { + public: + void CallOnHandleReady(HandleWatcher* watcher, + AsyncWaitCallback* callback, + MojoResult result) { + delete watcher; + callback->OnHandleReady(result); + } + + private: + friend class base::RefCountedThreadSafe; + virtual ~Context() {} +}; + +BindingsSupportImpl::BindingsSupportImpl() + : context_(new Context()) { +} + +BindingsSupportImpl::~BindingsSupportImpl() { + // All HandleWatcher instances created through this interface should have + // been destroyed. + DCHECK(context_->HasOneRef()); +} + +BindingsSupport::AsyncWaitID BindingsSupportImpl::AsyncWait( + Handle handle, + MojoWaitFlags flags, + AsyncWaitCallback* callback) { + // This instance will be deleted when done or cancelled. + HandleWatcher* watcher = new HandleWatcher(); + + // TODO(darin): Standardize on mojo::Handle instead of MojoHandle? + watcher->Start(handle.value, + flags, + MOJO_DEADLINE_INDEFINITE, + base::Bind(&Context::CallOnHandleReady, + context_, + watcher, + callback)); + return watcher; +} + +void BindingsSupportImpl::CancelWait(AsyncWaitID async_wait_id) { + HandleWatcher* watcher = static_cast(async_wait_id); + delete watcher; +} + +} // namespace common +} // namespace mojo diff --git a/mojo/common/bindings_support_impl.h b/mojo/common/bindings_support_impl.h new file mode 100644 index 0000000..1178644 --- /dev/null +++ b/mojo/common/bindings_support_impl.h @@ -0,0 +1,39 @@ +// Copyright 2013 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_COMMON_BINDINGS_SUPPORT_IMPL_H_ +#define MOJO_COMMON_BINDINGS_SUPPORT_IMPL_H_ + +#include "mojo/public/bindings/lib/bindings_support.h" + +#include "base/basictypes.h" +#include "base/compiler_specific.h" +#include "base/memory/ref_counted.h" +#include "mojo/common/mojo_common_export.h" + +namespace mojo { +namespace common { + +class MOJO_COMMON_EXPORT BindingsSupportImpl + : NON_EXPORTED_BASE(public BindingsSupport) { + public: + BindingsSupportImpl(); + virtual ~BindingsSupportImpl(); + + // BindingsSupport methods: + virtual AsyncWaitID AsyncWait(Handle handle, MojoWaitFlags flags, + AsyncWaitCallback* callback) OVERRIDE; + virtual void CancelWait(AsyncWaitID async_wait_id) OVERRIDE; + + private: + class Context; + scoped_refptr context_; + + DISALLOW_COPY_AND_ASSIGN(BindingsSupportImpl); +}; + +} // namespace common +} // namespace mojo + +#endif // MOJO_COMMON_BINDINGS_SUPPORT_IMPL_H_ diff --git a/mojo/common/handle_watcher.cc b/mojo/common/handle_watcher.cc index 1fef74a..a159185 100644 --- a/mojo/common/handle_watcher.cc +++ b/mojo/common/handle_watcher.cc @@ -47,7 +47,7 @@ class WatcherThreadManager { WatcherID StartWatching(MojoHandle handle, MojoWaitFlags wait_flags, base::TimeTicks deadline, - const base::Closure& callback); + const base::Callback& callback); // Stops watching a handle. void StopWatching(WatcherID watcher_id); @@ -65,7 +65,7 @@ class WatcherThreadManager { MojoHandle handle; MojoWaitFlags wait_flags; base::TimeTicks deadline; - base::Closure callback; + base::Callback callback; scoped_refptr message_loop; }; @@ -138,10 +138,11 @@ WatcherThreadManager* WatcherThreadManager::GetInstance() { return &instance.Get(); } -WatcherID WatcherThreadManager::StartWatching(MojoHandle handle, - MojoWaitFlags wait_flags, - base::TimeTicks deadline, - const base::Closure& callback) { +WatcherID WatcherThreadManager::StartWatching( + MojoHandle handle, + MojoWaitFlags wait_flags, + base::TimeTicks deadline, + const base::Callback& callback) { WatcherID id = 0; { static int next_id = 0; @@ -264,7 +265,8 @@ void WatcherThreadManager::RemoveAndNotify(WatcherID id, MojoResult result) { to_notify = i->second; id_to_callback_.erase(i); } - to_notify.message_loop->PostTask(FROM_HERE, to_notify.callback); + to_notify.message_loop->PostTask(FROM_HERE, + base::Bind(to_notify.callback, result)); } void WatcherThreadManager::RemoveHandle(MojoHandle handle) { @@ -328,7 +330,7 @@ struct HandleWatcher::StartState { WatcherID watcher_id; // Callback to notify when done. - base::Closure callback; + base::Callback callback; // When Start() is invoked a callback is passed to WatcherThreadManager // using a WeakRef from |weak_refactory_|. The callback invokes @@ -354,7 +356,7 @@ HandleWatcher::~HandleWatcher() { void HandleWatcher::Start(MojoHandle handle, MojoWaitFlags wait_flags, MojoDeadline deadline, - const base::Closure& callback) { + const base::Callback& callback) { DCHECK_NE(MOJO_HANDLE_INVALID, handle); DCHECK_NE(MOJO_WAIT_FLAG_NONE, wait_flags); @@ -379,10 +381,12 @@ void HandleWatcher::Stop() { WatcherThreadManager::GetInstance()->StopWatching(old_state->watcher_id); } -void HandleWatcher::OnHandleReady() { +void HandleWatcher::OnHandleReady(MojoResult result) { DCHECK(start_state_.get()); scoped_ptr old_state(start_state_.Pass()); - old_state->callback.Run(); + old_state->callback.Run(result); + + // NOTE: We may have been deleted during callback execution. } // static diff --git a/mojo/common/handle_watcher.h b/mojo/common/handle_watcher.h index 8e04aa7..a6a73ea 100644 --- a/mojo/common/handle_watcher.h +++ b/mojo/common/handle_watcher.h @@ -38,7 +38,7 @@ class MOJO_COMMON_EXPORT HandleWatcher { void Start(MojoHandle handle, MojoWaitFlags wait_flags, MojoDeadline deadline, - const base::Closure& callback); + const base::Callback& callback); // Stops listening. Does nothing if not in the process of listening. void Stop(); @@ -54,7 +54,7 @@ class MOJO_COMMON_EXPORT HandleWatcher { struct StartState; // See description of |StartState::weak_factory| for details. - void OnHandleReady(); + void OnHandleReady(MojoResult result); // If non-NULL Start() has been invoked. scoped_ptr start_state_; diff --git a/mojo/common/handle_watcher_unittest.cc b/mojo/common/handle_watcher_unittest.cc index ae1a175..a10585a 100644 --- a/mojo/common/handle_watcher_unittest.cc +++ b/mojo/common/handle_watcher_unittest.cc @@ -32,6 +32,14 @@ void RunUntilIdle() { run_loop.RunUntilIdle(); } +void DeleteWatcherAndForwardResult( + HandleWatcher* watcher, + base::Callback next_callback, + MojoResult result) { + delete watcher; + next_callback.Run(result); +} + // Helper class to manage the callback and running the message loop waiting for // message to be received. Typical usage is something like: // Schedule callback returned from GetCallback(). @@ -59,17 +67,22 @@ class CallbackHelper { run_loop.Run(); } - base::Closure GetCallback() { + base::Callback GetCallback() { return base::Bind(&CallbackHelper::OnCallback, weak_factory_.GetWeakPtr()); } void Start(HandleWatcher* watcher, MojoHandle handle) { + StartWithCallback(watcher, handle, GetCallback()); + } + + void StartWithCallback(HandleWatcher* watcher, MojoHandle handle, + const base::Callback& callback) { watcher->Start(handle, MOJO_WAIT_FLAG_READABLE, MOJO_DEADLINE_INDEFINITE, - GetCallback()); + callback); } private: - void OnCallback() { + void OnCallback(MojoResult result) { got_callback_ = true; if (run_loop_) run_loop_->Quit(); @@ -275,6 +288,20 @@ TEST_F(HandleWatcherTest, Deadline) { EXPECT_FALSE(callback_helper3.got_callback()); } +TEST_F(HandleWatcherTest, DeleteInCallback) { + ScopedMessagePipe test_pipe; + CallbackHelper callback_helper; + + HandleWatcher* watcher = new HandleWatcher(); + callback_helper.StartWithCallback(watcher, test_pipe.handle_1(), + base::Bind(&DeleteWatcherAndForwardResult, + watcher, + callback_helper.GetCallback())); + WriteToHandle(test_pipe.handle_0()); + callback_helper.RunUntilGotCallback(); + EXPECT_TRUE(callback_helper.got_callback()); +} + } // namespace test } // namespace common } // namespace mojo diff --git a/mojo/mojo.gyp b/mojo/mojo.gyp index a01aa88..3e71fe2 100644 --- a/mojo/mojo.gyp +++ b/mojo/mojo.gyp @@ -69,6 +69,8 @@ 'mojo_system', ], 'sources': [ + 'common/bindings_support_impl.cc', + 'common/bindings_support_impl.h', 'common/handle_watcher.cc', 'common/handle_watcher.h', 'common/scoped_message_pipe.cc', @@ -217,6 +219,7 @@ '../base/base.gyp:base', '../net/net.gyp:net', '../url/url.gyp:url_lib', + 'mojo_bindings', 'mojo_system', 'mojo_utility', 'native_viewport', @@ -257,6 +260,7 @@ '../base/base.gyp:base', '../ui/gl/gl.gyp:gl', '../url/url.gyp:url_lib', + 'mojo_common_lib', 'mojo_shell_lib', 'mojo_system', ], @@ -478,6 +482,7 @@ '../base/third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations', '../ui/gfx/gfx.gyp:gfx', '../ui/gl/gl.gyp:gl', + 'mojo_common_lib', 'mojo_jni_headers', 'mojo_shell_lib', ], diff --git a/mojo/public/bindings/lib/bindings_support.h b/mojo/public/bindings/lib/bindings_support.h index 15aa65b..a1979f6 100644 --- a/mojo/public/bindings/lib/bindings_support.h +++ b/mojo/public/bindings/lib/bindings_support.h @@ -15,22 +15,30 @@ class BindingsSupport { public: class AsyncWaitCallback { public: + virtual ~AsyncWaitCallback() {} virtual void OnHandleReady(MojoResult result) = 0; }; - // Asynchronously call MojoWait on a background thread, and return the result - // to the current thread via the given AsyncWaitCallback. - virtual bool AsyncWait(Handle handle, - MojoWaitFlags flags, - MojoDeadline deadline, - AsyncWaitCallback* callback) = 0; - - // Cancel an existing call to AsyncWait with the given callback. The - // callback's OnHandleReady method should not be called in this case. - virtual void CancelWait(AsyncWaitCallback* callback) = 0; + typedef void* AsyncWaitID; static void Set(BindingsSupport* support); static BindingsSupport* Get(); + + // Asynchronously call MojoWait on a background thread, and pass the result + // of MojoWait to the given AsyncWaitCallback on the current thread. Returns + // an AsyncWaitID that can be used with CancelWait to stop waiting. This + // identifier becomes invalid once the callback runs. + virtual AsyncWaitID AsyncWait(Handle handle, + MojoWaitFlags flags, + AsyncWaitCallback* callback) = 0; + + // Cancel an existing call to AsyncWait with the given AsyncWaitID. The + // corresponding AsyncWaitCallback's OnHandleReady method will not be called + // in this case. + virtual void CancelWait(AsyncWaitID id) = 0; + + protected: + virtual ~BindingsSupport() {} }; } // namespace mojo diff --git a/mojo/public/bindings/lib/connector.cc b/mojo/public/bindings/lib/connector.cc index f72bca3..3185536 100644 --- a/mojo/public/bindings/lib/connector.cc +++ b/mojo/public/bindings/lib/connector.cc @@ -20,10 +20,6 @@ Connector::Connector(Handle message_pipe) } Connector::~Connector() { - if (read_callback_.IsPending()) - read_callback_.Cancel(); - if (write_callback_.IsPending()) - write_callback_.Cancel(); } void Connector::SetIncomingReceiver(MessageReceiver* receiver) { @@ -58,24 +54,18 @@ void Connector::OnHandleReady(Callback* callback, MojoResult result) { void Connector::WaitToReadMore() { read_callback_.SetOwnerToNotify(this); - - bool ok = BindingsSupport::Get()->AsyncWait(message_pipe_, - MOJO_WAIT_FLAG_READABLE, - MOJO_DEADLINE_INDEFINITE, - &read_callback_); - if (!ok) - error_ = true; + read_callback_.SetAsyncWaitID( + BindingsSupport::Get()->AsyncWait(message_pipe_, + MOJO_WAIT_FLAG_READABLE, + &read_callback_)); } void Connector::WaitToWriteMore() { write_callback_.SetOwnerToNotify(this); - - bool ok = BindingsSupport::Get()->AsyncWait(message_pipe_, - MOJO_WAIT_FLAG_WRITABLE, - MOJO_DEADLINE_INDEFINITE, - &write_callback_); - if (!ok) - error_ = true; + write_callback_.SetAsyncWaitID( + BindingsSupport::Get()->AsyncWait(message_pipe_, + MOJO_WAIT_FLAG_WRITABLE, + &write_callback_)); } void Connector::ReadMore() { @@ -140,7 +130,7 @@ void Connector::WriteOne(Message* message, bool* wait_to_write) { MojoResult rv = WriteMessage(message_pipe_, message->data, message->data->header.num_bytes, - message->handles.data(), + &message->handles[0], static_cast(message->handles.size()), MOJO_WRITE_MESSAGE_FLAG_NONE); if (rv == MOJO_RESULT_OK) { @@ -155,12 +145,13 @@ void Connector::WriteOne(Message* message, bool* wait_to_write) { // ---------------------------------------------------------------------------- Connector::Callback::Callback() - : owner_(NULL) { + : owner_(NULL), + async_wait_id_(0) { } -void Connector::Callback::Cancel() { - owner_ = NULL; - BindingsSupport::Get()->CancelWait(this); +Connector::Callback::~Callback() { + if (owner_) + BindingsSupport::Get()->CancelWait(async_wait_id_); } void Connector::Callback::SetOwnerToNotify(Connector* owner) { @@ -168,8 +159,8 @@ void Connector::Callback::SetOwnerToNotify(Connector* owner) { owner_ = owner; } -bool Connector::Callback::IsPending() const { - return owner_ != NULL; +void Connector::Callback::SetAsyncWaitID(BindingsSupport::AsyncWaitID id) { + async_wait_id_ = id; } void Connector::Callback::OnHandleReady(MojoResult result) { diff --git a/mojo/public/bindings/lib/connector.h b/mojo/public/bindings/lib/connector.h index 808ab54..4953e82 100644 --- a/mojo/public/bindings/lib/connector.h +++ b/mojo/public/bindings/lib/connector.h @@ -42,15 +42,16 @@ class Connector : public MessageReceiver { class Callback : public BindingsSupport::AsyncWaitCallback { public: Callback(); + virtual ~Callback(); - void Cancel(); void SetOwnerToNotify(Connector* owner); - bool IsPending() const; + void SetAsyncWaitID(BindingsSupport::AsyncWaitID async_wait_id); virtual void OnHandleReady(MojoResult result) MOJO_OVERRIDE; private: Connector* owner_; + BindingsSupport::AsyncWaitID async_wait_id_; }; friend class Callback; @@ -69,6 +70,8 @@ class Connector : public MessageReceiver { Callback write_callback_; bool error_; + + MOJO_DISALLOW_COPY_AND_ASSIGN(Connector); }; } // namespace mojo diff --git a/mojo/public/tests/simple_bindings_support.cc b/mojo/public/tests/simple_bindings_support.cc index 979474d..a59a66e 100644 --- a/mojo/public/tests/simple_bindings_support.cc +++ b/mojo/public/tests/simple_bindings_support.cc @@ -15,47 +15,52 @@ SimpleBindingsSupport::SimpleBindingsSupport() { SimpleBindingsSupport::~SimpleBindingsSupport() { BindingsSupport::Set(NULL); + + for (WaiterList::iterator it = waiters_.begin(); it != waiters_.end(); ++it) + delete *it; } -bool SimpleBindingsSupport::AsyncWait(Handle handle, - MojoWaitFlags flags, - MojoDeadline deadline, - AsyncWaitCallback* callback) { - Waiter waiter; - waiter.handle = handle; - waiter.flags = flags; - waiter.deadline = deadline; - waiter.callback = callback; +BindingsSupport::AsyncWaitID SimpleBindingsSupport::AsyncWait( + Handle handle, + MojoWaitFlags flags, + AsyncWaitCallback* callback) { + Waiter* waiter = new Waiter(); + waiter->handle = handle; + waiter->flags = flags; + waiter->callback = callback; waiters_.push_back(waiter); - return true; + return waiter; } -void SimpleBindingsSupport::CancelWait(AsyncWaitCallback* callback) { - std::list::iterator it = waiters_.begin(); +void SimpleBindingsSupport::CancelWait(AsyncWaitID async_wait_id) { + Waiter* waiter = static_cast(async_wait_id); + + WaiterList::iterator it = waiters_.begin(); while (it != waiters_.end()) { - if (it->callback == callback) { - std::list::iterator doomed = it++; + if (*it == waiter) { + WaiterList::iterator doomed = it++; waiters_.erase(doomed); } else { ++it; } } + + delete waiter; } void SimpleBindingsSupport::Process() { typedef std::pair Result; std::list results; - // TODO(darin): Honor given deadline. - - std::list::iterator it = waiters_.begin(); + WaiterList::iterator it = waiters_.begin(); while (it != waiters_.end()) { - const Waiter& waiter = *it; + Waiter* waiter = *it; MojoResult result; - if (IsReady(waiter.handle, waiter.flags, &result)) { - results.push_back(std::make_pair(waiter.callback, result)); - std::list::iterator doomed = it++; + if (IsReady(waiter->handle, waiter->flags, &result)) { + results.push_back(std::make_pair(waiter->callback, result)); + WaiterList::iterator doomed = it++; waiters_.erase(doomed); + delete waiter; } else { ++it; } diff --git a/mojo/public/tests/simple_bindings_support.h b/mojo/public/tests/simple_bindings_support.h index c22de82..59131ba 100644 --- a/mojo/public/tests/simple_bindings_support.h +++ b/mojo/public/tests/simple_bindings_support.h @@ -17,11 +17,10 @@ class SimpleBindingsSupport : public BindingsSupport { SimpleBindingsSupport(); virtual ~SimpleBindingsSupport(); - virtual bool AsyncWait(Handle handle, - MojoWaitFlags flags, - MojoDeadline deadline, - AsyncWaitCallback* callback) MOJO_OVERRIDE; - virtual void CancelWait(AsyncWaitCallback* callback) MOJO_OVERRIDE; + virtual AsyncWaitID AsyncWait(Handle handle, + MojoWaitFlags flags, + AsyncWaitCallback* callback) MOJO_OVERRIDE; + virtual void CancelWait(AsyncWaitID async_wait_id) MOJO_OVERRIDE; // This method is called by unit tests to check the status of any handles // that we are asynchronously waiting on and to dispatch callbacks for any @@ -34,11 +33,11 @@ class SimpleBindingsSupport : public BindingsSupport { struct Waiter { Handle handle; MojoWaitFlags flags; - MojoDeadline deadline; AsyncWaitCallback* callback; }; - std::list waiters_; + typedef std::list WaiterList; + WaiterList waiters_; }; } // namespace test diff --git a/mojo/shell/context.cc b/mojo/shell/context.cc index c2e2f90..6a4be90 100644 --- a/mojo/shell/context.cc +++ b/mojo/shell/context.cc @@ -18,9 +18,11 @@ Context::Context() scoped_ptr(new NetworkDelegate()), storage_.profile_path()) { system::CoreImpl::Init(); + BindingsSupport::Set(&bindings_support_impl_); } Context::~Context() { + BindingsSupport::Set(NULL); } } // namespace shell diff --git a/mojo/shell/context.h b/mojo/shell/context.h index f2dec12..16c04b4 100644 --- a/mojo/shell/context.h +++ b/mojo/shell/context.h @@ -5,6 +5,7 @@ #ifndef MOJO_SHELL_CONTEXT_H_ #define MOJO_SHELL_CONTEXT_H_ +#include "mojo/common/bindings_support_impl.h" #include "mojo/shell/loader.h" #include "mojo/shell/storage.h" #include "mojo/shell/task_runners.h" @@ -34,6 +35,7 @@ class Context { TaskRunners task_runners_; Storage storage_; Loader loader_; + common::BindingsSupportImpl bindings_support_impl_; #if defined(OS_ANDROID) base::android::ScopedJavaGlobalRef activity_; -- cgit v1.1