Access ChromotingHost::clients_ only on network thread.

Previously ChromotingHost was doing some work on the main thread and
some on the network thread. |clients_| and some other members were
accessed without lock on both of these threads. Moved most of the
ChromotingHost activity to the network thread to avoid possible
race conditions.

BUG=96325
TEST=Chromoting works

Review URL: http://codereview.chromium.org/8495024

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@109556 0039d316-1c4b-4281-b951-d872f2087c98

16 files changed, 256 insertions, 314 deletions

diff --git a/remoting/host/chromoting_host.cc b/remoting/host/chromoting_host.cc
index de8c78d..e23102b 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.
diff --git a/remoting/host/chromoting_host.h b/remoting/host/chromoting_host.h
index 160f5dc..08ad9b4 100644
--- a/remoting/host/chromoting_host.h
+++ b/remoting/host/chromoting_host.h
@@ -163,9 +163,6 @@ class ChromotingHost : public base::RefCountedThreadSafe<ChromotingHost>,
                  bool allow_nat_traversal);
   virtual ~ChromotingHost();
 
-  // This method is called if a client is disconnected from the host.
-  void OnClientDisconnected(ClientSession* client);
-
   // Creates encoder for the specified configuration.
   Encoder* CreateEncoder(const protocol::SessionConfig& config);
 
@@ -182,11 +179,12 @@ class ChromotingHost : public base::RefCountedThreadSafe<ChromotingHost>,
   void StopScreenRecorder();
   void OnScreenRecorderStopped();
 
-  // The following methods are called during shutdown.
-  void ShutdownNetwork();
-  void ShutdownRecorder();
+  // Called from Shutdown() or OnScreenRecorderStopped() to finish shutdown.
   void ShutdownFinish();
 
+  // Unless specified otherwise all members of this class must be
+  // used on the network thread only.
+
   // Parameters specified when the host was created.
   ChromotingHostContext* context_;
   DesktopEnvironment* desktop_environment_;
@@ -199,6 +197,7 @@ class ChromotingHost : public base::RefCountedThreadSafe<ChromotingHost>,
   std::string local_jid_;
   scoped_ptr<protocol::SessionManager> session_manager_;
 
+  // StatusObserverList is thread-safe and can be used on any thread.
   StatusObserverList status_observers_;
 
   // The connections to remote clients.
@@ -207,35 +206,27 @@ class ChromotingHost : public base::RefCountedThreadSafe<ChromotingHost>,
   // Session manager for the host process.
   scoped_refptr<ScreenRecorder> recorder_;
 
-  // Tracks the internal state of the host.
-  // This variable is written on the main thread of ChromotingHostContext
-  // and read by jingle thread.
-  State state_;
-
   // Number of screen recorders that are currently being
   // stopped. Normally set to 0 or 1, but in some cases it may be
   // greater than 1, particularly if when second client can connect
-  // immidiately after previous one disconnected.
+  // immediately after previous one disconnected.
   int stopping_recorders_;
 
-  // Lock is to lock the access to |state_|.
-  base::Lock lock_;
+  // Tracks the internal state of the host.
+  State state_;
 
   // Configuration of the protocol.
   scoped_ptr<protocol::CandidateSessionConfig> protocol_config_;
 
-  bool is_curtained_;
-
-  // Whether or not the host is running in "IT2Me" mode, in which connections
-  // are pre-authenticated, and hence the local login challenge can be bypassed.
-  bool is_it2me_;
-
-  std::string access_code_;
-
-  // Stores list of closures that should be executed when we finish
+  // Stores list of tasks that should be executed when we finish
   // shutdown. Used only while |state_| is set to kStopping.
   std::vector<base::Closure> shutdown_tasks_;
 
+  // TODO(sergeyu): The following members do not belong to
+  // ChromotingHost and should be moved elsewhere.
+  bool is_curtained_;
+  bool is_it2me_;
+  std::string access_code_;
   UiStrings ui_strings_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromotingHost);
diff --git a/remoting/host/chromoting_host_unittest.cc b/remoting/host/chromoting_host_unittest.cc
index 45c33e9..8569761 100644
--- a/remoting/host/chromoting_host_unittest.cc
+++ b/remoting/host/chromoting_host_unittest.cc
@@ -156,6 +156,11 @@ class ChromotingHostTest : public testing::Test {
   }
 
   virtual void TearDown() OVERRIDE {
+    connection_ = NULL;
+    client_ = NULL;
+    connection2_ = NULL;
+    client2_ = NULL;
+    host_ = NULL;
     message_loop_.RunAllPending();
   }
 
@@ -188,9 +193,7 @@ class ChromotingHostTest : public testing::Test {
 
   // Helper method to remove a client connection from ChromotingHost.
   void RemoveClientSession() {
-    context_.network_message_loop()->PostTask(
-        FROM_HERE, base::Bind(
-            &ClientSession::OnConnectionClosed, client_, connection_));
+    client_->OnConnectionClosed(connection_);
   }
 
   static void AddClientToHost(scoped_refptr<ChromotingHost> host,
@@ -199,7 +202,9 @@ class ChromotingHostTest : public testing::Test {
   }
 
   void ShutdownHost() {
-    host_->Shutdown(base::Bind(&PostQuitTask, &message_loop_));
+    message_loop_.PostTask(
+        FROM_HERE, base::Bind(&ChromotingHost::Shutdown, host_,
+                              base::Bind(&PostQuitTask, &message_loop_)));
   }
 
  protected:
@@ -381,10 +386,15 @@ TEST_F(ChromotingHostTest, CurtainModeFail) {
       .Times(0);
   EXPECT_CALL(*disconnect_window_, Show(_, _))
       .Times(0);
-  EXPECT_CALL(*connection_.get(), Disconnect())
-      .WillOnce(QuitMainMessageLoop(&message_loop_));
+  EXPECT_CALL(*continue_window_, Hide())
+      .Times(AnyNumber());
+  EXPECT_CALL(*disconnect_window_, Hide())
+      .Times(AnyNumber());
   SimulateClientConnection(0, false);
-  RemoveClientSession();
+  context_.network_message_loop()->PostTask(
+      FROM_HERE, base::Bind(&ChromotingHostTest::RemoveClientSession,
+                            base::Unretained(this)));
+  PostQuitTask(&message_loop_);
   message_loop_.Run();
 }
 
@@ -397,8 +407,10 @@ TEST_F(ChromotingHostTest, CurtainModeFailSecond) {
     InSequence s;
     EXPECT_CALL(*curtain_, EnableCurtainMode(true))
         .WillOnce(QuitMainMessageLoop(&message_loop_));
-    EXPECT_CALL(*disconnect_window_, Show(_, _))
-        .Times(0);
+    EXPECT_CALL(*local_input_monitor_, Start(_))
+        .Times(1);
+    EXPECT_CALL(*disconnect_window_, Show(_, "user@domain"))
+        .Times(1);
     EXPECT_CALL(video_stub_, ProcessVideoPacket(_, _))
         .WillOnce(DoAll(
             InvokeWithoutArgs(
@@ -452,6 +464,8 @@ TEST_F(ChromotingHostTest, CurtainModeIT2Me) {
         .RetiresOnSaturation();
     EXPECT_CALL(*connection_.get(), Disconnect())
         .InSequence(s1, s2)
+        .WillOnce(InvokeWithoutArgs(
+            this, &ChromotingHostTest::RemoveClientSession))
         .RetiresOnSaturation();
     EXPECT_CALL(*local_input_monitor_, Stop())
         .Times(1)
diff --git a/remoting/host/client_session.cc b/remoting/host/client_session.cc
index d59c3a6..9baf6f7 100644
--- a/remoting/host/client_session.cc
+++ b/remoting/host/client_session.cc
@@ -101,7 +101,6 @@ void ClientSession::OnConnectionClosed(
   DCHECK_EQ(connection_.get(), connection);
   scoped_refptr<ClientSession> self = this;
   event_handler_->OnSessionClosed(this);
-  Disconnect();
 }
 
 void ClientSession::OnConnectionFailed(
@@ -110,7 +109,6 @@ void ClientSession::OnConnectionFailed(
   // TODO(sergeyu): Log failure reason?
   scoped_refptr<ClientSession> self = this;
   event_handler_->OnSessionClosed(this);
-  Disconnect();
 }
 
 void ClientSession::OnSequenceNumberUpdated(
@@ -120,9 +118,13 @@ void ClientSession::OnSequenceNumberUpdated(
 }
 
 void ClientSession::Disconnect() {
-  connection_->Disconnect();
+  DCHECK(connection_);
   authenticated_ = false;
   RestoreEventState();
+
+  // This triggers OnSessionClosed() and the session may be destroyed
+  // as the result, so this call must be the last in this method.
+  connection_->Disconnect();
 }
 
 void ClientSession::LocalMouseMoved(const SkIPoint& mouse_pos) {
diff --git a/remoting/host/client_session.h b/remoting/host/client_session.h
index 07fd387..fe67773 100644
--- a/remoting/host/client_session.h
+++ b/remoting/host/client_session.h
@@ -26,8 +26,6 @@ class ClientSession : public protocol::HostStub,
                       public base::RefCountedThreadSafe<ClientSession> {
  public:
   // Callback interface for passing events to the ChromotingHost.
-  // Called only for external events, i.e. OnSessionClosed() is not
-  // called when Disconnect() is called.
   class EventHandler {
    public:
     virtual ~EventHandler() {}
diff --git a/remoting/host/client_session_unittest.cc b/remoting/host/client_session_unittest.cc
index 548b49f..c7dfa89 100644
--- a/remoting/host/client_session_unittest.cc
+++ b/remoting/host/client_session_unittest.cc
@@ -25,7 +25,7 @@ class ClientSessionTest : public testing::Test {
  public:
   ClientSessionTest() {}
 
-  virtual void SetUp() {
+  virtual void SetUp() OVERRIDE {
     client_jid_ = "user@domain/rest-of-jid";
 
     // Set up a large default screen size that won't affect most tests.
@@ -44,6 +44,13 @@ class ClientSessionTest : public testing::Test {
         &input_stub_, &capturer_);
   }
 
+  virtual void TearDown() OVERRIDE {
+    client_session_ = NULL;
+    // Run message loop before destroying because protocol::Session is
+    // destroyed asynchronously.
+    message_loop_.RunAllPending();
+  }
+
  protected:
   SkISize default_screen_size_;
   MessageLoop message_loop_;
diff --git a/remoting/host/host_status_observer.h b/remoting/host/host_status_observer.h
index 132dc98..ccd03a2 100644
--- a/remoting/host/host_status_observer.h
+++ b/remoting/host/host_status_observer.h
@@ -14,27 +14,28 @@ namespace protocol {
 class ConnectionToClient;
 }
 
+// Interface for host status observer. All methods are invoked on the
+// network thread.
 class HostStatusObserver {
  public:
   HostStatusObserver() { }
   virtual ~HostStatusObserver() { }
 
-  // Called on the network thread when status of the XMPP changes.
+  // Called when status of the signalling channel changes.
   virtual void OnSignallingConnected(SignalStrategy* signal_strategy,
                                      const std::string& full_jid) = 0;
   virtual void OnSignallingDisconnected() = 0;
 
-  // Called on the network thread when an unauthorized user attempts
-  // to connect to the host.
+  // Called when an unauthorized user attempts to connect to the host.
   virtual void OnAccessDenied() = 0;
 
-  // Called on the main thread when a client authenticates, or disconnects.
-  // The observer must not tear-down ChromotingHost state on receipt of
-  // this callback; it is purely informational.
+  // Called when a client authenticates, or disconnects. Observers
+  // must not tear-down ChromotingHost state on receipt of this
+  // callback; it is purely informational.
   virtual void OnClientAuthenticated(const std::string& jid) = 0;
   virtual void OnClientDisconnected(const std::string& jid) = 0;
 
-  // Called on the main thread when the host shuts down.
+  // Called when the host shuts down.
   virtual void OnShutdown() = 0;
 };
 
diff --git a/remoting/host/plugin/host_script_object.cc b/remoting/host/plugin/host_script_object.cc
index 6fb0171..7a23ecc 100644
--- a/remoting/host/plugin/host_script_object.cc
+++ b/remoting/host/plugin/host_script_object.cc
@@ -351,7 +351,12 @@ void HostNPScriptObject::OnAccessDenied() {
 }
 
 void HostNPScriptObject::OnClientAuthenticated(const std::string& jid) {
-  DCHECK_EQ(MessageLoop::current(), host_context_.main_message_loop());
+  if (MessageLoop::current() != host_context_.main_message_loop()) {
+    host_context_.main_message_loop()->PostTask(FROM_HERE, base::Bind(
+        &HostNPScriptObject::OnClientAuthenticated,
+        base::Unretained(this), jid));
+    return;
+  }
 
   if (state_ == kDisconnecting) {
     // Ignore the new connection if we are disconnecting.
@@ -367,7 +372,12 @@ void HostNPScriptObject::OnClientAuthenticated(const std::string& jid) {
 }
 
 void HostNPScriptObject::OnClientDisconnected(const std::string& jid) {
-  DCHECK_EQ(MessageLoop::current(), host_context_.main_message_loop());
+  if (MessageLoop::current() != host_context_.main_message_loop()) {
+    host_context_.main_message_loop()->PostTask(FROM_HERE, base::Bind(
+        &HostNPScriptObject::OnClientDisconnected,
+        base::Unretained(this), jid));
+    return;
+  }
 
   client_username_.clear();
 
@@ -376,7 +386,11 @@ void HostNPScriptObject::OnClientDisconnected(const std::string& jid) {
 }
 
 void HostNPScriptObject::OnShutdown() {
-  DCHECK_EQ(MessageLoop::current(), host_context_.main_message_loop());
+  if (MessageLoop::current() != host_context_.main_message_loop()) {
+    host_context_.main_message_loop()->PostTask(FROM_HERE, base::Bind(
+        &HostNPScriptObject::OnShutdown, base::Unretained(this)));
+    return;
+  }
 
   host_ = NULL;
   if (state_ != kDisconnected) {
@@ -584,7 +598,11 @@ void HostNPScriptObject::DisconnectInternal() {
 }
 
 void HostNPScriptObject::OnShutdownFinished() {
-  DCHECK_EQ(MessageLoop::current(), host_context_.main_message_loop());
+  if (MessageLoop::current() != host_context_.main_message_loop()) {
+    host_context_.main_message_loop()->PostTask(FROM_HERE, base::Bind(
+        &HostNPScriptObject::OnShutdownFinished, base::Unretained(this)));
+    return;
+  }
 
   disconnected_event_.Signal();
 }
diff --git a/remoting/host/screen_recorder.cc b/remoting/host/screen_recorder.cc
index d779eca..951ef22 100644
--- a/remoting/host/screen_recorder.cc
+++ b/remoting/host/screen_recorder.cc
@@ -81,24 +81,27 @@ void ScreenRecorder::Stop(const base::Closure& done_task) {
 
 void ScreenRecorder::AddConnection(
     scoped_refptr<ConnectionToClient> connection) {
+  DCHECK(network_loop_->BelongsToCurrentThread());
+  connections_.push_back(connection);
+
   capture_loop_->PostTask(
       FROM_HERE, base::Bind(&ScreenRecorder::DoInvalidateFullScreen, this));
-
-  // Add the client to the list so it can receive update stream.
-  network_loop_->PostTask(
-      FROM_HERE, base::Bind(&ScreenRecorder::DoAddConnection,
-                            this, connection));
 }
 
 void ScreenRecorder::RemoveConnection(
     scoped_refptr<ConnectionToClient> connection) {
-  network_loop_->PostTask(
-      FROM_HERE, base::Bind(&ScreenRecorder::DoRemoveClient, this, connection));
+  DCHECK(network_loop_->BelongsToCurrentThread());
+
+  ConnectionToClientList::iterator it =
+      std::find(connections_.begin(), connections_.end(), connection);
+  if (it != connections_.end()) {
+    connections_.erase(it);
+  }
 }
 
 void ScreenRecorder::RemoveAllConnections() {
-  network_loop_->PostTask(
-      FROM_HERE, base::Bind(&ScreenRecorder::DoRemoveAllClients, this));
+  DCHECK(network_loop_->BelongsToCurrentThread());
+  connections_.clear();
 }
 
 void ScreenRecorder::UpdateSequenceNumber(int64 sequence_number) {
@@ -271,31 +274,6 @@ void ScreenRecorder::FrameSentCallback(VideoPacket* packet) {
       FROM_HERE, base::Bind(&ScreenRecorder::DoFinishOneRecording, this));
 }
 
-void ScreenRecorder::DoAddConnection(
-    scoped_refptr<ConnectionToClient> connection) {
-  DCHECK(network_loop_->BelongsToCurrentThread());
-
-  connections_.push_back(connection);
-}
-
-void ScreenRecorder::DoRemoveClient(
-    scoped_refptr<ConnectionToClient> connection) {
-  DCHECK(network_loop_->BelongsToCurrentThread());
-
-  ConnectionToClientList::iterator it =
-      std::find(connections_.begin(), connections_.end(), connection);
-  if (it != connections_.end()) {
-    connections_.erase(it);
-  }
-}
-
-void ScreenRecorder::DoRemoveAllClients() {
-  DCHECK(network_loop_->BelongsToCurrentThread());
-
-  // Clear the list of connections.
-  connections_.clear();
-}
-
 void ScreenRecorder::DoStopOnNetworkThread(const base::Closure& done_task) {
   DCHECK(network_loop_->BelongsToCurrentThread());
 
diff --git a/remoting/host/screen_recorder.h b/remoting/host/screen_recorder.h
index 158bf13..303405e 100644
--- a/remoting/host/screen_recorder.h
+++ b/remoting/host/screen_recorder.h
@@ -132,10 +132,6 @@ class ScreenRecorder : public base::RefCountedThreadSafe<ScreenRecorder> {
   void DoSendInit(scoped_refptr<protocol::ConnectionToClient> connection,
                   int width, int height);
 
-  void DoAddConnection(scoped_refptr<protocol::ConnectionToClient> connection);
-  void DoRemoveClient(scoped_refptr<protocol::ConnectionToClient> connection);
-  void DoRemoveAllClients();
-
   // Signal network thread to cease activities.
   void DoStopOnNetworkThread(const base::Closure& done_task);
 
diff --git a/remoting/host/screen_recorder_unittest.cc b/remoting/host/screen_recorder_unittest.cc
index 9540997..86608ba 100644
--- a/remoting/host/screen_recorder_unittest.cc
+++ b/remoting/host/screen_recorder_unittest.cc
@@ -87,7 +87,16 @@ class ScreenRecorderTest : public testing::Test {
         &capturer_, encoder_);
   }
 
+  virtual void TearDown() OVERRIDE {
+    record_ = NULL;
+    connection_ = NULL;
+    // Run message loop before destroying because protocol::Session is
+    // destroyed asynchronously.
+    message_loop_.RunAllPending();
+  }
+
  protected:
+  MessageLoop message_loop_;
   scoped_refptr<ScreenRecorder> record_;
 
   MockConnectionToClientEventHandler handler_;
@@ -99,7 +108,7 @@ class ScreenRecorderTest : public testing::Test {
   // The following mock objects are owned by ScreenRecorder.
   MockCapturer capturer_;
   MockEncoder* encoder_;
-  MessageLoop message_loop_;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(ScreenRecorderTest);
 };
diff --git a/remoting/protocol/connection_to_client.cc b/remoting/protocol/connection_to_client.cc
index 6b2bbd1..5e3c411 100644
--- a/remoting/protocol/connection_to_client.cc
+++ b/remoting/protocol/connection_to_client.cc
@@ -34,8 +34,8 @@ ConnectionToClient::ConnectionToClient(base::MessageLoopProxy* message_loop,
 }
 
 ConnectionToClient::~ConnectionToClient() {
-  // TODO(hclam): When we shut down the viewer we may have to close the
-  // connection.
+  if (session_.get())
+    message_loop_->DeleteSoon(FROM_HERE, session_.release());
 }
 
 void ConnectionToClient::SetEventHandler(EventHandler* event_handler) {
@@ -48,18 +48,21 @@ protocol::Session* ConnectionToClient::session() {
 }
 
 void ConnectionToClient::Disconnect() {
-  // This method can be called from main thread so perform threading switching.
-  if (!message_loop_->BelongsToCurrentThread()) {
-    message_loop_->PostTask(
-        FROM_HERE, base::Bind(&ConnectionToClient::Disconnect, this));
-    return;
-  }
+  DCHECK(message_loop_->BelongsToCurrentThread());
 
   CloseChannels();
 
-  // If there is a session then release it, causing it to close.
-  if (session_.get())
-    session_.reset();
+  DCHECK(session_.get());
+  Session* session = session_.release();
+
+  // It may not be safe to delete |session_| here becase this method
+  // may be invoked in resonse to a libjingle event and libjingle's
+  // sigslot doesn't handle it properly, so postpone the deletion.
+  message_loop_->DeleteSoon(FROM_HERE, session);
+
+  // This should trigger OnConnectionClosed() event and this object
+  // may be destroyed as the result.
+  session->Close();
 }
 
 void ConnectionToClient::UpdateSequenceNumber(int64 sequence_number) {
diff --git a/remoting/protocol/connection_to_client.h b/remoting/protocol/connection_to_client.h
index 3f34702..8723045 100644
--- a/remoting/protocol/connection_to_client.h
+++ b/remoting/protocol/connection_to_client.h
@@ -66,10 +66,7 @@ class ConnectionToClient :
   // Returns the connection in use.
   virtual Session* session();
 
-  // Disconnect the client connection. This method is allowed to be called
-  // more than once and calls after the first one will be ignored.
-  //
-  // After this method is called all the send method calls will be ignored.
+  // Disconnect the client connection.
   virtual void Disconnect();
 
   // Update the sequence number when received from the client. EventHandler
diff --git a/remoting/protocol/connection_to_client_unittest.cc b/remoting/protocol/connection_to_client_unittest.cc
index d3d1aed..81ea1f0 100644
--- a/remoting/protocol/connection_to_client_unittest.cc
+++ b/remoting/protocol/connection_to_client_unittest.cc
@@ -43,6 +43,11 @@ class ConnectionToClientTest : public testing::Test {
     message_loop_.RunAllPending();
   }
 
+  virtual void TearDown() OVERRIDE {
+    viewer_ = NULL;
+    message_loop_.RunAllPending();
+  }
+
   MessageLoop message_loop_;
   MockConnectionToClientEventHandler handler_;
   MockHostStub host_stub_;
@@ -101,13 +106,5 @@ TEST_F(ConnectionToClientTest, StateChange) {
   message_loop_.RunAllPending();
 }
 
-// Test that we can close client connection more than once.
-TEST_F(ConnectionToClientTest, Close) {
-  viewer_->Disconnect();
-  message_loop_.RunAllPending();
-  viewer_->Disconnect();
-  message_loop_.RunAllPending();
-}
-
 }  // namespace protocol
 }  // namespace remoting
diff --git a/remoting/protocol/jingle_session_manager.cc b/remoting/protocol/jingle_session_manager.cc
index cf85d0f..e1a935d 100644
--- a/remoting/protocol/jingle_session_manager.cc
+++ b/remoting/protocol/jingle_session_manager.cc
@@ -38,6 +38,8 @@ JingleSessionManager::JingleSessionManager(
 }
 
 JingleSessionManager::~JingleSessionManager() {
+  // Session manager can be destroyed only after all sessions are destroyed.
+  DCHECK(sessions_.empty());
   Close();
 }
 
@@ -111,13 +113,9 @@ void JingleSessionManager::Init(
 void JingleSessionManager::Close() {
   DCHECK(CalledOnValidThread());
 
-  // Close() can be called only after all sessions are destroyed.
-  DCHECK(sessions_.empty());
-
   if (!closed_) {
     cricket_session_manager_->RemoveClient(kChromotingXmlNamespace);
     jingle_signaling_connector_.reset();
-    cricket_session_manager_.reset();
     closed_ = true;
   }
 }
diff --git a/remoting/protocol/session.h b/remoting/protocol/session.h
index 27558d7..432b948 100644
--- a/remoting/protocol/session.h
+++ b/remoting/protocol/session.h
@@ -25,6 +25,11 @@ namespace protocol {
 // Generic interface for Chromotocol connection used by both client and host.
 // Provides access to the connection channels, but doesn't depend on the
 // protocol used for each channel.
+//
+// Because libjingle's sigslot class doesn't handle deletion properly
+// while it is being invoked all Session instances must be deleted
+// with a clean stack, i.e. not from event handlers, when sigslot may
+// be present in the stack.
 class Session : public base::NonThreadSafe {
  public:
   enum State {