Do not switch to a high-cost connection that is not receiving

webrtc/p2p/base/p2ptransportchannel_unittest.cc

Index: webrtc/p2p/base/p2ptransportchannel_unittest.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
index 7cd23daa38d2567dbab7cb16647c197f2ff8c094..18c34d30e6a34840e1fa639b260cfa59e71cf4fd 100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -54,8 +54,8 @@ static const SocketAddress kIPv6PublicAddrs[2] = {
     SocketAddress("2620:0:1000:1b03:2e41:38ff:fea6:f2a4", 0)
 };
 // For configuring multihomed clients.
-static const SocketAddress kAlternateAddrs[2] =
-    { SocketAddress("11.11.11.101", 0), SocketAddress("22.22.22.202", 0) };
+static const SocketAddress kAlternateAddrs[2] = {
+    SocketAddress("101.101.101.101", 0), SocketAddress("202.202.202.202", 0)};
 // Addresses for HTTP proxy servers.
 static const SocketAddress kHttpsProxyAddrs[2] =
     { SocketAddress("11.11.11.1", 443), SocketAddress("22.22.22.1", 443) };
@@ -695,7 +695,11 @@ class P2PTransportChannelTestBase : public testing::Test,
       CandidatePairInterface* selected_candidate_pair,
       int last_sent_packet_id,
       bool ready_to_send) {
-    ++selected_candidate_pair_switches_;
+    // Do not count if it switches to nullptr. This may happen if all
+    // connections timed out.
+    if (selected_candidate_pair != nullptr) {
+      ++selected_candidate_pair_switches_;
+    }
   }
 
   int reset_selected_candidate_pair_switches() {
@@ -1860,7 +1864,7 @@ class P2PTransportChannelMultihomedTest : public P2PTransportChannelTestBase {
     return nullptr;
   }
 
-  const cricket::Connection* GetConnectionWithLocalAddress(
+  cricket::Connection* GetConnectionWithLocalAddress(
       cricket::P2PTransportChannel* channel,
       const SocketAddress& address) {
     for (cricket::Connection* conn : channel->connections()) {
@@ -2180,6 +2184,62 @@ TEST_F(P2PTransportChannelMultihomedTest,
   DestroyChannels();
 }
 
+// Tests that if the remote side's network failed, it won't cause the local
+// side to switch connections and networks.
+TEST_F(P2PTransportChannelMultihomedTest, TestRemoteFailover) {
+  rtc::ScopedFakeClock clock;
+  // Start the clock at 100 seconds. P2PTransportChannel::IsPingable assumes
+  // that the clock time is sufficently large.
+  clock.AdvanceTime(rtc::TimeDelta::FromSeconds(100u));

[Taylor Brandstetter, 2016/08/11 22:54:40]

Can you explain why this is necessary? Can IsPingable be fixed?

[honghaiz3, 2016/08/12 17:04:26]

A backup connection is pinged at a slower rate (which is configurable). This is
controlled by checking the difference between now and the last time ping
response is received. When no ping response is received, the latter is 0. If the
clock time starts from 0, it may take a long time for the backup connection to
get the first chance to be pinged. In this test, we need to make sure the backup
connection become writable first.

It never has this issue with real clock. 

I think it may be good to initialize the FakeClock with the current reading of
the wall clock when it is created. That would have avoided several test issues
we have incurred and that will simulate more practical settings.  

[Taylor Brandstetter, 2016/08/15 17:37:32]

So then the issue is that 0 is treated as a special value meaning "no ping
responses received"? I know this is a corner case issue, but can the logic be
changed so that "no ping responses received" is detected in some other way?

Also, I disagree about initializing the fake clock with a wall clock time. The
fake clock should allow writing fully repeatable tests, so it should use a
repeatable initial time. And if we did want to do time "fuzzing" for a specific
test, it would be better to use fully random values rather than the wall clock
time. The wall-clock time on the VMs we run the tests on could be biased. I
believe this has caused at least one bug to go under the radar in the past; a
64-bit timestamp was being cast to a 32-bit timestamp but the bot didn't catch
this because the timestamp always ended up in a 32-bit value range.

[honghaiz3, 2016/08/16 00:43:07]

Added checking on the "no ping response is received using the rtt_samples(). 
If a 32-bit initial timestamp may be an issue, using 0 as the default initial
timestamp may be a bigger issue. Using the random value might be better than the
wall clock time in the case you described, but we need to choose the range and
distribution reasonably. I am not sure if it is worth the complexity. If using
the call-clock is an issue, it will be an issue with a non-fake clock anyway
because the latter is what we use for all non-fake clocks. So it will be as good
as the real clock.  

Probably we want to allow the test code to initialize the fake clock with a
chosen value. 

+  // The interface names are chosen so that |cellular| would have higher
+  // candidate priority and higher cost.
+  auto& wifi = kPublicAddrs;
+  auto& cellular = kAlternateAddrs;
+  AddAddress(0, wifi[0], "wifi0", rtc::ADAPTER_TYPE_WIFI);
+  AddAddress(0, cellular[0], "cellular0", rtc::ADAPTER_TYPE_CELLULAR);
+  AddAddress(1, wifi[1], "wifi0", rtc::ADAPTER_TYPE_WIFI);
+
+  // Use only local ports for simplicity.
+  SetAllocatorFlags(0, kOnlyLocalPorts);
+  SetAllocatorFlags(1, kOnlyLocalPorts);
+  // Create channels and let them go writable, as usual.
+  CreateChannels(1);
+  // Make the receiving timeout shorter for testing.
+  // Set the backup connection ping interval to 25s.
+  IceConfig config = CreateIceConfig(1000, GATHER_ONCE, 25000);
+  // Ping the best connection more frequently since we don't have traffic.
+  config.stable_writable_connection_ping_interval = 900;
+  ep1_ch1()->SetIceConfig(config);
+  ep2_ch1()->SetIceConfig(config);
+  // Need to wait to make sure the connections on both networks are writable.
+  EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
+                                 ep2_ch1()->receiving() &&
+                                 ep2_ch1()->writable(),
+                             3000, clock);
+  EXPECT_TRUE_SIMULATED_WAIT(
+      ep1_ch1()->selected_connection() &&
+          LocalCandidate(ep1_ch1())->address().EqualIPs(wifi[0]) &&
+          RemoteCandidate(ep1_ch1())->address().EqualIPs(wifi[1]),
+      kDefaultTimeout, clock);
+  Connection* backup_conn =
+      GetConnectionWithLocalAddress(ep1_ch1(), cellular[0]);
+  ASSERT_NE(nullptr, backup_conn);
+  // After a short while, the backup connection will be writable but not
+  // receiving because backup connection is pinged at a slower rate.
+  EXPECT_TRUE_SIMULATED_WAIT(
+      backup_conn->writable() && !backup_conn->receiving(), 5000, clock);
+  reset_selected_candidate_pair_switches();
+  // Blackhole any traffic to or from the remote WiFi networks.
+  LOG(LS_INFO) << "Failing over...";
+  fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, wifi[1]);
+
+  int num_switches = 0;
+  SIMULATED_WAIT((num_switches = reset_selected_candidate_pair_switches()) > 0,
+                 20000, clock);
+  EXPECT_EQ(0, num_switches);
+  DestroyChannels();
+}
+
 // Tests that a Wifi-Wifi connection has the highest precedence.
 TEST_F(P2PTransportChannelMultihomedTest, TestPreferWifiToWifiConnection) {
   // The interface names are chosen so that |cellular| would have higher