Revert of Allow the DTLS fingerprint verification to occur after the handshake.

webrtc/p2p/base/dtlstransportchannel_unittest.cc

Index: webrtc/p2p/base/dtlstransportchannel_unittest.cc
diff --git a/webrtc/p2p/base/dtlstransportchannel_unittest.cc b/webrtc/p2p/base/dtlstransportchannel_unittest.cc
index 119d3249f0ba636d113b02a29bb69d7a784aff97..6eb0f0e3f1aa857461a5e3feecf1451956cc0ef6 100644
--- a/webrtc/p2p/base/dtlstransportchannel_unittest.cc
+++ b/webrtc/p2p/base/dtlstransportchannel_unittest.cc
@@ -81,11 +81,10 @@
     ASSERT(!transport_);
     ssl_max_version_ = version;
   }
-  void SetupChannels(int count, cricket::IceRole role, int async_delay_ms = 0) {
+  void SetupChannels(int count, cricket::IceRole role) {
     transport_.reset(new cricket::DtlsTransport<cricket::FakeTransport>(
         "dtls content name", nullptr, certificate_));
     transport_->SetAsync(true);
-    transport_->SetAsyncDelay(async_delay_ms);
     transport_->SetIceRole(role);
     transport_->SetIceTiebreaker(
         (role == cricket::ICEROLE_CONTROLLING) ? 1 : 2);
@@ -120,11 +119,6 @@
         static_cast<cricket::FakeTransportChannel*>(wrapper->channel()) : NULL;
   }
 
-  cricket::DtlsTransportChannelWrapper* GetDtlsChannel(int component) {
-    cricket::TransportChannelImpl* ch = transport_->GetChannel(component);
-    return static_cast<cricket::DtlsTransportChannelWrapper*>(ch);
-  }
-
   // Offer DTLS if we have an identity; pass in a remote fingerprint only if
   // both sides support DTLS.
   void Negotiate(DtlsTestClient* peer, cricket::ContentAction action,
@@ -158,6 +152,7 @@
     EXPECT_EQ(expect_success,
               transport_->SetLocalTransportDescription(
                   MakeTransportDescription(cert, role), action, nullptr));
+    set_local_cert_ = (cert != nullptr);
   }
 
   void SetRemoteTransportDescription(
@@ -172,6 +167,7 @@
     EXPECT_EQ(expect_success,
               transport_->SetRemoteTransportDescription(
                   MakeTransportDescription(cert, role), action, nullptr));
+    set_remote_cert_ = (cert != nullptr);
   }
 
   // Allow any DTLS configuration to be specified (including invalid ones).
@@ -233,16 +229,7 @@
     return received_dtls_client_hellos_;
   }
 
-  int received_dtls_server_hellos() const {
-    return received_dtls_server_hellos_;
-  }
-
-  bool negotiated_dtls() const {
-    return transport_->local_description() &&
-           transport_->local_description()->identity_fingerprint &&
-           transport_->remote_description() &&
-           transport_->remote_description()->identity_fingerprint;
-  }
+  bool negotiated_dtls() const { return set_local_cert_ && set_remote_cert_; }
 
   void CheckRole(rtc::SSLRole role) {
     if (role == rtc::SSL_CLIENT) {
@@ -424,19 +411,18 @@
   std::set<int> received_;
   bool use_dtls_srtp_ = false;
   rtc::SSLProtocolVersion ssl_max_version_ = rtc::SSL_PROTOCOL_DTLS_12;
+  bool set_local_cert_ = false;
+  bool set_remote_cert_ = false;
   int received_dtls_client_hellos_ = 0;
   int received_dtls_server_hellos_ = 0;
   rtc::SentPacket sent_packet_;
 };
 
-// Base class for DtlsTransportChannelTest and DtlsEventOrderingTest, which
-// inherit from different variants of testing::Test.
-//
 // Note that this test always uses a FakeClock, due to the |fake_clock_| member
 // variable.
-class DtlsTransportChannelTestBase {
+class DtlsTransportChannelTest : public testing::Test {
  public:
-  DtlsTransportChannelTestBase()
+  DtlsTransportChannelTest()
       : client1_("P1"),
         client2_("P2"),
         channel_ct_(1),
@@ -509,9 +495,9 @@
     if (!rv)
       return false;
 
-    EXPECT_TRUE_SIMULATED_WAIT(
+    EXPECT_TRUE_WAIT(
         client1_.all_channels_writable() && client2_.all_channels_writable(),
-        kTimeout, fake_clock_);
+        kTimeout);
     if (!client1_.all_channels_writable() || !client2_.all_channels_writable())
       return false;
 
@@ -602,8 +588,7 @@
     LOG(LS_INFO) << "Expect packets, size=" << size;
     client2_.ExpectPackets(channel, size);
     client1_.SendPackets(channel, size, count, srtp);
-    EXPECT_EQ_SIMULATED_WAIT(count, client2_.NumPacketsReceived(), kTimeout,
-                             fake_clock_);
+    EXPECT_EQ_WAIT(count, client2_.NumPacketsReceived(), kTimeout);
   }
 
  protected:
@@ -615,9 +600,6 @@
   bool use_dtls_srtp_;
   rtc::SSLProtocolVersion ssl_expected_version_;
 };
-
-class DtlsTransportChannelTest : public DtlsTransportChannelTestBase,
-                                 public ::testing::Test {};
 
 // Test that transport negotiation of ICE, no DTLS works properly.
 TEST_F(DtlsTransportChannelTest, TestChannelSetupIce) {
@@ -902,9 +884,9 @@
               cricket::CONNECTIONROLE_ACTIVE, NF_REOFFER);
   bool rv = client1_.Connect(&client2_, false);
   EXPECT_TRUE(rv);
-  EXPECT_TRUE_SIMULATED_WAIT(
+  EXPECT_TRUE_WAIT(
       client1_.all_channels_writable() && client2_.all_channels_writable(),
-      kTimeout, fake_clock_);
+      kTimeout);
 
   TestTransfer(0, 1000, 100, true);
   TestTransfer(1, 1000, 100, true);
@@ -959,6 +941,72 @@
             certificate1->ssl_certificate().ToPEMString());
 }
 
+// Test that DTLS completes promptly if a ClientHello is received before the
+// transport channel is writable (allowing a ServerHello to be sent).
+TEST_F(DtlsTransportChannelTest, TestReceiveClientHelloBeforeWritable) {
+  MAYBE_SKIP_TEST(HaveDtls);
+  PrepareDtls(true, true, rtc::KT_DEFAULT);
+  // Exchange transport descriptions.
+  Negotiate(cricket::CONNECTIONROLE_ACTPASS, cricket::CONNECTIONROLE_ACTIVE);
+
+  // Make client2_ writable, but not client1_.
+  EXPECT_TRUE(client2_.Connect(&client1_, true));
+  EXPECT_TRUE_WAIT(client2_.all_raw_channels_writable(), kTimeout);
+
+  // Expect a DTLS ClientHello to be sent even while client1_ isn't writable.
+  EXPECT_EQ_WAIT(1, client1_.received_dtls_client_hellos(), kTimeout);
+  EXPECT_FALSE(client1_.all_raw_channels_writable());
+
+  // Now make client1_ writable and expect the handshake to complete
+  // without client2_ needing to retransmit the ClientHello.
+  EXPECT_TRUE(client1_.Connect(&client2_, true));
+  EXPECT_TRUE_WAIT(
+      client1_.all_channels_writable() && client2_.all_channels_writable(),
+      kTimeout);
+  EXPECT_EQ(1, client1_.received_dtls_client_hellos());
+}
+
+// Test that DTLS completes promptly if a ClientHello is received before the
+// transport channel has a remote fingerprint (allowing a ServerHello to be
+// sent).
+TEST_F(DtlsTransportChannelTest,
+       TestReceiveClientHelloBeforeRemoteFingerprint) {
+  MAYBE_SKIP_TEST(HaveDtls);
+  PrepareDtls(true, true, rtc::KT_DEFAULT);
+  client1_.SetupChannels(channel_ct_, cricket::ICEROLE_CONTROLLING);
+  client2_.SetupChannels(channel_ct_, cricket::ICEROLE_CONTROLLED);
+
+  // Make client2_ writable and give it local/remote certs, but don't yet give
+  // client1_ a remote fingerprint.
+  client1_.transport()->SetLocalTransportDescription(
+      MakeTransportDescription(client1_.certificate(),
+                               cricket::CONNECTIONROLE_ACTPASS),
+      cricket::CA_OFFER, nullptr);
+  client2_.Negotiate(&client1_, cricket::CA_ANSWER,
+                     cricket::CONNECTIONROLE_ACTIVE,
+                     cricket::CONNECTIONROLE_ACTPASS, 0);
+  EXPECT_TRUE(client2_.Connect(&client1_, true));
+  EXPECT_TRUE_WAIT(client2_.all_raw_channels_writable(), kTimeout);
+
+  // Expect a DTLS ClientHello to be sent even while client1_ doesn't have a
+  // remote fingerprint.
+  EXPECT_EQ_WAIT(1, client1_.received_dtls_client_hellos(), kTimeout);
+  EXPECT_FALSE(client1_.all_raw_channels_writable());
+
+  // Now make give client1_ its remote fingerprint and make it writable, and
+  // expect the handshake to complete without client2_ needing to retransmit
+  // the ClientHello.
+  client1_.transport()->SetRemoteTransportDescription(
+      MakeTransportDescription(client2_.certificate(),
+                               cricket::CONNECTIONROLE_ACTIVE),
+      cricket::CA_ANSWER, nullptr);
+  EXPECT_TRUE(client1_.Connect(&client2_, true));
+  EXPECT_TRUE_WAIT(
+      client1_.all_channels_writable() && client2_.all_channels_writable(),
+      kTimeout);
+  EXPECT_EQ(1, client1_.received_dtls_client_hellos());
+}
+
 // Test that packets are retransmitted according to the expected schedule.
 // Each time a timeout occurs, the retransmission timer should be doubled up to
 // 60 seconds. The timer defaults to 1 second, but for WebRTC we should be
@@ -976,8 +1024,7 @@
   // Make client2_ writable, but not client1_.
   // This means client1_ will send DTLS client hellos but get no response.
   EXPECT_TRUE(client2_.Connect(&client1_, true));
-  EXPECT_TRUE_SIMULATED_WAIT(client2_.all_raw_channels_writable(), kTimeout,
-                             fake_clock_);
+  EXPECT_TRUE_WAIT(client2_.all_raw_channels_writable(), kTimeout);
 
   // Wait for the first client hello to be sent.
   EXPECT_EQ_WAIT(1, client1_.received_dtls_client_hellos(), kTimeout);
@@ -1012,164 +1059,3 @@
                       CONNECT_BEFORE_NEGOTIATE));
   TestTransfer(0, 1000, 100, false);
 }
-
-// The following events can occur in many different orders:
-// 1. Caller receives remote fingerprint.
-// 2. Caller is writable.
-// 3. Caller receives ClientHello.
-// 4. DTLS handshake finishes.
-//
-// The tests below cover all causally consistent permutations of these events;
-// the caller must be writable and receive a ClientHello before the handshake
-// finishes, but otherwise any ordering is possible.
-//
-// For each permutation, the test verifies that a connection is established and
-// fingerprint verified without any DTLS packet needing to be retransmitted.
-//
-// Each permutation is also tested with a valid and invalid fingerprint,
-// ensuring that the handshake fails with an invalid fingerprint.
-enum DtlsTransportEvent {
-  CALLER_RECEIVES_FINGERPRINT,
-  CALLER_WRITABLE,
-  CALLER_RECEIVES_CLIENTHELLO,
-  HANDSHAKE_FINISHES
-};
-
-class DtlsEventOrderingTest
-    : public DtlsTransportChannelTestBase,
-      public ::testing::TestWithParam<
-          ::testing::tuple<std::vector<DtlsTransportEvent>, bool>> {
- protected:
-  // If |valid_fingerprint| is false, the caller will receive a fingerprint
-  // that doesn't match the callee's certificate, so the handshake should fail.
-  void TestEventOrdering(const std::vector<DtlsTransportEvent>& events,
-                         bool valid_fingerprint) {
-    // Pre-setup: Set local certificate on both caller and callee, and
-    // remote fingerprint on callee, but neither is writable and the caller
-    // doesn't have the callee's fingerprint.
-    PrepareDtls(true, true, rtc::KT_DEFAULT);
-    // Simulate packets being sent and arriving asynchronously.
-    // Otherwise the entire DTLS handshake would occur in one clock tick, and
-    // we couldn't inject method calls in the middle of it.
-    int simulated_delay_ms = 10;
-    client1_.SetupChannels(channel_ct_, cricket::ICEROLE_CONTROLLING,
-                           simulated_delay_ms);
-    client2_.SetupChannels(channel_ct_, cricket::ICEROLE_CONTROLLED,
-                           simulated_delay_ms);
-    client1_.SetLocalTransportDescription(client1_.certificate(),
-                                          cricket::CA_OFFER,
-                                          cricket::CONNECTIONROLE_ACTPASS, 0);
-    client2_.Negotiate(&client1_, cricket::CA_ANSWER,
-                       cricket::CONNECTIONROLE_ACTIVE,
-                       cricket::CONNECTIONROLE_ACTPASS, 0);
-
-    for (DtlsTransportEvent e : events) {
-      switch (e) {
-        case CALLER_RECEIVES_FINGERPRINT:
-          if (valid_fingerprint) {
-            client1_.SetRemoteTransportDescription(
-                client2_.certificate(), cricket::CA_ANSWER,
-                cricket::CONNECTIONROLE_ACTIVE, 0);
-          } else {
-            // Create a fingerprint with a correct algorithm but an invalid
-            // digest.
-            cricket::TransportDescription remote_desc =
-                MakeTransportDescription(client2_.certificate(),
-                                         cricket::CONNECTIONROLE_ACTIVE);
-            ++(remote_desc.identity_fingerprint->digest[0]);
-            // Even if certificate verification fails inside this method,
-            // it should return true as long as the fingerprint was formatted
-            // correctly.
-            EXPECT_TRUE(client1_.transport()->SetRemoteTransportDescription(
-                remote_desc, cricket::CA_ANSWER, nullptr));
-          }
-          break;
-        case CALLER_WRITABLE:
-          EXPECT_TRUE(client1_.Connect(&client2_, true));
-          EXPECT_TRUE_SIMULATED_WAIT(client1_.all_raw_channels_writable(),
-                                     kTimeout, fake_clock_);
-          break;
-        case CALLER_RECEIVES_CLIENTHELLO:
-          // Sanity check that a ClientHello hasn't already been received.
-          EXPECT_EQ(0, client1_.received_dtls_client_hellos());
-          // Making client2_ writable will cause it to send the ClientHello.
-          EXPECT_TRUE(client2_.Connect(&client1_, true));
-          EXPECT_TRUE_SIMULATED_WAIT(client2_.all_raw_channels_writable(),
-                                     kTimeout, fake_clock_);
-          EXPECT_EQ_SIMULATED_WAIT(1, client1_.received_dtls_client_hellos(),
-                                   kTimeout, fake_clock_);
-          break;
-        case HANDSHAKE_FINISHES:
-          // Sanity check that the handshake hasn't already finished.
-          EXPECT_FALSE(client1_.GetDtlsChannel(0)->IsDtlsConnected() ||
-                       client1_.GetDtlsChannel(0)->dtls_state() ==
-                           cricket::DTLS_TRANSPORT_FAILED);
-          EXPECT_TRUE_SIMULATED_WAIT(
-              client1_.GetDtlsChannel(0)->IsDtlsConnected() ||
-                  client1_.GetDtlsChannel(0)->dtls_state() ==
-                      cricket::DTLS_TRANSPORT_FAILED,
-              kTimeout, fake_clock_);
-          break;
-      }
-    }
-
-    cricket::DtlsTransportState expected_final_state =
-        valid_fingerprint ? cricket::DTLS_TRANSPORT_CONNECTED
-                          : cricket::DTLS_TRANSPORT_FAILED;
-    EXPECT_EQ_SIMULATED_WAIT(expected_final_state,
-                             client1_.GetDtlsChannel(0)->dtls_state(), kTimeout,
-                             fake_clock_);
-    EXPECT_EQ_SIMULATED_WAIT(expected_final_state,
-                             client2_.GetDtlsChannel(0)->dtls_state(), kTimeout,
-                             fake_clock_);
-
-    // Channel should be writable iff there was a valid fingerprint.
-    EXPECT_EQ(valid_fingerprint, client1_.GetDtlsChannel(0)->writable());
-    EXPECT_EQ(valid_fingerprint, client2_.GetDtlsChannel(0)->writable());
-
-    // Check that no hello needed to be retransmitted.
-    EXPECT_EQ(1, client1_.received_dtls_client_hellos());
-    EXPECT_EQ(1, client2_.received_dtls_server_hellos());
-
-    if (valid_fingerprint) {
-      TestTransfer(0, 1000, 100, false);
-    }
-  }
-};
-
-TEST_P(DtlsEventOrderingTest, TestEventOrdering) {
-  MAYBE_SKIP_TEST(HaveDtls);
-  TestEventOrdering(::testing::get<0>(GetParam()),
-                    ::testing::get<1>(GetParam()));
-}
-
-INSTANTIATE_TEST_CASE_P(
-    TestEventOrdering,
-    DtlsEventOrderingTest,
-    ::testing::Combine(
-        ::testing::Values(
-            std::vector<DtlsTransportEvent>{
-                CALLER_RECEIVES_FINGERPRINT, CALLER_WRITABLE,
-                CALLER_RECEIVES_CLIENTHELLO, HANDSHAKE_FINISHES},
-            std::vector<DtlsTransportEvent>{
-                CALLER_WRITABLE, CALLER_RECEIVES_FINGERPRINT,
-                CALLER_RECEIVES_CLIENTHELLO, HANDSHAKE_FINISHES},
-            std::vector<DtlsTransportEvent>{
-                CALLER_WRITABLE, CALLER_RECEIVES_CLIENTHELLO,
-                CALLER_RECEIVES_FINGERPRINT, HANDSHAKE_FINISHES},
-            std::vector<DtlsTransportEvent>{
-                CALLER_WRITABLE, CALLER_RECEIVES_CLIENTHELLO,
-                HANDSHAKE_FINISHES, CALLER_RECEIVES_FINGERPRINT},
-            std::vector<DtlsTransportEvent>{
-                CALLER_RECEIVES_FINGERPRINT, CALLER_RECEIVES_CLIENTHELLO,
-                CALLER_WRITABLE, HANDSHAKE_FINISHES},
-            std::vector<DtlsTransportEvent>{
-                CALLER_RECEIVES_CLIENTHELLO, CALLER_RECEIVES_FINGERPRINT,
-                CALLER_WRITABLE, HANDSHAKE_FINISHES},
-            std::vector<DtlsTransportEvent>{
-                CALLER_RECEIVES_CLIENTHELLO, CALLER_WRITABLE,
-                CALLER_RECEIVES_FINGERPRINT, HANDSHAKE_FINISHES},
-            std::vector<DtlsTransportEvent>{CALLER_RECEIVES_CLIENTHELLO,
-                                            CALLER_WRITABLE, HANDSHAKE_FINISHES,
-                                            CALLER_RECEIVES_FINGERPRINT}),
-        ::testing::Bool()));