Use continual gathering to restore backup connections

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 cbf5e76a7ae23a13376e6f8aa76ad10559978fbe..5c59e690a5c8b369208b851b55ffb3749784e81f 100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -388,6 +388,7 @@ class P2PTransportChannelTestBase : public testing::Test,
   }
   void RemoveAddress(int endpoint, const SocketAddress& addr) {
     GetEndpoint(endpoint)->network_manager_.RemoveInterface(addr);
+    fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, addr);
   }
   void SetProxy(int endpoint, rtc::ProxyType type) {
     rtc::ProxyInfo info;
@@ -1584,6 +1585,37 @@ TEST_F(P2PTransportChannelSameNatTest, TestConesBehindSameCone) {
 // In the future we will try different RTTs and configs for the different
 // interfaces, so that we can simulate a user with Ethernet and VPN networks.
 class P2PTransportChannelMultihomedTest : public P2PTransportChannelTestBase {
+ public:
+  const cricket::Connection* GetConnectionWithRemoteAddress(
+      cricket::P2PTransportChannel* channel,
+      const SocketAddress& address) {
+    for (cricket::Connection* conn : channel->connections()) {
+      if (conn->remote_candidate().address().EqualIPs(address)) {
+        return conn;
+      }
+    }
+    return nullptr;
+  }
+
+  const cricket::Connection* GetConnectionWithLocalAddress(
+      cricket::P2PTransportChannel* channel,
+      const SocketAddress& address) {
+    for (cricket::Connection* conn : channel->connections()) {
+      if (conn->local_candidate().address().EqualIPs(address)) {
+        return conn;
+      }
+    }
+    return nullptr;
+  }
+
+  void DestroyAllButBestConnection(cricket::P2PTransportChannel* channel) {
+    const cricket::Connection* best_connection = channel->best_connection();
+    for (cricket::Connection* conn : channel->connections()) {
+      if (conn != best_connection) {
+        conn->Destroy();
+      }
+    }
+  }
 };
 
 // Test that we can establish connectivity when both peers are multihomed.
@@ -1730,6 +1762,7 @@ TEST_F(P2PTransportChannelMultihomedTest, TestPreferWifiToWifiConnection) {
                        LocalCandidate(ep2_ch1())->address().EqualIPs(wifi[1]) &&
                        RemoteCandidate(ep2_ch1())->address().EqualIPs(wifi[0]),
                    1000);
+  DestroyChannels();

[Taylor Brandstetter, 2016/06/21 23:36:12]

Can you explain why this is needed? Shouldn't the destructor of the test clean
everything up?

 }
 
 // Tests that a Wifi-Cellular connection has higher precedence than
@@ -1760,6 +1793,7 @@ TEST_F(P2PTransportChannelMultihomedTest, TestPreferWifiOverCellularNetwork) {
   EXPECT_TRUE_WAIT(ep2_ch1()->best_connection() &&
                        LocalCandidate(ep2_ch1())->address().EqualIPs(wifi[1]),
                    1000);
+  DestroyChannels();
 }
 
 // Test that the backup connection is pinged at a rate no faster than
@@ -1797,6 +1831,8 @@ TEST_F(P2PTransportChannelMultihomedTest, TestPingBackupConnectionRate) {
       backup_conn->last_ping_response_received() - last_ping_response_ms;
   LOG(LS_INFO) << "Time elapsed: " << time_elapsed;
   EXPECT_GE(time_elapsed, backup_ping_interval);
+
+  DestroyChannels();
 }
 
 TEST_F(P2PTransportChannelMultihomedTest, TestGetState) {
@@ -1811,6 +1847,8 @@ TEST_F(P2PTransportChannelMultihomedTest, TestGetState) {
                  ep1_ch1()->GetState(), 1000);
   EXPECT_EQ_WAIT(cricket::TransportChannelState::STATE_COMPLETED,
                  ep2_ch1()->GetState(), 1000);
+
+  DestroyChannels();
 }
 
 // Tests that when a network interface becomes inactive, if and only if
@@ -1849,17 +1887,58 @@ TEST_F(P2PTransportChannelMultihomedTest, TestNetworkBecomesInactive) {
   rtc::Thread::Current()->ProcessMessages(500);
   EXPECT_EQ(num_ports, ep2_ch1()->ports().size());
   EXPECT_EQ(num_remote_candidates, ep1_ch1()->remote_candidates().size());
+
+  DestroyChannels();
 }
 
-/*
+// Tests that continual gathering will create new connections when a new
+// interface is added.
+TEST_F(P2PTransportChannelMultihomedTest,
+       TestContinualGatheringOnNewInterface) {
+  auto& wifi = kAlternateAddrs;
+  auto& cellular = kPublicAddrs;
+  AddAddress(0, wifi[0], "test_wifi0", rtc::ADAPTER_TYPE_WIFI);
+  AddAddress(1, cellular[1], "test_cell1", rtc::ADAPTER_TYPE_CELLULAR);
+  CreateChannels(1);
+  // Enable continual gathering.
+  ep1_ch1()->SetIceConfig(CreateIceConfig(1000, true));
+  ep2_ch1()->SetIceConfig(CreateIceConfig(1000, true));

[Taylor Brandstetter, 2016/06/21 23:36:12]

Would be nice to have a "CreateContinualGatheringConfig" helper method, since
the bool parameter is not obvious.

+  SetAllocatorFlags(0, kOnlyLocalPorts);
+  SetAllocatorFlags(1, kOnlyLocalPorts);
+  EXPECT_TRUE_WAIT_MARGIN(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
+                              ep2_ch1()->receiving() && ep2_ch1()->writable(),
+                          kDefaultTimeout, kDefaultTimeout);
+
+  // Add a new wifi interface on end point 2. We should expect a new connection
+  // to be created and the new one will be the best connection.
+  AddAddress(1, wifi[1], "test_wifi1", rtc::ADAPTER_TYPE_WIFI);
+  const cricket::Connection* conn;
+  EXPECT_TRUE_WAIT((conn = ep1_ch1()->best_connection()) != nullptr &&
+                       conn->remote_candidate().address().EqualIPs(wifi[1]),
+                   kDefaultTimeout);
+  EXPECT_TRUE_WAIT((conn = ep2_ch1()->best_connection()) != nullptr &&
+                       conn->local_candidate().address().EqualIPs(wifi[1]),
+                   kDefaultTimeout);
+
+  // Add a new cellular interface on end point 1, we should expect a new
+  // backup connection created using this new interface.
+  AddAddress(0, cellular[0], "test_cellular0", rtc::ADAPTER_TYPE_CELLULAR);
+  EXPECT_TRUE_WAIT(
+      ep1_ch1()->GetState() == cricket::STATE_COMPLETED &&
+          (conn = GetConnectionWithLocalAddress(ep1_ch1(), cellular[0])) &&
+          conn != ep1_ch1()->best_connection() && conn->writable(),
+      kDefaultTimeout);
+  EXPECT_TRUE_WAIT(
+      ep2_ch1()->GetState() == cricket::STATE_COMPLETED &&
+          (conn = GetConnectionWithRemoteAddress(ep2_ch1(), cellular[0])) &&
+          conn != ep2_ch1()->best_connection() && conn->receiving(),
+      kDefaultTimeout);
 
-TODO(pthatcher): Once have a way to handle network interfaces changes
-without signalling an ICE restart, put a test like this back.  In the
-mean time, this test only worked for GICE.  With ICE, it's currently
-not possible without an ICE restart.
+  DestroyChannels();
+}
 
-// Test that we can switch links in a coordinated fashion.
-TEST_F(P2PTransportChannelMultihomedTest, TestDrain) {
+// Tests that we can switch links via continual gathering.
+TEST_F(P2PTransportChannelMultihomedTest, TestContinualGatheringSwitchLinks) {
   AddAddress(0, kPublicAddrs[0]);
   AddAddress(1, kPublicAddrs[1]);
   // Use only local ports for simplicity.
@@ -1868,35 +1947,119 @@ TEST_F(P2PTransportChannelMultihomedTest, TestDrain) {
 
   // Create channels and let them go writable, as usual.
   CreateChannels(1);
-  EXPECT_TRUE_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
-                   ep2_ch1()->receiving() && ep2_ch1()->writable(),
-                   1000);
+  // Enable continual gathering.
+  ep1_ch1()->SetIceConfig(CreateIceConfig(1000, true));
+  ep2_ch1()->SetIceConfig(CreateIceConfig(1000, true));
+  EXPECT_TRUE_WAIT_MARGIN(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
+                              ep2_ch1()->receiving() && ep2_ch1()->writable(),
+                          kDefaultTimeout, kDefaultTimeout);
   EXPECT_TRUE(
       ep1_ch1()->best_connection() && ep2_ch1()->best_connection() &&
       LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]) &&
       RemoteCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[1]));
 
-
-  // Remove the public interface, add the alternate interface, and allocate
-  // a new generation of candidates for the new interface (via
-  // MaybeStartGathering()).
+  // Add the new address first and then remove the other one.
   LOG(LS_INFO) << "Draining...";
   AddAddress(1, kAlternateAddrs[1]);
   RemoveAddress(1, kPublicAddrs[1]);
-  ep2_ch1()->MaybeStartGathering();
-
-  // We should switch over to use the alternate address after
-  // an exchange of pings.
+  // We should switch to use the alternate address after an exchange of pings.
   EXPECT_TRUE_WAIT(
       ep1_ch1()->best_connection() && ep2_ch1()->best_connection() &&
       LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]) &&
       RemoteCandidate(ep1_ch1())->address().EqualIPs(kAlternateAddrs[1]),
       3000);
 
+  // Remove one address first and then add another address.
+  LOG(LS_INFO) << "Draining again...";
+  RemoveAddress(1, kAlternateAddrs[1]);
+  AddAddress(1, kAlternateAddrs[0]);
+  EXPECT_TRUE_WAIT(
+      ep1_ch1()->best_connection() && ep2_ch1()->best_connection() &&
+          LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]) &&
+          RemoteCandidate(ep1_ch1())->address().EqualIPs(kAlternateAddrs[0]),
+      3000);
+
+  DestroyChannels();
+}
+
+// Tests that if the backup connections are lost and then the interface with the
+// selected connection is gone, continual gathering will restore the
+// connectivity.
+TEST_F(P2PTransportChannelMultihomedTest,
+       TestBackupConnectionLostThenContinualGathering) {
+  auto& wifi = kAlternateAddrs;
+  auto& cellular = kPublicAddrs;
+  AddAddress(0, wifi[0], "test_wifi0", rtc::ADAPTER_TYPE_WIFI);
+  AddAddress(0, cellular[0], "test_cell0", rtc::ADAPTER_TYPE_CELLULAR);
+  AddAddress(1, wifi[1], "test_wifi1", rtc::ADAPTER_TYPE_WIFI);
+  AddAddress(1, cellular[1], "test_cell1", rtc::ADAPTER_TYPE_CELLULAR);
+  // Use only local ports for simplicity.
+  SetAllocatorFlags(0, kOnlyLocalPorts);
+  SetAllocatorFlags(1, kOnlyLocalPorts);
+
+  // Create channels and let them go writable, as usual.
+  CreateChannels(1);
+  // Enable continual gathering.
+  ep1_ch1()->SetIceConfig(CreateIceConfig(1000, true));
+  ep2_ch1()->SetIceConfig(CreateIceConfig(1000, true));
+  EXPECT_TRUE_WAIT_MARGIN(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
+                              ep2_ch1()->receiving() && ep2_ch1()->writable(),
+                          kDefaultTimeout, kDefaultTimeout);
+  EXPECT_TRUE(ep1_ch1()->best_connection() && ep2_ch1()->best_connection() &&
+              LocalCandidate(ep1_ch1())->address().EqualIPs(wifi[0]) &&
+              RemoteCandidate(ep1_ch1())->address().EqualIPs(wifi[1]));
+
+  // First destroy all backup connection.
+  DestroyAllButBestConnection(ep1_ch1());
+  // Then the interface of the best connection goes away.
+  RemoveAddress(0, wifi[0]);
+  EXPECT_TRUE_WAIT(
+      ep1_ch1()->best_connection() && ep2_ch1()->best_connection() &&
+          LocalCandidate(ep1_ch1())->address().EqualIPs(cellular[0]) &&
+          RemoteCandidate(ep1_ch1())->address().EqualIPs(wifi[1]),
+      kDefaultTimeout);
+
   DestroyChannels();
 }
 
-*/
+// Tests that the backup connection will be restored after it is destroyed.
+TEST_F(P2PTransportChannelMultihomedTest, TestRestoreBackupConnection) {
+  auto& wifi = kAlternateAddrs;
+  auto& cellular = kPublicAddrs;
+  AddAddress(0, wifi[0], "test_wifi0", rtc::ADAPTER_TYPE_WIFI);
+  AddAddress(0, cellular[0], "test_cell0", rtc::ADAPTER_TYPE_CELLULAR);
+  AddAddress(1, wifi[1], "test_wifi1", rtc::ADAPTER_TYPE_WIFI);
+  AddAddress(1, cellular[1], "test_cell1", rtc::ADAPTER_TYPE_CELLULAR);
+  // Use only local ports for simplicity.
+  SetAllocatorFlags(0, kOnlyLocalPorts);
+  SetAllocatorFlags(1, kOnlyLocalPorts);
+
+  // Create channels and let them go writable, as usual.
+  CreateChannels(1);
+  ep1_ch1()->SetIceConfig(CreateIceConfig(1000, true));
+  ep2_ch1()->SetIceConfig(CreateIceConfig(1000, true));
+  EXPECT_TRUE_WAIT_MARGIN(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
+                              ep2_ch1()->receiving() && ep2_ch1()->writable(),
+                          kDefaultTimeout, kDefaultTimeout);
+  EXPECT_TRUE(ep1_ch1()->best_connection() && ep2_ch1()->best_connection() &&
+              LocalCandidate(ep1_ch1())->address().EqualIPs(wifi[0]) &&
+              RemoteCandidate(ep1_ch1())->address().EqualIPs(wifi[1]));
+
+  // Destroy all backup connections.
+  ep1_ch1()->set_check_restore_backup_connection_interval(2000);
+  DestroyAllButBestConnection(ep1_ch1());
+  // Ensure the backup connection is removed first.
+  EXPECT_TRUE_WAIT(
+      GetConnectionWithLocalAddress(ep1_ch1(), cellular[0]) == nullptr,
+      kDefaultTimeout);
+  const cricket::Connection* conn;
+  EXPECT_TRUE_WAIT(
+      (conn = GetConnectionWithLocalAddress(ep1_ch1(), cellular[0])) &&
+          conn != ep1_ch1()->best_connection() && conn->writable(),
+      5000);

[Taylor Brandstetter, 2016/06/21 23:36:12]

Why 5000 specifically?

+
+  DestroyChannels();
+}
 
 // A collection of tests which tests a single P2PTransportChannel by sending
 // pings.