Bug 4865: Enable connectivity when the remote peer is on public internet.

webrtc/p2p/client/portallocator_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 70 matching lines @@
 }  // namespace cricket
 
 class PortAllocatorTest : public testing::Test, public sigslot::has_slots<> {
  public:
   PortAllocatorTest()
       : pss_(new rtc::PhysicalSocketServer),
         vss_(new rtc::VirtualSocketServer(pss_.get())),
         fss_(new rtc::FirewallSocketServer(vss_.get())),
         ss_scope_(fss_.get()),
         nat_factory_(vss_.get(), kNatUdpAddr, kNatTcpAddr),
-        nat_socket_factory_(&nat_factory_),
+        nat_socket_factory_(new rtc::BasicPacketSocketFactory(&nat_factory_)),
         stun_server_(cricket::TestStunServer::Create(Thread::Current(),
                                                      kStunAddr)),
         relay_server_(Thread::Current(), kRelayUdpIntAddr, kRelayUdpExtAddr,
                       kRelayTcpIntAddr, kRelayTcpExtAddr,
                       kRelaySslTcpIntAddr, kRelaySslTcpExtAddr),
         turn_server_(Thread::Current(), kTurnUdpIntAddr, kTurnUdpExtAddr),
         candidate_allocation_done_(false) {
     cricket::ServerAddresses stun_servers;
     stun_servers.insert(kStunAddr);
     // Passing the addresses of GTURN servers will enable GTURN in
     // Basicportallocator.
     allocator_.reset(new cricket::BasicPortAllocator(
         &network_manager_,
         stun_servers,
         kRelayUdpIntAddr, kRelayTcpIntAddr, kRelaySslTcpIntAddr));
     allocator_->set_step_delay(cricket::kMinimumStepDelay);
   }
 
   void AddInterface(const SocketAddress& addr) {
     network_manager_.AddInterface(addr);
   }
+  void AddInterfaceAsDefaultRoute(const SocketAddress& addr) {
+    AddInterface(addr);
+    // When a binding comes from the any address, the |addr| will be used as the
+    // srflx address.
+    vss_->SetDefaultRoute(addr.ipaddr());
+  }
   bool SetPortRange(int min_port, int max_port) {
     return allocator_->SetPortRange(min_port, max_port);
   }
+  // No STUN/TURN configuration.
+  void ResetWithNoServers() {
+    allocator_.reset(new cricket::BasicPortAllocator(&network_manager_));
+    allocator_->set_step_delay(cricket::kMinimumStepDelay);
+  }
   void ResetWithNatServer(const rtc::SocketAddress& stun_server) {
     nat_server_.reset(new rtc::NATServer(
         rtc::NAT_OPEN_CONE, vss_.get(), kNatUdpAddr, kNatTcpAddr, vss_.get(),
         rtc::SocketAddress(kNatUdpAddr.ipaddr(), 0)));
 
     ServerAddresses stun_servers;
     if (!stun_server.IsNil()) {
       stun_servers.insert(stun_server);
     }
     allocator_.reset(new cricket::BasicPortAllocator(
-        &network_manager_, &nat_socket_factory_, stun_servers));
+        &network_manager_, nat_socket_factory_.get(), stun_servers));
     allocator().set_step_delay(cricket::kMinimumStepDelay);
   }
 
+  void ResetWithStunServer(const rtc::SocketAddress& stun_server) {
+    nat_socket_factory_.reset(new rtc::BasicPacketSocketFactory());

[juberti1, 2015/08/13 21:23:59]

why do we need to change the socket factory here?

[guoweis_webrtc, 2015/08/14 05:24:25]

nat_socket_factory assumes there is NAT.

[juberti1, 2015/08/14 23:22:42]

OK, I was confused by the name, since typically you use a STUN server when there
is a NAT. 

I think this should be called ResetWithStunServerNoNat, and the previous
function called ResetWithStunServerAndNat. 

We should probably also rename ResetWithTurnServers to be
ResetWithTurnServersNoNat, and ResetWithNoServers to be ResetWithNoServersOrNat.
Alternatively, we could just add comments to the ResetWithXServers functions
saying that there is no NAT present, although I think that is a bit unexpected.

+
+    ServerAddresses stun_servers;
+    if (!stun_server.IsNil()) {
+      stun_servers.insert(stun_server);
+    }
+    allocator_.reset(new cricket::BasicPortAllocator(
+        &network_manager_, nat_socket_factory_.get(), stun_servers));
+    allocator().set_step_delay(cricket::kMinimumStepDelay);
+  }
+
   // Create a BasicPortAllocator without GTURN and add the TURN servers.
   void ResetWithTurnServers(const rtc::SocketAddress& udp_turn,
                             const rtc::SocketAddress& tcp_turn) {
     allocator_.reset(new cricket::BasicPortAllocator(&network_manager_));
     allocator().set_step_delay(cricket::kMinimumStepDelay);
     AddTurnServers(udp_turn, tcp_turn);
   }
 
   void AddTurnServers(const rtc::SocketAddress& udp_turn,
                       const rtc::SocketAddress& tcp_turn) {
@@ skipping 85 matching lines @@
                   (*it)->GetOption(rtc::Socket::OPT_SNDBUF,
                                    &send_buffer_size));
       } else {
         EXPECT_EQ(0, (*it)->GetOption(rtc::Socket::OPT_SNDBUF,
                                       &send_buffer_size));
         ASSERT_EQ(expected, send_buffer_size);
       }
     }
   }
 
-  void CheckDisableAdapterEnumeration() {
+  // This function starts the port/address gathering and check the existence of
+  // candidates as specified. When |expect_stun_candidate| is true,
+  // |stun_candidate_addr| carries the expected reflective address, which is
+  // also the related address for TURN candidate if it is expected. Otherwise,
+  // it should be ignore.
+  void CheckDisableAdapterEnumeration(uint32 total_ports,
+                                      bool expect_stun_candidate,

[juberti1, 2015/08/13 21:23:59]

can we omit the second param? We can just check if stun_candidate_addr.IsNil.

[guoweis_webrtc, 2015/08/14 05:24:25]

Done.

+                                      const rtc::IPAddress& stun_candidate_addr,
+                                      bool expect_relay_udp_candidate,
+                                      bool expect_relay_tcp_candidate) {

[juberti1, 2015/08/13 21:23:59]

It's not really a TCP candidate. maybe call this
expect_relay_candidate_udp_transport and expect_relay_candidate_tcp_transport.

Consider passing in the actual relay candidate addresses here too.

[guoweis_webrtc, 2015/08/14 05:24:25]

Done.

     EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP));
-    session_->set_flags(cricket::PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION);
+    session_->set_flags(cricket::PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION |
+                        cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG |
+                        cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET);
+    allocator().set_allow_tcp_listen(false);
     session_->StartGettingPorts();
     EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout);
 
-    // Only 2 candidates as local UDP/TCP are all 0s and get trimmed out.
-    EXPECT_EQ(2U, candidates_.size());
-    EXPECT_EQ(2U, ports_.size());  // One stunport and one turnport.
+    uint32 total_candidates = 0;
+    if (expect_stun_candidate) {
+      total_candidates++;

[juberti1, 2015/08/13 21:23:59]

Style: prefer preincrement (here and below)

[guoweis_webrtc, 2015/08/14 05:24:25]

Done.

+    }
+    if (expect_relay_udp_candidate) {
+      total_candidates++;
+    }
+    if (expect_relay_tcp_candidate) {
+      total_candidates++;
+    }
 
-    EXPECT_PRED5(CheckCandidate, candidates_[0],
-                 cricket::ICE_CANDIDATE_COMPONENT_RTP, "stun", "udp",
-                 rtc::SocketAddress(kNatUdpAddr.ipaddr(), 0));
-    EXPECT_EQ(
-        rtc::EmptySocketAddressWithFamily(candidates_[0].address().family()),
-        candidates_[0].related_address());
+    EXPECT_EQ(total_candidates, candidates_.size());
+    EXPECT_EQ(total_ports, ports_.size());
 
-    EXPECT_PRED5(CheckCandidate, candidates_[1],
-                 cricket::ICE_CANDIDATE_COMPONENT_RTP, "relay", "udp",
-                 rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0));
-    EXPECT_EQ(kNatUdpAddr.ipaddr(), candidates_[1].related_address().ipaddr());
+    if (expect_stun_candidate) {
+      EXPECT_PRED5(CheckCandidate, candidates_[0],
+                   cricket::ICE_CANDIDATE_COMPONENT_RTP, "stun", "udp",
+                   rtc::SocketAddress(stun_candidate_addr, 0));
+      EXPECT_EQ(
+          rtc::EmptySocketAddressWithFamily(candidates_[0].address().family()),
+          candidates_[0].related_address());
+    }
+    if (expect_relay_udp_candidate) {
+      EXPECT_PRED5(CheckCandidate, candidates_[1],
+                   cricket::ICE_CANDIDATE_COMPONENT_RTP, "relay", "udp",
+                   rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0));
+      EXPECT_EQ(stun_candidate_addr, candidates_[1].related_address().ipaddr());
+    }
+    if (expect_relay_tcp_candidate) {
+      EXPECT_PRED5(CheckCandidate, candidates_[2],
+                   cricket::ICE_CANDIDATE_COMPONENT_RTP, "relay", "udp",
+                   rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0));
+      EXPECT_EQ(stun_candidate_addr, candidates_[2].related_address().ipaddr());
+    }
   }
 
  protected:
   cricket::BasicPortAllocator& allocator() {
     return *allocator_;
   }
 
   void OnPortReady(cricket::PortAllocatorSession* ses,
                    cricket::PortInterface* port) {
     LOG(LS_INFO) << "OnPortReady: " << port->ToString();
@@ skipping 20 matching lines @@
     }
     return false;
   }
 
   rtc::scoped_ptr<rtc::PhysicalSocketServer> pss_;
   rtc::scoped_ptr<rtc::VirtualSocketServer> vss_;
   rtc::scoped_ptr<rtc::FirewallSocketServer> fss_;
   rtc::SocketServerScope ss_scope_;
   rtc::scoped_ptr<rtc::NATServer> nat_server_;
   rtc::NATSocketFactory nat_factory_;
-  rtc::BasicPacketSocketFactory nat_socket_factory_;
+  rtc::scoped_ptr<rtc::BasicPacketSocketFactory> nat_socket_factory_;
   rtc::scoped_ptr<cricket::TestStunServer> stun_server_;
   cricket::TestRelayServer relay_server_;
   cricket::TestTurnServer turn_server_;
   rtc::FakeNetworkManager network_manager_;
   rtc::scoped_ptr<cricket::BasicPortAllocator> allocator_;
   rtc::scoped_ptr<cricket::PortAllocatorSession> session_;
   std::vector<cricket::PortInterface*> ports_;
   std::vector<cricket::Candidate> candidates_;
   bool candidate_allocation_done_;
 };
@@ skipping 141 matching lines @@
   EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP));
   session_->StartGettingPorts();
   rtc::Thread::Current()->ProcessMessages(100);
   // Without network adapter, we should not get any candidate.
   EXPECT_EQ(0U, candidates_.size());
   EXPECT_TRUE(candidate_allocation_done_);
 }
 
 // Test that we should only get STUN and TURN candidates when adapter
 // enumeration is disabled.
-TEST_F(PortAllocatorTest, TestDisableAdapterEnumeration) {
+TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationBehindNat) {
   AddInterface(kClientAddr);
   // GTURN is not configured here.
   ResetWithNatServer(kStunAddr);
   AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress());
-
-  CheckDisableAdapterEnumeration();
+  // Expect to see 3 ports: STUN, TURN/UDP and TCP ports, and both STUN and
+  // TURN/UDP candidates.
+  CheckDisableAdapterEnumeration(3U, true, kNatUdpAddr.ipaddr(), true, false);
 }
 
-// Test that even with multiple interfaces, the result should be only 1 Stun
-// candidate since we bind to any address (i.e. all 0s).
-TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationMultipleInterfaces) {
+// Test that even with multiple interfaces, the result should still be one STUN
+// and one TURN candidate since we bind to any address (i.e. all 0s).
+TEST_F(PortAllocatorTest,
+       TestDisableAdapterEnumerationBehindNatMultipleInterfaces) {
   AddInterface(kPrivateAddr);
   AddInterface(kPrivateAddr2);
   ResetWithNatServer(kStunAddr);
   AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress());
+  // Expect to see 3 ports: STUN, TURN/UDP and TCP ports, and both STUN and
+  // TURN/UDP candidates.
+  CheckDisableAdapterEnumeration(3U, true, kNatUdpAddr.ipaddr(), true, false);
+}
 
-  CheckDisableAdapterEnumeration();
+// Test that we should get STUN, TURN/UDP and TURN/TCP candidates when a
+// TURN/TCP server is specified.
+TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationBehindNatWithTcp) {
+  turn_server_.AddInternalSocket(kTurnTcpIntAddr, cricket::PROTO_TCP);
+  AddInterface(kClientAddr);
+  // GTURN is not configured here.
+  ResetWithNatServer(kStunAddr);
+  AddTurnServers(kTurnUdpIntAddr, kTurnTcpIntAddr);
+  // Expect to see 4 ports - STUN, TURN/UDP, TURN/TCP and TCP port. STUN,
+  // TURN/UDP, and TURN/TCP candidates.
+  CheckDisableAdapterEnumeration(4U, true, kNatUdpAddr.ipaddr(), true, true);
+}
+
+// Test that we should only get STUN and TURN candidates when adapter
+// enumeration is disabled. Since the endpoint is not behind NAT, the srflx
+// address should be the public client interface.
+TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationWithoutNat) {
+  AddInterfaceAsDefaultRoute(kClientAddr);
+  ResetWithStunServer(kStunAddr);
+  AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress());
+  // Expect to see 3 ports: STUN, TURN/UDP and TCP ports, but only both STUN and
+  // TURN candidates. The STUN candidate should have kClientAddr as srflx
+  // address, and TURN candidate with kClientAddr as the related address.
+  CheckDisableAdapterEnumeration(3U, true, kClientAddr.ipaddr(), true, false);
+}
+
+// Test that when adapter enumeration is disabled, for endpoints without
+// STUN/TURN specified, no candidate is generated.
+TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationWithoutNat_NoCandidate) {

[juberti1, 2015/08/13 21:23:59]

Call this ...WithoutNatOrServers

[guoweis_webrtc, 2015/08/14 05:24:25]

Done.

+  AddInterfaceAsDefaultRoute(kClientAddr);
+  ResetWithNoServers();
+  // Expect to see 2 ports: STUN and TCP ports, but no candidate.
+  CheckDisableAdapterEnumeration(2U, false, rtc::IPAddress(), false, false);
 }
 
 // Disable for asan, see
 // https://code.google.com/p/webrtc/issues/detail?id=4743 for details.
 #if !defined(ADDRESS_SANITIZER)
 
 // Test that we can get OnCandidatesAllocationDone callback when all the ports
 // are disabled.
 TEST_F(PortAllocatorTest, TestDisableAllPorts) {
   AddInterface(kClientAddr);
@@ skipping 698 matching lines @@
   AllocationSequenceForTest alloc_sequence(
       static_cast<cricket::BasicPortAllocatorSession*>(session_.get()),
       &network1, &config, flag);
   // This simply tests it will not crash if udp_socket_ in the
   // AllocationSequence is null, which is chosen in the constructor.
   cricket::RelayServerConfig relay_server(cricket::RELAY_TURN);
   relay_server.ports.push_back(
       cricket::ProtocolAddress(kTurnUdpIntAddr, cricket::PROTO_UDP, false));
   alloc_sequence.CreateTurnPort(relay_server);
 }