Bug 1332271 - Do not cancel timer when captive portal request times out. r=mcmanus, a=jcristau
authorValentin Gosu <valentin.gosu@gmail.com>
Tue, 24 Jan 2017 15:43:30 +0100
changeset 377933 c81058f0cbf8889eddd04058f9636e021eb43368
parent 377932 eb17c495d36f8cbce8e129afe4642c56d1bb0da6
child 377934 99104f0554c30a6fa277bf44a47e6179e336b4f9
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmcmanus, jcristau
bugs1332271
milestone53.0a2
Bug 1332271 - Do not cancel timer when captive portal request times out. r=mcmanus, a=jcristau The fact that nsICaptivePortalServiceCallback.complete got called with a true argument made it difficult to be sure when the you were actually in a captive portal, and when the network timed out. Moreover, one artefact of the initial plan for the captive portal service was that we'd cancel the timer after the first request succeeded, making the backoff mechanism not run at all, and only checked for CP when instructed by nsIOService. This patch changes captivedetect.js to send back success=false when the retry count is exceeded - it's equivalent to an aborted request anyway - doesn't cancel the timeer, and changes how we set the current state of the captive portal. MozReview-Commit-ID: 4RV50KPbEdt
netwerk/base/CaptivePortalService.cpp
toolkit/components/captivedetect/captivedetect.js
toolkit/components/captivedetect/test/unit/test_captive_portal_not_found_404.js
--- a/netwerk/base/CaptivePortalService.cpp
+++ b/netwerk/base/CaptivePortalService.cpp
@@ -40,16 +40,18 @@ CaptivePortalService::CaptivePortalServi
   , mMaxInterval(25*kDefaultInterval)
   , mBackoffFactor(5.0)
 {
   mLastChecked = TimeStamp::Now();
 }
 
 CaptivePortalService::~CaptivePortalService()
 {
+  LOG(("CaptivePortalService::~CaptivePortalService isParentProcess:%d\n",
+       XRE_GetProcessType() == GeckoProcessType_Default));
 }
 
 nsresult
 CaptivePortalService::PerformCheck()
 {
   LOG(("CaptivePortalService::PerformCheck mRequestInProgress:%d mInitialized:%d mStarted:%d\n",
         mRequestInProgress, mInitialized, mStarted));
   // Don't issue another request if last one didn't complete
@@ -71,16 +73,17 @@ CaptivePortalService::PerformCheck()
   mRequestInProgress = true;
   mCaptivePortalDetector->CheckCaptivePortal(kInterfaceName, this);
   return NS_OK;
 }
 
 nsresult
 CaptivePortalService::RearmTimer()
 {
+  LOG(("CaptivePortalService::RearmTimer\n"));
   // Start a timer to recheck
   MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
   if (mTimer) {
     mTimer->Cancel();
   }
 
   if (!mTimer) {
     mTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
@@ -287,23 +290,22 @@ CaptivePortalService::Observe(nsISupport
     // The user needs to log in. We are in a captive portal.
     mState = LOCKED_PORTAL;
     mLastChecked = TimeStamp::Now();
     mEverBeenCaptive = true;
   } else if (!strcmp(aTopic, kCaptivePortalLoginSuccessEvent)) {
     // The user has successfully logged in. We have connectivity.
     mState = UNLOCKED_PORTAL;
     mLastChecked = TimeStamp::Now();
-    mRequestInProgress = false;
     mSlackCount = 0;
     mDelay = mMinInterval;
+
     RearmTimer();
   } else if (!strcmp(aTopic, kAbortCaptivePortalLoginEvent)) {
     // The login has been aborted
-    mRequestInProgress = false;
     mState = UNKNOWN;
     mLastChecked = TimeStamp::Now();
     mSlackCount = 0;
   }
 
   // Send notification so that the captive portal state is mirrored in the
   // content process.
   nsCOMPtr<nsIObserverService> observerService = services::GetObserverService();
@@ -331,25 +333,26 @@ CaptivePortalService::Prepare()
 }
 
 NS_IMETHODIMP
 CaptivePortalService::Complete(bool success)
 {
   LOG(("CaptivePortalService::Complete(success=%d) mState=%d\n", success, mState));
   MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
   mLastChecked = TimeStamp::Now();
-  if ((mState == UNKNOWN || mState == NOT_CAPTIVE) && success) {
-    mState = NOT_CAPTIVE;
-    // If this check succeeded and we have never been in a captive portal
-    // since the service was started, there is no need to keep polling
-    if (!mEverBeenCaptive) {
-      mDelay = 0;
-      if (mTimer) {
-        mTimer->Cancel();
-      }
+
+  // Note: this callback gets called when:
+  // 1. the request is completed, and content is valid (success == true)
+  // 2. when the request is aborted or times out (success == false)
+
+  if (success) {
+    if (mEverBeenCaptive) {
+      mState = UNLOCKED_PORTAL;
+    } else {
+      mState = NOT_CAPTIVE;
     }
   }
 
   mRequestInProgress = false;
   return NS_OK;
 }
 
 } // namespace net
--- a/toolkit/components/captivedetect/captivedetect.js
+++ b/toolkit/components/captivedetect/captivedetect.js
@@ -348,17 +348,17 @@ CaptivePortalDetector.prototype = {
     gSysMsgr.broadcastMessage(kCaptivePortalSystemMessage, {});
   },
 
   _mayRetry: function _mayRetry() {
     if (this._runningRequest.retryCount++ < this._maxRetryCount) {
       debug("retry-Detection: " + this._runningRequest.retryCount + "/" + this._maxRetryCount);
       this._startDetection();
     } else {
-      this.executeCallback(true);
+      this.executeCallback(false);
     }
   },
 
   executeCallback: function executeCallback(success) {
     if (this._runningRequest) {
       debug("callback executed");
       if (this._runningRequest.hasOwnProperty("callback")) {
         this._runningRequest.callback.complete(success);
--- a/toolkit/components/captivedetect/test/unit/test_captive_portal_not_found_404.js
+++ b/toolkit/components/captivedetect/test/unit/test_captive_portal_not_found_404.js
@@ -30,17 +30,17 @@ function test_portal_not_found() {
   let callback = {
     QueryInterface: XPCOMUtils.generateQI([Ci.nsICaptivePortalCallback]),
     prepare: function prepare() {
       do_check_eq(++step, 1);
       gCaptivePortalDetector.finishPreparation(kInterfaceName);
     },
     complete: function complete(success) {
       do_check_eq(++step, 2);
-      do_check_true(success);
+      do_check_false(success);
       do_check_eq(attempt, 6);
       gServer.stop(do_test_finished);
     },
   };
 
   gCaptivePortalDetector.checkCaptivePortal(kInterfaceName, callback);
 }