Keep on sending stun binding requests on zero-cost networks.

webrtc/p2p/base/stunport.cc

Index: webrtc/p2p/base/stunport.cc
diff --git a/webrtc/p2p/base/stunport.cc b/webrtc/p2p/base/stunport.cc
index 8f37dd5218bb745b5e06ab6d8b2574faefc9aa44..39f530894ead9a8a672bc40e07fc8ff32a934a4b 100644
--- a/webrtc/p2p/base/stunport.cc
+++ b/webrtc/p2p/base/stunport.cc
@@ -25,18 +25,15 @@ namespace cricket {
 // TODO: Move these to a common place (used in relayport too)
 const int KEEPALIVE_DELAY = 10 * 1000;  // 10 seconds - sort timeouts
 const int RETRY_TIMEOUT = 50 * 1000;    // ICE says 50 secs
-// Stop sending STUN binding requests after this amount of time
-// (in milliseconds) because the connection binding requests should keep
-// the NAT binding alive.
-const int KEEP_ALIVE_TIMEOUT = 2 * 60 * 1000;  // 2 minutes
+const int KEEPALIVE_LIFETIME = 2 * 60 * 1000;  // 2 minutes

[pthatcher1, 2016/02/26 22:33:52]

This could use a comment explaining what it is.

Plus, isn't this now just value for high-cost networks?  So shouldn't it be:

HIGH_COST_STUN_PORT_KEEPALIVE_LIFETIME

?

[honghaiz3, 2016/02/29 19:34:11]

Done.

 
 // Handles a binding request sent to the STUN server.
 class StunBindingRequest : public StunRequest {
  public:
   StunBindingRequest(UDPPort* port,
                      const rtc::SocketAddress& addr,
-                     uint32_t deadline)
-      : port_(port), server_addr_(addr), deadline_(deadline) {
+                     int lifetime)
+      : port_(port), server_addr_(addr), lifetime_(lifetime) {
     start_time_ = rtc::Time();
   }
 
@@ -63,10 +60,11 @@ class StunBindingRequest : public StunRequest {
     }
 
     // We will do a keep-alive regardless of whether this request succeeds.
-    // It will be stopped after |deadline_| mostly to conserve the battery life.
-    if (rtc::Time() <= deadline_) {
+    // It will be stopped after its lifetime has passed (note that a negative
+    // lifetime indicates it will be sent indefinitely).
+    if (lifetime_ < 0 || rtc::TimeDiff(rtc::Time(), start_time_) <= lifetime_) {
       port_->requests_.SendDelayed(
-          new StunBindingRequest(port_, server_addr_, deadline_),
+          new StunBindingRequest(port_, server_addr_, lifetime_),
           port_->stun_keepalive_delay());
     }
   }
@@ -85,9 +83,10 @@ class StunBindingRequest : public StunRequest {
     port_->OnStunBindingOrResolveRequestFailed(server_addr_);
 
     uint32_t now = rtc::Time();
-    if (now <= deadline_ && rtc::TimeDiff(now, start_time_) <= RETRY_TIMEOUT) {
+    if ((lifetime_ < 0 || rtc::TimeDiff(now, start_time_) <= lifetime_) &&

[pthatcher1, 2016/02/26 22:33:52]

It seems like we could use a helper method of BeyondLifetime(now).

[honghaiz3, 2016/02/29 19:34:11]

Done, although this will lose the benefit of not requiring to evaluate
rtc::Time() if lifetime is infinite. 

+        rtc::TimeDiff(now, start_time_) <= RETRY_TIMEOUT) {
       port_->requests_.SendDelayed(
-          new StunBindingRequest(port_, server_addr_, deadline_),
+          new StunBindingRequest(port_, server_addr_, lifetime_),
           port_->stun_keepalive_delay());
     }
   }
@@ -104,7 +103,9 @@ class StunBindingRequest : public StunRequest {
   UDPPort* port_;
   const rtc::SocketAddress server_addr_;
   uint32_t start_time_;
-  uint32_t deadline_;
+  // The time duration for which this request will be rescheduled.
+  // A negative value means this request will be rescheduled indefinitely.
+  int lifetime_;
 };
 
 UDPPort::AddressResolver::AddressResolver(
@@ -397,8 +398,12 @@ void UDPPort::SendStunBindingRequest(const rtc::SocketAddress& stun_addr) {
   } else if (socket_->GetState() == rtc::AsyncPacketSocket::STATE_BOUND) {
     // Check if |server_addr_| is compatible with the port's ip.
     if (IsCompatibleAddress(stun_addr)) {
-      requests_.Send(new StunBindingRequest(this, stun_addr,
-                                            rtc::Time() + KEEP_ALIVE_TIMEOUT));
+      // If this is a zero-cost network, it will keep on sending STUN binding
+      // requests indefinitely to keep the NAT binding alive. Otherwise,
+      // stop sending STUN binding requests after KEEPALIVE_LIFETIME.
+      int keepalive_lifetime = (network_cost() == 0) ? -1 : KEEPALIVE_LIFETIME;

[pthatcher1, 2016/02/26 22:33:52]

I think we should make a constant for UNLIMITED_LIFETIME to make this a bit more
readable.

You could use it above also as "if (lifetime_ <= UNLIMITED_LIFETIME)"

[honghaiz3, 2016/02/29 19:34:11]

Done. Used INIFINTE_LIFETIME. Also the logic changes to if (lifetime_ ==
INFINITE_LIFETIME || ....), which Looks more readable. 

+      requests_.Send(
+          new StunBindingRequest(this, stun_addr, keepalive_lifetime));
     } else {
       // Since we can't send stun messages to the server, we should mark this
       // port ready.