Prepare for ICE-renomination

webrtc/p2p/base/port.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 736 matching lines @@
     }
     uint32_t network_info = connection_->port()->Network()->id();
     network_info = (network_info << 16) | connection_->port()->network_cost();
     request->AddAttribute(
         new StunUInt32Attribute(STUN_ATTR_NETWORK_INFO, network_info));
 
     // Adding ICE_CONTROLLED or ICE_CONTROLLING attribute based on the role.
     if (connection_->port()->GetIceRole() == ICEROLE_CONTROLLING) {
       request->AddAttribute(new StunUInt64Attribute(
           STUN_ATTR_ICE_CONTROLLING, connection_->port()->IceTiebreaker()));
-      // Since we are trying aggressive nomination, sending USE-CANDIDATE
-      // attribute in every ping.
-      // If we are dealing with a ice-lite end point, nomination flag
-      // in Connection will be set to false by default. Once the connection
-      // becomes "best connection", nomination flag will be turned on.
+      // We should have either USE_CANDIDATE attribute or ICE_NOMINATION_VALUE
+      // attribute but not both. That was enforced in p2ptransportchannel.
       if (connection_->use_candidate_attr()) {
         request->AddAttribute(new StunByteStringAttribute(
             STUN_ATTR_USE_CANDIDATE));
       }
+      if (connection_->nominating_value() >
+          connection_->acknowledged_nominating_value()) {

[pthatcher1, 2016/07/28 22:57:26]

I think it would be more robust if it were:

connection_->nomination() && connection_->nomination() !=
connection_->acked_nomination()

[honghaiz3, 2016/08/03 04:46:56]

Done.

+        request->AddAttribute(new StunUInt32Attribute(
+            STUN_ATTR_NOMINATION_VALUE, connection_->nominating_value()));

[pthatcher1, 2016/07/28 22:57:26]

I'd prefer STUN_ATTR_NOMINATION.

[honghaiz3, 2016/08/03 04:46:56]

Done.

+      }
     } else if (connection_->port()->GetIceRole() == ICEROLE_CONTROLLED) {
       request->AddAttribute(new StunUInt64Attribute(
           STUN_ATTR_ICE_CONTROLLED, connection_->port()->IceTiebreaker()));
     } else {
       ASSERT(false);
     }
 
     // Adding PRIORITY Attribute.
     // Changing the type preference to Peer Reflexive and local preference
     // and component id information is unchanged from the original priority.
@@ skipping 42 matching lines @@
 //
 // Connection
 //
 
 Connection::Connection(Port* port,
                        size_t index,
                        const Candidate& remote_candidate)
     : port_(port),
       local_candidate_index_(index),
       remote_candidate_(remote_candidate),
+      recv_rate_tracker_(100, 10u),
+      send_rate_tracker_(100, 10u),
       write_state_(STATE_WRITE_INIT),
       receiving_(false),
       connected_(true),
       pruned_(false),
       use_candidate_attr_(false),
-      nominated_(false),
       remote_ice_mode_(ICEMODE_FULL),
       requests_(port->thread()),
       rtt_(DEFAULT_RTT),
       last_ping_sent_(0),
       last_ping_received_(0),
       last_data_received_(0),
       last_ping_response_received_(0),
-      recv_rate_tracker_(100, 10u),
-      send_rate_tracker_(100, 10u),
       reported_(false),
       state_(STATE_WAITING),
       receiving_timeout_(WEAK_CONNECTION_RECEIVE_TIMEOUT),
       time_created_ms_(rtc::TimeMillis()) {
   // All of our connections start in WAITING state.
   // TODO(mallinath) - Start connections from STATE_FROZEN.
   // Wire up to send stun packets
   requests_.SignalSendPacket.connect(this, &Connection::OnSendStunPacket);
   LOG_J(LS_INFO, this) << "Connection created";
 }
@@ skipping 181 matching lines @@
 
   // This is a validated stun request from remote peer.
   port_->SendBindingResponse(msg, remote_addr);
 
   // If it timed out on writing check, start up again
   if (!pruned_ && write_state_ == STATE_WRITE_TIMEOUT) {
     set_write_state(STATE_WRITE_INIT);
   }
 
   if (port_->GetIceRole() == ICEROLE_CONTROLLED) {
-    const StunByteStringAttribute* use_candidate_attr =
-        msg->GetByteString(STUN_ATTR_USE_CANDIDATE);
-    if (use_candidate_attr) {
-      set_nominated(true);
+    const StunUInt32Attribute* nomination_value_attr =
+        msg->GetUInt32(STUN_ATTR_NOMINATION_VALUE);
+    int nomination_value = 0;
+    if (nomination_value_attr) {
+      nomination_value = static_cast<int>(nomination_value_attr->value());
+      if (nomination_value < 0) {
+        LOG(LS_ERROR) << "Negative nomination value: " << nomination_value;

[pthatcher1, 2016/07/28 22:57:26]

Should we use a uint32_t instead of an int?  It can't be negative. 

[honghaiz3, 2016/08/03 04:46:56]

I think the code style says we should not use uint32 because a value does not
take negative values. It recommends using ASSERT but here we don't want to
assert error simply because remote sides sends an invalid attribute. 

[pthatcher1, 2016/08/03 22:13:25]

It's not just because it can't be negative.  What's the range of possible values
for the STUN attribute that we receive from the controlling side?  Does it use
the entire 32-bit space (as the call to GetUint32 implies)?  If so, on a 32-bit
machine, if an int is 32 bits and the remote side sends us a nomination value of
2**31+1, we'll think it's negative, which would be bad.   Am I correct?

[honghaiz3, 2016/08/03 23:39:51]

Done, although I don't think it should go to the negative range (>2billion
connection switches?).

+      }
+    } else {
+      const StunByteStringAttribute* use_candidate_attr =
+          msg->GetByteString(STUN_ATTR_USE_CANDIDATE);
+      if (use_candidate_attr) {
+        nomination_value = 1;
+      }
+    }
+    if (nomination_value > 0) {
+      set_nominated_value(nomination_value);
       SignalNominated(this);
     }
   }
   // Set the remote cost if the network_info attribute is available.
   // Note: If packets are re-ordered, we may get incorrect network cost
   // temporarily, but it should get the correct value shortly after that.
   const StunUInt32Attribute* network_attr =
       msg->GetUInt32(STUN_ATTR_NETWORK_INFO);
   if (network_attr) {
     uint32_t network_info = network_attr->value();
@@ skipping 113 matching lines @@
   // Update the receiving state.
   UpdateReceiving(now);
   if (dead(now)) {
     Destroy();
   }
 }
 
 void Connection::Ping(int64_t now) {
   last_ping_sent_ = now;
   ConnectionRequest *req = new ConnectionRequest(this);
-  pings_since_last_response_.push_back(SentPing(req->id(), now));
+  pings_since_last_response_.push_back(
+      SentPing(req->id(), now, nominating_value_));
   LOG_J(LS_VERBOSE, this) << "Sending STUN ping "
-                          << ", id=" << rtc::hex_encode(req->id());
+                          << ", id=" << rtc::hex_encode(req->id())
+                          << ", nominating_value=" << nominating_value_;
   requests_.Send(req);
   state_ = STATE_INPROGRESS;
   num_pings_sent_++;
 }
 
 void Connection::ReceivedPing() {
   last_ping_received_ = rtc::TimeMillis();
   UpdateReceiving(last_ping_received_);
 }
 
-void Connection::ReceivedPingResponse(int rtt) {
+void Connection::ReceivedPingResponse(int rtt, const std::string& request_id) {
   // We've already validated that this is a STUN binding response with
   // the correct local and remote username for this connection.
   // So if we're not already, become writable. We may be bringing a pruned
   // connection back to life, but if we don't really want it, we can always
   // prune it again.
+  auto iter = std::find_if(
+      pings_since_last_response_.begin(), pings_since_last_response_.end(),
+      [request_id](const SentPing& ping) { return ping.id == request_id; });
+  if (iter != pings_since_last_response_.end() &&
+      iter->nomination_value > acknowledged_nominating_value_) {
+    acknowledged_nominating_value_ = iter->nomination_value;
+  }
+
+  pings_since_last_response_.clear();
   last_ping_response_received_ = rtc::TimeMillis();
   UpdateReceiving(last_ping_response_received_);
   set_write_state(STATE_WRITABLE);
   set_state(STATE_SUCCEEDED);
-  pings_since_last_response_.clear();
   rtt_samples_++;
   rtt_ = (RTT_RATIO * rtt_ + rtt) / (RTT_RATIO + 1);
 }
 
 bool Connection::dead(int64_t now) const {
   if (last_received() > 0) {
     // If it has ever received anything, we keep it alive until it hasn't
     // received anything for DEAD_CONNECTION_RECEIVE_TIMEOUT. This covers the
     // normal case of a successfully used connection that stops working. This
     // also allows a remote peer to continue pinging over a locally inactive
@@ skipping 52 matching lines @@
   };
   const std::string ICESTATE[4] = {
     "W",  // STATE_WAITING
     "I",  // STATE_INPROGRESS
     "S",  // STATE_SUCCEEDED
     "F"   // STATE_FAILED
   };
   const Candidate& local = local_candidate();
   const Candidate& remote = remote_candidate();
   std::stringstream ss;
-  ss << "Conn[" << ToDebugId()
-     << ":" << port_->content_name()
-     << ":" << local.id() << ":" << local.component()
-     << ":" << local.generation()
-     << ":" << local.type() << ":" << local.protocol()
-     << ":" << local.address().ToSensitiveString()
-     << "->" << remote.id() << ":" << remote.component()
-     << ":" << remote.priority()
-     << ":" << remote.type() << ":"
-     << remote.protocol() << ":" << remote.address().ToSensitiveString() << "|"
-     << CONNECT_STATE_ABBREV[connected()]
-     << RECEIVE_STATE_ABBREV[receiving()]
-     << WRITE_STATE_ABBREV[write_state()]
-     << ICESTATE[state()] << "|"
-     << priority() << "|";
+  ss << "Conn[" << ToDebugId() << ":" << port_->content_name() << ":"
+     << local.id() << ":" << local.component() << ":" << local.generation()
+     << ":" << local.type() << ":" << local.protocol() << ":"
+     << local.address().ToSensitiveString() << "->" << remote.id() << ":"
+     << remote.component() << ":" << remote.priority() << ":" << remote.type()
+     << ":" << remote.protocol() << ":" << remote.address().ToSensitiveString()
+     << "|" << CONNECT_STATE_ABBREV[connected()]
+     << RECEIVE_STATE_ABBREV[receiving()] << WRITE_STATE_ABBREV[write_state()]
+     << ICESTATE[state()] << "|" << nominated_value() << "|" << priority()
+     << "|";
   if (rtt_ < DEFAULT_RTT) {
     ss << rtt_ << "]";
   } else {
     ss << "-]";
   }
   return ss.str();
 }
 
 std::string Connection::ToSensitiveString() const {
   return ToString();
 }
 
 void Connection::OnConnectionRequestResponse(ConnectionRequest* request,
                                              StunMessage* response) {
   // Log at LS_INFO if we receive a ping response on an unwritable
   // connection.
   rtc::LoggingSeverity sev = !writable() ? rtc::LS_INFO : rtc::LS_VERBOSE;
 
   int rtt = request->Elapsed();
 
-  ReceivedPingResponse(rtt);
-
   if (LOG_CHECK_LEVEL_V(sev)) {
-    bool use_candidate = (
-        response->GetByteString(STUN_ATTR_USE_CANDIDATE) != nullptr);
     std::string pings;
     PrintPingsSinceLastResponse(&pings, 5);
     LOG_JV(sev, this) << "Received STUN ping response"
                       << ", id=" << rtc::hex_encode(request->id())
                       << ", code=0"  // Makes logging easier to parse.
                       << ", rtt=" << rtt
-                      << ", use_candidate=" << use_candidate
                       << ", pings_since_last_response=" << pings;
   }
+  ReceivedPingResponse(rtt, request->id());
 
   stats_.recv_ping_responses++;
 
   MaybeAddPrflxCandidate(request, response);
 }
 
 void Connection::OnConnectionRequestErrorResponse(ConnectionRequest* request,
                                                   StunMessage* response) {
   const StunErrorCodeAttribute* error_attr = response->GetErrorCode();
   int error_code = STUN_ERROR_GLOBAL_FAILURE;
@@ skipping 26 matching lines @@
   // Log at LS_INFO if we miss a ping on a writable connection.
   rtc::LoggingSeverity sev = writable() ? rtc::LS_INFO : rtc::LS_VERBOSE;
   LOG_JV(sev, this) << "Timing-out STUN ping "
                     << rtc::hex_encode(request->id())
                     << " after " << request->Elapsed() << " ms";
 }
 
 void Connection::OnConnectionRequestSent(ConnectionRequest* request) {
   // Log at LS_INFO if we send a ping on an unwritable connection.
   rtc::LoggingSeverity sev = !writable() ? rtc::LS_INFO : rtc::LS_VERBOSE;
-  bool use_candidate = use_candidate_attr();
   LOG_JV(sev, this) << "Sent STUN ping"
                     << ", id=" << rtc::hex_encode(request->id())
-                    << ", use_candidate=" << use_candidate;
+                    << ", use_candidate=" << use_candidate_attr()
+                    << ", nominating value=" << nominating_value();
   stats_.sent_ping_requests_total++;
   if (stats_.recv_ping_responses == 0) {
     stats_.sent_ping_requests_before_first_response++;
   }
 }
 
 void Connection::HandleRoleConflictFromPeer() {
   port_->SignalRoleConflict(port_);
 }
 
@@ skipping 144 matching lines @@
     ASSERT(sent < 0);
     error_ = port_->GetError();
     stats_.sent_discarded_packets++;
   } else {
     send_rate_tracker_.AddSamples(sent);
   }
   return sent;
 }
 
 }  // namespace cricket