Made the ChromotingHost class not ref-counted.

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"
@@ skipping 96 matching lines @@
   // Also called internally, from the network thread.
   void Disconnect();
 
   // Request a NAT policy notification.
   void RequestNatPolicy();
 
   // remoting::HostStatusObserver implementation.
   virtual void OnAccessDenied(const std::string& jid) OVERRIDE;
   virtual void OnClientAuthenticated(const std::string& jid) OVERRIDE;
   virtual void OnClientDisconnected(const std::string& jid) OVERRIDE;
-  virtual void OnShutdown() OVERRIDE;
 
  private:
   friend class base::RefCountedThreadSafe<It2MeImpl>;
 
   virtual ~It2MeImpl();
 
   // Updates state of the host. Can be called only on the network thread.
   void SetState(State state);
 
   // Returns true if the host is connected.
   bool IsConnected() const;
 
   // Called by Connect() to check for policies and start connection process.
   void ReadPolicyAndConnect(const std::string& uid,
                             const std::string& auth_token,
                             const std::string& auth_service);
 
   // Called by ReadPolicyAndConnect once policies have been read.
   void FinishConnect(const std::string& uid,
                      const std::string& auth_token,
                      const std::string& auth_service);
 
   // Called when the support host registration completes.
   void OnReceivedSupportID(bool success,
                            const std::string& support_id,
                            const base::TimeDelta& lifetime);
 
-  // Called when ChromotingHost::Shutdown() has completed.
-  void OnShutdownFinished();
+  // Shuts down |host_| on the network thread and posts ShutdownOnUiThread()
+  // to shut down UI thread resources.
+  void ShutdownOnNetworkThread();
+
+  // Shuts down |desktop_environment_factory_| and |policy_watcher_| on
+  // the UI thread.
+  void ShutdownOnUiThread();
 
   // Called when initial policies are read, and when they change.
   void OnPolicyUpdate(scoped_ptr<base::DictionaryValue> policies);
 
   // Handlers for NAT traversal and host domain policies.
   void UpdateNatPolicy(bool nat_traversal_enabled);
   void UpdateHostDomainPolicy(const std::string& host_domain);
 
   // Caller supplied fields.
   scoped_ptr<ChromotingHostContext> host_context_;
   scoped_refptr<base::SingleThreadTaskRunner> plugin_task_runner_;
   base::WeakPtr<HostNPScriptObject> script_object_;
   XmppSignalStrategy::XmppServerConfig xmpp_server_config_;
   std::string directory_bot_jid_;
 
   State state_;
 
   scoped_refptr<RsaKeyPair> host_key_pair_;
   scoped_ptr<SignalStrategy> signal_strategy_;
   scoped_ptr<RegisterSupportHostRequest> register_request_;
   scoped_ptr<LogToServer> log_to_server_;
   scoped_ptr<DesktopEnvironmentFactory> desktop_environment_factory_;
   scoped_ptr<HostEventLogger> host_event_logger_;
 
-  scoped_refptr<ChromotingHost> host_;
+  scoped_ptr<ChromotingHost> host_;
   int failed_login_attempts_;
 
   scoped_ptr<policy_hack::PolicyWatcher> policy_watcher_;
 
   // Host the current nat traversal policy setting.
   bool nat_traversal_enabled_;
 
   // The host domain policy setting.
   std::string required_host_domain_;
 
@@ skipping 65 matching lines @@
 void HostNPScriptObject::It2MeImpl::Disconnect() {
   if (!host_context_->network_task_runner()->BelongsToCurrentThread()) {
     DCHECK(plugin_task_runner_->BelongsToCurrentThread());
     host_context_->network_task_runner()->PostTask(
         FROM_HERE, base::Bind(&It2MeImpl::Disconnect, this));
     return;
   }
 
   switch (state_) {
     case kDisconnected:
-      OnShutdownFinished();
+      ShutdownOnNetworkThread();
       return;
 
     case kStarting:
       SetState(kDisconnecting);
       SetState(kDisconnected);
-      OnShutdownFinished();
+      ShutdownOnNetworkThread();
       return;
 
     case kDisconnecting:
       return;
 
     default:
       SetState(kDisconnecting);

[Sergey Ulanov, 2013/04/30 18:26:46]

nit: we don't need disconnecting state anymore because we always go from
disconnecting state to disconnected synchronously, so this code can be
simplified further. Feel free to keep it for now, but maybe add TODO for it.

[alexeypa (please no reviews), 2013/04/30 18:39:56]

The disconnecting state is still needed because we post
It2MeImpl::ShutdownOnNetworkThread() to the network thread at line 286. 

[Sergey Ulanov, 2013/04/30 18:55:19]

Right, but potentially we could just always post a new task for Disconnect(),
and in that case we wouldn't need to post task in line 286.

 
       if (!host_) {
-        OnShutdownFinished();
+        SetState(kDisconnected);
+        ShutdownOnNetworkThread();
         return;
       }
 
-      // ChromotingHost::Shutdown() may destroy SignalStrategy
-      // synchronously, but SignalStrategy::Listener handlers are not
-      // allowed to destroy SignalStrategy, so post task to call
-      // Shutdown() later.
+      // Deleting the host destroys SignalStrategy synchronously, but
+      // SignalStrategy::Listener handlers are not allowed to destroy
+      // SignalStrategy, so post task to destroy the host later.
       host_context_->network_task_runner()->PostTask(
-          FROM_HERE, base::Bind(
-              &ChromotingHost::Shutdown, host_,
-              base::Bind(&It2MeImpl::OnShutdownFinished, this)));
+          FROM_HERE, base::Bind(&It2MeImpl::ShutdownOnNetworkThread, this));
       return;
   }
 }
 
 void HostNPScriptObject::It2MeImpl::RequestNatPolicy() {
   if (!host_context_->network_task_runner()->BelongsToCurrentThread()) {
     DCHECK(plugin_task_runner_->BelongsToCurrentThread());
     host_context_->network_task_runner()->PostTask(
         FROM_HERE, base::Bind(&It2MeImpl::RequestNatPolicy, this));
     return;
@@ skipping 67 matching lines @@
   NetworkSettings network_settings(
      nat_traversal_enabled_ ?
      NetworkSettings::NAT_TRAVERSAL_ENABLED :
      NetworkSettings::NAT_TRAVERSAL_DISABLED);
   if (!nat_traversal_enabled_) {
     network_settings.min_port = NetworkSettings::kDefaultMinPort;
     network_settings.max_port = NetworkSettings::kDefaultMaxPort;
   }
 
   // Create the host.
-  host_ = new ChromotingHost(
+  host_.reset(new ChromotingHost(
       signal_strategy_.get(),
       desktop_environment_factory_.get(),
       CreateHostSessionManager(network_settings,
                                host_context_->url_request_context_getter()),
       host_context_->audio_task_runner(),
       host_context_->input_task_runner(),
       host_context_->video_capture_task_runner(),
       host_context_->video_encode_task_runner(),
       host_context_->network_task_runner(),
-      host_context_->ui_task_runner());
+      host_context_->ui_task_runner()));
   host_->AddStatusObserver(this);
   log_to_server_.reset(
       new LogToServer(host_->AsWeakPtr(), ServerLogEntry::IT2ME,
                       signal_strategy_.get(), directory_bot_jid_));
 
   // Disable audio by default.
   // TODO(sergeyu): Add UI to enable it.
   scoped_ptr<protocol::CandidateSessionConfig> protocol_config =
       protocol::CandidateSessionConfig::CreateDefault();
   protocol::CandidateSessionConfig::DisableAudioChannel(protocol_config.get());
   host_->set_protocol_config(protocol_config.Pass());
 
   // Create event logger.
   host_event_logger_ =
       HostEventLogger::Create(host_->AsWeakPtr(), kApplicationName);
 
   // Connect signaling and start the host.
   signal_strategy_->Connect();
   host_->Start(uid);
 
   SetState(kRequestedAccessCode);
   return;
 }
 
-void HostNPScriptObject::It2MeImpl::OnShutdownFinished() {
-  if (!host_context_->ui_task_runner()->BelongsToCurrentThread()) {
-    host_context_->ui_task_runner()->PostTask(
-        FROM_HERE, base::Bind(&It2MeImpl::OnShutdownFinished, this));
-    return;
+void HostNPScriptObject::It2MeImpl::ShutdownOnNetworkThread() {
+  DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
+  DCHECK(state_ == kDisconnecting || state_ == kDisconnected);
+
+  if (state_ == kDisconnecting) {
+    host_event_logger_.reset();
+    host_->RemoveStatusObserver(this);

[Sergey Ulanov, 2013/04/30 18:26:46]

Don't need this anymore, I think.

[alexeypa (please no reviews), 2013/04/30 18:39:56]

ObserverList<>::~ObserverList DCHECKs that all observers were unregistered.

+    host_.reset();
+
+    register_request_.reset();
+    log_to_server_.reset();
+    signal_strategy_.reset();
+    SetState(kDisconnected);
   }
 
-  // Note that OnShutdownFinished() may be called more than once.
+  host_context_->ui_task_runner()->PostTask(
+      FROM_HERE, base::Bind(&It2MeImpl::ShutdownOnUiThread, this));
+}
+
+void HostNPScriptObject::It2MeImpl::ShutdownOnUiThread() {
+  DCHECK(host_context_->ui_task_runner()->BelongsToCurrentThread());
 
   // Destroy the DesktopEnvironmentFactory, to free thread references.
   desktop_environment_factory_.reset();
 
   // Stop listening for policy updates.
   if (policy_watcher_.get()) {
     base::WaitableEvent policy_watcher_stopped_(true, false);
     policy_watcher_->StopWatching(&policy_watcher_stopped_);
     policy_watcher_stopped_.Wait();
     policy_watcher_.reset();
@@ skipping 46 matching lines @@
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
   // Pass the client user name to the script object before changing state.
   plugin_task_runner_->PostTask(
       FROM_HERE, base::Bind(&HostNPScriptObject::StoreClientUsername,
                             script_object_, std::string()));
 
   Disconnect();
 }
 
-void HostNPScriptObject::It2MeImpl::OnShutdown() {
-  DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
-
-  register_request_.reset();
-  log_to_server_.reset();
-  signal_strategy_.reset();
-  host_event_logger_.reset();
-  host_->RemoveStatusObserver(this);
-  host_ = NULL;
-
-  if (state_ != kDisconnected) {
-    SetState(kDisconnected);
-  }
-}
-
 void HostNPScriptObject::It2MeImpl::OnPolicyUpdate(
     scoped_ptr<base::DictionaryValue> policies) {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
   bool nat_policy;
   if (policies->GetBoolean(policy_hack::PolicyWatcher::kNatPolicyName,
                            &nat_policy)) {
     UpdateNatPolicy(nat_policy);
   }
   std::string host_domain;
@@ skipping 1076 matching lines @@
 }
 
 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