A new authenticated connection evicts an old one.

remoting/host/chromoting_host_unittest.cc

Index: remoting/host/chromoting_host_unittest.cc
diff --git a/remoting/host/chromoting_host_unittest.cc b/remoting/host/chromoting_host_unittest.cc
index 23295ae92cfa1b31bb156aff2f225251109beb8f..2efa272445dd1ea008fb56919f32d6ca3cfe6944 100644
--- a/remoting/host/chromoting_host_unittest.cc
+++ b/remoting/host/chromoting_host_unittest.cc
@@ -6,7 +6,6 @@
 #include "remoting/host/capturer_fake.h"
 #include "remoting/host/chromoting_host.h"
 #include "remoting/host/chromoting_host_context.h"
-#include "remoting/host/desktop_environment_fake.h"
 #include "remoting/host/host_mock_objects.h"
 #include "remoting/host/in_memory_host_config.h"
 #include "remoting/proto/video.pb.h"
@@ -41,20 +40,6 @@ void PostQuitTask(MessageLoop* message_loop) {
   message_loop->PostTask(FROM_HERE, new MessageLoop::QuitTask());
 }
 
-void BeginSessionRequest(protocol::HostStub* host_stub) {
-  LocalLoginCredentials* credentials =
-      new LocalLoginCredentials();
-  credentials->set_type(protocol::PASSWORD);
-  credentials->set_username("hello");
-
-  const std::string password = "world!";
-  credentials->set_credential(password.data(), password.length());
-
-  host_stub->BeginSessionRequest(
-      credentials,
-      new DeleteTask<LocalLoginCredentials>(credentials));
-}
-
 // Run the task and delete it afterwards. This action is used to deal with
 // done callbacks.
 ACTION(RunDoneTask) {
@@ -66,7 +51,7 @@ ACTION_P(QuitMainMessageLoop, message_loop) {
   PostQuitTask(message_loop);
 }
 
-}  // namepace
+}  // namespace
 
 class ChromotingHostTest : public testing::Test {
  public:
@@ -90,57 +75,95 @@ class ChromotingHostTest : public testing::Test {
 
     Capturer* capturer = new CapturerFake(context_.main_message_loop());
     host_stub_ = new MockHostStub();
+    host_stub2_ = new MockHostStub();
     input_stub_ = new MockInputStub();
+    input_stub2_ = new MockInputStub();
     DesktopEnvironment* desktop =
-        new DesktopEnvironmentFake(capturer, input_stub_);
+        new DesktopEnvironment(capturer, input_stub_);
     host_ = ChromotingHost::Create(&context_, config_, desktop);
     connection_ = new MockConnectionToClient(
         &message_loop_, &handler_, host_stub_, input_stub_);
+    connection2_ = new MockConnectionToClient(
+        &message_loop_, &handler_, host_stub2_, input_stub2_);
     session_ = new MockSession();
+    session2_ = new MockSession();
     session_config_.reset(SessionConfig::CreateDefault());
+    session_config2_.reset(SessionConfig::CreateDefault());
 
     ON_CALL(video_stub_, ProcessVideoPacket(_, _))
         .WillByDefault(
             DoAll(DeleteArg<0>(), DeleteArg<1>()));
+    ON_CALL(video_stub2_, ProcessVideoPacket(_, _))
+        .WillByDefault(
+            DoAll(DeleteArg<0>(), DeleteArg<1>()));
     ON_CALL(*connection_.get(), video_stub())
         .WillByDefault(Return(&video_stub_));
     ON_CALL(*connection_.get(), client_stub())
         .WillByDefault(Return(&client_stub_));
     ON_CALL(*connection_.get(), session())
         .WillByDefault(Return(session_));
+    ON_CALL(*connection2_.get(), video_stub())
+        .WillByDefault(Return(&video_stub2_));
+    ON_CALL(*connection2_.get(), client_stub())
+        .WillByDefault(Return(&client_stub2_));
+    ON_CALL(*connection2_.get(), session())
+        .WillByDefault(Return(session2_));
     ON_CALL(*session_.get(), config())
         .WillByDefault(Return(session_config_.get()));
+    ON_CALL(*session2_.get(), config())
+        .WillByDefault(Return(session_config2_.get()));
     EXPECT_CALL(*connection_.get(), video_stub())
         .Times(AnyNumber());
     EXPECT_CALL(*connection_.get(), client_stub())
         .Times(AnyNumber());
     EXPECT_CALL(*connection_.get(), session())
         .Times(AnyNumber());
+    EXPECT_CALL(*connection2_.get(), video_stub())
+        .Times(AnyNumber());
+    EXPECT_CALL(*connection2_.get(), client_stub())
+        .Times(AnyNumber());
+    EXPECT_CALL(*connection2_.get(), session())
+        .Times(AnyNumber());
     EXPECT_CALL(*session_.get(), config())
         .Times(AnyNumber());
+    EXPECT_CALL(*session2_.get(), config())
+        .Times(AnyNumber());
+
   }
 
   virtual void TearDown() {
   }
 
-  // Helper metjod to pretend a client is connected to ChromotingHost.
-  void SimulateClientConnection() {
+  // Helper method to pretend a client is connected to ChromotingHost.
+  void SimulateClientConnection(int connection_index) {
+    scoped_refptr<MockConnectionToClient> connection =
+        (connection_index == 0) ? connection_ : connection2_;
+    scoped_refptr<ClientSession> client = new ClientSession(host_.get(),
+                                                            connection);
+    connection->set_host_stub(client.get());
+
     context_.network_message_loop()->PostTask(
         FROM_HERE,
         NewRunnableMethod(host_.get(),
-                          &ChromotingHost::set_connection,
-                          connection_));
+                          &ChromotingHost::add_client,
+                          client));
     context_.network_message_loop()->PostTask(
         FROM_HERE,
         NewRunnableMethod(host_.get(),
                           &ChromotingHost::OnClientConnected,
-                          connection_));
+                          connection));
     context_.network_message_loop()->PostTask(
         FROM_HERE,
-        NewRunnableFunction(&BeginSessionRequest, host_->host_stub()));
+        NewRunnableMethod(host_.get(),
+                          &ChromotingHost::LocalLoginSucceeded,
+                          connection));
   }
 
-  // Helper method to remove a client connection from ChromotongHost.
+  void SimulateSecondClientConnection() {

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

This is nit-picky, but I dislike the lack of symmetry between
SimulateClientConnection(0) and SimulateSecondClientConnection.

I'd either do SimulateFirstConnection and SimulateSecondConnection, or bind the
argument in via a callback.  Either method would be fine with me.

For the last option, you want to look at "CreateFunctor()".  The call should be
something like "CreateFunctor(this,
&ChromotingHostTest::SimulateClientConnection, 1)" where pass the result into
Invoke I think.  Do a google.com/codesearch over the chromium sources for
CreateFunctor to see how it's done.  Note that I'm in the middle of replacing
this API with base::Callback, but CreateFunctor should work for now.

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

Done (with CreateFunctor).

+    SimulateClientConnection(1);
+  }
+
+  // Helper method to remove a client connection from ChromotingHost.
   void RemoveClientConnection() {
     context_.network_message_loop()->PostTask(
         FROM_HERE,
@@ -162,6 +185,13 @@ class ChromotingHostTest : public testing::Test {
   MockClientStub client_stub_;
   MockHostStub* host_stub_;
   MockInputStub* input_stub_;
+  scoped_refptr<MockConnectionToClient> connection2_;
+  scoped_refptr<MockSession> session2_;
+  scoped_ptr<SessionConfig> session_config2_;
+  MockVideoStub video_stub2_;
+  MockClientStub client_stub2_;
+  MockHostStub* host_stub2_;
+  MockInputStub* input_stub2_;
 };
 
 TEST_F(ChromotingHostTest, StartAndShutdown) {
@@ -192,7 +222,7 @@ TEST_F(ChromotingHostTest, Connect) {
   EXPECT_CALL(*connection_.get(), Disconnect())
       .RetiresOnSaturation();
 
-  SimulateClientConnection();
+  SimulateClientConnection(0);
   message_loop_.Run();
 }
 
@@ -222,7 +252,7 @@ TEST_F(ChromotingHostTest, Reconnect) {
       .WillOnce(QuitMainMessageLoop(&message_loop_))
       .RetiresOnSaturation();
 
-  SimulateClientConnection();
+  SimulateClientConnection(0);
   message_loop_.Run();
 
   // Connect the client again.
@@ -239,7 +269,48 @@ TEST_F(ChromotingHostTest, Reconnect) {
   EXPECT_CALL(*connection_.get(), Disconnect())
       .RetiresOnSaturation();
 
-  SimulateClientConnection();
+  SimulateClientConnection(0);
+  message_loop_.Run();
+}
+
+TEST_F(ChromotingHostTest, ConnectTwice) {
+  host_->Start(NewRunnableFunction(&PostQuitTask, &message_loop_));
+
+  EXPECT_CALL(client_stub_, BeginSessionResponse(_, _))
+      .Times(1)
+      .WillRepeatedly(RunDoneTask());
+
+  EXPECT_CALL(client_stub2_, BeginSessionResponse(_, _))
+      .Times(1)
+      .WillRepeatedly(RunDoneTask());
+
+  // When a video packet is received we connect the second mock
+  // connection.
+  {
+    InSequence s;

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

I'm always a little dubious when I see InSequences.  I know the pattern was here
already, but as a sanity check anyways, do we really need to guarantee the
sequence of operations to assert the invariants we are testing for in this case?

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

I think it's worth knowing that no video is sent to
connection 1 after any video is sent to connection 2: it's a
(weak) test that connection 1 has been disconnected.

+    EXPECT_CALL(video_stub_, ProcessVideoPacket(_, _))
+        .WillOnce(DoAll(
+            InvokeWithoutArgs(
+                this, &ChromotingHostTest::SimulateSecondClientConnection),
+            RunDoneTask()))
+        .RetiresOnSaturation();
+    EXPECT_CALL(video_stub_, ProcessVideoPacket(_, _))
+        .Times(AnyNumber());
+    EXPECT_CALL(video_stub2_, ProcessVideoPacket(_, _))
+        .WillOnce(DoAll(
+            InvokeWithoutArgs(host_.get(), &ChromotingHost::Shutdown),
+            RunDoneTask()))
+        .RetiresOnSaturation();
+    EXPECT_CALL(video_stub2_, ProcessVideoPacket(_, _))
+        .Times(AnyNumber());
+  }
+
+  EXPECT_CALL(*connection_.get(), Disconnect())
+      .RetiresOnSaturation();
+  EXPECT_CALL(*connection2_.get(), Disconnect())
+      .RetiresOnSaturation();
+
+  SimulateClientConnection(0);
   message_loop_.Run();
 }