Bugfixes in JingleSession

remoting/protocol/jingle_session_unittest.cc

Index: remoting/protocol/jingle_session_unittest.cc
diff --git a/remoting/protocol/jingle_session_unittest.cc b/remoting/protocol/jingle_session_unittest.cc
index e47413fb612966ea09e3cdb85edf91d8bc784075..add05adf74c4613d585825e178d271f883c744ad 100644
--- a/remoting/protocol/jingle_session_unittest.cc
+++ b/remoting/protocol/jingle_session_unittest.cc
@@ -68,6 +68,19 @@ class MockSessionEventHandler : public Session::EventHandler {
 
 class FakeTransport : public Transport {
  public:
+  SendTransportInfoCallback send_transport_info_callback() {
+    return send_transport_info_callback_;
+  }
+
+  const std::vector<std::unique_ptr<buzz::XmlElement>>& received_messages() {
+    return received_messages_;
+  }
+
+  void set_on_message_callback(const base::Closure& on_message_callback) {
+    on_message_callback_ = on_message_callback;
+  }
+
+  // Transport interface.
   void Start(Authenticator* authenticator,
              SendTransportInfoCallback send_transport_info_callback) override {
     send_transport_info_callback_ = send_transport_info_callback;
@@ -76,20 +89,15 @@ class FakeTransport : public Transport {
   bool ProcessTransportInfo(buzz::XmlElement* transport_info) override {
     received_messages_.push_back(
         base::MakeUnique<buzz::XmlElement>(*transport_info));
+    if (!on_message_callback_.is_null())
+      on_message_callback_.Run();
     return true;
   }
 
-  SendTransportInfoCallback send_transport_info_callback() {
-    return send_transport_info_callback_;
-  }
-
-  const std::vector<std::unique_ptr<buzz::XmlElement>>& received_messages() {
-    return received_messages_;
-  }
-
  private:
   SendTransportInfoCallback send_transport_info_callback_;
   std::vector<std::unique_ptr<buzz::XmlElement>> received_messages_;
+  base::Closure on_message_callback_;
 };
 
 std::unique_ptr<buzz::XmlElement> CreateTransportInfo(const std::string& id) {
@@ -117,9 +125,9 @@ class JingleSessionTest : public testing::Test {
     host_session_->SetTransport(&host_transport_);
   }
 
-  void DeleteSession() {
-    host_session_.reset();
-  }
+  void DeleteHostSession() { host_session_.reset(); }
+
+  void DeleteClientSession() { client_session_.reset(); }
 
  protected:
   void TearDown() override {
@@ -166,13 +174,11 @@ class JingleSessionTest : public testing::Test {
     client_signal_strategy_.reset();
   }
 
-  void InitiateConnection(int auth_round_trips,
-                          FakeAuthenticator::Action auth_action,
-                          bool expect_fail) {
+  void SetHostExpectation(bool expect_fail) {
     EXPECT_CALL(host_server_listener_, OnIncomingSession(_, _))
-        .WillOnce(DoAll(
-            WithArg<0>(Invoke(this, &JingleSessionTest::SetHostSession)),
-            SetArgumentPointee<1>(protocol::SessionManager::ACCEPT)));
+        .WillOnce(
+            DoAll(WithArg<0>(Invoke(this, &JingleSessionTest::SetHostSession)),
+                  SetArgumentPointee<1>(protocol::SessionManager::ACCEPT)));
 
     {
       InSequence dummy;
@@ -197,42 +203,49 @@ class JingleSessionTest : public testing::Test {
             .Times(AtMost(1));
       }
     }
+  }
 
-    {
-      InSequence dummy;
+  void SetClientExpectation(bool expect_fail) {
+    InSequence dummy;
 
+    EXPECT_CALL(client_session_event_handler_,
+                OnSessionStateChange(Session::ACCEPTED))
+        .Times(AtMost(1));
+    EXPECT_CALL(client_session_event_handler_,
+                OnSessionStateChange(Session::AUTHENTICATING))
+        .Times(AtMost(1));
+    if (expect_fail) {
       EXPECT_CALL(client_session_event_handler_,
-                  OnSessionStateChange(Session::ACCEPTED))
-          .Times(AtMost(1));
+                  OnSessionStateChange(Session::FAILED))
+          .Times(1);
+    } else {
       EXPECT_CALL(client_session_event_handler_,
-                  OnSessionStateChange(Session::AUTHENTICATING))
+                  OnSessionStateChange(Session::AUTHENTICATED))
+          .Times(1);
+      // Expect that the connection will be closed eventually.
+      EXPECT_CALL(client_session_event_handler_,
+                  OnSessionStateChange(Session::CLOSED))
           .Times(AtMost(1));
-      if (expect_fail) {
-        EXPECT_CALL(client_session_event_handler_,
-                    OnSessionStateChange(Session::FAILED))
-            .Times(1);
-      } else {
-        EXPECT_CALL(client_session_event_handler_,
-                    OnSessionStateChange(Session::AUTHENTICATED))
-            .Times(1);
-        // Expect that the connection will be closed eventually.
-        EXPECT_CALL(client_session_event_handler_,
-                    OnSessionStateChange(Session::CLOSED))
-            .Times(AtMost(1));
-      }
     }
+  }
 
-    std::unique_ptr<Authenticator> authenticator(new FakeAuthenticator(
-        FakeAuthenticator::CLIENT, auth_round_trips, auth_action, true));
-
+  void ConnectClient(std::unique_ptr<Authenticator> authenticator) {
     client_session_ =
         client_server_->Connect(kHostJid, std::move(authenticator));
     client_session_->SetEventHandler(&client_session_event_handler_);
     client_session_->SetTransport(&client_transport_);
-
     base::RunLoop().RunUntilIdle();
   }
 
+  void InitiateConnection(int auth_round_trips,
+                          FakeAuthenticator::Action auth_action,
+                          bool expect_fail) {
+    SetHostExpectation(expect_fail);
+    SetClientExpectation(expect_fail);
+    ConnectClient(base::MakeUnique<FakeAuthenticator>(
+        FakeAuthenticator::CLIENT, auth_round_trips, auth_action, true));
+  }
+
   void ExpectRouteChange(const std::string& channel_name) {
     EXPECT_CALL(host_session_event_handler_,
                 OnSessionRouteChange(channel_name, _))
@@ -314,16 +327,40 @@ TEST_F(JingleSessionTest, ConnectWithMultistep) {
   InitiateConnection(3, FakeAuthenticator::ACCEPT, false);
 }
 
-TEST_F(JingleSessionTest, ConnectWithOutofOrderIqs) {
+TEST_F(JingleSessionTest, ConnectWithOutOfOrderIqs) {
   CreateSessionManagers(1, FakeAuthenticator::ACCEPT);
   InitiateConnection(1, FakeAuthenticator::ACCEPT, false);
   client_signal_strategy_->SimulatePackgeReordering();
+
   // Verify that out of order transport messages are received correctly.
   host_transport_.send_transport_info_callback().Run(CreateTransportInfo("1"));
   host_transport_.send_transport_info_callback().Run(CreateTransportInfo("2"));
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(client_transport_.received_messages()[0]->Attr(buzz::QN_ID), "1");
-  EXPECT_EQ(client_transport_.received_messages()[1]->Attr(buzz::QN_ID), "2");
+
+  ASSERT_EQ(client_transport_.received_messages().size(), 2U);
+  EXPECT_EQ("1", client_transport_.received_messages()[0]->Attr(buzz::QN_ID));
+  EXPECT_EQ("2", client_transport_.received_messages()[1]->Attr(buzz::QN_ID));
+}
+
+// Verify that out-of-order messages are handled correctly when the session is
+// torn down after the first message.
+TEST_F(JingleSessionTest, ConnectWithOutOfOrderIqsDestroyOnFirstMessage) {
+  CreateSessionManagers(1, FakeAuthenticator::ACCEPT);
+  InitiateConnection(1, FakeAuthenticator::ACCEPT, false);
+  client_signal_strategy_->SimulatePackgeReordering();
+
+  // Verify that out of order transport messages are received correctly.
+  host_transport_.send_transport_info_callback().Run(CreateTransportInfo("1"));
+  host_transport_.send_transport_info_callback().Run(CreateTransportInfo("2"));
+
+  // Destroy the session as soon as the first message is received.
+  client_transport_.set_on_message_callback(base::Bind(
+      &JingleSessionTest::DeleteClientSession, base::Unretained(this)));
+
+  base::RunLoop().RunUntilIdle();
+
+  ASSERT_EQ(client_transport_.received_messages().size(), 1U);
+  EXPECT_EQ("1", client_transport_.received_messages()[0]->Attr(buzz::QN_ID));
 }
 
 // Verify that connection is terminated when single-step auth fails.
@@ -404,8 +441,8 @@ TEST_F(JingleSessionTest, DeleteSessionOnIncomingConnection) {
       .Times(AtMost(1));
 
   EXPECT_CALL(host_session_event_handler_,
-      OnSessionStateChange(Session::AUTHENTICATING))
-      .WillOnce(InvokeWithoutArgs(this, &JingleSessionTest::DeleteSession));
+              OnSessionStateChange(Session::AUTHENTICATING))
+      .WillOnce(InvokeWithoutArgs(this, &JingleSessionTest::DeleteHostSession));
 
   std::unique_ptr<Authenticator> authenticator(new FakeAuthenticator(
       FakeAuthenticator::CLIENT, 3, FakeAuthenticator::ACCEPT, true));
@@ -422,17 +459,17 @@ TEST_F(JingleSessionTest, DeleteSessionOnAuth) {
   CreateSessionManagers(3, 2, FakeAuthenticator::ACCEPT);
 
   EXPECT_CALL(host_server_listener_, OnIncomingSession(_, _))
-      .WillOnce(DoAll(
-          WithArg<0>(Invoke(this, &JingleSessionTest::SetHostSession)),
-          SetArgumentPointee<1>(protocol::SessionManager::ACCEPT)));
+      .WillOnce(
+          DoAll(WithArg<0>(Invoke(this, &JingleSessionTest::SetHostSession)),
+                SetArgumentPointee<1>(protocol::SessionManager::ACCEPT)));
 
   EXPECT_CALL(host_session_event_handler_,
       OnSessionStateChange(Session::ACCEPTED))
       .Times(AtMost(1));
 
   EXPECT_CALL(host_session_event_handler_,
-      OnSessionStateChange(Session::AUTHENTICATING))
-      .WillOnce(InvokeWithoutArgs(this, &JingleSessionTest::DeleteSession));
+              OnSessionStateChange(Session::AUTHENTICATING))
+      .WillOnce(InvokeWithoutArgs(this, &JingleSessionTest::DeleteHostSession));
 
   std::unique_ptr<Authenticator> authenticator(new FakeAuthenticator(
       FakeAuthenticator::CLIENT, 3, FakeAuthenticator::ACCEPT, true));
@@ -448,5 +485,52 @@ TEST_F(JingleSessionTest, TestMultistepAuth) {
       InitiateConnection(3, FakeAuthenticator::ACCEPT, false));
 }
 
+// Verify that incoming transport-info messages are handled correctly while in
+// AUTHENTICATING state.
+TEST_F(JingleSessionTest, TransportInfoDuringAuthentication) {
+  CreateSessionManagers(2, FakeAuthenticator::ACCEPT);
+
+  SetHostExpectation(false);
+  {
+    InSequence dummy;
+
+    EXPECT_CALL(client_session_event_handler_,
+                OnSessionStateChange(Session::ACCEPTED))
+        .Times(AtMost(1));
+    EXPECT_CALL(client_session_event_handler_,
+                OnSessionStateChange(Session::AUTHENTICATING))
+        .Times(AtMost(1));
+  }
+
+  // Create connection and pause it before authentication is finished.
+  FakeAuthenticator* authenticator = new FakeAuthenticator(
+      FakeAuthenticator::CLIENT, 2, FakeAuthenticator::ACCEPT, true);
+  authenticator->set_pause_message_index(4);
+  ConnectClient(base::WrapUnique(authenticator));
+
+  // Send 2 transport messages.
+  host_transport_.send_transport_info_callback().Run(CreateTransportInfo("1"));
+  host_transport_.send_transport_info_callback().Run(CreateTransportInfo("2"));
+
+  base::RunLoop().RunUntilIdle();
+
+  // The transport-info messages should not be received here because
+  // authentication hasn't finished.
+  EXPECT_TRUE(client_transport_.received_messages().empty());
+
+  // Destroy the session as soon as the first message is received.
+  client_transport_.set_on_message_callback(base::Bind(
+      &JingleSessionTest::DeleteClientSession, base::Unretained(this)));
+
+  // Resume authentication.
+  authenticator->Resume();
+  base::RunLoop().RunUntilIdle();
+
+  // Verify that transport-info that the first transport-info message was
+  // received.
+  ASSERT_EQ(client_transport_.received_messages().size(), 1U);
+  EXPECT_EQ("1", client_transport_.received_messages()[0]->Attr(buzz::QN_ID));
+}
+
 }  // namespace protocol
 }  // namespace remoting