[Chromoting] Introducing refcount-based life time management of the message loops in the service (d…

remoting/host/plugin/host_script_object.cc

 // Copyright (c) 2012 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 "remoting/host/plugin/host_script_object.h"
 
 #include "base/bind.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
 #include "base/message_loop.h"
 #include "base/message_loop_proxy.h"
 #include "base/string_util.h"
 #include "base/sys_string_conversions.h"
 #include "base/threading/platform_thread.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "net/base/net_util.h"
 #include "remoting/base/auth_token_util.h"
+#include "remoting/base/auto_message_loop.h"
 #include "remoting/host/chromoting_host.h"
 #include "remoting/host/chromoting_host_context.h"
 #include "remoting/host/desktop_environment.h"
 #include "remoting/host/host_config.h"
 #include "remoting/host/host_event_logger.h"
 #include "remoting/host/host_key_pair.h"
 #include "remoting/host/host_secret.h"
 #include "remoting/host/it2me_host_user_interface.h"
 #include "remoting/host/network_settings.h"
 #include "remoting/host/pin_hash.h"
@@ skipping 85 matching lines @@
     policy_watcher_->StopWatching(&policy_watcher_stopped_);
     policy_watcher_stopped_.Wait();
     policy_watcher_.reset();
   }
 
   if (host_context_.get()) {
     // 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
     // is destroyed.
-    disconnected_event_.Reset();
     DisconnectInternal();
-    disconnected_event_.Wait();
 
     // UI needs to be shut down on the UI thread before we destroy the
     // host context (because it depends on the context object), but
     // only after the host has been shut down (becase the UI object is
     // registered as status observer for the host, and we can't
     // unregister it from this thread).
     it2me_host_user_interface_.reset();
 
     // Stops all threads.
     host_context_.reset();
+
+    // |disconnected_event_| is signalled when the last reference to the plugin
+    // thread is dropped.
+    disconnected_event_.Wait();

[Wez, 2012/08/28 17:34:53]

Since is blocked on this event, the plugin thread isn't available to process
tasks at this point, so you can just block waiting on the Threads in
|host_context_| exiting, rather than keep a separate event.

[alexeypa (please no reviews), 2012/08/28 19:18:51]

Waiting on the event does more than waiting for the threads to stop. It also
makes sure that all objects referencing the main loop where also destroyed. An
example is |desktop_environment_| that should be destroyed because it keeps
references to the main loop.

[Wez, 2012/08/30 18:20:12]

As discussed offline, we don't currently have the capability to continue pumping
messages posted to the main/plugin thread's task runner once we've been asked to
shutdown, so waiting for objects that references the plugin TaskRunner to drop
those references isn't valid; instead we should either drop posted tasks or use
an AutoThreadTaskRunner just to CHECK for tasks being posted after we've stopped
processing them?

[Wez, 2012/08/30 18:25:40]

Actually the plugin TaskRunner won't even return false at this point, will it,
in which case wrapping it to get CHECKs won't do anything.

[alexeypa (please no reviews), 2012/08/30 22:08:07]

I added a comment before "plugin_task_runner_->Detach();".

[alexeypa (please no reviews), 2012/08/30 22:08:07]

PluginThreadTaskRunner returns false during shutdown.

   }
 
   worker_thread_.Stop();
 }
 
 bool HostNPScriptObject::Init() {
   DCHECK(plugin_task_runner_->BelongsToCurrentThread());
   VLOG(2) << "Init";
 
-  host_context_.reset(new ChromotingHostContext(plugin_task_runner_));
+  host_context_.reset(new ChromotingHostContext(new AutoMessageLoop(
+      NULL, plugin_task_runner_,
+      base::Bind(&base::WaitableEvent::Signal,
+                 base::Unretained(&disconnected_event_)))));
   if (!host_context_->Start()) {
     host_context_.reset();
     return false;
   }
 
   policy_watcher_.reset(
       policy_hack::PolicyWatcher::Create(host_context_->network_task_runner()));
   policy_watcher_->StartWatching(
       base::Bind(&HostNPScriptObject::OnPolicyUpdate,
                  base::Unretained(this)));
@@ skipping 707 matching lines @@
 void HostNPScriptObject::DisconnectInternal() {
   if (!host_context_->network_task_runner()->BelongsToCurrentThread()) {
     host_context_->network_task_runner()->PostTask(
         FROM_HERE, base::Bind(&HostNPScriptObject::DisconnectInternal,
                               base::Unretained(this)));
     return;
   }
 
   switch (state_) {
     case kDisconnected:
-      disconnected_event_.Signal();
       return;
 
     case kStarting:
+      desktop_environment_.reset();
       SetState(kDisconnecting);
       SetState(kDisconnected);
-      disconnected_event_.Signal();
       return;
 
     case kDisconnecting:
       return;
 
     default:
       SetState(kDisconnecting);
 
       if (!host_) {
         OnShutdownFinished();
         return;
       }
       // ChromotingHost::Shutdown() may destroy SignalStrategy
       // synchronously, but SignalStrategy::Listener handlers are not
       // allowed to destroy SignalStrategy, so post task to call
       // Shutdown() later.
       host_context_->network_task_runner()->PostTask(
           FROM_HERE, base::Bind(
               &ChromotingHost::Shutdown, host_,
               base::Bind(&HostNPScriptObject::OnShutdownFinished,
                          base::Unretained(this))));
       return;
   }
 }
 
 void HostNPScriptObject::OnShutdownFinished() {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
-  disconnected_event_.Signal();
+  desktop_environment_.reset();

[Wez, 2012/08/28 17:34:53]

Is this from a DesktopEnvironment->DesktopEnvironmentFactory CL?

[alexeypa (please no reviews), 2012/08/28 19:18:51]

No, it references the main message loop, so it has to be destroyed.

 }
 
 void HostNPScriptObject::OnPolicyUpdate(
     scoped_ptr<base::DictionaryValue> policies) {
   if (!host_context_->network_task_runner()->BelongsToCurrentThread()) {
     host_context_->network_task_runner()->PostTask(
         FROM_HERE,
         base::Bind(&HostNPScriptObject::OnPolicyUpdate,
                    base::Unretained(this), base::Passed(&policies)));
     return;
@@ skipping 371 matching lines @@
   return is_good;
 }
 
 void HostNPScriptObject::SetException(const std::string& exception_string) {
   DCHECK(plugin_task_runner_->BelongsToCurrentThread());
   g_npnetscape_funcs->setexception(parent_, exception_string.c_str());
   LOG(INFO) << exception_string;
 }
 
 }  // namespace remoting