Access ChromotingHost::clients_ only on network thread.

remoting/host/chromoting_host.cc

Index: remoting/host/chromoting_host.cc
diff --git a/remoting/host/chromoting_host.cc b/remoting/host/chromoting_host.cc
index de8c78d163f22bb06a06383bf5fb6c54735de4fd..e23102b03791f4342eb1b0ee403842800370440e 100644
--- a/remoting/host/chromoting_host.cc
+++ b/remoting/host/chromoting_host.cc
@@ -54,8 +54,8 @@ ChromotingHost::ChromotingHost(ChromotingHostContext* context,
       config_(config),
       access_verifier_(access_verifier),
       allow_nat_traversal_(allow_nat_traversal),
-      state_(kInitial),
       stopping_recorders_(0),
+      state_(kInitial),
       protocol_config_(protocol::CandidateSessionConfig::CreateDefault()),
       is_curtained_(false),
       is_it2me_(false) {
@@ -78,12 +78,9 @@ void ChromotingHost::Start() {
   DCHECK(access_verifier_.get());
 
   // Make sure this object is not started.
-  {
-    base::AutoLock auto_lock(lock_);
-    if (state_ != kInitial)
-      return;
-    state_ = kStarted;
-  }
+  if (state_ != kInitial)
+    return;
+  state_ = kStarted;
 
   // Use an XMPP connection to the Talk network for session signalling.
   std::string xmpp_login;
@@ -105,37 +102,57 @@ void ChromotingHost::Start() {
 
 // This method is called when we need to destroy the host process.
 void ChromotingHost::Shutdown(const base::Closure& shutdown_task) {
-  if (MessageLoop::current() != context_->main_message_loop()) {
-    context_->main_message_loop()->PostTask(
-        FROM_HERE,
-        base::Bind(&ChromotingHost::Shutdown, this, shutdown_task));
+  if (!context_->network_message_loop()->BelongsToCurrentThread()) {
+    context_->network_message_loop()->PostTask(
+        FROM_HERE, base::Bind(&ChromotingHost::Shutdown, this, shutdown_task));
     return;
   }
 
   // No-op if this object is not started yet.
-  {
-    base::AutoLock auto_lock(lock_);
-    if (state_ == kInitial || state_ == kStopped) {
+  if (state_ == kInitial || state_ == kStopped) {
       // Nothing to do if we are not started.
-      state_ = kStopped;
-      context_->main_message_loop()->PostTask(FROM_HERE, shutdown_task);
-      return;
-    }
-    if (!shutdown_task.is_null())
-      shutdown_tasks_.push_back(shutdown_task);
-    if (state_ == kStopping)
-      return;
-    state_ = kStopping;
+    state_ = kStopped;
+    context_->network_message_loop()->PostTask(FROM_HERE, shutdown_task);
+    return;
   }
+  if (!shutdown_task.is_null())
+    shutdown_tasks_.push_back(shutdown_task);
+  if (state_ == kStopping)
+    return;
+  state_ = kStopping;
 
   // Disconnect all of the clients, implicitly stopping the ScreenRecorder.
   while (!clients_.empty()) {
-    scoped_refptr<ClientSession> client = clients_.front();
-    client->Disconnect();
-    OnClientDisconnected(client);
+    clients_.front()->Disconnect();
+  }
+
+  // Stop session manager.
+  if (session_manager_.get()) {
+    session_manager_->Close();
+    // It may not be safe to delete |session_manager_| here becase
+    // this method may be invoked in response to a libjingle event and
+    // libjingle's sigslot doesn't handle it properly, so postpone the
+    // deletion.
+    context_->network_message_loop()->DeleteSoon(
+        FROM_HERE, session_manager_.release());
   }
 
-  ShutdownNetwork();
+  // Stop XMPP connection synchronously.
+  if (signal_strategy_.get()) {
+    signal_strategy_->Close();
+    signal_strategy_.reset();
+
+    for (StatusObserverList::iterator it = status_observers_.begin();
+         it != status_observers_.end(); ++it) {
+      (*it)->OnSignallingDisconnected();
+    }
+  }
+
+  if (recorder_.get()) {
+    StopScreenRecorder();
+  } else {
+    ShutdownFinish();
+  }
 }
 
 void ChromotingHost::AddStatusObserver(HostStatusObserver* observer) {
@@ -147,32 +164,82 @@ void ChromotingHost::AddStatusObserver(HostStatusObserver* observer) {
 // protocol::ClientSession::EventHandler implementation.
 void ChromotingHost::OnSessionAuthenticated(ClientSession* client) {
   DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
-  protocol::Session* session = client->connection()->session();
-  context_->main_message_loop()->PostTask(
-      FROM_HERE, base::Bind(&ChromotingHost::AddAuthenticatedClient,
-                            this, make_scoped_refptr(client),
-                            session->config(), session->jid()));
+
+  // Disconnect all other clients.
+  // Iterate over a copy of the list of clients, to avoid mutating the list
+  // while iterating over it.
+  ClientList clients_copy(clients_);
+  for (ClientList::const_iterator other_client = clients_copy.begin();
+       other_client != clients_copy.end(); ++other_client) {
+    if ((*other_client) != client) {
+      (*other_client)->Disconnect();
+    }
+  }
+
+  // Create a new RecordSession if there was none.
+  if (!recorder_.get()) {
+    // Then we create a ScreenRecorder passing the message loops that
+    // it should run on.
+    Encoder* encoder = CreateEncoder(client->connection()->session()->config());
+
+    recorder_ = new ScreenRecorder(context_->main_message_loop(),
+                                   context_->encode_message_loop(),
+                                   context_->network_message_loop(),
+                                   desktop_environment_->capturer(),
+                                   encoder);
+  }
+
+  // Immediately add the connection and start the session.
+  recorder_->AddConnection(client->connection());
+  recorder_->Start();
+
+  // Notify observers that there is at least one authenticated client.
+  const std::string& jid = client->connection()->session()->jid();
+  for (StatusObserverList::iterator it = status_observers_.begin();
+       it != status_observers_.end(); ++it) {
+    (*it)->OnClientAuthenticated(jid);
+  }
+  // TODO(jamiewalch): Tidy up actions to be taken on connect/disconnect,
+  // including closing the connection on failure of a critical operation.
+  EnableCurtainMode(true);
+
+  std::string username = jid.substr(0, jid.find('/'));
+  desktop_environment_->OnConnect(username);
 }
 
 void ChromotingHost::OnSessionClosed(ClientSession* client) {
   DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
 
-  VLOG(1) << "Connection to client closed.";
-  context_->main_message_loop()->PostTask(
-      FROM_HERE, base::Bind(&ChromotingHost::OnClientDisconnected, this,
-                            make_scoped_refptr(client)));
+  scoped_refptr<ClientSession> client_ref = client;
+
+  ClientList::iterator it = std::find(clients_.begin(), clients_.end(), client);
+  CHECK(it != clients_.end());
+  clients_.erase(it);
+
+  if (recorder_.get()) {
+    recorder_->RemoveConnection(client->connection());
+  }
+
+  for (StatusObserverList::iterator it = status_observers_.begin();
+       it != status_observers_.end(); ++it) {
+    (*it)->OnClientDisconnected(client->client_jid());
+  }
+
+  if (AuthenticatedClientsCount() == 0) {
+    if (recorder_.get()) {
+      // Stop the recorder if there are no more clients.
+      StopScreenRecorder();
+    }
+
+    // Disable the "curtain" if there are no more active clients.
+    EnableCurtainMode(false);
+    desktop_environment_->OnLastDisconnect();
+  }
 }
 
 void ChromotingHost::OnSessionSequenceNumber(ClientSession* session,
                                              int64 sequence_number) {
-  // Update the sequence number in ScreenRecorder.
-  if (MessageLoop::current() != context_->main_message_loop()) {
-    context_->main_message_loop()->PostTask(
-        FROM_HERE, base::Bind(&ChromotingHost::OnSessionSequenceNumber, this,
-                              make_scoped_refptr(session), sequence_number));
-    return;
-  }
-
+  DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
   if (recorder_.get())
     recorder_->UpdateSequenceNumber(sequence_number);
 }
@@ -232,7 +299,6 @@ void ChromotingHost::OnIncomingSession(
       protocol::SessionManager::IncomingSessionResponse* response) {
   DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
 
-  base::AutoLock auto_lock(lock_);
   if (state_ != kStarted) {
     *response = protocol::SessionManager::DECLINE;
     return;
@@ -291,16 +357,14 @@ void ChromotingHost::OnIncomingSession(
       new protocol::ConnectionToClient(context_->network_message_loop(),
                                        session);
   ClientSession* client = new ClientSession(
-      this, connection,
-      desktop_environment_->event_executor(),
+      this, connection, desktop_environment_->event_executor(),
       desktop_environment_->capturer());
-
   clients_.push_back(client);
 }
 
 void ChromotingHost::set_protocol_config(
     protocol::CandidateSessionConfig* config) {
-  DCHECK(config_.get());
+  DCHECK(config);
   DCHECK_EQ(state_, kInitial);
   protocol_config_.reset(config);
 }
@@ -311,6 +375,7 @@ void ChromotingHost::LocalMouseMoved(const SkIPoint& new_pos) {
         FROM_HERE, base::Bind(&ChromotingHost::LocalMouseMoved, this, new_pos));
     return;
   }
+
   ClientList::iterator client;
   for (client = clients_.begin(); client != clients_.end(); ++client) {
     client->get()->LocalMouseMoved(new_pos);
@@ -318,11 +383,12 @@ void ChromotingHost::LocalMouseMoved(const SkIPoint& new_pos) {
 }
 
 void ChromotingHost::PauseSession(bool pause) {
-  if (context_->main_message_loop() != MessageLoop::current()) {
-    context_->main_message_loop()->PostTask(
+  if (!context_->network_message_loop()->BelongsToCurrentThread()) {
+    context_->network_message_loop()->PostTask(
         FROM_HERE, base::Bind(&ChromotingHost::PauseSession, this, pause));
     return;
   }
+
   ClientList::iterator client;
   for (client = clients_.begin(); client != clients_.end(); ++client) {
     client->get()->set_awaiting_continue_approval(pause);
@@ -337,39 +403,6 @@ void ChromotingHost::SetUiStrings(const UiStrings& ui_strings) {
   ui_strings_ = ui_strings;
 }
 
-void ChromotingHost::OnClientDisconnected(ClientSession* client) {
-  DCHECK_EQ(context_->main_message_loop(), MessageLoop::current());
-
-  scoped_refptr<ClientSession> client_ref = client;
-
-  ClientList::iterator it;
-  for (it = clients_.begin(); it != clients_.end(); ++it) {
-    if (it->get() == client)
-      break;
-  }
-  clients_.erase(it);
-
-  if (recorder_.get()) {
-    recorder_->RemoveConnection(client->connection());
-  }
-
-  for (StatusObserverList::iterator it = status_observers_.begin();
-       it != status_observers_.end(); ++it) {
-    (*it)->OnClientDisconnected(client->client_jid());
-  }
-
-  if (AuthenticatedClientsCount() == 0) {
-    if (recorder_.get()) {
-      // Stop the recorder if there are no more clients.
-      StopScreenRecorder();
-    }
-
-    // Disable the "curtain" if there are no more active clients.
-    EnableCurtainMode(false);
-    desktop_environment_->OnLastDisconnect();
-  }
-}
-
 // TODO(sergeyu): Move this to SessionManager?
 Encoder* ChromotingHost::CreateEncoder(const protocol::SessionConfig& config) {
   const protocol::ChannelConfig& video_config = config.video_config();
@@ -392,6 +425,8 @@ std::string ChromotingHost::GenerateHostAuthToken(
 }
 
 int ChromotingHost::AuthenticatedClientsCount() const {
+  DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
+
   int authenticated_clients = 0;
   for (ClientList::const_iterator it = clients_.begin(); it != clients_.end();
        ++it) {
@@ -411,72 +446,19 @@ void ChromotingHost::EnableCurtainMode(bool enable) {
   is_curtained_ = enable;
 }
 
-void ChromotingHost::AddAuthenticatedClient(
-    ClientSession* client,
-    const protocol::SessionConfig& config,
-    const std::string& jid) {
-  DCHECK_EQ(context_->main_message_loop(), MessageLoop::current());
-
-  // Disconnect all other clients.
-  // Iterate over a copy of the list of clients, to avoid mutating the list
-  // while iterating over it.
-  ClientList clients_copy(clients_);
-  for (ClientList::const_iterator other_client = clients_copy.begin();
-       other_client != clients_copy.end(); ++other_client) {
-    if ((*other_client) != client) {
-      (*other_client)->Disconnect();
-      OnClientDisconnected(*other_client);
-    }
-  }
-
-  // Those disconnections should have killed the screen recorder.
-  CHECK(recorder_.get() == NULL);
-
-  // Create a new RecordSession if there was none.
-  if (!recorder_.get()) {
-    // Then we create a ScreenRecorder passing the message loops that
-    // it should run on.
-    Encoder* encoder = CreateEncoder(config);
-
-    recorder_ = new ScreenRecorder(context_->main_message_loop(),
-                                   context_->encode_message_loop(),
-                                   context_->network_message_loop(),
-                                   desktop_environment_->capturer(),
-                                   encoder);
-  }
-
-  // Immediately add the connection and start the session.
-  recorder_->AddConnection(client->connection());
-  recorder_->Start();
-  // Notify observers that there is at least one authenticated client.
-  for (StatusObserverList::iterator it = status_observers_.begin();
-       it != status_observers_.end(); ++it) {
-    (*it)->OnClientAuthenticated(jid);
-  }
-  // TODO(jamiewalch): Tidy up actions to be taken on connect/disconnect,
-  // including closing the connection on failure of a critical operation.
-  EnableCurtainMode(true);
-  if (is_it2me_) {
-    std::string username = jid;
-    size_t pos = username.find('/');
-    if (pos != std::string::npos)
-      username.replace(pos, std::string::npos, "");
-    desktop_environment_->OnConnect(username);
-  }
-}
-
 void ChromotingHost::StopScreenRecorder() {
-  DCHECK(MessageLoop::current() == context_->main_message_loop());
+  DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
   DCHECK(recorder_.get());
 
   ++stopping_recorders_;
-  recorder_->Stop(base::Bind(&ChromotingHost::OnScreenRecorderStopped, this));
+  scoped_refptr<ScreenRecorder> recorder = recorder_;
   recorder_ = NULL;
+  recorder->Stop(base::Bind(&ChromotingHost::OnScreenRecorderStopped, this));
 }
 
 void ChromotingHost::OnScreenRecorderStopped() {
-  if (MessageLoop::current() != context_->main_message_loop()) {
-    context_->main_message_loop()->PostTask(
+  if (!context_->network_message_loop()->BelongsToCurrentThread()) {
+    context_->network_message_loop()->PostTask(
         FROM_HERE, base::Bind(&ChromotingHost::OnScreenRecorderStopped, this));
     return;
   }
@@ -484,68 +466,14 @@ void ChromotingHost::OnScreenRecorderStopped() {
   --stopping_recorders_;
   DCHECK_GE(stopping_recorders_, 0);
 
-  bool stopping;
-  {
-    base::AutoLock auto_lock(lock_);
-    stopping = state_ == kStopping;
-  }
-
-  if (!stopping_recorders_ && stopping)
+  if (!stopping_recorders_ && state_ == kStopping)
     ShutdownFinish();
 }
 
-void ChromotingHost::ShutdownNetwork() {
-  if (!context_->network_message_loop()->BelongsToCurrentThread()) {
-    context_->network_message_loop()->PostTask(
-        FROM_HERE, base::Bind(&ChromotingHost::ShutdownNetwork, this));
-    return;
-  }
-
-  // Stop chromotocol session manager.
-  if (session_manager_.get()) {
-    session_manager_->Close();
-    session_manager_.reset();
-  }
-
-  // Stop XMPP connection.
-  if (signal_strategy_.get()) {
-    signal_strategy_->Close();
-    signal_strategy_.reset();
-
-    for (StatusObserverList::iterator it = status_observers_.begin();
-         it != status_observers_.end(); ++it) {
-      (*it)->OnSignallingDisconnected();
-    }
-  }
-
-  ShutdownRecorder();
-}
-
-void ChromotingHost::ShutdownRecorder() {
-  if (MessageLoop::current() != context_->main_message_loop()) {
-    context_->main_message_loop()->PostTask(
-        FROM_HERE, base::Bind(&ChromotingHost::ShutdownRecorder, this));
-    return;
-  }
-
-  if (recorder_.get()) {
-    StopScreenRecorder();
-  } else if (!stopping_recorders_) {
-    ShutdownFinish();
-  }
-}
-
 void ChromotingHost::ShutdownFinish() {
-  if (MessageLoop::current() != context_->main_message_loop()) {
-    context_->main_message_loop()->PostTask(
-        FROM_HERE, base::Bind(&ChromotingHost::ShutdownFinish, this));
-    return;
-  }
+  DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
 
-  {
-    base::AutoLock auto_lock(lock_);
-    state_ = kStopped;
-  }
+  state_ = kStopped;
 
   // Keep reference to |this|, so that we don't get destroyed while
   // sending notifications.