Do not switch best connection on the controlled side too frequently

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 c0e8d9770d8b11fb5e63b3c0221247d6eb2e7e82..3a00738d33dd3e92e2cf604ce33eb0325f727b9b 100644
--- a/webrtc/p2p/base/port.cc
+++ b/webrtc/p2p/base/port.cc
@@ -956,11 +956,14 @@ void Connection::OnReadPacket(
     recv_rate_tracker_.AddSamples(size);
     SignalReadPacket(this, data, size, packet_time);
 
-    // If timed out sending writability checks, start up again
-    if (!pruned_ && (write_state_ == STATE_WRITE_TIMEOUT)) {
+    // If the connection has timed out, it might be because it was pruned or
+    // it failed the writability check. Start it up again if it was not pruned
+    // (so it is due to writability check failure), or it was pruned but it has
+    // been nominated by the controlling side.

[pthatcher1, 2016/06/22 19:18:37]

Perhaps the wording could be thus:

If writability timed out, it could be either because the connection was pruned
or because checks are failing.  If it's the later (not pruned), then reset the
connection state to the INIT state since clearly we can receive data on this
connection.  If it's the former (pruned) and the connection was nominated, do
the same, but also unprune the connection.  

[honghaiz3, 2016/06/22 20:01:24]

All changes here are removed. 
Acknowledged.

+    if (write_state_ == STATE_WRITE_TIMEOUT && (nominated() || !pruned_)) {
       LOG(LS_WARNING) << "Received a data packet on a timed-out Connection. "
                       << "Resetting state to STATE_WRITE_INIT.";
-      set_write_state(STATE_WRITE_INIT);
+      ResetWriteStateAndUnprune();

[pthatcher1, 2016/06/22 19:18:37]

Even though I corrected the comment, I don't understand why this is the behavior
that we want.  With aggressive nomination *all* of the connections will be
nominated, which means reset the write state for *all* connections that receive
media.  So basically the rule would be "unprune if we receive media from the
controlling side".   Do we really want that?  If so, can we save it for a future
CL?  I'd rather not block this CL on trying to get this part right.

And even if we do want this behavior, it would be more clear as this:
if (pruned_ && ice_role_ == ICE_CONTROLLED) {
  // If controlling side is sending on pruned candidate pair;  we should
probably unprune.
  Unprune();
}
if (!pruned_ && (write_state_ == STATE_WRITE_TIMEOUT)) {
  set_write_state(STATE_WRITE_INIT);
}


But even then, what prevents the candidate pair from getting re-pruned shortly
after this?  

[honghaiz3, 2016/06/22 20:01:24]

Acknowledged.

     }
   } else if (!msg) {
     // The packet was STUN, but failed a check and was handled internally.
@@ -1035,19 +1038,21 @@ void Connection::HandleBindingRequest(IceMessage* msg) {
   // 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);
-  }
-
+  bool nominated = false;
   if (port_->GetIceRole() == ICEROLE_CONTROLLED) {
-    const StunByteStringAttribute* use_candidate_attr =
-        msg->GetByteString(STUN_ATTR_USE_CANDIDATE);
-    if (use_candidate_attr) {
+    bool nominated = msg->GetByteString(STUN_ATTR_USE_CANDIDATE) != nullptr;

[pthatcher1, 2016/06/22 19:18:37]

You are making the variable, so the outer |nominated| will always be false.

[honghaiz3, 2016/06/22 20:01:24]

Sorry for the mistake. 
That means the Unprune is indeed not so much useful (the issue will still happen
in some corner cases). 

+    if (nominated) {
       set_nominated(true);
       SignalNominated(this);
     }
   }
+  // If it timed out on writing check, start up again if it is not pruned or
+  // it is just nominated now, because the nomination may increase its priority
+  // in the pruning process.
+  if (write_state_ == STATE_WRITE_TIMEOUT && (nominated || !pruned_)) {
+    ResetWriteStateAndUnprune();
+  }

[pthatcher1, 2016/06/22 19:18:37]

When the controlling side is aggressive *all* the candidate pairs will be
nominated, so we'd basically be unpruning any candidate pairs that the
controlling is pinging, which means we never prune until we stop getting pings. 
Again, this change doesn't seem necessary for this CL, so could we leave it out
and discuss it later?

Also, with the current code, nomianted is always false, so it's never triggered
anyway.  And it's still passing the unit tests :).

[honghaiz3, 2016/06/22 20:01:24]

Acknowledged.

+
   // 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.
@@ -1081,7 +1086,6 @@ void Connection::Prune() {
 }
 
 void Connection::Destroy() {
-  LOG_J(LS_VERBOSE, this) << "Connection destroyed";
   port_->thread()->Post(RTC_FROM_HERE, this, MSG_DELETE);
 }
 
@@ -1424,6 +1428,14 @@ int64_t Connection::last_received() const {
              std::max(last_ping_received_, last_ping_response_received_));
 }
 
+void Connection::ResetWriteStateAndUnprune() {
+  write_state_ = STATE_WRITE_INIT;
+  LOG_J(INFO, this) << "Connection unpruned";
+  if (pruned_) {
+    pruned_ = false;
+  }
+}
+
 ConnectionInfo Connection::stats() {
   stats_.recv_bytes_second = round(recv_rate_tracker_.ComputeRate());
   stats_.recv_total_bytes = recv_rate_tracker_.TotalSampleCount();