Fix service process crash when chromoting is enabled and browser shuts down.

This also make disable remoting to actually work.

BUG=69713, 71615
TEST=None

Review URL: http://codereview.chromium.org/6410104

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@74146 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 95 insertions, 34 deletions

diff --git a/chrome/browser/dom_ui/options/advanced_options_handler.cc b/chrome/browser/dom_ui/options/advanced_options_handler.cc
index 4629d84..ba90d13 100644
--- a/chrome/browser/dom_ui/options/advanced_options_handler.cc
+++ b/chrome/browser/dom_ui/options/advanced_options_handler.cc
@@ -23,6 +23,8 @@
 #include "chrome/browser/printing/cloud_print/cloud_print_url.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/remoting/setup_flow.h"
+#include "chrome/browser/service/service_process_control.h"
+#include "chrome/browser/service/service_process_control_manager.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/browser/tab_contents/tab_contents_view.h"
 #include "chrome/browser/ui/options/options_util.h"
@@ -299,6 +301,9 @@ void AdvancedOptionsHandler::RegisterMessages() {
   dom_ui_->RegisterMessageCallback("showRemotingSetupDialog",
       NewCallback(this,
                   &AdvancedOptionsHandler::ShowRemotingSetupDialog));
+  dom_ui_->RegisterMessageCallback("disableRemoting",
+      NewCallback(this,
+                  &AdvancedOptionsHandler::DisableRemoting));
 #endif
 #if defined(OS_WIN)
   // Setup Windows specific callbacks.
@@ -531,6 +536,15 @@ void AdvancedOptionsHandler::RemoveRemotingSection() {
 void AdvancedOptionsHandler::ShowRemotingSetupDialog(const ListValue* args) {
   remoting::SetupFlow::OpenSetupDialog(dom_ui_->GetProfile());
 }
+
+void AdvancedOptionsHandler::DisableRemoting(const ListValue* args) {
+  ServiceProcessControl* process_control =
+      ServiceProcessControlManager::GetInstance()->GetProcessControl(
+          dom_ui_->GetProfile());
+  if (!process_control || !process_control->is_connected())
+    return;
+  process_control->DisableRemotingHost();
+}
 #endif
 
 void AdvancedOptionsHandler::SetupMetricsReportingCheckbox() {
diff --git a/chrome/browser/dom_ui/options/advanced_options_handler.h b/chrome/browser/dom_ui/options/advanced_options_handler.h
index bc8e1e9..96d202f 100644
--- a/chrome/browser/dom_ui/options/advanced_options_handler.h
+++ b/chrome/browser/dom_ui/options/advanced_options_handler.h
@@ -122,6 +122,9 @@ class AdvancedOptionsHandler
 
   // Callback for Setup Remoting button.
   void ShowRemotingSetupDialog(const ListValue* args);
+
+  // Disable Remoting.
+  void DisableRemoting(const ListValue* args);
 #endif
 
   // Setup the checked state for the metrics reporting checkbox.
diff --git a/chrome/browser/resources/options/advanced_options.js b/chrome/browser/resources/options/advanced_options.js
index 1fcceef..2196314 100644
--- a/chrome/browser/resources/options/advanced_options.js
+++ b/chrome/browser/resources/options/advanced_options.js
@@ -143,11 +143,14 @@ var OptionsPage = options.OptionsPage;
       }
 
       if ($('remotingSetupButton')) {
-        $('remotingSetupButton').onclick = function(event) {
-          chrome.send('showRemotingSetupDialog');
-        }
+          $('remotingSetupButton').onclick = function(event) {
+              chrome.send('showRemotingSetupDialog');
+          }
+          $('remotingStopButton').onclick = function(event) {
+              chrome.send('disableRemoting');
+          }
       }
-    }
+  }
   };
 
   //
diff --git a/chrome/service/remoting/chromoting_host_manager.cc b/chrome/service/remoting/chromoting_host_manager.cc
index aaffdac..3b73a9f 100644
--- a/chrome/service/remoting/chromoting_host_manager.cc
+++ b/chrome/service/remoting/chromoting_host_manager.cc
@@ -20,6 +20,7 @@ ChromotingHostManager::ChromotingHostManager(Observer* observer)
 }
 
 void ChromotingHostManager::Initialize(
+    MessageLoop* main_message_loop,
     base::MessageLoopProxy* file_message_loop) {
   FilePath user_data_dir;
   PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
@@ -31,6 +32,7 @@ void ChromotingHostManager::Initialize(
     VLOG(1) << "Failed to read chromoting config file.";
   }
 
+  main_message_loop_ = main_message_loop;
   chromoting_config_ = config;
 
   if (!IsConfigInitialized()) {
@@ -42,11 +44,14 @@ void ChromotingHostManager::Initialize(
   }
 }
 
-void ChromotingHostManager::Teardown() {
-  Stop();
+void ChromotingHostManager::Teardown(Task* done_task) {
+  Stop(done_task);
 }
 
-ChromotingHostManager::~ChromotingHostManager() {}
+ChromotingHostManager::~ChromotingHostManager() {
+  DCHECK(!chromoting_host_);
+  DCHECK(!chromoting_context_.get());
+}
 
 bool ChromotingHostManager::IsConfigInitialized() {
   std::string host_id;
@@ -96,12 +101,16 @@ void ChromotingHostManager::Enable() {
 }
 
 void ChromotingHostManager::Disable() {
-  if (IsEnabled()) {
-    SetEnabled(false);
-    observer_->OnChromotingHostDisabled();
-  }
+  if (!IsEnabled())
+    return;
+
+  SetEnabled(false);
+
+  // TODO(hclam): Immediately reporting will cause threading problems
+  // ServiceProcess thinks we can shutdown safely.
+  observer_->OnChromotingHostDisabled();
 
-  Stop();
+  Stop(NULL);
 }
 
 void ChromotingHostManager::GetHostInfo(ChromotingHostInfo* host_info) {
@@ -120,17 +129,18 @@ void ChromotingHostManager::GetHostInfo(ChromotingHostInfo* host_info) {
                                 &host_info->login);
 }
 
-void ChromotingHostManager::Stop() {
+void ChromotingHostManager::Stop(Task* done_task) {
   // Stop the host if it is started.
   if (chromoting_host_) {
-    // Shutdown the chromoting host asynchronously. This will signal the host to
-    // shutdown, we'll actually wait for all threads to stop when we destroy
-    // the chromoting context.
-    chromoting_host_->Shutdown();
-    chromoting_host_ = NULL;
+    // Save the shutdown task, it will be executed when chromoting host
+    // is stopped.
+    shutdown_task_.reset(done_task);
 
-    chromoting_context_->Stop();
-    chromoting_context_.reset();
+    // Shutdown the chromoting host asynchronously.
+    chromoting_host_->Shutdown();
+  } else if (done_task) {
+    done_task->Run();
+    delete done_task;
   }
 }
 
@@ -170,6 +180,19 @@ void ChromotingHostManager::Start() {
 }
 
 void ChromotingHostManager::OnShutdown() {
+  if (MessageLoop::current() != main_message_loop_) {
+    main_message_loop_->PostTask(FROM_HERE,
+        NewRunnableMethod(this, &ChromotingHostManager::OnShutdown));
+    return;
+  }
+  chromoting_context_->Stop();
+  chromoting_context_.reset();
+  chromoting_host_ = NULL;
+
+  if (shutdown_task_.get()) {
+    shutdown_task_->Run();
+    shutdown_task_.reset();
+  }
 }
 
 }  // namespace remoting
diff --git a/chrome/service/remoting/chromoting_host_manager.h b/chrome/service/remoting/chromoting_host_manager.h
index b64bfe9..4ca1d6f 100644
--- a/chrome/service/remoting/chromoting_host_manager.h
+++ b/chrome/service/remoting/chromoting_host_manager.h
@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "base/message_loop.h"
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
 #include "remoting/host/chromoting_host.h"
@@ -32,16 +33,20 @@ class ChromotingHostManager
   class Observer {
    public:
     virtual ~Observer() {}
-    virtual void OnChromotingHostEnabled() {}
-    virtual void OnChromotingHostDisabled() {}
+    virtual void OnChromotingHostEnabled() = 0;
+    virtual void OnChromotingHostDisabled() = 0;
   };
 
   // Caller keeps ownership of |observer|. |observer| must not be
   // destroyed while this object exists.
   explicit ChromotingHostManager(Observer* observer);
 
-  void Initialize(base::MessageLoopProxy* file_message_loop);
-  void Teardown();
+  void Initialize(MessageLoop* main_message_loop,
+                  base::MessageLoopProxy* file_message_loop);
+
+  // Shutdown ChromotingHostManager. |done_task| will be executed when done.
+  // This method must be called before ChromotingHostManager is destroyed.
+  void Teardown(Task* done_task);
 
   // Return the reference to the chromoting host only if it has started.
   remoting::ChromotingHost* GetChromotingHost() { return chromoting_host_; }
@@ -68,7 +73,7 @@ class ChromotingHostManager
 
   void SetEnabled(bool enabled);
   void Start();
-  void Stop();
+  void Stop(Task* done_task);
 
   void OnShutdown();
 
@@ -77,6 +82,9 @@ class ChromotingHostManager
   scoped_refptr<remoting::MutableHostConfig> chromoting_config_;
   scoped_ptr<remoting::ChromotingHostContext> chromoting_context_;
   scoped_refptr<remoting::ChromotingHost> chromoting_host_;
+
+  MessageLoop* main_message_loop_;
+  scoped_ptr<Task> shutdown_task_;
 };
 
 }  // namespace remoting
diff --git a/chrome/service/service_process.cc b/chrome/service/service_process.cc
index 9de4cc2..be3e48e 100644
--- a/chrome/service/service_process.cc
+++ b/chrome/service/service_process.cc
@@ -119,7 +119,8 @@ bool ServiceProcess::Initialize(MessageLoop* message_loop,
 #if defined(ENABLE_REMOTING)
   // Initialize chromoting host manager.
   remoting_host_manager_ = new remoting::ChromotingHostManager(this);
-  remoting_host_manager_->Initialize(file_thread_->message_loop_proxy());
+  remoting_host_manager_->Initialize(message_loop,
+                                     file_thread_->message_loop_proxy());
 #endif
 
   // Enable Cloud Print if needed. First check the command-line.
@@ -156,10 +157,6 @@ bool ServiceProcess::Teardown() {
   service_prefs_.reset();
   cloud_print_proxy_.reset();
 
-#if defined(ENABLE_REMOTING)
-  remoting_host_manager_->Teardown();
-#endif
-
   ipc_server_.reset();
   // Signal this event before shutting down the service process. That way all
   // background threads can cleanup.
@@ -174,9 +171,22 @@ bool ServiceProcess::Teardown() {
   return true;
 }
 
+static void QuitMessageLoop(MessageLoop* message_loop) {
+  message_loop->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+}
+
+// This method is called when a shutdown command is received from IPC channel
+// or there was an error in the IPC channel.
 void ServiceProcess::Shutdown() {
+#if defined(ENABLE_REMOTING)
+  // During shutdown of remoting host it has some left over operations on
+  // the UI thread. So we let the teardown to proceed asynchronously
+  remoting_host_manager_->Teardown(
+      NewRunnableFunction(&QuitMessageLoop, main_message_loop_));
+#else
   // Quit the main message loop.
   main_message_loop_->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+#endif
 }
 
 bool ServiceProcess::HandleClientDisconnect() {
@@ -216,11 +226,11 @@ void ServiceProcess::OnCloudPrintProxyDisabled(bool persist_state) {
   OnServiceDisabled();
 }
 
-void ServiceProcess::OnRemotingHostEnabled() {
+void ServiceProcess::OnChromotingHostEnabled() {
   OnServiceEnabled();
 }
 
-void ServiceProcess::OnRemotingHostDisabled() {
+void ServiceProcess::OnChromotingHostDisabled() {
   OnServiceDisabled();
 }
 
diff --git a/chrome/service/service_process.h b/chrome/service/service_process.h
index f293143..ac3a1286 100644
--- a/chrome/service/service_process.h
+++ b/chrome/service/service_process.h
@@ -86,8 +86,8 @@ class ServiceProcess : public CloudPrintProxy::Client,
   virtual void OnCloudPrintProxyDisabled(bool persist_state);
 
   // ChromotingHostManager::Observer interface.
-  virtual void OnRemotingHostEnabled();
-  virtual void OnRemotingHostDisabled();
+  virtual void OnChromotingHostEnabled();
+  virtual void OnChromotingHostDisabled();
 
 #if defined(ENABLE_REMOTING)
   // Return the reference to the chromoting host only if it has started.