Ensuring that UDP TURN servers are always used as STUN servers.

webrtc/p2p/client/portallocator_unittest.cc

Index: webrtc/p2p/client/portallocator_unittest.cc
diff --git a/webrtc/p2p/client/portallocator_unittest.cc b/webrtc/p2p/client/portallocator_unittest.cc
index 017c37a6f6e673cee11f721905fdbe93761b58ac..fc7cd50a9a8582c925ed045ebb1fd1266a0be583 100644
--- a/webrtc/p2p/client/portallocator_unittest.cc
+++ b/webrtc/p2p/client/portallocator_unittest.cc
@@ -784,7 +784,7 @@ TEST_F(PortAllocatorTest, TestSharedSocketWithNat) {
   EXPECT_EQ(3U, candidates_.size());
 }
 
-// Test TURN port in shared socket mode with UDP and TCP TURN server adderesses.
+// Test TURN port in shared socket mode with UDP and TCP TURN server addresses.
 TEST_F(PortAllocatorTest, TestSharedSocketWithoutNatUsingTurn) {
   turn_server_.AddInternalSocket(kTurnTcpIntAddr, cricket::PROTO_TCP);
   AddInterface(kClientAddr);
@@ -916,6 +916,113 @@ TEST_F(PortAllocatorTest, TestSharedSocketWithNatUsingTurnAsStun) {
   EXPECT_EQ(1U, ports_[1]->Candidates().size());
 }
 
+// Test that even when both a STUN and TURN server are configured, the TURN
+// server is used as a STUN server and we get a 'stun' candidate.
+TEST_F(PortAllocatorTest, TestSharedSocketWithNatUsingTurnAndStun) {
+  AddInterface(kClientAddr);
+  // Configure with STUN server but destroy it, so we can ensure that it's
+  // the TURN server actually being used as a STUN server.
+  ResetWithNatServer(kStunAddr);
+  stun_server_.reset();
+  AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress());
+
+  // Must set the step delay to 0 to make sure the relay allocation phase is
+  // started before the STUN candidates are obtained, so that the STUN binding
+  // response is processed when both StunPort and TurnPort exist to reproduce
+  // webrtc issue 3537.
+  allocator_->set_step_delay(0);
+  allocator_->set_flags(allocator().flags() |
+                        cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG |
+                        cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET |
+                        cricket::PORTALLOCATOR_DISABLE_TCP);
+
+  EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP));
+  session_->StartGettingPorts();
+
+  ASSERT_EQ_WAIT(3U, candidates_.size(), kDefaultAllocationTimeout);
+  EXPECT_PRED5(CheckCandidate, candidates_[0],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "local", "udp", kClientAddr);
+  EXPECT_PRED5(CheckCandidate, candidates_[1],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "stun", "udp",
+      rtc::SocketAddress(kNatAddr.ipaddr(), 0));
+  EXPECT_PRED5(CheckCandidate, candidates_[2],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "relay", "udp",
+      rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0));
+  EXPECT_EQ(candidates_[2].related_address(), candidates_[1].address());
+
+  // Don't bother waiting for STUN timeout, since we already verified
+  // that we got a STUN candidate from the TURN server.
+}
+
+// Test that when only a TCP TURN server is available, we do NOT use it as
+// a UDP STUN server, as this could leak our IP address. Thus we should only
+// expect two ports, a UDPPort and TurnPort.
+TEST_F(PortAllocatorTest, TestNonSharedSocketWithoutNatUsingTurnTcp) {
+  turn_server_.AddInternalSocket(kTurnTcpIntAddr, cricket::PROTO_TCP);
+  AddInterface(kClientAddr);
+  allocator_.reset(new cricket::BasicPortAllocator(&network_manager_));
+
+  AddTurnServers(rtc::SocketAddress(), kTurnTcpIntAddr);
+
+  allocator_->set_step_delay(cricket::kMinimumStepDelay);
+  allocator_->set_flags(allocator().flags() |
+                        cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG |
+                        cricket::PORTALLOCATOR_DISABLE_TCP);
+
+  EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP));
+  session_->StartGettingPorts();
+
+  ASSERT_EQ_WAIT(2U, candidates_.size(), kDefaultAllocationTimeout);
+  ASSERT_EQ(2U, ports_.size());
+  EXPECT_PRED5(CheckCandidate, candidates_[0],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "local", "udp", kClientAddr);
+  EXPECT_PRED5(CheckCandidate, candidates_[1],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "relay", "udp",
+      rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0));
+  EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout);
+  EXPECT_EQ(2U, candidates_.size());
+  EXPECT_EQ(1U, ports_[0]->Candidates().size());
+  EXPECT_EQ(1U, ports_[1]->Candidates().size());
+}
+
+// Test that even when PORTALLOCATOR_ENABLE_SHARED_SOCKET is NOT enabled, the
+// TURN server is used as the STUN server and we get 'local', 'stun', and
+// 'relay' candidates.
+TEST_F(PortAllocatorTest, TestNonSharedSocketWithNatUsingTurnAsStun) {
+  AddInterface(kClientAddr);
+  // Use an empty SocketAddress to add a NAT without STUN server.
+  ResetWithNatServer(SocketAddress());
+  AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress());
+
+  allocator_->set_step_delay(0);
+  allocator_->set_flags(allocator().flags() |
+                        cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG |
+                        cricket::PORTALLOCATOR_DISABLE_TCP);
+
+  EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP));
+  session_->StartGettingPorts();
+
+  ASSERT_EQ_WAIT(3U, candidates_.size(), kDefaultAllocationTimeout);
+  ASSERT_EQ(3U, ports_.size());
+  EXPECT_PRED5(CheckCandidate, candidates_[0],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "local", "udp", kClientAddr);
+  EXPECT_PRED5(CheckCandidate, candidates_[1],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "stun", "udp",
+      rtc::SocketAddress(kNatAddr.ipaddr(), 0));
+  EXPECT_PRED5(CheckCandidate, candidates_[2],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "relay", "udp",
+      rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0));
+  // Not using shared socket, so the STUN request's server reflexive address
+  // should be different than the TURN request's server reflexive address.
+  EXPECT_NE(candidates_[2].related_address(), candidates_[1].address());

[juberti1, 2015/06/29 18:54:49]

But candidates_[2] is not server reflexive, so this comment is confusing.

I think this test should come right after the previous TurnAsStun tests, since
it is most similar to them.

[Taylor Brandstetter, 2015/06/29 20:18:35]

candidates_[2] isn't server reflexive, but its related_address is; see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

Also, I moved the test as you suggested.

+
+  EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout);
+  EXPECT_EQ(3U, candidates_.size());
+  EXPECT_EQ(1U, ports_[0]->Candidates().size());
+  EXPECT_EQ(1U, ports_[1]->Candidates().size());
+  EXPECT_EQ(1U, ports_[2]->Candidates().size());
+}
+
 // This test verifies when PORTALLOCATOR_ENABLE_SHARED_SOCKET flag is enabled
 // and fail to generate STUN candidate, local UDP candidate is generated
 // properly.