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 64 matching lines @@
       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),
       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());
 }
 
 void ChromotingHost::Start(const std::string& xmpp_login) {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(CalledOnValidThread());

[Wez, 2013/04/02 19:58:33]

Can we also DCHECK that Start() is only called from the Initial state?  Do we
still need the separate Stopped state?

[alexeypa (please no reviews), 2013/04/08 23:28:17]

Done.

 
   LOG(INFO) << "Starting host";
 
   // Make sure this object is not started.
   if (state_ != kInitial)
     return;
   state_ = kStarted;
 
   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;
-  }
+void ChromotingHost::Shutdown() {
+  DCHECK(CalledOnValidThread());

[Wez, 2013/04/02 19:58:33]

nit: Can we also DCHECK that Shutdown is never called from the Initial or
Stopped states?

[alexeypa (please no reviews), 2013/04/08 23:28:17]

Done.

 
   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);
+      // We are already stopping.
       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)
+      if (state_ == kStopping) {

[Wez, 2013/04/02 19:58:33]

How can we ever end up not in the kStopping state at this point?  Even if
DisconnectSession() triggers another Shutdown() we'll ignore that via the
kStopping case.

[alexeypa (please no reviews), 2013/04/08 23:28:17]

Done.

+        state_ = kStopped;
         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) {
     *response = protocol::SessionManager::DECLINE;
     return;
   }
 
   if (login_backoff_.ShouldRejectRequest()) {
     *response = protocol::SessionManager::OVERLOAD;
     return;
   }
@@ skipping 30 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);
   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;
+  DCHECK(CalledOnValidThread());
 
   // 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();
+  FOR_EACH_OBSERVER(HostStatusObserver, status_observers_, OnShutdown());
 
   weak_factory_.InvalidateWeakPtrs();
 }
 
 }  // namespace remoting