A new authenticated connection evicts an old one.

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/stl_util-inl.h"
 #include "base/task.h"
 #include "build/build_config.h"
 #include "remoting/base/constants.h"
@@ skipping 39 matching lines @@
 
 ChromotingHost::ChromotingHost(ChromotingHostContext* context,
                                MutableHostConfig* config,
                                DesktopEnvironment* environment)
     : context_(context),
       config_(config),
       desktop_environment_(environment),
       state_(kInitial),
       protocol_config_(protocol::CandidateSessionConfig::CreateDefault()) {
   DCHECK(desktop_environment_.get());
-  desktop_environment_->set_event_handler(this);
 }
 
 ChromotingHost::~ChromotingHost() {
 }
 
 void ChromotingHost::Start(Task* shutdown_task) {
   if (MessageLoop::current() != context_->main_message_loop()) {
     context_->main_message_loop()->PostTask(
         FROM_HERE,
         NewRunnableMethod(this, &ChromotingHost::Start, shutdown_task));
@@ skipping 62 matching lines @@
       return;
     }
     state_ = kStopped;
   }
 
   // Make sure ScreenRecorder doesn't write to the connection.
   if (recorder_.get()) {
     recorder_->RemoveAllConnections();
   }
 
-  // Disconnect the client.
-  if (connection_) {
-    connection_->Disconnect();
+  // Disconnect the clients.
+  for (size_t i = 0; i < clients_.size(); i++) {
+    clients_[i]->Disconnect();
   }

[awong, 2011/03/22 15:11:56]

Do we care to call clients_.clear() here to drop all the scoped_refptrs?  Or do
we not want to potentially delete the ClientSessions yet?

[simonmorris, 2011/03/23 10:35:20]

Added clients_.clear() .

 
   // Stop the heartbeat sender.
   if (heartbeat_sender_) {
     heartbeat_sender_->Stop();
   }
 
   // Stop chromotocol session manager.
   if (session_manager_) {
     session_manager_->Close(
         NewRunnableMethod(this, &ChromotingHost::OnServerClosed));
   }
 
   // Disconnect from the talk network.
   if (jingle_client_) {
     jingle_client_->Close();
   }
 
   if (recorder_.get()) {
     recorder_->Stop(shutdown_task_.release());
   } else {
     shutdown_task_->Run();
     shutdown_task_.reset();
   }
 }
 
 // This method is called when a client connects.
 void ChromotingHost::OnClientConnected(ConnectionToClient* connection) {
   DCHECK_EQ(context_->main_message_loop(), MessageLoop::current());
-
-  // 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.
-    DCHECK(desktop_environment_->capturer());
-
-    Encoder* encoder = CreateEncoder(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(connection);
 }
 
 void ChromotingHost::OnClientDisconnected(ConnectionToClient* connection) {
   DCHECK_EQ(context_->main_message_loop(), MessageLoop::current());
 
   // Remove the connection from the session manager and stop the session.
-  // TODO(hclam): Stop only if the last connection disconnected.

[Sergey Ulanov, 2011/03/22 23:19:14]

I don't think we can remove this TODO yet. Recorder is still stopped if there
are still active connections

[simonmorris, 2011/03/23 10:35:20]

Done.

   if (recorder_.get()) {
     recorder_->RemoveConnection(connection);
-    recorder_->Stop(NULL);
-    recorder_ = NULL;
+    // If the client is authenticated, then it was the only reason for running
+    // the recorder.

[awong, 2011/03/22 15:11:56]

Not completely sure what this means:

  "the only reason for running the recorder"

I think we're saying that a screen recorder only exists for an authenticated
session, correct?

Is there a race condition here if a new connection is made, authenticates, and
then suddenly disconnects?  I guess all that would happen is that both clients
would get disconnected...not worth worrying about then?

[simonmorris, 2011/03/23 10:35:20]

Clarified comment.

I don't think that scenario causes a problem.

+    if (connection->client_authenticated()) {
+      recorder_->Stop(NULL);
+      recorder_ = NULL;
+    }
   }
 
   // Close the connection to connection just to be safe.
   connection->Disconnect();
 
   // Also remove reference to ConnectionToClient from this object.
-  connection_ = NULL;
+  std::vector<scoped_refptr<ClientSession> >::iterator it;
+  for (it = clients_.begin(); it != clients_.end(); ++it) {
+    if (it->get()->connection() == connection) {
+      clients_.erase(it);
+      break;
+    }
+  }
 }
 
 ////////////////////////////////////////////////////////////////////////////
 // protocol::ConnectionToClient::EventHandler implementations
 void ChromotingHost::OnConnectionOpened(ConnectionToClient* connection) {
   DCHECK_EQ(context_->network_message_loop(), MessageLoop::current());
 
   // Completes the connection to the client.
   VLOG(1) << "Connection to client established.";
   context_->main_message_loop()->PostTask(
@@ skipping 59 matching lines @@
     // TODO(sergeyu): We should try reconnecting here instead of terminating
     // the host.
     Shutdown();
   }
 }
 
 void ChromotingHost::OnNewClientSession(
     protocol::Session* session,
     protocol::SessionManager::IncomingSessionResponse* response) {
   base::AutoLock auto_lock(lock_);
-  // TODO(hclam): Allow multiple clients to connect to the host.
-  if (connection_.get() || state_ != kStarted) {
+  if (state_ != kStarted) {
     *response = protocol::SessionManager::DECLINE;
     return;
   }
 
   // Check that the client has access to the host.
   if (!access_verifier_.VerifyPermissions(session->jid(),
                                           session->initiator_token())) {
     *response = protocol::SessionManager::DECLINE;
     return;
   }
@@ skipping 14 matching lines @@
   }
 
   session->set_config(config);
   session->set_receiver_token(
       GenerateHostAuthToken(session->initiator_token()));
 
   *response = protocol::SessionManager::ACCEPT;
 
   VLOG(1) << "Client connected: " << session->jid();
 
-  // If we accept the connected then create a connection object.
-  connection_ = new ConnectionToClient(context_->network_message_loop(),
-                                       this,
-                                       desktop_environment_.get(),
-                                       desktop_environment_->input_stub());
-  connection_->Init(session);
+  // We accept the connection, so create a connection object.
+  ConnectionToClient* connection = new ConnectionToClient(
+      context_->network_message_loop(),
+      this,
+      NULL,

[awong, 2011/03/22 15:11:56]

Is this parameter ever non-NULL now?

[simonmorris, 2011/03/23 10:35:20]

Only in unit tests, and it's better to be more concise here
at the expense of verbosity there. Removed that parameter.

+      desktop_environment_->input_stub());
+
+  // Create a client object.
+  ClientSession* client = new ClientSession(this, connection);
+  connection->set_host_stub(client);
+
+  connection->Init(session);
+
+  clients_.push_back(client);
 }
 
 void ChromotingHost::set_protocol_config(
     protocol::CandidateSessionConfig* config) {
   DCHECK(config_.get());
   DCHECK_EQ(state_, kInitial);
   protocol_config_.reset(config);
 }
 
-protocol::HostStub* ChromotingHost::host_stub() const {
-  return desktop_environment_.get();
-}
-
 void ChromotingHost::OnServerClosed() {
   // Don't need to do anything here.
 }
 
 // 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();
   }
   // TODO(sergeyu): Enable VP8 on ARM builds.
 #if !defined(ARCH_CPU_ARM_FAMILY)
   else if (video_config.codec == protocol::ChannelConfig::CODEC_VP8) {
     return new remoting::EncoderVp8();
   }
 #endif
 
   return NULL;
 }
 
 std::string ChromotingHost::GenerateHostAuthToken(
     const std::string& encoded_client_token) {
   // TODO(ajwong): Return the signature of this instead.
   return encoded_client_token;
 }
 
-void ChromotingHost::LocalLoginSucceeded() {
+void ChromotingHost::LocalLoginSucceeded(
+    scoped_refptr<ConnectionToClient> connection) {
   if (MessageLoop::current() != context_->main_message_loop()) {
     context_->main_message_loop()->PostTask(
         FROM_HERE,
-        NewRunnableMethod(this, &ChromotingHost::LocalLoginSucceeded));
+        NewRunnableMethod(this,
+                          &ChromotingHost::LocalLoginSucceeded,
+                          connection));
     return;
   }
 
   protocol::LocalLoginStatus* status = new protocol::LocalLoginStatus();
   status->set_success(true);
-  connection_->client_stub()->BeginSessionResponse(
+  connection->client_stub()->BeginSessionResponse(
       status, new DeleteTask<protocol::LocalLoginStatus>(status));
 
-  connection_->OnClientAuthenticated();
+  connection->OnClientAuthenticated();
+
+  // Disconnect all other clients.
+  std::vector<scoped_refptr<ClientSession> > clients_copy(clients_);

[awong, 2011/03/22 15:11:56]

Add a note for why we need to copy |clients_|.

[simonmorris, 2011/03/23 10:35:20]

Done.

+  std::vector<scoped_refptr<ClientSession> >::const_iterator client;
+  for (client = clients_copy.begin(); client != clients_copy.end(); client++) {
+    ConnectionToClient* connection_other = client->get()->connection();
+    if (connection_other != connection) {
+      OnClientDisconnected(connection_other);
+    }
+  }
+  // Those disconnections should have killed the screen recorder.
+  DCHECK(recorder_.get() == NULL);

[awong, 2011/03/22 15:11:56]

This should be a hard requirement right?

I would CHECK fail here rather than DCHECK.  Otherwise, in release mode, we'd
skip this, and happily reuse the old recorder below.

[simonmorris, 2011/03/23 10:35:20]

Done.

+
+  // 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.
+    DCHECK(desktop_environment_->capturer());

[awong, 2011/03/22 15:11:56]

It's not obvious why we're checking for the existence of capturer() here.  I
would have expected that similar to context_, destktop_environment_ was assumed
to be initialized once we started executing code in this class.

[simonmorris, 2011/03/23 10:35:20]

A comment elsewhere suggests that this test is a relic of old
code. Deleted.

+
+    Encoder* encoder = CreateEncoder(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(connection);
   recorder_->Start();
 }
 
-void ChromotingHost::LocalLoginFailed() {
+void ChromotingHost::LocalLoginFailed(
+    scoped_refptr<ConnectionToClient> connection) {
   if (MessageLoop::current() != context_->main_message_loop()) {
     context_->main_message_loop()->PostTask(
         FROM_HERE,
-        NewRunnableMethod(this, &ChromotingHost::LocalLoginFailed));
+        NewRunnableMethod(this, &ChromotingHost::LocalLoginFailed, connection));
     return;
   }
 
   protocol::LocalLoginStatus* status = new protocol::LocalLoginStatus();
   status->set_success(false);
-  connection_->client_stub()->BeginSessionResponse(
+  connection->client_stub()->BeginSessionResponse(
       status, new DeleteTask<protocol::LocalLoginStatus>(status));
 }
 
 }  // namespace remoting