Create a new connection if a candidate reuses an address

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 47a64e1d0f92f3309cd4104c0c11fc43d7b4d661..1bf953c98bea082382ca3e1f71a8dfaeada5bd35 100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -2453,6 +2453,39 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBasedOnMediaReceived) {
   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) {