Access ChromotingHost::clients_ only on network thread.

remoting/host/chromoting_host.cc

 // Copyright (c) 2011 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 "base/bind.h"
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/message_loop_proxy.h"
@@ skipping 36 matching lines @@
 ChromotingHost::ChromotingHost(ChromotingHostContext* context,
                                MutableHostConfig* config,
                                DesktopEnvironment* environment,
                                AccessVerifier* access_verifier,
                                bool allow_nat_traversal)
     : context_(context),
       desktop_environment_(environment),
       config_(config),
       access_verifier_(access_verifier),
       allow_nat_traversal_(allow_nat_traversal),
+      stopping_recorders_(0),
       state_(kInitial),
-      stopping_recorders_(0),
       protocol_config_(protocol::CandidateSessionConfig::CreateDefault()),
       is_curtained_(false),
       is_it2me_(false) {
   DCHECK(desktop_environment_);
   desktop_environment_->set_host(this);
 }
 
 ChromotingHost::~ChromotingHost() {
 }
 
@@ skipping 29 matching lines @@
 
   signal_strategy_.reset(
       new XmppSignalStrategy(context_->jingle_thread(), xmpp_login,
                              xmpp_auth_token,
                              xmpp_auth_service));
   signal_strategy_->Init(this);
 }
 
 // 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;
   }
 
   // No-op if this object is not started yet.
   {
     base::AutoLock auto_lock(lock_);
     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)
       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);
+    OnSessionClosed(client);
   }
 
-  ShutdownNetwork();
+  // 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.

+  }
+
+  // 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) {
   DCHECK_EQ(state_, kInitial);
   status_observers_.push_back(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);
 }
 
 ////////////////////////////////////////////////////////////////////////////
 // SignalStrategy::StatusObserver implementations
 void ChromotingHost::OnStateChange(
     SignalStrategy::StatusObserver::State state) {
   DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
 
@@ skipping 98 matching lines @@
 
   *response = protocol::SessionManager::ACCEPT;
 
   LOG(INFO) << "Client connected: " << session->jid();
 
   // Create a client object.
   scoped_refptr<protocol::ConnectionToClient> connection =
       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_EQ(state_, kInitial);
   protocol_config_.reset(config);

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

Not part of this CL, but this is client-specific, so it belongs on the
ClientSession, not here.

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

Actually it is not client-specific - this sets CandidateSessionConfig which
contains list of supported protocol configurations. It is used by simple_host to
force specific configuration.

 }
 
 void ChromotingHost::LocalMouseMoved(const SkIPoint& new_pos) {
   if (!context_->network_message_loop()->BelongsToCurrentThread()) {
     context_->network_message_loop()->PostTask(
         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);
   }
 }
 
 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);
   }
   desktop_environment_->OnPause(pause);

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

Is this callable on the network thread?

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

Yes. All DesktopEnvironment methods can be called on any thread - it needs to
post to the ui thread anyway.

 }
 
 void ChromotingHost::SetUiStrings(const UiStrings& ui_strings) {
   DCHECK_EQ(context_->main_message_loop(), MessageLoop::current());
   DCHECK_EQ(state_, kInitial);
 
   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();
 
   if (video_config.codec == protocol::ChannelConfig::CODEC_VERBATIM) {
     return EncoderRowBased::CreateVerbatimEncoder();
   } else if (video_config.codec == protocol::ChannelConfig::CODEC_ZIP) {
     return EncoderRowBased::CreateZlibEncoder();
   } else if (video_config.codec == protocol::ChannelConfig::CODEC_VP8) {
     return new remoting::EncoderVp8();
   }
 
   return NULL;
 }
 
 std::string ChromotingHost::GenerateHostAuthToken(
     const std::string& encoded_client_token) {
   // TODO(ajwong): Return the signature of this instead.
   return encoded_client_token;
 }
 
 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) {
     if (it->get()->authenticated())
       ++authenticated_clients;
   }
   return authenticated_clients;
 }
 
 void ChromotingHost::EnableCurtainMode(bool enable) {
   // TODO(jamiewalch): This will need to be more sophisticated when we think
   // about proper crash recovery and daemon mode.
   // TODO(wez): CurtainMode shouldn't be driven directly by ChromotingHost.
   if (is_it2me_ || enable == is_curtained_)
     return;
   desktop_environment_->curtain()->EnableCurtainMode(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));
   recorder_ = NULL;

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

Does ScreenRecorder::Stop() guarantee to NOT invoke the callback before it
returns?  If not then |this| may not exist by the time Stop() returns in that
situation.

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

Good catch. Fixed, though it should not be a problem with the current
ScreenRecorder.

 }
 
 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;
   }
 
   --stopping_recorders_;
   DCHECK_GE(stopping_recorders_, 0);
 
   bool stopping;
   {
     base::AutoLock auto_lock(lock_);
     stopping = state_ == kStopping;
   }
 
   if (!stopping_recorders_ && stopping)
     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;
   }
 
   // Keep reference to |this|, so that we don't get destroyed while
   // sending notifications.
   scoped_refptr<ChromotingHost> self(this);
 
   // Notify observers.
   for (StatusObserverList::iterator it = status_observers_.begin();
        it != status_observers_.end(); ++it) {
     (*it)->OnShutdown();

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

We're notifying observers on a different thread to previously; are all the
observers designed to cope with that?

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

There is only one observer that cares about this event (see HostNPScriptObject)
and I've updated it.

   }
 
   for (std::vector<Task*>::iterator it = shutdown_tasks_.begin();
        it != shutdown_tasks_.end(); ++it) {
     (*it)->Run();
     delete *it;
   }
   shutdown_tasks_.clear();
 }
 
+
 }  // namespace remoting