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

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.
  */
 
 #include "webrtc/p2p/client/basicportallocator.h"
 
 #include <string>
 #include <vector>
 
 #include "webrtc/p2p/base/basicpacketsocketfactory.h"
 #include "webrtc/p2p/base/common.h"
 #include "webrtc/p2p/base/port.h"
 #include "webrtc/p2p/base/relayport.h"
 #include "webrtc/p2p/base/stunport.h"
 #include "webrtc/p2p/base/tcpport.h"
 #include "webrtc/p2p/base/turnport.h"
 #include "webrtc/p2p/base/udpport.h"
+#include "webrtc/base/checks.h"
 #include "webrtc/base/common.h"
 #include "webrtc/base/helpers.h"
 #include "webrtc/base/logging.h"
 
 using rtc::CreateRandomId;
 using rtc::CreateRandomString;
 
 namespace {
 
 enum {
@@ skipping 399 matching lines @@
 void BasicPortAllocatorSession::OnCandidateReady(
     Port* port, const Candidate& c) {
   ASSERT(rtc::Thread::Current() == network_thread_);
   PortData* data = FindPort(port);
   ASSERT(data != NULL);
   // Discarding any candidate signal if port allocation status is
   // already in completed state.
   if (data->complete())
     return;
 
-  // Send candidates whose protocol is enabled.
-  std::vector<Candidate> candidates;
   ProtocolType pvalue;
-  bool candidate_allowed_to_send = CheckCandidateFilter(c);
-  if (StringToProto(c.protocol().c_str(), &pvalue) &&
-      data->sequence()->ProtocolEnabled(pvalue) &&
-      candidate_allowed_to_send) {
+  bool candidate_type_enabled = CheckCandidateFilter(c);

[pthatcher1, 2015/08/07 21:28:19]

I think candidate_signalable would be a good name here.

[guoweis_webrtc, 2015/08/13 14:17:28]

Done.

+  bool candidate_is_any_address = c.address().IsAnyIP() && port->SharedSocket();

[pthatcher1, 2015/08/07 21:28:19]

I think something like candidate_unsignalable_but_pairable would be good, to
indicate that we wan't signal it, but we still want to pair it with signalled
candidates from the remote side.

Or if that name is too long, you could do:

bool candidate_pairable = candidate_signalable || (c.address().IsAnyIP() &&
port->SharedSocket());

[guoweis_webrtc, 2015/08/13 14:17:28]

I don't mind the long name. I think candidate_pairable is a bit too subtle.

+  bool candidate_protocol_enabled =
+      StringToProto(c.protocol().c_str(), &pvalue) &&
+      data->sequence()->ProtocolEnabled(pvalue);
+
+  // These can't be true at the same time. The any address candidate shouldn't
+  // be allowed to send.
+  DCHECK(!candidate_type_enabled || !candidate_is_any_address);

[pthatcher1, 2015/08/07 21:28:19]

DCHECK(!(candidate_type_enabled && candidate_is_any_address));

Would be more clear, and match the comment more clearly.

Or even better:

DCHECK(!(candidate_signalable && candidate_unsignalable_but_pairable))


Or just remove the DCHECK, since it's really covered by CheckCandidateFilter
anyway.

[guoweis_webrtc, 2015/08/13 14:17:28]

Done.

+
+  if (candidate_type_enabled && candidate_protocol_enabled) {

[pthatcher1, 2015/08/07 21:28:19]

if (candidate_signalable && candidate_protocol_enabled) {
  // ...
}

Would look nice here

[guoweis_webrtc, 2015/08/13 14:17:28]

Done.

+    std::vector<Candidate> candidates;
     candidates.push_back(c);
-  }
-
-  if (!candidates.empty()) {
     SignalCandidatesReady(this, candidates);
   }
 
-  // Moving to READY state as we have atleast one candidate from the port.
-  // Since this port has atleast one candidate we should forward this port
-  // to listners, to allow connections from this port.
-  // Also we should make sure that candidate gathered from this port is allowed
-  // to send outside.
-  if (!data->ready() && candidate_allowed_to_send) {
+  // Port has been made ready. Nothing to do here.
+  if (data->ready()) {
+    return;
+  }
+
+  // Move the port to the READY state, either because we have a usable candidate
+  // from the port, or simply because the port is bound to the any address and
+  // therefore has no host candidate. This will trigger the port to start
+  // creating candidate pairs (connections) and issue connectivity checks.
+  if (candidate_type_enabled || candidate_is_any_address) {

[pthatcher1, 2015/08/07 21:28:19]

if (candidate_pairable) {
  // ...
}

Would look nice here.

     data->set_ready();
     SignalPortReady(this, port);
   }
 }
 
 void BasicPortAllocatorSession::OnPortComplete(Port* port) {
   ASSERT(rtc::Thread::Current() == network_thread_);
   PortData* data = FindPort(port);
   ASSERT(data != NULL);
 
@@ skipping 28 matching lines @@
   for (std::vector<PortData>::iterator it = ports_.begin();
        it != ports_.end(); ++it) {
     if (it->sequence() != seq)
       continue;
 
     const std::vector<Candidate>& potentials = it->port()->Candidates();
     for (size_t i = 0; i < potentials.size(); ++i) {
       if (!CheckCandidateFilter(potentials[i]))
         continue;
       ProtocolType pvalue;
-      if (!StringToProto(potentials[i].protocol().c_str(), &pvalue))
-        continue;
-      if (pvalue == proto) {
+      bool candidate_protocol_enabled =
+          StringToProto(potentials[i].protocol().c_str(), &pvalue) &&
+          pvalue == proto;
+      if (candidate_protocol_enabled) {
         candidates.push_back(potentials[i]);
       }
     }
   }
 
   if (!candidates.empty()) {
     SignalCandidatesReady(this, candidates);
   }
 }
 
@@ skipping 595 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