Added detection timeouts and usage of Retry-After HTTP header.

chrome/browser/chromeos/net/network_portal_detector.cc

Index: chrome/browser/chromeos/net/network_portal_detector.cc
diff --git a/chrome/browser/chromeos/net/network_portal_detector.cc b/chrome/browser/chromeos/net/network_portal_detector.cc
index a70418bbfd120d80ea06ce1abe875c3fe0e5e9f9..54f2f13362bf22eeea3f2b5d82437b5d3136bd1f 100644
--- a/chrome/browser/chromeos/net/network_portal_detector.cc
+++ b/chrome/browser/chromeos/net/network_portal_detector.cc
@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
+#include "base/message_loop.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/cros/cros_library.h"
 #include "chrome/common/chrome_switches.h"
@@ -20,6 +21,10 @@ namespace chromeos {
 
 namespace {
 
+const int kMaxRequestAttempts = 3;

[Nikita (slow), 2012/11/20 12:58:45]

nit: Please add comments for these contants

[ygorshenin1, 2012/11/21 09:08:06]

Done.

+const int kMinTimeBetweenAttemptsSec = 3;
+const int kRequestTimeoutSec = 10;
+
 std::string CaptivePortalStateString(
     NetworkPortalDetector::CaptivePortalState state) {
   switch (state) {
@@ -44,7 +49,12 @@ NetworkPortalDetector* g_network_portal_detector = NULL;
 
 NetworkPortalDetector::NetworkPortalDetector(
     const scoped_refptr<net::URLRequestContextGetter>& request_context)
-    : test_url_(CaptivePortalDetector::kDefaultURL) {
+    : test_url_(CaptivePortalDetector::kDefaultURL),
+      weak_ptr_factory_(this),
+      attempt_count_(0),
+      min_time_between_attempts_(
+          base::TimeDelta::FromSeconds(kMinTimeBetweenAttemptsSec)),
+      request_timeout_(base::TimeDelta::FromSeconds(kRequestTimeoutSec)) {
   captive_portal_detector_.reset(
       new CaptivePortalDetector(request_context));
 }
@@ -64,6 +74,9 @@ void NetworkPortalDetector::Init() {
 void NetworkPortalDetector::Shutdown() {
   DCHECK(CalledOnValidThread());
 
+  detection_task_.Cancel();
+  detection_timeout_.Cancel();
+
   captive_portal_detector_->Cancel();
   captive_portal_detector_.reset();
   observers_.Clear();
@@ -74,17 +87,21 @@ void NetworkPortalDetector::Shutdown() {
 
 void NetworkPortalDetector::AddObserver(Observer* observer) {
   DCHECK(CalledOnValidThread());
+
   if (!observers_.HasObserver(observer))
     observers_.AddObserver(observer);
 }
 
 void NetworkPortalDetector::RemoveObserver(Observer* observer) {
   DCHECK(CalledOnValidThread());
+
   observers_.RemoveObserver(observer);
 }
 
 NetworkPortalDetector::CaptivePortalState
 NetworkPortalDetector::GetCaptivePortalState(const Network* network) {
+  DCHECK(CalledOnValidThread());
+
   if (!network)
     return CAPTIVE_PORTAL_STATE_UNKNOWN;
   CaptivePortalStateMap::const_iterator it =
@@ -109,22 +126,27 @@ void NetworkPortalDetector::OnNetworkManagerChanged(NetworkLibrary* cros) {
 
   if (active_network_id_ != new_active_network_id) {
     active_network_id_ = new_active_network_id;
-    if (IsCheckingForPortal()) {
-      // Network is changed, so detection results will be incorrect.
-      state_ = STATE_CHECKING_FOR_PORTAL_NETWORK_CHANGED;
-    } else if (Network::IsConnectedState(connection_state)) {
-      // Start captive portal detection, if possible.
-      DetectCaptivePortal();
+    attempt_count_ = 0;
+    if (IsPortalCheckPending()) {
+      detection_task_.Cancel();
+      detection_timeout_.Cancel();
+    } else if (IsCheckingForPortal()) {
+      captive_portal_detector_->Cancel();
     }
-  } else if (!IsCheckingForPortal() &&
-             Network::IsConnectedState(connection_state)) {
+    state_ = STATE_IDLE;
+  }
+  if (!IsCheckingForPortal() && !IsPortalCheckPending() &&
+      Network::IsConnectedState(connection_state) &&
+      attempt_count_ < kMaxRequestAttempts) {
     DCHECK(active_network);
+
     // Initiate Captive Portal detection only if network's captive
-    // portal state is unknown (e.g. for freshly created networks) or offline.
+    // portal state is unknown (e.g. for freshly created networks) or
+    // offline.
     CaptivePortalState state = GetCaptivePortalState(active_network);
     if (state == CAPTIVE_PORTAL_STATE_UNKNOWN ||
         state == CAPTIVE_PORTAL_STATE_OFFLINE) {
-      DetectCaptivePortal();
+      DetectCaptivePortal(base::TimeDelta());
     }
   }
 }
@@ -150,41 +172,88 @@ bool NetworkPortalDetector::IsEnabled() {
       switches::kDisableChromeCaptivePortalDetector);
 }
 
-void NetworkPortalDetector::DetectCaptivePortal() {
+void NetworkPortalDetector::DetectCaptivePortal(const base::TimeDelta& delay) {
+  DCHECK(!IsPortalCheckPending());
+  DCHECK(!IsCheckingForPortal());
+  DCHECK(attempt_count_ < kMaxRequestAttempts);
+

[Nikita (slow), 2012/11/20 12:58:45]

I think it makes sense to clear tasks here just in case.

detection_task_.Cancel()
detection_timeout_.Cancel();

[ygorshenin1, 2012/11/21 09:08:06]

Done.

+  state_ = STATE_PORTAL_CHECK_PENDING;
+
+  next_attempt_delay_ = delay;
+  if (attempt_count_ > 0) {
+    base::TimeTicks now = GetCurrentTimeTicks();
+    base::TimeDelta elapsed_time;
+    if (now > attempt_start_time_)
+      elapsed_time = now - attempt_start_time_;
+    if (elapsed_time < min_time_between_attempts_ &&
+        min_time_between_attempts_ - elapsed_time > next_attempt_delay_) {
+      next_attempt_delay_ = min_time_between_attempts_ - elapsed_time;
+    }
+  }
+  detection_task_.Reset(
+      base::Bind(&NetworkPortalDetector::DetectCaptivePortalTask,
+                 weak_ptr_factory_.GetWeakPtr()));
+  MessageLoop::current()->PostDelayedTask(FROM_HERE,
+                                          detection_task_.callback(),
+                                          next_attempt_delay_);
+}
+
+void NetworkPortalDetector::DetectCaptivePortalTask() {
+  DCHECK(IsPortalCheckPending());
+
   state_ = STATE_CHECKING_FOR_PORTAL;
+
+  ++attempt_count_;
+  attempt_start_time_ = GetCurrentTimeTicks();
+
+  LOG(INFO) << "Portal detection starting attempt " << attempt_count_

[Nikita (slow), 2012/11/20 12:58:45]

Note that by default LOG(INFO) is not captured in Chrome log.

Makes sense to change logging to VLOG() for portal service and increase log
level for this module.

[ygorshenin1, 2012/11/21 09:08:06]

Done.

+            << " of " << kMaxRequestAttempts;
+
   captive_portal_detector_->DetectCaptivePortal(
       test_url_,
       base::Bind(&NetworkPortalDetector::OnPortalDetectionCompleted,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
+  detection_timeout_.Reset(
+      base::Bind(&NetworkPortalDetector::PortalDetectionTimeout,
+                 weak_ptr_factory_.GetWeakPtr()));
+  MessageLoop::current()->PostDelayedTask(FROM_HERE,
+                                          detection_timeout_.callback(),
+                                          request_timeout_);
+}
+
+void NetworkPortalDetector::PortalDetectionTimeout() {
+  DCHECK(CalledOnValidThread());
+  DCHECK(IsCheckingForPortal());
+
+  LOG(ERROR) << "Portal detection timeout";
+
+  captive_portal_detector_->Cancel();
+  CaptivePortalDetector::Results results;
+  results.result = captive_portal::RESULT_NO_RESPONSE;
+  OnPortalDetectionCompleted(results);
 }
 
 void NetworkPortalDetector::OnPortalDetectionCompleted(
     const CaptivePortalDetector::Results& results) {
   DCHECK(CalledOnValidThread());
-
   DCHECK(IsCheckingForPortal());
 
+  LOG(ERROR) << "On portal detection completed";

[Nikita (slow), 2012/11/20 12:58:45]

LOG(INFO) or VLOG - see above.

[ygorshenin1, 2012/11/21 09:08:06]

Done.

+
+  state_ = STATE_IDLE;
+  detection_timeout_.Cancel();
+
   NetworkLibrary* cros = CrosLibrary::Get()->GetNetworkLibrary();
   const Network* active_network = cros->active_network();
   if (!active_network)
     return;
 
-  if (state_ == STATE_CHECKING_FOR_PORTAL_NETWORK_CHANGED) {
-    // Can't use detection results, because network was changed. So
-    // retry portal detection for the active network or, if possible.
-    if (Network::IsConnectedState(active_network->connection_state()) &&
-        GetCaptivePortalState(active_network) ==
-        CAPTIVE_PORTAL_STATE_UNKNOWN) {
-      DetectCaptivePortal();
-    }
-    return;
-  }
-
-  state_ = STATE_IDLE;
-
   switch (results.result) {
     case captive_portal::RESULT_NO_RESPONSE:
-      SetCaptivePortalState(active_network, CAPTIVE_PORTAL_STATE_OFFLINE);
+      if (attempt_count_ >= kMaxRequestAttempts)
+        SetCaptivePortalState(active_network, CAPTIVE_PORTAL_STATE_OFFLINE);
+      else
+        DetectCaptivePortal(results.retry_after_delta);
       break;
     case captive_portal::RESULT_INTERNET_CONNECTED:
       SetCaptivePortalState(active_network, CAPTIVE_PORTAL_STATE_ONLINE);
@@ -197,14 +266,18 @@ void NetworkPortalDetector::OnPortalDetectionCompleted(
   }
 }
 
+bool NetworkPortalDetector::IsPortalCheckPending() const {
+  return state_ == STATE_PORTAL_CHECK_PENDING;
+}
+
 bool NetworkPortalDetector::IsCheckingForPortal() const {
-  return state_ == STATE_CHECKING_FOR_PORTAL ||
-      state_ == STATE_CHECKING_FOR_PORTAL_NETWORK_CHANGED;
+  return state_ == STATE_CHECKING_FOR_PORTAL;
 }
 
 void NetworkPortalDetector::SetCaptivePortalState(const Network* network,
                                                   CaptivePortalState state) {
   DCHECK(network);
+
   CaptivePortalStateMap::const_iterator it =
       captive_portal_state_map_.find(network->unique_id());
   if (it == captive_portal_state_map_.end() ||
@@ -222,4 +295,11 @@ void NetworkPortalDetector::NotifyPortalStateChanged(const Network* network,
   FOR_EACH_OBSERVER(Observer, observers_, OnPortalStateChanged(network, state));
 }
 
+base::TimeTicks NetworkPortalDetector::GetCurrentTimeTicks() const {
+  if (time_ticks_for_testing_.is_null())
+    return base::TimeTicks::Now();
+  else
+    return time_ticks_for_testing_;
+}
+
 }  // namespace chromeos