Prepare for ICE-renomination

webrtc/p2p/base/port.cc

Index: webrtc/p2p/base/port.cc
diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc
index b6173ebf45768be331c589631f81f9aee8ccbc80..46743f4f1a566adb3eacd4ce21665bc839b332b9 100644
--- a/webrtc/p2p/base/port.cc
+++ b/webrtc/p2p/base/port.cc
@@ -754,15 +754,17 @@ class ConnectionRequest : public StunRequest {
     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()));
@@ -825,12 +827,13 @@ Connection::Connection(Port* port,
     : 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),
@@ -838,8 +841,6 @@ Connection::Connection(Port* port,
       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),
@@ -1041,10 +1042,23 @@ void Connection::HandleBindingRequest(IceMessage* msg) {
   }
 
   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);
     }
   }
@@ -1178,9 +1192,11 @@ void Connection::UpdateState(int64_t now) {
 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_++;
@@ -1191,17 +1207,25 @@ void Connection::ReceivedPing() {
   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);
 }
@@ -1274,21 +1298,16 @@ std::string Connection::ToString() const {
   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 {
@@ -1309,20 +1328,16 @@ void Connection::OnConnectionRequestResponse(ConnectionRequest* request,
 
   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++;
 
@@ -1369,10 +1384,10 @@ void Connection::OnConnectionRequestTimeout(ConnectionRequest* request) {
 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++;