Do not switch a connection if the new connection is not ready to send packets.

webrtc/p2p/base/p2ptransportchannel_unittest.cc

 /*
  *  Copyright 2009 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 839 matching lines @@
   }
 
   void ConnectSignalNominated(Connection* conn) {
     conn->SignalNominated.connect(this,
                                   &P2PTransportChannelTestBase::OnNominated);
   }
 
   void OnNominated(Connection* conn) { nominated_ = true; }
   bool nominated() { return nominated_; }
 
+  Connection* GetTopConnection(P2PTransportChannel* channel) {
+    if (channel->connections().empty()) {
+      return nullptr;
+    }
+    return channel->connections().front();
+  }

[Taylor Brandstetter, 2016/08/08 23:01:05]

I think these unit tests should move away from accessing "connections_"
directly, since it's an implementation detail. Could the tests that depend on
this method be rewritten so they don't need it?

[honghaiz3, 2016/08/11 22:45:53]

Done.

+
  private:
   rtc::Thread* main_;
   std::unique_ptr<rtc::PhysicalSocketServer> pss_;
   std::unique_ptr<rtc::VirtualSocketServer> vss_;
   std::unique_ptr<rtc::NATSocketServer> nss_;
   std::unique_ptr<rtc::FirewallSocketServer> ss_;
   rtc::SocketServerScope ss_scope_;
   std::unique_ptr<TestStunServer> stun_server_;
   TestTurnServer turn_server_;
   TestRelayServer relay_server_;
@@ skipping 295 matching lines @@
   // Emulate no remote credentials coming in.
   set_remote_ice_credential_source(FROM_CANDIDATE);
   CreateChannels(1);
   // Only have remote credentials come in for ep2, not ep1.
   ep2_ch1()->SetRemoteIceCredentials(kIceUfrag[0], kIcePwd[0]);
 
   // Pause sending ep2's candidates to ep1 until ep1 receives the peer reflexive
   // candidate.
   PauseCandidates(1);
 
-  // The caller should have the selected connection connected to the peer
-  // reflexive candidate.
-  const Connection* selected_connection = NULL;
-  WAIT((selected_connection = ep1_ch1()->selected_connection()) != NULL, 2000);
-  EXPECT_EQ("prflx",
-            ep1_ch1()->selected_connection()->remote_candidate().type());
+  // The caller should have the top connection connected to the peer
+  // reflexive candidate. Note that the top connection is not selected yet as
+  // it is not ready to send.
+  const Connection* top_connection = NULL;
+  ASSERT_TRUE_WAIT((top_connection = GetTopConnection(ep1_ch1())) != NULL,

[Taylor Brandstetter, 2016/08/08 23:01:05]

For example, in this test I think we can do the EXPECTS after the connection is
selected (after SetRemoteIceCredentials but before ResumeCandidates). What this
test really wants to verify is that a prflx connection can be used before a
candidate is signaled, and that its remote candidate is replaced once one is
received.

[honghaiz3, 2016/08/11 22:45:53]

Done.

+                   2000);
+  EXPECT_EQ("prflx", top_connection->remote_candidate().type());
 
   // Because we don't have a remote pwd, we don't ping yet.
-  EXPECT_EQ(kIceUfrag[1],
-            ep1_ch1()->selected_connection()->remote_candidate().username());
-  EXPECT_EQ("",
-            ep1_ch1()->selected_connection()->remote_candidate().password());
+  EXPECT_EQ(kIceUfrag[1], top_connection->remote_candidate().username());
+  EXPECT_TRUE(top_connection->remote_candidate().password().empty());
   // Because we don't have ICE credentials yet, we don't know the generation.
-  EXPECT_EQ(0u,
-            ep1_ch1()->selected_connection()->remote_candidate().generation());
+  EXPECT_EQ(0u, top_connection->remote_candidate().generation());
   EXPECT_TRUE(nullptr == ep1_ch1()->FindNextPingableConnection());
 
   // Add two sets of remote ICE credentials, so that the ones used by the
   // candidate will be generation 1 instead of 0.
   ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[3], kIcePwd[3]);
   ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[1], kIcePwd[1]);
   // After setting the remote ICE credentials, the password and generation
   // of the peer reflexive candidate should be updated.
-  EXPECT_EQ(kIcePwd[1],
-            ep1_ch1()->selected_connection()->remote_candidate().password());
-  EXPECT_EQ(1u,
-            ep1_ch1()->selected_connection()->remote_candidate().generation());
+  EXPECT_EQ(kIcePwd[1], top_connection->remote_candidate().password());
+  EXPECT_EQ(1u, top_connection->remote_candidate().generation());

[Taylor Brandstetter, 2016/08/15 17:55:49]

Previously this tested that a prflx candidate's generation would be updated once
ICE credentials are received, allowing it to be replaced by a non-prflx
candidate (there used to be bugs for this flow). If that coverage is being
removed from these tests, could it be added as a separate test?

[honghaiz3, 2016/08/16 17:29:15]

It is more natural to test all these (ufrag, pwd, and generation) at one place
when ice_credentials are updated. And if we want to do the same test as the
original one (basically the client get the stun ping request before getting any
signaling (offer/candidate)), it is best to just peek into the internal  state
(i.e., connection() and TopConnection) as I did in the first revision. And I do
not think it is super-critical to avoid that. WDYT?

[Taylor Brandstetter, 2016/08/16 18:48:31]

I don't think we need to peek at the internal state for this. If we want to wait
until endpoint 1 receives a STUN ping, we can just wait for endpoint 2 to become
writable, or even use GetStats.

[honghaiz3, 2016/08/16 21:11:36]

Done. True if we only need to check the ICE generation after remote ICE
candidate is received. 

   EXPECT_TRUE(nullptr != ep1_ch1()->FindNextPingableConnection());
 
   ResumeCandidates(1);
 
-  WAIT(ep2_ch1()->selected_connection() != NULL, 2000);
-  // Verify ep1's selected connection is updated to use the 'local' candidate.
+  EXPECT_TRUE_WAIT(ep2_ch1()->selected_connection() != NULL, 2000);
+  // Verify ep1's top connection is updated to use the 'local' candidate.
   EXPECT_EQ_WAIT("local",
-                 ep1_ch1()->selected_connection()->remote_candidate().type(),
-                 2000);
-  EXPECT_EQ(selected_connection, ep1_ch1()->selected_connection());
+                 GetTopConnection(ep1_ch1())->remote_candidate().type(), 2000);
+  EXPECT_EQ(top_connection, GetTopConnection(ep1_ch1()));
   DestroyChannels();
 }
 
 // Test that we properly create a connection on a STUN ping from unknown address
 // when the signaling is slow and the end points are behind NAT.
 TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignalingWithNAT) {
   ConfigureEndpoints(OPEN, NAT_SYMMETRIC, kDefaultPortAllocatorFlags,
                      kDefaultPortAllocatorFlags);
   // Emulate no remote credentials coming in.
   set_remote_ice_credential_source(FROM_CANDIDATE);
   CreateChannels(1);
   // Only have remote credentials come in for ep2, not ep1.
   ep2_ch1()->SetRemoteIceCredentials(kIceUfrag[0], kIcePwd[0]);
   // Pause sending ep2's candidates to ep1 until ep1 receives the peer reflexive
   // candidate.
   PauseCandidates(1);
 
-  // The caller should have the selected connection connected to the peer
-  // reflexive candidate.
-  WAIT(ep1_ch1()->selected_connection() != NULL, 2000);
-  EXPECT_EQ("prflx",
-            ep1_ch1()->selected_connection()->remote_candidate().type());
+  // The caller's top connection should be connected to the peer reflexive
+  // candidate.
+  Connection* top_connection = nullptr;
+  ASSERT_TRUE_WAIT((top_connection = GetTopConnection(ep1_ch1())) != nullptr,
+                   2000);
+  EXPECT_EQ("prflx", top_connection->remote_candidate().type());
 
   // Because we don't have a remote pwd, we don't ping yet.
-  EXPECT_EQ(kIceUfrag[1],
-            ep1_ch1()->selected_connection()->remote_candidate().username());
-  EXPECT_EQ("",
-            ep1_ch1()->selected_connection()->remote_candidate().password());
+  EXPECT_EQ(kIceUfrag[1], top_connection->remote_candidate().username());
+  EXPECT_EQ("", top_connection->remote_candidate().password());
   // Because we don't have ICE credentials yet, we don't know the generation.
-  EXPECT_EQ(0u,
-            ep1_ch1()->selected_connection()->remote_candidate().generation());
+  EXPECT_EQ(0u, top_connection->remote_candidate().generation());
   EXPECT_TRUE(nullptr == ep1_ch1()->FindNextPingableConnection());
 
   // Add two sets of remote ICE credentials, so that the ones used by the
   // candidate will be generation 1 instead of 0.
   ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[3], kIcePwd[3]);
   ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[1], kIcePwd[1]);
   // After setting the remote ICE credentials, the password and generation
   // of the peer reflexive candidate should be updated.
-  EXPECT_EQ(kIcePwd[1],
-            ep1_ch1()->selected_connection()->remote_candidate().password());
-  EXPECT_EQ(1u,
-            ep1_ch1()->selected_connection()->remote_candidate().generation());
+  EXPECT_EQ(kIcePwd[1], top_connection->remote_candidate().password());
+  EXPECT_EQ(1u, top_connection->remote_candidate().generation());
 
   ResumeCandidates(1);
 
   const Connection* selected_connection = NULL;
-  WAIT((selected_connection = ep2_ch1()->selected_connection()) != NULL, 2000);
+  EXPECT_TRUE_WAIT(
+      (selected_connection = ep2_ch1()->selected_connection()) != NULL, 2000);
 
   // Wait to verify the connection is not culled.
-  WAIT(ep1_ch1()->writable(), 2000);
-  EXPECT_EQ(ep2_ch1()->selected_connection(), selected_connection);
+  EXPECT_TRUE_WAIT(ep1_ch1()->writable(), 2000);
   EXPECT_EQ("prflx",
             ep1_ch1()->selected_connection()->remote_candidate().type());
+  EXPECT_EQ(selected_connection, ep2_ch1()->selected_connection());
   DestroyChannels();
 }
 
 // Test that we properly create a connection on a STUN ping from unknown address
 // when the signaling is slow, even if the new candidate is created due to the
 // remote peer doing an ICE restart, pairing this candidate across generations.
 //
 // Previously this wasn't working due to a bug where the peer reflexive
 // candidate was only updated for the newest generation candidate pairs, and
 // not older-generation candidate pairs created by pairing candidates across
@@ skipping 1274 matching lines @@
     ch->SignalSelectedCandidatePairChanged.connect(
         this, &P2PTransportChannelPingTest::OnSelectedCandidatePairChanged);
     ch->SignalReadyToSend.connect(this,
                                   &P2PTransportChannelPingTest::OnReadyToSend);
     ch->SignalStateChanged.connect(
         this, &P2PTransportChannelPingTest::OnChannelStateChanged);
   }
 
   Connection* WaitForConnectionTo(P2PTransportChannel* ch,
                                   const std::string& ip,
-                                  int port_num) {
-    EXPECT_TRUE_WAIT(GetConnectionTo(ch, ip, port_num) != nullptr, 3000);
+                                  int port_num,
+                                  rtc::FakeClock* clock = nullptr) {
+    if (clock == nullptr) {
+      EXPECT_TRUE_WAIT(GetConnectionTo(ch, ip, port_num) != nullptr, 3000);
+    } else {
+      EXPECT_TRUE_SIMULATED_WAIT(GetConnectionTo(ch, ip, port_num) != nullptr,
+                                 3000, *clock);
+    }
     return GetConnectionTo(ch, ip, port_num);
   }
 
   Port* GetPort(P2PTransportChannel* ch) {
     if (ch->ports().empty()) {
       return nullptr;
     }
     return static_cast<Port*>(ch->ports()[0]);
   }
 
@@ skipping 464 matching lines @@
   ch.SetIceConfig(CreateIceConfig(500, GATHER_ONCE));
   EXPECT_EQ(500, ch.receiving_timeout());
   EXPECT_EQ(50, ch.check_receiving_interval());
   ch.MaybeStartGathering();
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1));
   Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
   ASSERT_TRUE(conn1 != nullptr);
 
   conn1->ReceivedPing();
   conn1->OnReadPacket("ABC", 3, rtc::CreatePacketTime(0));
-  EXPECT_TRUE_WAIT(ch.selected_connection() != nullptr, 1000);
   EXPECT_TRUE_WAIT(ch.receiving(), 1000);
   EXPECT_TRUE_WAIT(!ch.receiving(), 1000);
 }
 
 // The controlled side will select a connection as the "selected connection"
 // based on priority until the controlling side nominates a connection, at which
 // point the controlled side will select that connection as the
 // "selected connection". Plus, SignalSelectedCandidatePair will be fired if the
 // selected connection changes and SignalReadyToSend will be fired if the new
 // selected connection is writable.
 TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) {
   FakePortAllocator pa(rtc::Thread::Current(), nullptr);
   P2PTransportChannel ch("receiving state change", 1, &pa);
   PrepareChannel(&ch);
   ch.SetIceRole(ICEROLE_CONTROLLED);
   ch.MaybeStartGathering();
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1));
   Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
   ASSERT_TRUE(conn1 != nullptr);
-  EXPECT_EQ(conn1, ch.selected_connection());
-  EXPECT_EQ(conn1, last_selected_candidate_pair());
-  EXPECT_EQ(-1, last_sent_packet_id());
   // Channel is not ready to send because it is not writable.
   EXPECT_FALSE(channel_ready_to_send());
-
   int last_packet_id = 0;
   const char* data = "ABCDEFGH";
   int len = static_cast<int>(strlen(data));
   EXPECT_EQ(-1, SendData(ch, data, len, ++last_packet_id));
+  EXPECT_EQ(-1, last_sent_packet_id());
+
+  // A connection need to be writable before it is selected for transmission.

[Taylor Brandstetter, 2016/08/08 23:01:05]

nit: "needs"

[honghaiz3, 2016/08/11 22:45:53]

Done.

+  conn1->ReceivedPingResponse(LOW_RTT, "id");
+  EXPECT_EQ_WAIT(conn1, ch.selected_connection(), kDefaultTimeout);
+  EXPECT_EQ(conn1, last_selected_candidate_pair());
+  EXPECT_EQ(len, SendData(ch, data, len, ++last_packet_id));
+
   // When a higher priority candidate comes in, the new connection is chosen
   // as the selected connection.
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 10));
   Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
   ASSERT_TRUE(conn2 != nullptr);
-  EXPECT_EQ(conn2, ch.selected_connection());
+  conn2->ReceivedPingResponse(LOW_RTT, "id");
+  EXPECT_EQ_WAIT(conn2, ch.selected_connection(), kDefaultTimeout);
   EXPECT_EQ(conn2, last_selected_candidate_pair());
-  EXPECT_EQ(-1, last_sent_packet_id());
-  EXPECT_FALSE(channel_ready_to_send());
+  EXPECT_TRUE(channel_ready_to_send());
+  EXPECT_EQ(last_packet_id, last_sent_packet_id());
 
   // If a stun request with use-candidate attribute arrives, the receiving
   // connection will be set as the selected connection, even though
   // its priority is lower.
-  EXPECT_EQ(-1, SendData(ch, data, len, ++last_packet_id));
+  EXPECT_EQ(len, SendData(ch, data, len, ++last_packet_id));
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "3.3.3.3", 3, 1));
   Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3);
   ASSERT_TRUE(conn3 != nullptr);
   // Because it has a lower priority, the selected connection is still conn2.
   EXPECT_EQ(conn2, ch.selected_connection());
   conn3->ReceivedPingResponse(LOW_RTT, "id");  // Become writable.
   // But if it is nominated via use_candidate, it is chosen as the selected
   // connection.
   NominateConnection(conn3);
   EXPECT_EQ(conn3, ch.selected_connection());
   EXPECT_EQ(conn3, last_selected_candidate_pair());
-  EXPECT_EQ(-1, last_sent_packet_id());
+  EXPECT_EQ(last_packet_id, last_sent_packet_id());
   EXPECT_TRUE(channel_ready_to_send());
 
   // Even if another higher priority candidate arrives, it will not be set as
   // the selected connection because the selected connection is nominated by
   // the controlling side.
   EXPECT_EQ(len, SendData(ch, data, len, ++last_packet_id));
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "4.4.4.4", 4, 100));
   Connection* conn4 = WaitForConnectionTo(&ch, "4.4.4.4", 4);
   ASSERT_TRUE(conn4 != nullptr);
   EXPECT_EQ(conn3, ch.selected_connection());
@@ skipping 31 matching lines @@
       new StunByteStringAttribute(STUN_ATTR_USERNAME, kIceUfrag[1]));
   uint32_t prflx_priority = ICE_TYPE_PREFERENCE_PRFLX << 24;
   request.AddAttribute(
       new StunUInt32Attribute(STUN_ATTR_PRIORITY, prflx_priority));
   TestUDPPort* port = static_cast<TestUDPPort*>(GetPort(&ch));
   port->SignalUnknownAddress(port, rtc::SocketAddress("1.1.1.1", 1), PROTO_UDP,
                              &request, kIceUfrag[1], false);
   Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
   ASSERT_TRUE(conn1 != nullptr);
   EXPECT_TRUE(port->sent_binding_response());
-  EXPECT_EQ(conn1, ch.selected_connection());
+  EXPECT_NE(conn1, ch.selected_connection());
   conn1->ReceivedPingResponse(LOW_RTT, "id");
-  EXPECT_EQ(conn1, ch.selected_connection());
+  EXPECT_EQ_WAIT(conn1, ch.selected_connection(), kDefaultTimeout);
   port->set_sent_binding_response(false);
 
   // Another connection is nominated via use_candidate.
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 1));
   Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
   ASSERT_TRUE(conn2 != nullptr);
   // Because it has a lower priority, the selected connection is still conn1.
   EXPECT_EQ(conn1, ch.selected_connection());
   // When it is nominated via use_candidate and writable, it is chosen as the
   // selected connection.
@@ skipping 45 matching lines @@
 // the "selected connection".
 TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBasedOnMediaReceived) {
   FakePortAllocator pa(rtc::Thread::Current(), nullptr);
   P2PTransportChannel ch("receiving state change", 1, &pa);
   PrepareChannel(&ch);
   ch.SetIceRole(ICEROLE_CONTROLLED);
   ch.MaybeStartGathering();
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 10));
   Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
   ASSERT_TRUE(conn1 != nullptr);
-  EXPECT_EQ(conn1, ch.selected_connection());
+  conn1->ReceivedPingResponse(LOW_RTT, "id");
+  EXPECT_EQ_WAIT(conn1, ch.selected_connection(), kDefaultTimeout);
 
   // If a data packet is received on conn2, the selected connection should
   // switch to conn2 because the controlled side must mirror the media path
   // chosen by the controlling side.
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 1));
   Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
   ASSERT_TRUE(conn2 != nullptr);
-  conn2->ReceivedPing();  // Start receiving.
+  conn2->ReceivedPingResponse(LOW_RTT, "id");  // Become writable and receiving.
   conn2->OnReadPacket("ABC", 3, rtc::CreatePacketTime(0));
   EXPECT_EQ(conn2, ch.selected_connection());
   conn2->ReceivedPingResponse(LOW_RTT, "id");  // Become writable.
 
   // Now another STUN message with an unknown address and use_candidate will
   // nominate the selected connection.
   IceMessage request;
   request.SetType(STUN_BINDING_REQUEST);
   request.AddAttribute(
       new StunByteStringAttribute(STUN_ATTR_USERNAME, kIceUfrag[1]));
@@ skipping 61 matching lines @@
        TestControlledAgentNominationTakesHigherPrecedenceThanDataReceiving) {
   rtc::ScopedFakeClock clock;
 
   FakePortAllocator pa(rtc::Thread::Current(), nullptr);
   P2PTransportChannel ch("SwitchSelectedConnection", 1, &pa);
   PrepareChannel(&ch);
   ch.SetIceRole(ICEROLE_CONTROLLED);
   ch.MaybeStartGathering();
   // The connections have decreasing priority.
   Connection* conn1 =
-      CreateConnectionWithCandidate(ch, clock, "1.1.1.1", 1, 10, false);
+      CreateConnectionWithCandidate(ch, clock, "1.1.1.1", 1, 10, true);
   ASSERT_TRUE(conn1 != nullptr);
   Connection* conn2 =
-      CreateConnectionWithCandidate(ch, clock, "2.2.2.2", 2, 9, false);
+      CreateConnectionWithCandidate(ch, clock, "2.2.2.2", 2, 9, true);
   ASSERT_TRUE(conn2 != nullptr);
 
   // conn1 received data; it is the selected connection.
   // Advance the clock to have a non-zero last-data-receiving time.
   SIMULATED_WAIT(false, 1, clock);
   conn1->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0));
   EXPECT_EQ(1, reset_selected_candidate_pair_switches());
   EXPECT_EQ(conn1, last_selected_candidate_pair());
 
   // conn2 is nominated; it becomes the selected connection.
   NominateConnection(conn2);
   EXPECT_EQ(1, reset_selected_candidate_pair_switches());
   EXPECT_EQ(conn2, last_selected_candidate_pair());
 
+  // conn1 is selected because it has higher priority and also nominated.
   NominateConnection(conn1);
   EXPECT_EQ(1, reset_selected_candidate_pair_switches());
   EXPECT_EQ(conn1, last_selected_candidate_pair());
 
-  // conn2 received data more recently; it is selected now because it
-  // received data more recently.
-  SIMULATED_WAIT(false, 1, clock);
-  conn2->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0));
-  EXPECT_EQ(1, reset_selected_candidate_pair_switches());
-  EXPECT_EQ(conn2, last_selected_candidate_pair());

[Taylor Brandstetter, 2016/08/08 23:01:05]

Why were these expectations removed?

[honghaiz3, 2016/08/11 22:45:53]

I thought it is kind of redundant with the previous test because both check that
Data receiving has higher precedence than priority. 
But it has slight difference. In this test, both connections are nominated while
they are not in the previous one. 
So I put it back (with a little fix). 

-
   // Make sure sorting won't reselect candidate pair.
   SIMULATED_WAIT(false, 10, clock);
   EXPECT_EQ(0, reset_selected_candidate_pair_switches());
 }
 
 TEST_F(P2PTransportChannelPingTest,
        TestControlledAgentSelectsConnectionWithHigherNomination) {
   rtc::ScopedFakeClock clock;
 
   FakePortAllocator pa(rtc::Thread::Current(), nullptr);
   P2PTransportChannel ch("test", 1, &pa);
   PrepareChannel(&ch);
   ch.SetIceRole(ICEROLE_CONTROLLED);
   ch.MaybeStartGathering();
   // The connections have decreasing priority.
   Connection* conn1 =
-      CreateConnectionWithCandidate(ch, clock, "1.1.1.1", 1, 10, false);
+      CreateConnectionWithCandidate(ch, clock, "1.1.1.1", 1, 10, true);
   ASSERT_TRUE(conn1 != nullptr);
   Connection* conn2 =
-      CreateConnectionWithCandidate(ch, clock, "2.2.2.2", 2, 9, false);
+      CreateConnectionWithCandidate(ch, clock, "2.2.2.2", 2, 9, true);
   ASSERT_TRUE(conn2 != nullptr);
 
   // conn1 is the selected connection because it has a higher priority,
   EXPECT_EQ_SIMULATED_WAIT(conn1, last_selected_candidate_pair(),
                            kDefaultTimeout, clock);
   reset_selected_candidate_pair_switches();
 
   // conn2 is nominated; it becomes selected.
   NominateConnection(conn2);
   EXPECT_EQ(1, reset_selected_candidate_pair_switches());
@@ skipping 42 matching lines @@
   ch.MaybeStartGathering();
   // The connections have decreasing priority.
   Connection* conn1 =
       CreateConnectionWithCandidate(ch, clock, "1.1.1.1", 1, 10, false);
   ASSERT_TRUE(conn1 != nullptr);
   Connection* conn2 =
       CreateConnectionWithCandidate(ch, clock, "2.2.2.2", 2, 9, false);
   ASSERT_TRUE(conn2 != nullptr);
 
   NominateConnection(conn1);
-  EXPECT_EQ(1, reset_selected_candidate_pair_switches());
+  // There is no selected connection because no connection is writable.
+  EXPECT_EQ(0, reset_selected_candidate_pair_switches());
 
   // conn2 becomes writable; it is selected even though it is not nominated.
   conn2->ReceivedPingResponse(LOW_RTT, "id");
-
   EXPECT_EQ_SIMULATED_WAIT(1, reset_selected_candidate_pair_switches(),
                            kDefaultTimeout, clock);
   EXPECT_EQ_SIMULATED_WAIT(conn2, last_selected_candidate_pair(),
                            kDefaultTimeout, clock);
 
   // If conn1 is also writable, it will become selected.
   conn1->ReceivedPingResponse(LOW_RTT, "id");
   EXPECT_EQ_SIMULATED_WAIT(1, reset_selected_candidate_pair_switches(),
                            kDefaultTimeout, clock);
   EXPECT_EQ_SIMULATED_WAIT(conn1, last_selected_candidate_pair(),
@@ skipping 38 matching lines @@
   // Verify that a ping with the new ufrag can be received on the new
   // connection.
   EXPECT_EQ(0, conn2->last_ping_received());
   ReceivePingOnConnection(conn2, kIceUfrag[2], 1 /* priority */);
   EXPECT_TRUE(conn2->last_ping_received() > 0);
 }
 
 // When the current selected connection is strong, lower-priority connections
 // will be pruned. Otherwise, lower-priority connections are kept.
 TEST_F(P2PTransportChannelPingTest, TestDontPruneWhenWeak) {
+  rtc::ScopedFakeClock clock;
   FakePortAllocator pa(rtc::Thread::Current(), nullptr);
   P2PTransportChannel ch("test channel", 1, &pa);
   PrepareChannel(&ch);
   ch.SetIceRole(ICEROLE_CONTROLLED);
   ch.MaybeStartGathering();
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1));
   Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
   ASSERT_TRUE(conn1 != nullptr);
-  EXPECT_EQ(conn1, ch.selected_connection());
+  EXPECT_EQ(nullptr, ch.selected_connection());
   conn1->ReceivedPingResponse(LOW_RTT, "id");  // Becomes writable and receiving
 
   // When a higher-priority, nominated candidate comes in, the connections with
   // lower-priority are pruned.
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 10));
-  Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
+  Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2, &clock);
   ASSERT_TRUE(conn2 != nullptr);
   conn2->ReceivedPingResponse(LOW_RTT, "id");  // Becomes writable and receiving
   NominateConnection(conn2);
-  EXPECT_TRUE_WAIT(conn1->pruned(), 3000);
+  EXPECT_TRUE_SIMULATED_WAIT(conn1->pruned(), 3000, clock);
 
   ch.SetIceConfig(CreateIceConfig(500, GATHER_ONCE));
   // Wait until conn2 becomes not receiving.
-  EXPECT_TRUE_WAIT(!conn2->receiving(), 3000);
+  EXPECT_TRUE_SIMULATED_WAIT(!conn2->receiving(), 3000, clock);
 
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "3.3.3.3", 3, 1));
-  Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3);
+  Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3, &clock);
   ASSERT_TRUE(conn3 != nullptr);
   // The selected connection should still be conn2. Even through conn3 has lower
   // priority and is not receiving/writable, it is not pruned because the
   // selected connection is not receiving.
-  WAIT(conn3->pruned(), 1000);
+  SIMULATED_WAIT(conn3->pruned(), 1000, clock);
   EXPECT_FALSE(conn3->pruned());
 }
 
 TEST_F(P2PTransportChannelPingTest, TestDontPruneHighPriorityConnections) {
   rtc::ScopedFakeClock clock;
   FakePortAllocator pa(rtc::Thread::Current(), nullptr);
   P2PTransportChannel ch("test channel", 1, &pa);
   PrepareChannel(&ch);
   ch.SetIceRole(ICEROLE_CONTROLLED);
   ch.MaybeStartGathering();
@@ skipping 32 matching lines @@
   EXPECT_TRUE_WAIT(conn2->pruned(), 1000);
   EXPECT_EQ(TransportChannelState::STATE_COMPLETED, ch.GetState());
   conn1->Prune();  // All connections are pruned.
   // Need to wait until the channel state is updated.
   EXPECT_EQ_WAIT(TransportChannelState::STATE_FAILED, ch.GetState(), 1000);
 }
 
 // Test that when a low-priority connection is pruned, it is not deleted
 // right away, and it can become active and be pruned again.
 TEST_F(P2PTransportChannelPingTest, TestConnectionPrunedAgain) {
+  rtc::ScopedFakeClock clock;
   FakePortAllocator pa(rtc::Thread::Current(), nullptr);
   P2PTransportChannel ch("test channel", 1, &pa);
   PrepareChannel(&ch);
   IceConfig config = CreateIceConfig(1000, GATHER_ONCE);
   config.receiving_switching_delay = rtc::Optional<int>(800);
   ch.SetIceConfig(config);
   ch.MaybeStartGathering();
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 100));
-  Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
+  Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1, &clock);
   ASSERT_TRUE(conn1 != nullptr);
-  EXPECT_EQ(conn1, ch.selected_connection());
+  EXPECT_EQ(nullptr, ch.selected_connection());
   conn1->ReceivedPingResponse(LOW_RTT, "id");  // Becomes writable and receiving
+  EXPECT_EQ_SIMULATED_WAIT(conn1, ch.selected_connection(), kDefaultTimeout,
+                           clock);
 
   // Add a low-priority connection |conn2|, which will be pruned, but it will
   // not be deleted right away. Once the current selected connection becomes not
   // receiving, |conn2| will start to ping and upon receiving the ping response,
   // it will become the selected connection.
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 1));
-  Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
+  Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2, &clock);
   ASSERT_TRUE(conn2 != nullptr);
-  EXPECT_TRUE_WAIT(!conn2->active(), 1000);
+  EXPECT_TRUE_SIMULATED_WAIT(!conn2->active(), kDefaultTimeout, clock);
   // |conn2| should not send a ping yet.
   EXPECT_EQ(Connection::STATE_WAITING, conn2->state());
   EXPECT_EQ(TransportChannelState::STATE_COMPLETED, ch.GetState());
   // Wait for |conn1| becoming not receiving.
-  EXPECT_TRUE_WAIT(!conn1->receiving(), 3000);
+  EXPECT_TRUE_SIMULATED_WAIT(!conn1->receiving(), 3000, clock);
   // Make sure conn2 is not deleted.
-  conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
+  conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2, &clock);
   ASSERT_TRUE(conn2 != nullptr);
-  EXPECT_EQ_WAIT(Connection::STATE_INPROGRESS, conn2->state(), 1000);
+  EXPECT_EQ_SIMULATED_WAIT(Connection::STATE_INPROGRESS, conn2->state(),
+                           kDefaultTimeout, clock);
   conn2->ReceivedPingResponse(LOW_RTT, "id");
-  EXPECT_EQ_WAIT(conn2, ch.selected_connection(), 1000);
+  EXPECT_EQ_SIMULATED_WAIT(conn2, ch.selected_connection(), kDefaultTimeout,
+                           clock);
   EXPECT_EQ(TransportChannelState::STATE_CONNECTING, ch.GetState());
 
   // When |conn1| comes back again, |conn2| will be pruned again.
   conn1->ReceivedPingResponse(LOW_RTT, "id");
-  EXPECT_EQ_WAIT(conn1, ch.selected_connection(), 1000);
-  EXPECT_TRUE_WAIT(!conn2->active(), 1000);
+  EXPECT_EQ_SIMULATED_WAIT(conn1, ch.selected_connection(), kDefaultTimeout,
+                           clock);
+  EXPECT_TRUE_SIMULATED_WAIT(!conn2->active(), kDefaultTimeout, clock);
   EXPECT_EQ(TransportChannelState::STATE_COMPLETED, ch.GetState());
 }
 
 // Test that if all connections in a channel has timed out on writing, they
 // will all be deleted. We use Prune to simulate write_time_out.
 TEST_F(P2PTransportChannelPingTest, TestDeleteConnectionsIfAllWriteTimedout) {
   FakePortAllocator pa(rtc::Thread::Current(), nullptr);
   P2PTransportChannel ch("test channel", 1, &pa);
   PrepareChannel(&ch);
   ch.MaybeStartGathering();
@@ skipping 338 matching lines @@
 
   // TCP Relay/Relay is the next.
   VerifyNextPingableConnection(RELAY_PORT_TYPE, RELAY_PORT_TYPE,
                                TCP_PROTOCOL_NAME);
 
   // Finally, Local/Relay will be pinged.
   VerifyNextPingableConnection(LOCAL_PORT_TYPE, RELAY_PORT_TYPE);
 }
 
 }  // namespace cricket {