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

8 files changed, 146 insertions, 60 deletions

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<ChromotingHost>,
  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<ChromotingHost>,
     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<ChromotingHost>,
 
   // Parameters specified when the host was created.
   ChromotingHostContext* context_;
+  DesktopEnvironment* desktop_environment_;
   scoped_refptr<MutableHostConfig> config_;
-  scoped_ptr<DesktopEnvironment> desktop_environment_;
   scoped_ptr<AccessVerifier> 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> logger_;
   MessageLoop message_loop_;
   MockConnectionToClientEventHandler handler_;
+  scoped_ptr<DesktopEnvironment> desktop_environment_;
   scoped_refptr<ChromotingHost> host_;
   scoped_refptr<InMemoryHostConfig> 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<UIThreadProxy> {
+ 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<UIThreadProxy>;
+
+  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<DesktopEnvironment> continue_window_timer_;
 
+  scoped_refptr<UIThreadProxy> 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<ChromotingHost> 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*>(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<HostPluginLogger> logger_;
 
   scoped_ptr<RegisterSupportHostRequest> register_request_;
-  scoped_refptr<ChromotingHost> host_;
   scoped_refptr<MutableHostConfig> host_config_;
   ChromotingHostContext host_context_;
+  scoped_ptr<DesktopEnvironment> desktop_environment_;
+
+  scoped_refptr<ChromotingHost> 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<ChromotingHost> host;
     logger_.reset(new remoting::Logger());
-    DesktopEnvironment* desktop_environment;
+    scoped_ptr<DesktopEnvironment> 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<ChromotingHost> host =
+        ChromotingHost::Create(&context, config, desktop_environment.get(),
+                               access_verifier.release(), logger_.get(),
+                               false);
     host->set_it2me(is_it2me_);
 
     if (protocol_config_.get()) {