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 a8f4640c65f7a0c51a19a6b4dfef4fa983e4c18a..0dcc1b3ee3d18248a328f0a61728c49109aff01f 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) {
@@ -105,10 +105,9 @@ void ChromotingHost::Start() {
 
 // This method is called when we need to destroy the host process.
 void ChromotingHost::Shutdown(Task* 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;
   }
 
@@ -118,7 +117,7 @@ void ChromotingHost::Shutdown(Task* shutdown_task) {
     if (state_ == kInitial || state_ == kStopped) {
       // Nothing to do if we are not started.
       state_ = kStopped;
-      context_->main_message_loop()->PostTask(FROM_HERE, shutdown_task);
+      context_->network_message_loop()->PostTask(FROM_HERE, shutdown_task);
       return;
     }
     if (shutdown_task)
@@ -132,10 +131,32 @@ void ChromotingHost::Shutdown(Task* shutdown_task) {
   while (!clients_.empty()) {
     scoped_refptr<ClientSession> client = clients_.front();
     client->Disconnect();
-    OnClientDisconnected(client);
+    OnSessionClosed(client);
+  }
+
+  // Stop chromotocol session manager.

[Wez, 2011/11/09 02:32:22]

nit: chromotocol -> Chromotocol.

[Wez, 2011/11/09 02:32:22]

Clarify that we're guaranteed no more callbacks after Close()?

[Sergey Ulanov, 2011/11/09 21:24:00]

Changed to Stop session manager.

[Sergey Ulanov, 2011/11/09 21:24:00]

This is specified in SessionManager header. Don't think it is necessary to
repeat it here.

+  if (session_manager_.get()) {
+    session_manager_->Close();
+    context_->network_message_loop()->PostTask(FROM_HERE, base::Bind(
+        &DeletePointer<protocol::SessionManager>, session_manager_.release()));

[Wez, 2011/11/09 02:32:22]

This looks like a DeleteSoon()?  Clarify in the comment above why we need to
defer deletion until the next time we hit the main loop?

[Sergey Ulanov, 2011/11/09 21:24:00]

Done.

   }
 
-  ShutdownNetwork();
+  // Stop XMPP connection.

[Wez, 2011/11/09 02:32:22]

Clarify that this is synchronous?

[Sergey Ulanov, 2011/11/09 21:24:00]

Done.

+  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 +168,90 @@ 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();
+      OnSessionClosed(*other_client);

[Wez, 2011/11/09 02:32:22]

If Disconnect() is made to trigger OnSessionClosed(), as previously suggested,
then this can go away.

[Sergey Ulanov, 2011/11/09 21:24:00]

I don't think that Disconnect() should trigger OnSessionClosed()

[Wez, 2011/11/09 23:18:45]

Why not?  The EventHandler really only cares that the session has closed -
whether that's because the peer closed it, or we did, doesn't seem relevant to
the EventHandler.

[Sergey Ulanov, 2011/11/10 21:06:14]

Ok. I've changed Disconnect() to trigger OnSessionClosed().

+    }
+  }
+
+  // Those disconnections should have killed the screen recorder.
+  CHECK(recorder_.get() == NULL);

[Wez, 2011/11/09 02:32:22]

We're in OnSessionAuthenticated(), so surely this client is authenticated, in
which case the recorder will have been kept around?

[Sergey Ulanov, 2011/11/09 21:24:00]

Recorder is created after client is authenticated, i.e. in this method. Because
we allow only one client at a time recorder_ should not exist when this method
is called.

[Wez, 2011/11/09 23:18:45]

But by the time we reach here the client we're adding is marked as authenticated
(or might be), such that when we disconnect all other clients the recorder won't
be torn down?

[Sergey Ulanov, 2011/11/10 21:06:14]

I see your point now. Removed this CHECK because it is useless anyway.

+
+  // 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();
+
+  const std::string& jid = client->connection()->session()->jid();

[Wez, 2011/11/09 02:32:22]

nit: Move this after the Notify observers comment?

[Sergey Ulanov, 2011/11/09 21:24:00]

Done.

+
+  // 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);
+
+  std::string username = jid;

[Wez, 2011/11/09 02:32:22]

Why not just take a copy of the JID in the first place, so you don't need to
take a copy into a separate variable down here?

[Sergey Ulanov, 2011/11/09 21:24:00]

Simplified this code to use substr().

+  size_t pos = username.find('/');
+  if (pos != std::string::npos)
+    username.erase(pos);
+  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);
 }
@@ -291,10 +370,8 @@ 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);
 }
 
@@ -311,6 +388,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,14 +396,15 @@ 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,
         NewRunnableMethod(this,
                           &ChromotingHost::PauseSession,
                           pause));
     return;
   }
+
   ClientList::iterator client;
   for (client = clients_.begin(); client != clients_.end(); ++client) {
     client->get()->set_awaiting_continue_approval(pause);
@@ -340,39 +419,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();
@@ -395,6 +441,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) {
@@ -414,62 +462,8 @@ 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_;
@@ -478,8 +472,8 @@ void ChromotingHost::StopScreenRecorder() {
 }
 
 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;
   }
@@ -497,53 +491,8 @@ void ChromotingHost::OnScreenRecorderStopped() {
     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_);
@@ -568,4 +517,5 @@ void ChromotingHost::ShutdownFinish() {
   shutdown_tasks_.clear();
 }
 
+
 }  // namespace remoting