From ea41e2bcaff5770690f7d7033fbf1079c44a770d Mon Sep 17 00:00:00 2001 From: "sergeyu@chromium.org" Date: Fri, 29 Jul 2011 01:39:01 +0000 Subject: Fix crashes when shutting down DesktopEnvironment BUG=90602,90108 TEST=Host plugin doesn't crash. Review URL: http://codereview.chromium.org/7514031 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@94614 0039d316-1c4b-4281-b951-d872f2087c98 --- remoting/host/chromoting_host.cc | 4 +- remoting/host/chromoting_host.h | 11 +-- remoting/host/chromoting_host_unittest.cc | 8 ++- remoting/host/desktop_environment.cc | 105 ++++++++++++++++++++++------- remoting/host/desktop_environment.h | 20 +++++- remoting/host/plugin/host_script_object.cc | 37 ++++++---- remoting/host/plugin/host_script_object.h | 5 +- remoting/host/simple_host_process.cc | 16 ++--- 8 files changed, 146 insertions(+), 60 deletions(-) (limited to 'remoting/host') diff --git a/remoting/host/chromoting_host.cc b/remoting/host/chromoting_host.cc index 0ae98a7..a1e0e51 100644 --- a/remoting/host/chromoting_host.cc +++ b/remoting/host/chromoting_host.cc @@ -53,8 +53,8 @@ ChromotingHost::ChromotingHost(ChromotingHostContext* context, Logger* logger, bool allow_nat_traversal) : context_(context), - config_(config), desktop_environment_(environment), + config_(config), access_verifier_(access_verifier), logger_(logger), allow_nat_traversal_(allow_nat_traversal), @@ -62,7 +62,7 @@ ChromotingHost::ChromotingHost(ChromotingHostContext* context, protocol_config_(protocol::CandidateSessionConfig::CreateDefault()), is_curtained_(false), is_it2me_(false) { - DCHECK(desktop_environment_.get()); + DCHECK(desktop_environment_); desktop_environment_->set_host(this); logger_->SetThread(MessageLoop::current()); } diff --git a/remoting/host/chromoting_host.h b/remoting/host/chromoting_host.h index 2938b6c..5b45d43 100644 --- a/remoting/host/chromoting_host.h +++ b/remoting/host/chromoting_host.h @@ -71,8 +71,9 @@ class ChromotingHost : public base::RefCountedThreadSafe, public: // Factory methods that must be used to create ChromotingHost // instances. Returned instance takes ownership of - // |access_verifier| and |environment|. It does NOT take ownership - // of |context| and |logger|. + // |access_verifier|. It does NOT take ownership of |context|, + // |environment| and |logger|, but they should not be deleted until + // returned host is destroyed. static ChromotingHost* Create(ChromotingHostContext* context, MutableHostConfig* config, DesktopEnvironment* environment, @@ -161,8 +162,8 @@ class ChromotingHost : public base::RefCountedThreadSafe, kStopped, }; - // Takes ownership of |access_verifier| and |environment|, and adds a - // reference to |config|. Does NOT take ownership of |context|. + // Takes ownership of |access_verifier|, and adds a reference to + // |config|. Caller keeps ownership of |context| and |environment|. ChromotingHost(ChromotingHostContext* context, MutableHostConfig* config, DesktopEnvironment* environment, @@ -193,8 +194,8 @@ class ChromotingHost : public base::RefCountedThreadSafe, // Parameters specified when the host was created. ChromotingHostContext* context_; + DesktopEnvironment* desktop_environment_; scoped_refptr config_; - scoped_ptr desktop_environment_; scoped_ptr access_verifier_; Logger* logger_; bool allow_nat_traversal_; diff --git a/remoting/host/chromoting_host_unittest.cc b/remoting/host/chromoting_host_unittest.cc index 453e4cc..47bbe0c 100644 --- a/remoting/host/chromoting_host_unittest.cc +++ b/remoting/host/chromoting_host_unittest.cc @@ -100,13 +100,14 @@ class ChromotingHostTest : public testing::Test { disconnect_window_ = new MockDisconnectWindow(); continue_window_ = new MockContinueWindow(); local_input_monitor_ = new MockLocalInputMonitor(); - DesktopEnvironment* desktop = + desktop_environment_.reset( new DesktopEnvironment(&context_, capturer, event_executor_, curtain_, disconnect_window_, continue_window_, - local_input_monitor_); + local_input_monitor_)); MockAccessVerifier* access_verifier = new MockAccessVerifier(); - host_ = ChromotingHost::Create(&context_, config_, desktop, + host_ = ChromotingHost::Create(&context_, config_, + desktop_environment_.get(), access_verifier, logger_.get(), false); credentials_.set_type(protocol::PASSWORD); credentials_.set_username("user"); @@ -211,6 +212,7 @@ class ChromotingHostTest : public testing::Test { scoped_ptr logger_; MessageLoop message_loop_; MockConnectionToClientEventHandler handler_; + scoped_ptr desktop_environment_; scoped_refptr host_; scoped_refptr config_; MockChromotingHostContext context_; diff --git a/remoting/host/desktop_environment.cc b/remoting/host/desktop_environment.cc index 6082acf..e13ac2f 100644 --- a/remoting/host/desktop_environment.cc +++ b/remoting/host/desktop_environment.cc @@ -4,6 +4,8 @@ #include "remoting/host/desktop_environment.h" +#include "base/bind.h" +#include "base/compiler_specific.h" #include "remoting/host/capturer.h" #include "remoting/host/chromoting_host.h" #include "remoting/host/chromoting_host_context.h" @@ -17,6 +19,43 @@ static const int kContinueWindowTimeoutSecs = 10 * 60; namespace remoting { +// UIThreadProxy proxies DesktopEnvironment method calls to the UI +// thread. This is neccessary so that DesktopEnvironment can be +// deleted synchronously even while there are pending tasks on the +// message queue. +class UIThreadProxy : public base::RefCountedThreadSafe { + public: + UIThreadProxy(ChromotingHostContext* context) + : context_(context) { + } + + void Detach() { + DCHECK(context_->IsUIThread()); + context_ = NULL; + } + + void CallOnUIThread(const base::Closure& closure) { + if (context_) { + context_->PostToUIThread(FROM_HERE, NewRunnableMethod( + this, &UIThreadProxy::CallClosure, closure)); + } + } + + private: + friend class base::RefCountedThreadSafe; + + virtual ~UIThreadProxy() { } + + void CallClosure(const base::Closure& closure) { + if (context_) + closure.Run(); + } + + ChromotingHostContext* context_; + + DISALLOW_COPY_AND_ASSIGN(UIThreadProxy); +}; + // static DesktopEnvironment* DesktopEnvironment::Create(ChromotingHostContext* context) { Capturer* capturer = Capturer::Create(); @@ -47,30 +86,63 @@ DesktopEnvironment::DesktopEnvironment(ChromotingHostContext* context, disconnect_window_(disconnect_window), continue_window_(continue_window), local_input_monitor_(local_input_monitor), - is_monitoring_local_inputs_(false) { + is_monitoring_local_inputs_(false), + proxy_(new UIThreadProxy(context)) { } DesktopEnvironment::~DesktopEnvironment() { } +void DesktopEnvironment::Shutdown() { + DCHECK(context_->IsUIThread()); + + MonitorLocalInputs(false); + ShowDisconnectWindow(false, std::string()); + ShowContinueWindow(false); + StartContinueWindowTimer(false); + + proxy_->Detach(); +} + void DesktopEnvironment::OnConnect(const std::string& username) { + proxy_->CallOnUIThread(base::Bind( + &DesktopEnvironment::ProcessOnConnect, base::Unretained(this), username)); +} + +void DesktopEnvironment::OnLastDisconnect() { + proxy_->CallOnUIThread(base::Bind( + &DesktopEnvironment::ProcessOnLastDisconnect, base::Unretained(this))); +} + +void DesktopEnvironment::OnPause(bool pause) { + proxy_->CallOnUIThread(base::Bind( + &DesktopEnvironment::ProcessOnPause, base::Unretained(this), pause)); +} + +void DesktopEnvironment::ProcessOnConnect(const std::string& username) { + DCHECK(context_->IsUIThread()); + MonitorLocalInputs(true); ShowDisconnectWindow(true, username); StartContinueWindowTimer(true); } -void DesktopEnvironment::OnLastDisconnect() { +void DesktopEnvironment::ProcessOnLastDisconnect() { + DCHECK(context_->IsUIThread()); + MonitorLocalInputs(false); ShowDisconnectWindow(false, std::string()); ShowContinueWindow(false); StartContinueWindowTimer(false); } -void DesktopEnvironment::OnPause(bool pause) { +void DesktopEnvironment::ProcessOnPause(bool pause) { StartContinueWindowTimer(!pause); } void DesktopEnvironment::MonitorLocalInputs(bool enable) { + DCHECK(context_->IsUIThread()); + if (enable == is_monitoring_local_inputs_) return; if (enable) { @@ -83,13 +155,7 @@ void DesktopEnvironment::MonitorLocalInputs(bool enable) { void DesktopEnvironment::ShowDisconnectWindow(bool show, const std::string& username) { - if (!context_->IsUIThread()) { - context_->PostToUIThread( - FROM_HERE, - NewRunnableMethod(this, &DesktopEnvironment::ShowDisconnectWindow, - show, username)); - return; - } + DCHECK(context_->IsUIThread()); if (show) { disconnect_window_->Show(host_, username); @@ -99,12 +165,7 @@ void DesktopEnvironment::ShowDisconnectWindow(bool show, } void DesktopEnvironment::ShowContinueWindow(bool show) { - if (!context_->IsUIThread()) { - context_->PostToUIThread( - FROM_HERE, - NewRunnableMethod(this, &DesktopEnvironment::ShowContinueWindow, show)); - return; - } + DCHECK(context_->IsUIThread()); if (show) { continue_window_->Show(host_); @@ -114,14 +175,8 @@ void DesktopEnvironment::ShowContinueWindow(bool show) { } void DesktopEnvironment::StartContinueWindowTimer(bool start) { - if (context_->main_message_loop() != MessageLoop::current()) { - context_->main_message_loop()->PostTask( - FROM_HERE, - NewRunnableMethod(this, - &DesktopEnvironment::StartContinueWindowTimer, - start)); - return; - } + DCHECK(context_->IsUIThread()); + if (continue_window_timer_.IsRunning() == start) return; if (start) { @@ -134,6 +189,8 @@ void DesktopEnvironment::StartContinueWindowTimer(bool start) { } void DesktopEnvironment::ContinueWindowTimerFunc() { + DCHECK(context_->IsUIThread()); + host_->PauseSession(true); ShowContinueWindow(true); } diff --git a/remoting/host/desktop_environment.h b/remoting/host/desktop_environment.h index 0f473e3..99fd33d 100644 --- a/remoting/host/desktop_environment.h +++ b/remoting/host/desktop_environment.h @@ -9,7 +9,6 @@ #include "base/basictypes.h" #include "base/memory/scoped_ptr.h" -#include "base/threading/thread.h" #include "base/timer.h" namespace remoting { @@ -22,6 +21,7 @@ class Curtain; class DisconnectWindow; class EventExecutor; class LocalInputMonitor; +class UIThreadProxy; class DesktopEnvironment { public: @@ -37,6 +37,10 @@ class DesktopEnvironment { LocalInputMonitor* monitor); virtual ~DesktopEnvironment(); + // Shuts down the object and all its resources synchronously. Must + // be called on the UI thread. + void Shutdown(); + void set_host(ChromotingHost* host) { host_ = host; } Capturer* capturer() const { return capturer_.get(); } @@ -53,6 +57,10 @@ class DesktopEnvironment { void OnPause(bool pause); private: + void ProcessOnConnect(const std::string& username); + void ProcessOnLastDisconnect(); + void ProcessOnPause(bool pause); + void MonitorLocalInputs(bool enable); // Show or hide the Disconnect window on the UI thread. If |show| is false, @@ -98,13 +106,19 @@ class DesktopEnvironment { // Timer controlling the "continue session" dialog. The timer is started when // a connection is made or re-confirmed. On expiry, inputs to the host are // blocked and the dialog is shown. + // + // TODO(sergeyu): It is wrong that we use OneShotTimer on the UI + // thread of the plugin. UI thread runs MessageLoop that is compiled + // as part of chrome, but the timer compiled as part of the + // plugin. This will crash when plugin and chrome are compiled with + // different version of MessageLoop. See crbug.com/90785 . base::OneShotTimer continue_window_timer_; + scoped_refptr proxy_; + DISALLOW_COPY_AND_ASSIGN(DesktopEnvironment); }; } // namespace remoting -DISABLE_RUNNABLE_METHOD_REFCOUNT(remoting::DesktopEnvironment); - #endif // REMOTING_HOST_DESKTOP_ENVIRONMENT_H_ diff --git a/remoting/host/plugin/host_script_object.cc b/remoting/host/plugin/host_script_object.cc index 460655a..d133e3c 100644 --- a/remoting/host/plugin/host_script_object.cc +++ b/remoting/host/plugin/host_script_object.cc @@ -83,6 +83,10 @@ HostNPScriptObject::HostNPScriptObject(NPP plugin, NPObject* parent) HostNPScriptObject::~HostNPScriptObject() { CHECK_EQ(base::PlatformThread::CurrentId(), np_thread_id_); + // Shutdown DesktopEnvironment first so that it doesn't try to post + // tasks on the UI thread while we are stopping the host. + desktop_environment_->Shutdown(); + // Disconnect synchronously. We cannot disconnect asynchronously // here because |host_context_| needs to be stopped on the plugin // thread, but the plugin thread may not exist after the instance @@ -92,7 +96,9 @@ HostNPScriptObject::~HostNPScriptObject() { DisconnectInternal(); disconnected_event_.Wait(); + // Stop all threads. host_context_.Stop(); + if (log_debug_info_func_) { g_npnetscape_funcs->releaseobject(log_debug_info_func_); } @@ -374,22 +380,22 @@ void HostNPScriptObject::ConnectInternal( return; } - // Create the Host. - DesktopEnvironment* desktop_environment = - DesktopEnvironment::Create(&host_context_); - // TODO(sergeyu): Use firewall traversal policy settings here. - scoped_refptr host = - ChromotingHost::Create(&host_context_, host_config, desktop_environment, - access_verifier.release(), logger_.get(), false); - host->AddStatusObserver(this); - host->AddStatusObserver(register_request.get()); - host->set_it2me(true); - // Nothing went wrong, so lets save the host, config and request. - host_ = host; host_config_ = host_config; register_request_.reset(register_request.release()); + // Create DesktopEnvironment. + desktop_environment_.reset(DesktopEnvironment::Create(&host_context_)); + + // Create the Host. + // TODO(sergeyu): Use firewall traversal policy settings here. + host_ = ChromotingHost::Create( + &host_context_, host_config_, desktop_environment_.get(), + access_verifier.release(), logger_.get(), false); + host_->AddStatusObserver(this); + host_->AddStatusObserver(register_request_.get()); + host_->set_it2me(true); + // Start the Host. host_->Start(); @@ -466,9 +472,8 @@ void HostNPScriptObject::OnReceivedSupportID( } void HostNPScriptObject::OnStateChanged(State state) { - if (destructing_.IsSet()) { + if (destructing_.IsSet()) return; - } if (!host_context_.IsUIThread()) { host_context_.PostToUIThread( @@ -485,6 +490,9 @@ void HostNPScriptObject::OnStateChanged(State state) { } void HostNPScriptObject::LogDebugInfo(const std::string& message) { + if (destructing_.IsSet()) + return; + if (!host_context_.IsUIThread()) { host_context_.PostToUIThread( FROM_HERE, @@ -528,6 +536,7 @@ void HostNPScriptObject::PostTaskToNPThread( task); } +// static void HostNPScriptObject::NPTaskSpringboard(void* task) { Task* real_task = reinterpret_cast(task); real_task->Run(); diff --git a/remoting/host/plugin/host_script_object.h b/remoting/host/plugin/host_script_object.h index 5a759ce..b0d057b 100644 --- a/remoting/host/plugin/host_script_object.h +++ b/remoting/host/plugin/host_script_object.h @@ -31,6 +31,7 @@ class Location; namespace remoting { class ChromotingHost; +class DesktopEnvironment; class MutableHostConfig; class RegisterSupportHostRequest; class SignalStrategy; @@ -137,9 +138,11 @@ class HostNPScriptObject : public HostStatusObserver { scoped_ptr logger_; scoped_ptr register_request_; - scoped_refptr host_; scoped_refptr host_config_; ChromotingHostContext host_context_; + scoped_ptr desktop_environment_; + + scoped_refptr host_; int failed_login_attempts_; base::WaitableEvent disconnected_event_; diff --git a/remoting/host/simple_host_process.cc b/remoting/host/simple_host_process.cc index e58fd59..0a732ed 100644 --- a/remoting/host/simple_host_process.cc +++ b/remoting/host/simple_host_process.cc @@ -176,9 +176,8 @@ class SimpleHost { } // Construct a chromoting host. - scoped_refptr host; logger_.reset(new remoting::Logger()); - DesktopEnvironment* desktop_environment; + scoped_ptr desktop_environment; if (fake_) { remoting::Capturer* capturer = new remoting::CapturerFake(); @@ -192,17 +191,18 @@ class SimpleHost { remoting::ContinueWindow::Create(); remoting::LocalInputMonitor* local_input_monitor = remoting::LocalInputMonitor::Create(); - desktop_environment = + desktop_environment.reset( new DesktopEnvironment(&context, capturer, event_executor, curtain, disconnect_window, continue_window, - local_input_monitor); + local_input_monitor)); } else { - desktop_environment = DesktopEnvironment::Create(&context); + desktop_environment.reset(DesktopEnvironment::Create(&context)); } - host = ChromotingHost::Create(&context, config, desktop_environment, - access_verifier.release(), logger_.get(), - false); + scoped_refptr host = + ChromotingHost::Create(&context, config, desktop_environment.get(), + access_verifier.release(), logger_.get(), + false); host->set_it2me(is_it2me_); if (protocol_config_.get()) { -- cgit v1.1