Access Session::config() and Session::jid() on the correct thread only.

BUG=88600
TEST=Unittests

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

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

13 files changed, 78 insertions, 60 deletions

diff --git a/remoting/host/chromoting_host.cc b/remoting/host/chromoting_host.cc
index 8947a27..11a347b 100644
--- a/remoting/host/chromoting_host.cc
+++ b/remoting/host/chromoting_host.cc
@@ -300,6 +300,7 @@ void ChromotingHost::OnIncomingSession(
   // We accept the connection, so create a connection object.
   ConnectionToClient* connection = new ConnectionToClient(
       context_->network_message_loop(), this);
+  connection->Init(session);
 
   // Create a client object.
   ClientSession* client = new ClientSession(
@@ -310,8 +311,6 @@ void ChromotingHost::OnIncomingSession(
   connection->set_host_stub(client);
   connection->set_input_stub(client);
 
-  connection->Init(session);
-
   clients_.push_back(client);
 }
 
@@ -361,42 +360,39 @@ void ChromotingHost::OnClientDisconnected(ConnectionToClient* connection) {
   DCHECK_EQ(context_->main_message_loop(), MessageLoop::current());
 
   // Find the client session corresponding to the given connection.
-  ClientList::iterator client;
-  for (client = clients_.begin(); client != clients_.end(); ++client) {
-    if (client->get()->connection() == connection)
+  ClientList::iterator it;
+  for (it = clients_.begin(); it != clients_.end(); ++it) {
+    if (it->get()->connection() == connection)
       break;
   }
-  if (client == clients_.end())
+  if (it == clients_.end())
     return;
 
-  // Remove the connection from the session manager and stop the session.
-  // TODO(hclam): Stop only if the last connection disconnected.
+  scoped_refptr<ClientSession> client = *it;
+
+  clients_.erase(it);
+
   if (recorder_.get()) {
     recorder_->RemoveConnection(connection);
-    // The recorder only exists to serve the unique authenticated client.
-    // If that client has disconnected, then we can kill the recorder.
-    if (client->get()->authenticated())
-      StopScreenRecorder();
   }
 
   // Close the connection to client just to be safe.
   connection->Disconnect();
 
-  // Also remove reference to ConnectionToClient from this object.
-  int old_authenticated_clients = AuthenticatedClientsCount();
-  clients_.erase(client);
-
-  // Notify the observers of the change, if any.
-  int authenticated_clients = AuthenticatedClientsCount();
-  if (old_authenticated_clients != authenticated_clients) {
+  if (client->authenticated()) {
     for (StatusObserverList::iterator it = status_observers_.begin();
          it != status_observers_.end(); ++it) {
-      (*it)->OnClientDisconnected(connection);
+      (*it)->OnClientDisconnected(client->client_jid());
     }
   }
 
-  // Disable the "curtain" if there are no more active clients.
   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);
     if (is_it2me_) {
       desktop_environment_->OnLastDisconnect();
@@ -447,12 +443,19 @@ void ChromotingHost::EnableCurtainMode(bool enable) {
 
 void ChromotingHost::LocalLoginSucceeded(
     scoped_refptr<ConnectionToClient> connection) {
-  if (MessageLoop::current() != context_->main_message_loop()) {
-    context_->main_message_loop()->PostTask(
-        FROM_HERE, base::Bind(&ChromotingHost::LocalLoginSucceeded, this,
-                              connection));
-    return;
-  }
+  DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
+
+  context_->main_message_loop()->PostTask(
+      FROM_HERE, base::Bind(&ChromotingHost::AddAuthenticatedClient,
+                            this, connection, connection->session()->config(),
+                            connection->session()->jid()));
+}
+
+void ChromotingHost::AddAuthenticatedClient(
+    scoped_refptr<ConnectionToClient> connection,
+    const protocol::SessionConfig& config,
+    const std::string& jid) {
+  DCHECK_EQ(context_->main_message_loop(), MessageLoop::current());
 
   protocol::LocalLoginStatus* status = new protocol::LocalLoginStatus();
   status->set_success(true);
@@ -477,7 +480,7 @@ void ChromotingHost::LocalLoginSucceeded(
   if (!recorder_.get()) {
     // Then we create a ScreenRecorder passing the message loops that
     // it should run on.
-    Encoder* encoder = CreateEncoder(connection->session()->config());
+    Encoder* encoder = CreateEncoder(config);
 
     recorder_ = new ScreenRecorder(context_->main_message_loop(),
                                    context_->encode_message_loop(),
@@ -492,13 +495,13 @@ void ChromotingHost::LocalLoginSucceeded(
   // Notify observers that there is at least one authenticated client.
   for (StatusObserverList::iterator it = status_observers_.begin();
        it != status_observers_.end(); ++it) {
-    (*it)->OnClientAuthenticated(connection);
+    (*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 = connection->session()->jid();
+    std::string username = jid;
     size_t pos = username.find('/');
     if (pos != std::string::npos)
       username.replace(pos, std::string::npos, "");
@@ -531,7 +534,10 @@ void ChromotingHost::ProcessPreAuthentication(
       break;
   }
   CHECK(client != clients_.end());
-  client->get()->OnAuthorizationComplete(true);
+
+  context_->network_message_loop()->PostTask(
+      FROM_HERE, base::Bind(&ClientSession::OnAuthorizationComplete,
+                            client->get(), true));
 }
 
 void ChromotingHost::StopScreenRecorder() {
diff --git a/remoting/host/chromoting_host.h b/remoting/host/chromoting_host.h
index 7b5cea5..3f34233 100644
--- a/remoting/host/chromoting_host.h
+++ b/remoting/host/chromoting_host.h
@@ -125,6 +125,11 @@ class ChromotingHost : public base::RefCountedThreadSafe<ChromotingHost>,
       protocol::Session* session,
       protocol::SessionManager::IncomingSessionResponse* response) OVERRIDE;
 
+  void AddAuthenticatedClient(
+      scoped_refptr<protocol::ConnectionToClient> connection,
+      const protocol::SessionConfig& config,
+      const std::string& jid);
+
   // Sets desired configuration for the protocol. Ownership of the
   // |config| is transferred to the object. Must be called before Start().
   void set_protocol_config(protocol::CandidateSessionConfig* config);
diff --git a/remoting/host/chromoting_host_unittest.cc b/remoting/host/chromoting_host_unittest.cc
index 120fb04..24e49b9 100644
--- a/remoting/host/chromoting_host_unittest.cc
+++ b/remoting/host/chromoting_host_unittest.cc
@@ -114,6 +114,7 @@ class ChromotingHostTest : public testing::Test {
     session2_.reset(new MockSession());
     session_config_ = SessionConfig::GetDefault();
     session_jid_ = "user@domain/rest-of-jid";
+    session2_jid_ = "user2@domain/rest-of-jid";
     session_config2_ = SessionConfig::GetDefault();
 
     ON_CALL(video_stub_, ProcessVideoPacket(_, _))
@@ -136,10 +137,12 @@ class ChromotingHostTest : public testing::Test {
         .WillByDefault(Return(session2_.get()));
     ON_CALL(*session_.get(), config())
         .WillByDefault(ReturnRef(session_config_));
-    ON_CALL(*session_, jid())
-        .WillByDefault(ReturnRef(session_jid_));
     ON_CALL(*session2_.get(), config())
         .WillByDefault(ReturnRef(session_config2_));
+    EXPECT_CALL(*session_, jid())
+        .WillRepeatedly(ReturnRef(session_jid_));
+    EXPECT_CALL(*session2_, jid())
+        .WillRepeatedly(ReturnRef(session2_jid_));
     EXPECT_CALL(*connection_.get(), video_stub())
         .Times(AnyNumber());
     EXPECT_CALL(*connection_.get(), client_stub())
@@ -226,6 +229,7 @@ class ChromotingHostTest : public testing::Test {
   MockClientStub client_stub_;
   MockHostStub host_stub_;
   scoped_refptr<MockConnectionToClient> connection2_;
+  std::string session2_jid_;
   scoped_ptr<MockSession> session2_;
   SessionConfig session_config2_;
   MockVideoStub video_stub2_;
diff --git a/remoting/host/client_session.cc b/remoting/host/client_session.cc
index 9999db0..72666fb 100644
--- a/remoting/host/client_session.cc
+++ b/remoting/host/client_session.cc
@@ -33,6 +33,7 @@ ClientSession::ClientSession(
     : event_handler_(event_handler),
       user_authenticator_(user_authenticator),
       connection_(connection),
+      client_jid_(connection->session()->jid()),
       input_stub_(input_stub),
       authenticated_(false),
       awaiting_continue_approval_(false),
diff --git a/remoting/host/client_session.h b/remoting/host/client_session.h
index 12962ce..d9cd337 100644
--- a/remoting/host/client_session.h
+++ b/remoting/host/client_session.h
@@ -72,6 +72,8 @@ class ClientSession : public protocol::HostStub,
     awaiting_continue_approval_ = awaiting;
   }
 
+  const std::string& client_jid() { return client_jid_; }
+
   // Indicate that local mouse activity has been detected. This causes remote
   // inputs to be ignored for a short time so that the local user will always
   // have the upper hand in 'pointer wars'.
@@ -102,6 +104,8 @@ class ClientSession : public protocol::HostStub,
   // The connection to the client.
   scoped_refptr<protocol::ConnectionToClient> connection_;
 
+  std::string client_jid_;
+
   // The input stub to which this object delegates.
   protocol::InputStub* input_stub_;
 
diff --git a/remoting/host/client_session_unittest.cc b/remoting/host/client_session_unittest.cc
index fca7cd2..07b2e92 100644
--- a/remoting/host/client_session_unittest.cc
+++ b/remoting/host/client_session_unittest.cc
@@ -23,19 +23,27 @@ using protocol::MockConnectionToClient;
 using protocol::MockConnectionToClientEventHandler;
 using protocol::MockHostStub;
 using protocol::MockInputStub;
+using protocol::MockSession;
 
 using testing::_;
 using testing::DeleteArg;
 using testing::InSequence;
 using testing::Return;
+using testing::ReturnRef;
 
 class ClientSessionTest : public testing::Test {
  public:
   ClientSessionTest() {}
 
   virtual void SetUp() {
+    client_jid_ = "user@domain/rest-of-jid";
+    EXPECT_CALL(session_, jid()).WillRepeatedly(ReturnRef(client_jid_));
+
     connection_ = new MockConnectionToClient(
         &connection_event_handler_, &host_stub_, &input_stub_);
+
+    EXPECT_CALL(*connection_, session()).WillRepeatedly(Return(&session_));
+
     user_authenticator_ = new MockUserAuthenticator();
     client_session_ = new ClientSession(
         &session_event_handler_,
@@ -49,6 +57,8 @@ class ClientSessionTest : public testing::Test {
 
  protected:
   MessageLoop message_loop_;
+  std::string client_jid_;
+  MockSession session_;
   MockConnectionToClientEventHandler connection_event_handler_;
   MockHostStub host_stub_;
   MockInputStub input_stub_;
diff --git a/remoting/host/heartbeat_sender.cc b/remoting/host/heartbeat_sender.cc
index 32ddd49..1f28521 100644
--- a/remoting/host/heartbeat_sender.cc
+++ b/remoting/host/heartbeat_sender.cc
@@ -90,10 +90,8 @@ void HeartbeatSender::OnSignallingDisconnected() {
 // Ignore any notifications other than signalling
 // connected/disconnected events.
 void HeartbeatSender::OnAccessDenied() { }
-void HeartbeatSender::OnClientAuthenticated(
-    remoting::protocol::ConnectionToClient* client) { }
-void HeartbeatSender::OnClientDisconnected(
-    remoting::protocol::ConnectionToClient* client) { }
+void HeartbeatSender::OnClientAuthenticated(const std::string& jid) { }
+void HeartbeatSender::OnClientDisconnected(const std::string& jid) { }
 void HeartbeatSender::OnShutdown() { }
 
 void HeartbeatSender::DoSendStanza() {
diff --git a/remoting/host/heartbeat_sender.h b/remoting/host/heartbeat_sender.h
index f30c07a..2100b67 100644
--- a/remoting/host/heartbeat_sender.h
+++ b/remoting/host/heartbeat_sender.h
@@ -74,10 +74,8 @@ class HeartbeatSender : public HostStatusObserver {
   virtual void OnSignallingConnected(SignalStrategy* signal_strategy,
                                      const std::string& full_jid) OVERRIDE;
   virtual void OnSignallingDisconnected() OVERRIDE;
-  virtual void OnClientAuthenticated(
-      remoting::protocol::ConnectionToClient* client) OVERRIDE;
-  virtual void OnClientDisconnected(
-      remoting::protocol::ConnectionToClient* client) OVERRIDE;
+  virtual void OnClientAuthenticated(const std::string& jid) OVERRIDE;
+  virtual void OnClientDisconnected(const std::string& jid) OVERRIDE;
   virtual void OnAccessDenied() OVERRIDE;
   virtual void OnShutdown() OVERRIDE;
 
diff --git a/remoting/host/host_status_observer.h b/remoting/host/host_status_observer.h
index 3de73fd..132dc98 100644
--- a/remoting/host/host_status_observer.h
+++ b/remoting/host/host_status_observer.h
@@ -31,10 +31,8 @@ class HostStatusObserver {
   // 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.
-  virtual void OnClientAuthenticated(
-      remoting::protocol::ConnectionToClient* client) = 0;
-  virtual void OnClientDisconnected(
-      remoting::protocol::ConnectionToClient* client) = 0;
+  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.
   virtual void OnShutdown() = 0;
diff --git a/remoting/host/plugin/host_script_object.cc b/remoting/host/plugin/host_script_object.cc
index 2541a52..598b96e 100644
--- a/remoting/host/plugin/host_script_object.cc
+++ b/remoting/host/plugin/host_script_object.cc
@@ -323,8 +323,7 @@ void HostNPScriptObject::OnAccessDenied() {
     DisconnectInternal();
 }
 
-void HostNPScriptObject::OnClientAuthenticated(
-    remoting::protocol::ConnectionToClient* client) {
+void HostNPScriptObject::OnClientAuthenticated(const std::string& jid) {
   DCHECK_EQ(MessageLoop::current(), host_context_.main_message_loop());
 
   if (state_ == kDisconnecting) {
@@ -332,7 +331,7 @@ void HostNPScriptObject::OnClientAuthenticated(
     return;
   }
 
-  client_username_ = client->session()->jid();
+  client_username_ = jid;
   size_t pos = client_username_.find('/');
   if (pos != std::string::npos)
     client_username_.replace(pos, std::string::npos, "");
@@ -340,8 +339,7 @@ void HostNPScriptObject::OnClientAuthenticated(
   SetState(kConnected);
 }
 
-void HostNPScriptObject::OnClientDisconnected(
-    remoting::protocol::ConnectionToClient* client) {
+void HostNPScriptObject::OnClientDisconnected(const std::string& jid) {
   DCHECK_EQ(MessageLoop::current(), host_context_.main_message_loop());
 
   client_username_.clear();
diff --git a/remoting/host/plugin/host_script_object.h b/remoting/host/plugin/host_script_object.h
index c04a373..2ae146e 100644
--- a/remoting/host/plugin/host_script_object.h
+++ b/remoting/host/plugin/host_script_object.h
@@ -74,10 +74,8 @@ class HostNPScriptObject : public HostStatusObserver {
                                      const std::string& full_jid) OVERRIDE;
   virtual void OnSignallingDisconnected() OVERRIDE;
   virtual void OnAccessDenied() OVERRIDE;
-  virtual void OnClientAuthenticated(
-      remoting::protocol::ConnectionToClient* client) OVERRIDE;
-  virtual void OnClientDisconnected(
-      remoting::protocol::ConnectionToClient* client) OVERRIDE;
+  virtual void OnClientAuthenticated(const std::string& jid) OVERRIDE;
+  virtual void OnClientDisconnected(const std::string& jid) OVERRIDE;
   virtual void OnShutdown() OVERRIDE;
 
   // Post LogDebugInfo to the correct proxy (and thus, on the correct thread).
diff --git a/remoting/host/register_support_host_request.cc b/remoting/host/register_support_host_request.cc
index 38d3c4a..d9e2166 100644
--- a/remoting/host/register_support_host_request.cc
+++ b/remoting/host/register_support_host_request.cc
@@ -84,9 +84,9 @@ void RegisterSupportHostRequest::OnSignallingDisconnected() {
 // connected/disconnected events.
 void RegisterSupportHostRequest::OnAccessDenied() { }
 void RegisterSupportHostRequest::OnClientAuthenticated(
-    remoting::protocol::ConnectionToClient* client) { }
+    const std::string& jid) { }
 void RegisterSupportHostRequest::OnClientDisconnected(
-    remoting::protocol::ConnectionToClient* client) { }
+    const std::string& jid) { }
 void RegisterSupportHostRequest::OnShutdown() { }
 
 XmlElement* RegisterSupportHostRequest::CreateRegistrationRequest(
diff --git a/remoting/host/register_support_host_request.h b/remoting/host/register_support_host_request.h
index e48643fd..3462bcd 100644
--- a/remoting/host/register_support_host_request.h
+++ b/remoting/host/register_support_host_request.h
@@ -59,10 +59,8 @@ class RegisterSupportHostRequest : public HostStatusObserver {
   virtual void OnSignallingConnected(SignalStrategy* signal_strategy,
                                      const std::string& full_jid) OVERRIDE;
   virtual void OnSignallingDisconnected() OVERRIDE;
-  virtual void OnClientAuthenticated(
-      remoting::protocol::ConnectionToClient* client) OVERRIDE;
-  virtual void OnClientDisconnected(
-      remoting::protocol::ConnectionToClient* client) OVERRIDE;
+  virtual void OnClientAuthenticated(const std::string& jid) OVERRIDE;
+  virtual void OnClientDisconnected(const std::string& jid) OVERRIDE;
   virtual void OnAccessDenied() OVERRIDE;
   virtual void OnShutdown() OVERRIDE;