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/27 18:42:54]

I think we also need a test that if we receive a STUN ping with the new
ufrag/pwd, we will process it correctly.

[honghaiz3, 2016/05/31 18:57:31]

I tried to do it but found that it is kind of unnecessary and it would be
complicated to do so. 
The ping test essentially checks 
1) Whether GetConnectionTo() finds the right connection, 
2) Whether the ufrag in the remote candidate matches the one in the stun ping
request. 
Both of them have already been tested here. 
The operation of combining these two are actually in each subclass of Port. 
Trying to test the combined operation needs to expose methods in the subclasses
and some extra hackings, which may not be really worth it. WDYT?

+
 // When the current best connection is strong, lower-priority connections will
 // be pruned. Otherwise, lower-priority connections are kept.
 TEST_F(P2PTransportChannelPingTest, TestDontPruneWhenWeak) {