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

webrtc/p2p/client/basicportallocator.cc

 /*
  *  Copyright 2004 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 907 matching lines @@
 
   if (port) {
     // If shared socket is enabled, STUN candidate will be allocated by the
     // UDPPort.
     if (IsFlagSet(PORTALLOCATOR_ENABLE_SHARED_SOCKET)) {
       udp_port_ = port;
       port->SignalDestroyed.connect(this, &AllocationSequence::OnPortDestroyed);
 
       // If STUN is not disabled, setting stun server address to port.
       if (!IsFlagSet(PORTALLOCATOR_DISABLE_STUN)) {
-        // If config has stun_servers, use it to get server reflexive candidate
-        // otherwise use first TURN server which supports UDP.
         if (config_ && !config_->StunServers().empty()) {

[pthatcher1, 2015/07/06 22:28:32]

Does it make sense to check for !empty() still?  Should we just always
set_server_addresses?

[Taylor Brandstetter, 2015/07/07 00:00:04]

It's a little redundant but at least it means you won't see a log message about
STUN candidates if there are none.

           LOG(LS_INFO) << "AllocationSequence: UDPPort will be handling the "
                        <<  "STUN candidate generation.";
           port->set_server_addresses(config_->StunServers());
-        } else if (config_ &&
-                   config_->SupportsProtocol(RELAY_TURN, PROTO_UDP)) {
-          port->set_server_addresses(config_->GetRelayServerAddresses(
-              RELAY_TURN, PROTO_UDP));
-          LOG(LS_INFO) << "AllocationSequence: TURN Server address will be "
-                       << " used for generating STUN candidate.";
         }
       }
     }
 
     session_->AddAllocatedPort(port, this, true);
   }
 }
 
 void AllocationSequence::CreateTCPPorts() {
   if (IsFlagSet(PORTALLOCATOR_DISABLE_TCP)) {
@@ skipping 212 matching lines @@
       password(password) {
   if (!stun_servers.empty())
     stun_address = *(stun_servers.begin());
 }
 
 ServerAddresses PortConfiguration::StunServers() {
   if (!stun_address.IsNil() &&
       stun_servers.find(stun_address) == stun_servers.end()) {
     stun_servers.insert(stun_address);
   }
+  // Every UDP TURN server should also be used as a STUN server.
+  ServerAddresses turn_servers = GetRelayServerAddresses(RELAY_TURN,
+                                                         PROTO_UDP);

[pthatcher1, 2015/07/06 22:28:32]

Please run "git cl format" and use whatever style it chooses here.

[pthatcher1, 2015/07/06 22:28:32]

Do we still need the check for SupportsProtocol(RELAY_TURN, PROTO_UDP)?  

[Taylor Brandstetter, 2015/07/07 00:00:04]

Done

[Taylor Brandstetter, 2015/07/07 00:00:04]

GetRelayServerAddresses(RELAY_TURN, PROTO_UDP) will do that for us.

+  for (ServerAddresses::iterator turn_server = turn_servers.begin();

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

I think this could be done as 

for (auto turn_server : turn_servers)

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

Good point. In this case I think I'll use "for (rtc::SocketAddress turn_server :
turn_servers)", since "auto" doesn't really improve readability.

[pthatcher1, 2015/07/06 22:28:32]

I think 

for (const rtc::SocketAddress& turn_server : turn_servers) {
  ...
}

Would be even better.  It's how we normally have it in the code base.

[Taylor Brandstetter, 2015/07/07 00:00:04]

Already did that. :)

+       turn_server != turn_servers.end(); ++turn_server) {
+    if (stun_servers.find(*turn_server) == stun_servers.end()) {
+      stun_servers.insert(*turn_server);
+    }
+  }
   return stun_servers;
 }
 
 void PortConfiguration::AddRelay(const RelayServerConfig& config) {
   relays.push_back(config);
 }
 
 bool PortConfiguration::SupportsProtocol(
     const RelayServerConfig& relay, ProtocolType type) const {
   PortList::const_iterator relay_port;
@@ skipping 21 matching lines @@
   ServerAddresses servers;
   for (size_t i = 0; i < relays.size(); ++i) {
     if (relays[i].type == turn_type && SupportsProtocol(relays[i], type)) {
       servers.insert(relays[i].ports.front().address);
     }
   }
   return servers;
 }
 
 }  // namespace cricket