Create a new connection if a candidate reuses an address

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 2435 matching lines @@
   EXPECT_EQ(conn3, ch.best_connection());
 
   // Now another data packet will not switch the best connection because the
   // best connection was nominated by the controlling side.
   conn2->ReceivedPing();
   conn2->ReceivedPingResponse();
   conn2->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0));
   EXPECT_EQ(conn3, ch.best_connection());
 }
 
+// Test that if a new remote candidate has the same address and port with
+// an old one, it will be used to create a new connection.
+TEST_F(P2PTransportChannelPingTest, TestAddRemoteCandidateWithAddressReuse) {
+  cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
+  cricket::P2PTransportChannel ch("candidate reuse", 1, &pa);
+  PrepareChannel(&ch);
+  ch.Connect();
+  ch.MaybeStartGathering();
+  const std::string host_address = "1.1.1.1";
+  const int port_num = 1;
+
+  // kIceUfrag[1] is the current generation ufrag.
+  cricket::Candidate candidate =
+      CreateHostCandidate(host_address, port_num, 1, kIceUfrag[1]);
+  ch.AddRemoteCandidate(candidate);
+  cricket::Connection* conn1 = WaitForConnectionTo(&ch, host_address, port_num);
+  ASSERT_TRUE(conn1 != nullptr);
+  EXPECT_EQ(0u, conn1->remote_candidate().generation());
+
+  // Simply adding the same candidate again won't create a new connection.
+  ch.AddRemoteCandidate(candidate);
+  cricket::Connection* conn2 = GetConnectionTo(&ch, host_address, port_num);
+  EXPECT_EQ(conn1, conn2);
+
+  // Update the ufrag of the candidate and add it again.
+  candidate.set_username(kIceUfrag[2]);
+  ch.AddRemoteCandidate(candidate);
+  conn2 = GetConnectionTo(&ch, host_address, port_num);
+  EXPECT_NE(conn1, conn2);
+  EXPECT_EQ(kIceUfrag[2], conn2->remote_candidate().username());
+  EXPECT_EQ(1u, conn2->remote_candidate().generation());

[pthatcher1, 2016/05/31 20:09:29]

Can't we right here inject a STUN ping into the connection and verify that the
last ping received was that one?

[honghaiz3, 2016/05/31 23:25:23]

Added per your suggestion, although I still feel it is a little strange to
include that here. 
All it (the faked ping message) does is to construct a Ping message using the
info from the connection except for the remote ufrag. But since we have already
checked that the remote ufrag is the same as the one in the connection remote
candidate, it is actually using the info from the connection completely to check
whether the connection can handle a ping request.
Perhaps, it can be tested independently that a connection can handle a ping
request properly. 
But that kind of tests are already there in port_unittest file. 

[pthatcher1, 2016/06/01 01:57:04]

I agree it's not optimal, but I still think it's better than nothing.  

[honghaiz3, 2016/06/01 16:37:59]

Acknowledged.

+}
+
 // When the current best connection is strong, lower-priority connections will
 // be pruned. Otherwise, lower-priority connections are kept.
 TEST_F(P2PTransportChannelPingTest, TestDontPruneWhenWeak) {
   cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
   cricket::P2PTransportChannel ch("test channel", 1, &pa);
   PrepareChannel(&ch);
   ch.SetIceRole(cricket::ICEROLE_CONTROLLED);
   ch.Connect();
   ch.MaybeStartGathering();
   ch.AddRemoteCandidate(CreateHostCandidate("1.1.1.1", 1, 1));
@@ skipping 391 matching lines @@
 
   // TCP Relay/Relay is the next.
   VerifyNextPingableConnection(cricket::RELAY_PORT_TYPE,
                                cricket::RELAY_PORT_TYPE,
                                cricket::TCP_PROTOCOL_NAME);
 
   // Finally, Local/Relay will be pinged.
   VerifyNextPingableConnection(cricket::LOCAL_PORT_TYPE,
                                cricket::RELAY_PORT_TYPE);
 }