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 0add358ec91392917091e957dfc39d28224be5af..b910d1f84d404a0464af78b6640f6bcf77254d4b 100644
--- a/webrtc/p2p/base/port.cc
+++ b/webrtc/p2p/base/port.cc
@@ -956,11 +956,13 @@ 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)) {
+    // Start up again if timed out sending writability checks, or
+    // this is nominated on the controlled side because on the controlled side,
+    // because receiving data will increase its priority in the pruning process.

[pthatcher1, 2016/06/22 06:36:34]

I don't understand this comment.

[honghaiz3, 2016/06/22 08:03:17]

Revised.

+    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 06:36:34]

So the change is to unprune when we received media on a nominated pair?  Why
would we need to do that?   Is that just so that we unprune backup candidate
pairs on the controlled side when the controlling side switches to one?  If so,
is this just here until we're fully switched over to "renomination"?   Also, can
the comment reflect that such logic is only for backup candidate pairs of
aggressively controlling peers?  Finally, why did we not need this logic before?
 

[honghaiz3, 2016/06/22 08:03:17]

I think there is a bug in the existing code that sometimes a connection used by
the controlling side may be pruned before it is selected by the controlled side.
Consider the case that two connections having the same candidate priority.
Whichever first becomes writable will make the other one pruned in the
controlled side but the other one may be the one chosen by the controlling side.
Yes we do have the logic on the controlled side that it only start to prune only
if it selected connection is nominated but the condition may not be so much
useful because with aggressive nomination, it nominated every connection before
the best connection becomes writable. 

     }
   } else if (!msg) {
     // The packet was STUN, but failed a check and was handled internally.
@@ -1035,19 +1037,25 @@ 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);
-  }
-
+  // Whether it is nominated in this request but not nominated before.
+  bool just_nominated = false;
   if (port_->GetIceRole() == ICEROLE_CONTROLLED) {
-    const StunByteStringAttribute* use_candidate_attr =
-        msg->GetByteString(STUN_ATTR_USE_CANDIDATE);
-    if (use_candidate_attr) {
+    bool use_candidate = msg->GetByteString(STUN_ATTR_USE_CANDIDATE) != nullptr;
+    if (use_candidate) {
+      if (!nominated()) {
+        just_nominated = true;
+      }
       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 && (just_nominated || !pruned_)) {

[Taylor Brandstetter, 2016/06/21 18:33:26]

Why is the connection only restored if it's "just_nominated"? The controlling
agent could switch connections back and forth over the course of a session due
to RTT, right?

[pthatcher1, 2016/06/22 06:36:34]

Yeah, I don't understand the "just nominated" logic either.  If we don't prune
until a candidate pair is nominated and writable, then the fact that the
candidate pair was pruned means a better candidate pair was writable and
nominated.  So why would be unprune this candidate pair, which we know to be
worse?  It seems contrary to the concept of pruning when the controlling side is
aggressive.  We're just going to prune it again once the better candidate pair
is nominated again.

[honghaiz3, 2016/06/22 08:03:17]

I think you are right. I may have over-thought this. 

[honghaiz3, 2016/06/22 08:03:17]

See my reply in the above comments. 

+    ResetWriteStateAndUnprune();
+  }
+
   // 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.