Made the ChromotingHost class not ref-counted.

remoting/host/chromoting_host.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/chromoting_host.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 58 matching lines @@
     scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner)
     : desktop_environment_factory_(desktop_environment_factory),
       session_manager_(session_manager.Pass()),
       audio_task_runner_(audio_task_runner),
       input_task_runner_(input_task_runner),
       video_capture_task_runner_(video_capture_task_runner),
       video_encode_task_runner_(video_encode_task_runner),
       network_task_runner_(network_task_runner),
       ui_task_runner_(ui_task_runner),
       signal_strategy_(signal_strategy),
-      state_(kInitial),
+      started_(false),
       protocol_config_(protocol::CandidateSessionConfig::CreateDefault()),
       login_backoff_(&kDefaultBackoffPolicy),
       authenticating_client_(false),
       reject_authenticating_client_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
   DCHECK(signal_strategy);
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
 
   if (!desktop_environment_factory_->SupportsAudioCapture()) {
     protocol::CandidateSessionConfig::DisableAudioChannel(
         protocol_config_.get());
   }
 }
 
 ChromotingHost::~ChromotingHost() {
-  DCHECK(clients_.empty());
+  DCHECK(CalledOnValidThread());
+
+  // Disconnect all of the clients.
+  while (!clients_.empty()) {
+    clients_.front()->DisconnectSession();
+  }
+
+  // Destroy session manager.

[Wez, 2013/04/09 01:08:21]

nit: Why does this need destroying explicitly rather than leaving the
scoped_ptr<> to do it?

[alexeypa (please no reviews), 2013/04/12 18:22:26]

Done.

+  session_manager_.reset();
+
+  // Clear |desktop_environment_factory_| and |signal_strategy_| to
+  // ensure we don't try to touch them after running shutdown tasks

[Wez, 2013/04/09 01:08:21]

Is there any risk of that, if we're being deleted?  Should this comment read "to
ensure that we don't try to use them as a result of OnShutdown() activity"?

[alexeypa (please no reviews), 2013/04/12 18:22:26]

Done.

+  desktop_environment_factory_ = NULL;
+  signal_strategy_ = NULL;
+
+  // Notify observers.
+  if (started_) {
+    FOR_EACH_OBSERVER(HostStatusObserver, status_observers_, OnShutdown());
+  }
 }
 
 void ChromotingHost::Start(const std::string& xmpp_login) {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(CalledOnValidThread());
+  DCHECK(!started_);
 
   LOG(INFO) << "Starting host";
-
-  // Make sure this object is not started.
-  if (state_ != kInitial)
-    return;
-  state_ = kStarted;
-
+  started_ = true;
   FOR_EACH_OBSERVER(HostStatusObserver, status_observers_,
                     OnStart(xmpp_login));
 
   // Start the SessionManager, supplying this ChromotingHost as the listener.
   session_manager_->Init(signal_strategy_, this);
 }
 
-// This method is called when we need to destroy the host process.
-void ChromotingHost::Shutdown(const base::Closure& shutdown_task) {
-  if (!network_task_runner_->BelongsToCurrentThread()) {
-    network_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&ChromotingHost::Shutdown, this, shutdown_task));
-    return;
-  }
-
-  switch (state_) {
-    case kInitial:
-    case kStopped:
-      // Nothing to do if we are not started.
-      state_ = kStopped;
-      if (!shutdown_task.is_null())
-        network_task_runner_->PostTask(FROM_HERE, shutdown_task);
-      break;
-
-    case kStopping:
-      // We are already stopping. Just save the task.
-      if (!shutdown_task.is_null())
-        shutdown_tasks_.push_back(shutdown_task);
-      break;
-
-    case kStarted:
-      if (!shutdown_task.is_null())
-        shutdown_tasks_.push_back(shutdown_task);
-      state_ = kStopping;
-
-      // Disconnect all of the clients.
-      while (!clients_.empty()) {
-        clients_.front()->DisconnectSession();
-      }
-
-      // Run the remaining shutdown tasks.
-      if (state_ == kStopping)
-        ShutdownFinish();
-
-      break;
-  }
-}
-
 void ChromotingHost::AddStatusObserver(HostStatusObserver* observer) {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(CalledOnValidThread());
   status_observers_.AddObserver(observer);
 }
 
 void ChromotingHost::RemoveStatusObserver(HostStatusObserver* observer) {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(CalledOnValidThread());
   status_observers_.RemoveObserver(observer);
 }
 
 void ChromotingHost::RejectAuthenticatingClient() {
   DCHECK(authenticating_client_);
   reject_authenticating_client_ = true;
 }
 
 void ChromotingHost::SetAuthenticatorFactory(
     scoped_ptr<protocol::AuthenticatorFactory> authenticator_factory) {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(CalledOnValidThread());
   session_manager_->set_authenticator_factory(authenticator_factory.Pass());
 }
 
 void ChromotingHost::SetMaximumSessionDuration(
     const base::TimeDelta& max_session_duration) {
   max_session_duration_ = max_session_duration;
 }
 
 ////////////////////////////////////////////////////////////////////////////
 // protocol::ClientSession::EventHandler implementation.
 void ChromotingHost::OnSessionAuthenticated(ClientSession* client) {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(CalledOnValidThread());
 
   login_backoff_.Reset();
 
   // Disconnect all other clients. |it| should be advanced before Disconnect()
   // is called to avoid it becoming invalid when the client is removed from
   // the list.
   ClientList::iterator it = clients_.begin();
   while (it != clients_.end()) {
     ClientSession* other_client = *it++;
     if (other_client != client)
@@ skipping 12 matching lines @@
   FOR_EACH_OBSERVER(HostStatusObserver, status_observers_,
                     OnClientAuthenticated(jid));
   authenticating_client_ = false;
 
   if (reject_authenticating_client_) {
     client->DisconnectSession();
   }
 }
 
 void ChromotingHost::OnSessionChannelsConnected(ClientSession* client) {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(CalledOnValidThread());
 
   // Notify observers.
   FOR_EACH_OBSERVER(HostStatusObserver, status_observers_,
                     OnClientConnected(client->client_jid()));
 }
 
 void ChromotingHost::OnSessionAuthenticationFailed(ClientSession* client) {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(CalledOnValidThread());
 
   // Notify observers.
   FOR_EACH_OBSERVER(HostStatusObserver, status_observers_,
                     OnAccessDenied(client->client_jid()));
 }
 
 void ChromotingHost::OnSessionClosed(ClientSession* client) {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(CalledOnValidThread());
 
   ClientList::iterator it = std::find(clients_.begin(), clients_.end(), client);
   CHECK(it != clients_.end());
 
   if (client->is_authenticated()) {
     FOR_EACH_OBSERVER(HostStatusObserver, status_observers_,
                       OnClientDisconnected(client->client_jid()));
   }
 
   clients_.erase(it);
   delete client;
-
-  if (state_ == kStopping && clients_.empty())
-    ShutdownFinish();
 }
 
 void ChromotingHost::OnSessionSequenceNumber(ClientSession* session,
                                              int64 sequence_number) {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(CalledOnValidThread());
 }
 
 void ChromotingHost::OnSessionRouteChange(
     ClientSession* session,
     const std::string& channel_name,
     const protocol::TransportRoute& route) {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(CalledOnValidThread());
   FOR_EACH_OBSERVER(HostStatusObserver, status_observers_,
                     OnClientRouteChange(session->client_jid(), channel_name,
                                         route));
 }
 
 void ChromotingHost::OnSessionManagerReady() {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(CalledOnValidThread());
   // Don't need to do anything here, just wait for incoming
   // connections.
 }
 
 void ChromotingHost::OnIncomingSession(
       protocol::Session* session,
       protocol::SessionManager::IncomingSessionResponse* response) {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(CalledOnValidThread());
 
-  if (state_ != kStarted) {
+  if (!started_) {
     *response = protocol::SessionManager::DECLINE;
     return;
   }
 
   if (login_backoff_.ShouldRejectRequest()) {
     *response = protocol::SessionManager::OVERLOAD;
     return;
   }
 
   // We treat each incoming connection as a failure to authenticate,
@@ skipping 28 matching lines @@
       network_task_runner_,
       ui_task_runner_,
       connection.Pass(),
       desktop_environment_factory_,
       max_session_duration_);
   clients_.push_back(client);
 }
 
 void ChromotingHost::set_protocol_config(
     scoped_ptr<protocol::CandidateSessionConfig> config) {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(CalledOnValidThread());
   DCHECK(config.get());
-  DCHECK_EQ(state_, kInitial);
+  DCHECK(!started_);
   protocol_config_ = config.Pass();
 }
 
 void ChromotingHost::DisconnectAllClients() {
-  if (!network_task_runner_->BelongsToCurrentThread()) {
-    network_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&ChromotingHost::DisconnectAllClients, this));
-    return;
-  }
+  DCHECK(CalledOnValidThread());
 
   while (!clients_.empty()) {
     size_t size = clients_.size();
     clients_.front()->DisconnectSession();
     CHECK_EQ(clients_.size(), size - 1);
   }
 }
 
-void ChromotingHost::ShutdownFinish() {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
-  DCHECK_EQ(state_, kStopping);
-
-  state_ = kStopped;
-
-  // Destroy session manager.
-  session_manager_.reset();
-
-  // Clear |desktop_environment_factory_| and |signal_strategy_| to
-  // ensure we don't try to touch them after running shutdown tasks
-  desktop_environment_factory_ = NULL;
-  signal_strategy_ = NULL;
-
-  // Keep reference to |this|, so that we don't get destroyed while
-  // sending notifications.
-  scoped_refptr<ChromotingHost> self(this);
-
-  // Notify observers.
-  FOR_EACH_OBSERVER(HostStatusObserver, status_observers_,
-                    OnShutdown());
-
-  for (std::vector<base::Closure>::iterator it = shutdown_tasks_.begin();
-       it != shutdown_tasks_.end(); ++it) {
-    it->Run();
-  }
-  shutdown_tasks_.clear();
-
-  weak_factory_.InvalidateWeakPtrs();
-}
-
 }  // namespace remoting