Closed Bug 752982 Opened 12 years ago Closed 11 years ago

Captive portal pop up dialog is not automatically triggered on Wifi connect

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
blocking-b2g shira+
blocking-basecamp -
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: tchung, Assigned: fabrice)

References

Details

(Whiteboard: [mno11] [triaged:1/18])

Attachments

(2 files, 33 obsolete files)

26.44 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
31.91 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
Cross posting from https://github.com/andreasgal/gaia/issues/1361.

When connecting to a network that requires captive portal sign-in, the popup dialog does not get triggered automatically. Instead, the page just launches a Server Not Found error, with no further instructions.

Repro
1) nexus S, Gaia commit: 2012-05-04 22:41: 20430a3.....
2) connect to a SSID that requires a captive portal dialog signin (Corporate site like QCOM, coffee shop, etc..)
3) Verify first that your network is connected via Wifi settings "Connected to _____" 
4) knowing this SSID requires captive portal sign in, go and launch browser
5) type in any site to try and trigger the expected captive portal.

6) Verify no captive portal auto re-direct site is opened. Instead, just a 404 error.

Expected:
- a captive portal dialog should appear in SSID's like coffee shops and corporate sites, etc.. Not showing anything but a Server not Found is bad.

Actual;
- no captive portal dialogs
Captive Portal on Wiki:
http://en.wikipedia.org/wiki/Captive_portal
It mentions the implementation methods.
Assignee: nobody → schien
two possible approaches from my current understanding:
1. Fetch a known web page and check if redirection being triggered. It'll work for most of scenarios, but might have problem on private cooperate network.
2. Detect HTTP status code 511 which described in RFC 6585, but it depends on the implementation of network infrastructure. Most of legacy network infrastructures don't return HTTP status code 511.

I'll implement the first method. Any other suggestion or detection algorithm is welcome.
Attached patch WIP for captive portal detection (obsolete) — Splinter Review
Proposed approach & WIP:
add CaptivePortalDetector in services folder, which listen to "network-interface-init-completed"event sent from network interface (e.g. WifiManager) and trigger captive portal detection algorithm.
Captive portal detection algorithm is to check the content of a known website. If the content doesn't match our expectation, fire a mozChromeEvent to notify gaia to fetch the captive portal login page. (gaia modification is not included in the patch)

TODO: 
* callback interface to notify network interface about the result of captive portal detection.
* monitor captive portal login progress to determine if login success.
Attachment #646975 - Flags: feedback?
WIP v2 for Captive Portal Detection. 
* use preference to store captive portal detection parameters
* add HttpObserver to monitor login progress and check if public network connectivity granted.

TODO:
* complete HttpObserver monitor algorithm
Attachment #646975 - Attachment is obsolete: true
Attachment #646975 - Flags: feedback?
Attachment #647499 - Flags: feedback?
Attachment #647499 - Flags: feedback? → feedback?(tlee)
blocking-basecamp: --- → ?
Core captive portal detection is done, please help review implementation.
* public API change to XPCOM interface instead of nsIObserverService event
* allow multiple interface to initiate detection request at the same time
* captive portal detection request can be abort

TODO:
* discuss the interface visibility of nsINetworkInterface with NetworkManager owner. currently nsINetworkInterface is only existed while MOZ_B2G_RIL option is turned on, but CaptivePortalDetector need this interface for all application.
* discuss UI flow with gaia owner
Attachment #647499 - Attachment is obsolete: true
Attachment #647499 - Flags: feedback?(tlee)
Attachment #648594 - Flags: feedback?(tlee)
(In reply to Shih-Chiang Chien from comment #5)
> Created attachment 648594 [details] [diff] [review]
> captive portal detection v0.9 (core detection algorithm is done)
> 
> TODO:
> * discuss the interface visibility of nsINetworkInterface with
> NetworkManager owner. currently nsINetworkInterface is only existed while
> MOZ_B2G_RIL option is turned on, but CaptivePortalDetector need this
> interface for all application.

Bug 770698 seems to have similar problem. We can have a more complete check in that issue instead.
Not required for V1, not blocking the release on this.
blocking-basecamp: ? → -
Comment on attachment 648594 [details] [diff] [review]
captive portal detection v0.9 (core detection algorithm is done)

Review of attachment 648594 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/wifi/WifiWorker.js
@@ +1508,5 @@
> +        };
> +        gCaptivePortalService.checkCaptivePortal(WifiNetworkInterface, callback);
> +      } else {
> +        notify();
> +      }

I suggest to move this chunk to a separated bug.  Please file a new bug if you plan to have another bug for this.

::: services/captivedetect/captivedetect.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Add modeline at the head.

@@ +11,5 @@
> +
> +//XXX need to turn off debug while check-in
> +const DEBUG = true; // set to true to show debug messages
> +
> +const CAPTIVEPORTALDETECTOR_CONTRACTID = "@mozilla.org/netwerk/captive-detector;1";

Should be prefixed with "k" for constants, such as kCAPTIVEPORTALDECTOR_CONTRACTID.

@@ +35,5 @@
> +  // maintain a progress table, store callback and ongoing XHR
> +  this._progressTable = [];
> +  
> +  // create HttpObserver for later use, monitoring login process
> +  this._loginObserver = {

This is a big chunk of code.  Please move it to a separated class.  Mix it with this constructor with make the control flow hard to be read.  You need to skip more than one hundred of lines to find the next instruction.  It is not good for readability.

@@ +62,5 @@
> +        debug("detach HttpObserver for login activity");
> +      }
> +    },
> +    observeActivity: function(aHttpChannel, aActivityType, aActivitySubtype, aTimestamp, aExtraSizeData, aExtraStringData) {
> +      if (this._state == LOGIN_OBSERVER_STATE_ATTACHED 

no trailing spaces.

@@ +87,5 @@
> +        //TODO periodically detect public network connectivity
> +        let oReq = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +          .createInstance(Ci.nsIXMLHttpRequest);
> +        oReq.open("GET", self._canonicalSiteURL, true);
> +        

no trailing spaces too.

@@ +93,5 @@
> +        oReq.ontimeout = function () {
> +          debug("xhr timeout for periodic public network detection");
> +          if (!loginObserver_self._timerFired) {
> +            loginObserver_self._timerFired = true;
> +            loginObserver_self._timer.initWithCallback(loginObserver_self, self._pollingtime, loginObserver_self._timer.TYPE_ONE_SHOT);

80 characters or less for a line. https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style?redirectlocale=en-US&redirectslug=Mozilla_Coding_Style_Guide#Line_Length

::: services/captivedetect/nsICaptivePortalDetector.idl
@@ +51,5 @@
> +{
> +  /**
> +   * invoke callbacks after captive portal detection finished
> +   */
> +  void invoke();

Since nsICaptivePortalDelector processes detections one by one, you have better to add a prepare() callback here to give underlying interfaces a chance of preparing routing, DNS, ...etc.  With out prepare(), some interfaces may change routing, DNS, ... etc, in the middle of your processing of earlier requests.

@@ +57,5 @@
> +
> +[scriptable, uuid(2f827c5a-f551-477f-af09-71adbfbd854a)]
> +interface nsICaptivePortalDetector : nsISupports
> +{
> +  void checkCaptivePortal(in nsINetworkInterface networkInterface, in nsICaptivePortalCallback callback);

You should document this interface to the truth that the detector is processing requests one by one.

@@ +58,5 @@
> +[scriptable, uuid(2f827c5a-f551-477f-af09-71adbfbd854a)]
> +interface nsICaptivePortalDetector : nsISupports
> +{
> +  void checkCaptivePortal(in nsINetworkInterface networkInterface, in nsICaptivePortalCallback callback);
> +  void abort(in nsINetworkInterface networkInterface);

You don't really use nsINetworkInterface in the detector.  Why do you make the detector depends on nsICaptivePortalDetector?  Do you have any plan that requires an interface?

::: services/captivedetect/test/chrome/test_bug752892.js
@@ +46,5 @@
> +}
> +
> +function addCaptivePortalLoginEventHandler() {
> +  var hiddenWindow = Services.appShell.hiddenDOMWindow;
> +  hiddenWindow.addEventListener("mozChromeEvent", eventHandler);

Why don't you just pass the eventHandler as an argument?

::: services/captivedetect/test/chrome/test_bug752892_portal_found.html
@@ +6,5 @@
> +<head>
> +  <meta charset="utf-8">
> +  <title>Test for Bug 752892</title>
> +  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="application/javascript" src="http://mochi.test:8888/chrome/services/captivedetect/test/chrome/test_bug752892.js"></script>

You use run_test() that comes from test_bug752892.js.  If you really want to reuse it.  Please move it to your test_captivedetect.js or somewhere more reasonable and meaningful.

@@ +30,5 @@
> +	  is(e.detail.type, "captive-portal-login", "Received mozChromeEvent type");
> +	  var xhr = new XMLHttpRequest();
> +	  xhr.open("GET", "http://mochi.test:8888/chrome/services/captivedetect/test/chrome/canonicalSite.sjs");
> +	  xhr.send();
> +	  setTimeout(function () {

You have setTimeout() at a lot of testcases.  It is not acceptable.  The accumulate of all setTimeout is a big delay, even they are very short periods.
Attachment #648594 - Flags: feedback?(tlee)
Core implementation for CaptivePortalDetector service
* add |prepare()| in nsICaptivePortalCallback for preparation of network interface before executing captive portal detection procedure
* use interface name instead of nsINetworkInterface object to associate detection request and network interface
* nit according to coding convention

NOTE:
1. will submit another review for test case
2. file bug 780793 for WiFi modification on using CaptivePortalDetector
Attachment #648594 - Attachment is obsolete: true
Attachment #649518 - Flags: review?(tlee)
file gaia issue #3204 for tracking the UI implementation
https://github.com/mozilla-b2g/gaia/issues/3204
Test case for CaptivePortalDetector service
Attachment #649567 - Flags: review?(tlee)
* fix some typo
* change contract ID to |@mozilla.org/services/captive-detector;1| for clarity
Attachment #649518 - Attachment is obsolete: true
Attachment #649518 - Flags: review?(tlee)
Attachment #649569 - Flags: review?(tlee)
check following page for the format of patches.
 https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

First line of the comment should be in the pattern 'Bug XXX: part 1 - Captive portal detection.' and 'Bug XXX: part 2 - Test case for Captive portal.'.  And, the description of patches be in the pattern 'Part X - XXXXXXXX'.  You need to make sure patches can be applied in the given order without broken the building at any point.  In this case, your testcases depend on the detector, you should apply testcases after the detector, even it is not enabled at this monent.
correct patch format
Attachment #649567 - Attachment is obsolete: true
Attachment #649569 - Attachment is obsolete: true
Attachment #649567 - Flags: review?(tlee)
Attachment #649569 - Flags: review?(tlee)
Attachment #649933 - Flags: review?(tlee)
* fix typo in package-manifest.in
* verified on both browser and b2g platform
Attachment #649933 - Attachment is obsolete: true
Attachment #649933 - Flags: review?(tlee)
Attachment #650763 - Flags: review?(tlee)
* add mozContentEvent handling for user abort login procedure
* add param |success| to nsICaptivePortalCallback.invoke()
* fix http monitoring method in loginObserver
Attachment #650763 - Attachment is obsolete: true
Attachment #650763 - Flags: review?(tlee)
Attachment #651121 - Flags: review?(tlee)
Try run for 257a33a8ec76 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=257a33a8ec76
Results (out of 206 total builds):
    success: 181
    warnings: 21
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/schien@mozilla.com-257a33a8ec76
* remove mozChromeEvent handling, use nsIObserverService instead
* remove mozContentEvent handling, use |cancelLogin| instead

NOTE: mozChromeEvent/mozContent event handling should move to shell.js on b2g platform
Attachment #651121 - Attachment is obsolete: true
Attachment #651121 - Flags: review?(tlee)
Attachment #651993 - Flags: review?(tlee)
This bug is not blocking the release of b2g v1. Please work on blocking bugs only.
Comment on attachment 651993 [details] [diff] [review]
captive portal detection v4 (xpcshell/mochitest test cases)

Review of attachment 651993 [details] [diff] [review]:
-----------------------------------------------------------------

GJ! There is no big issue in logic for this patch, but there are several coding style issues.

::: services/captivedetect/captivedetect.js
@@ +18,5 @@
> +
> +const kOpenCaptivePortalLoginEvent = "captive-portal-login";
> +const kAbortCaptivePortalLoginEvent = "captive-portal-login-abort";
> +
> +function LoginObserver(captiveDetector) {

please use the same style of CaptivePortalDector.  Define methods in prototype.  And, you should add an QueryInterface for this class.

@@ +31,5 @@
> +  let _state = LOGIN_OBSERVER_STATE_DETACHED;
> +  let _timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +  let _activityDistributor = Cc["@mozilla.org/network/http-activity-distributor;1"]
> +                              .getService(Ci.nsIHttpActivityDistributor);
> + 

tailing spaces

@@ +36,5 @@
> +  this.attach = function attach() {
> +    if (_state == LOGIN_OBSERVER_STATE_DETACHED) {
> +      _activityDistributor.addObserver(this);
> +      _state = LOGIN_OBSERVER_STATE_ATTACHED;
> +      this._phase = LOGIN_OBSERVER_PHASE_IDLE;

If you remove _phase and redefine *_PHAS_* as *_STATE_*, the code will be more clear and simple.

@@ +52,5 @@
> +    }
> +  };
> +
> +  this.observeActivity = function(aHttpChannel, aActivityType, aActivitySubtype,
> +                                  aTimestamp, aExtraSizeData, aExtraStringData) {

This function will be called several times for every burst.  It means this function maybe a performance/responsibility critical.  I suggest to maintain an activity counter, and this function only doing increasing the counter.  The counter is checked periodically that you don't need calling cancel() on timer.

Please leave a comment before this function to explain what is this function for.

@@ +56,5 @@
> +                                  aTimestamp, aExtraSizeData, aExtraStringData) {
> +    if (_state == LOGIN_OBSERVER_STATE_ATTACHED
> +        && aActivityType == Ci.nsIHttpActivityObserver.ACTIVITY_TYPE_HTTP_TRANSACTION) {
> +      switch(aActivitySubtype) {
> +        case Ci.nsIHttpActivityObserver.ACTIVITY_SUBTYPE_RESPONSE_COMPLETE:

since you have only one case for this "switch" statement.  Please use "if" statement, it is more clear semantically.

@@ +77,5 @@
> +      }
> +    }
> +  };
> +
> +  this.notify = function() {

Leave a comment before this function to explain what is this function for.

@@ +79,5 @@
> +  };
> +
> +  this.notify = function() {
> +    if (_state == LOGIN_OBSERVER_STATE_ATTACHED) {
> +      this._phase = LOGIN_OBSERVER_PHASE_POLLING;

If you check the activity counter periodically, you don't need this phase anymore.  You just need to keep an |last_activity_counter| variable and update the variable after your polling

@@ +83,5 @@
> +      this._phase = LOGIN_OBSERVER_PHASE_POLLING;
> +      // periodically detect public network connectivity
> +      let oReq = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +                   .createInstance(Ci.nsIXMLHttpRequest);
> +      oReq.open("GET", captiveDetector._canonicalSiteURL, true);

You should reuse _startDetect() here.  Please refactor this block of code and _startDetect().

@@ +149,5 @@
> +    if (aCallback) {
> +      let callback = aCallback.QueryInterface(Ci.nsICaptivePortalCallback);
> +      request["callback"] = callback;
> +    }
> +    this._requestQueue.push(request);

You can refactor code fragments maintaining |_requestQueue| into functions to make the code more clear.  For example, define function |addChecking()| and |finishChecking()|.  You can check the queue length to determine if a new task should be start.

@@ +175,5 @@
> +            this._sendEvent(kAbortCaptivePortalLoginEvent, details);
> +          }
> +        }
> +        if (this._requestQueue[i].hasOwnProperty("xhr")) {
> +          this._requestQueue[i].xhr.abort();

Only first request, aka _requestQueue[0], can have |xhr|, right?

@@ +190,5 @@
> +    }
> +
> +  },
> +
> +  finishPreparation: function finishPreparation(aIfname) {

just aInterfaceName.  It is longer but clear.

@@ +193,5 @@
> +
> +  finishPreparation: function finishPreparation(aIfname) {
> +    debug("finish preparation phase for interface '" + aIfname + "'");
> +    if (this._requestQueue.length > 0
> +        && this._requestQueue[0].ifname == aIfname) {

Only the request at head of the queue should call this function, right?  If so, log and raise an exception when this function is called for wrong interface name.

@@ +229,5 @@
> +
> +    // try connect to a verification site
> +    let oReq = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +                 .createInstance(Ci.nsIXMLHttpRequest);
> +    oReq.open("GET", this._canonicalSiteURL, true);

Refactor this following lines to share the code with your observer.

@@ +248,5 @@
> +      if (oReq.readyState == 4) {
> +        debug("xhr finished: " + oReq.status);
> +        if ((oReq.status == 200)
> +            || (oReq.status >= 300 && oReq.status <= 399)) {
> +          // verify if page content matched with expectationsubjesubjectct

expectationsubjesubjectct?  typo?

@@ +253,5 @@
> +          if (oReq.status != 200 || !self._validateContent(oReq.responseText)) {
> +            // redirect by network provider
> +            debug("detect captive portal");
> +
> +            let id = self.getRandomId();

A sequence number is more safe.

@@ +266,5 @@
> +            return;
> +          }
> +        } else if (oReq.status == 0) {
> +          // procedure aborted, should do nothing and the rest will be handled
> +          // by requester

But you should note the caller for this, right?  As I know, you can also get this if the network or the host is not reachable.  Please confirm it.

@@ +307,5 @@
> +    return (content == this._canonicalSiteExpectedContent);
> +  },
> +
> +  getRandomId: function getRandomId() {
> +    let randomId = Cc["@mozilla.org/uuid-generator;1"]

I think a serial number is good enough and more efficient.

::: services/captivedetect/test/chrome/commonTestUtils.js
@@ +50,5 @@
> +                                     "@mozilla.org/services/captive-detector;1",
> +                                     "nsICaptivePortalDetector");
> +}
> +
> +CaptiveDetectorTest.prototype.initPrefs = function initPrefs(prefs) {

keep consistent style for definitions of prototypes.  For example, XXX.prototype = {initPrefs: ....}.

@@ +52,5 @@
> +}
> +
> +CaptiveDetectorTest.prototype.initPrefs = function initPrefs(prefs) {
> +  this.originalPrefs = {};
> +  for (var key in prefs) {

Since you have only 3 fix preferences, just inline them here.  It is an over-engineering if only for here. But, it would be useful to put it into services/sync/tps/extensions/tps/modules/prefs.jsm.  I suggest to file another bug for this.

@@ +123,5 @@
> +      hiddenWindow.removeEventListener("test-finish", hdlr);
> +      self.after();
> +    });
> +    this.before();
> +    this.test()

This is not a good example of template pattern since you provide only few logic or lines of code, it is not worthy for applying this pattern.  And, this pattern hurt the readability of the testcases.  Since the control-flow of testcases are broken into small pieces, you need more intelligence to understood it.  It also causes more lines of code. I think you need merely some convenience functions.  See following explanation following (SIMPETEST).

@@ +126,5 @@
> +    this.before();
> +    this.test()
> +  },
> +  finish: function finish() {
> +    this.sendCustomEvent("test-finish", "");

Why don't you just call the finish-function?

::: services/captivedetect/test/chrome/test_bug752892_abort.html
@@ +45,5 @@
> +
> +  testcase.after = function() {
> +    wifiTester.restorePrefs();
> +    wifiTester.removeAllEventHandler();
> +    SimpleTest.finish();

I don't think you need this line since handlers of "test-finish" are removed, no one will receive the event emitted by this line of code.

@@ +55,5 @@
> +    testcase.finish();
> +  };
> +
> +  testcase.run_test();
> +

SIMPLETEST: You can do this straightly.

function finish_test() {
  // after part
  wifiTest......
}

// before part
TestSite.config(...); ......

// test part
wifiTester.start(...); ......
finish_test();

::: services/captivedetect/test/chrome/test_bug752892_multiple_request.html
@@ +13,5 @@
> +<body onload="test()">
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=752892">Mozilla Bug 752892</a>
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +  

remove tailing spaces.

@@ +60,5 @@
> +
> +  testcase.after = function() {
> +    wifiTester.restorePrefs();
> +    wifiTester.removeAllEventHandler();
> +    SimpleTest.finish();

I don't think you need this line, too.

::: services/captivedetect/test/test_captivedetect.js
@@ +8,5 @@
> +XPCOMUtils.defineLazyServiceGetter(this, "gCaptivePortalDetector",
> +                                   "@mozilla.org/services/captive-detector;1",
> +                                   "nsICaptivePortalDetector");
> +
> +function createCallback(func_prepare, func_invoke) {

I found this is the 2nd copy of the same function.  This function very simple and short, you save little BYTEs here, and the flow of the code are divided into small pieces that is not good for readability.  And variable name "callback" is very self-explain, you should remove this function and expand the definition of the "callback" at the place using it.

::: services/captivedetect/test/xpcshell.ini
@@ +1,2 @@
> +[DEFAULT]
> +head = 

remove  tailing spaces.
Attachment #651993 - Flags: review?(tlee) → review-
Update according to comment 20
Attachment #651993 - Attachment is obsolete: true
Attachment #669100 - Flags: review?(tlee)
1. Update patch according to unit test
2. Rebase to m-c tip
Attachment #669100 - Attachment is obsolete: true
Attachment #669100 - Flags: review?(tlee)
Attachment #671350 - Flags: review?(tlee)
1. Update patch according to comment 20.
2. Use xpcshell instead of mochitest.
Attachment #671351 - Flags: review?(tlee)
Without captive portal detection, installed apps may be swept out from the offline cache if the device connect to a WIFI hot spot with captive portal.  The updater will get a page of captive portal when trying to retrieve offline cache manifest, and swept out all pages already in the offline cache.  And, then, the swept apps may never come back without a reinstallation(install after an uninstalling).
blocking-basecamp: - → ?
I thought we weren't doing the captive portal dialog thing?
Whiteboard: [blocked-on-input Chris Lee]
Correct, this is not planned for v1.  However, we'll likely need to address this shortly after v1.
Whiteboard: [blocked-on-input Chris Lee]
I filed bug 804060 on the appcache issue.
blocking-basecamp: ? → -
Comment on attachment 671350 [details] [diff] [review]
Part 1 - captive portal detection service implementation v5.1 (rebase to m-c tip)

switch reviewer to mrbkap after discussing with thinker.
Attachment #671350 - Flags: review?(tlee) → review?(mrbkap)
Attachment #671351 - Flags: review?(tlee) → review?(mrbkap)
Update according to JS component changes.
Attachment #671350 - Attachment is obsolete: true
Attachment #671350 - Flags: review?(mrbkap)
Attachment #681429 - Flags: review?(mrbkap)
I'm going to review this next. Very sorry for the wait.
Whiteboard: mno11
blocking-basecamp: - → ?
The service being implemented here is above the Necko layer, but cc'ing some networking people in case something comes up where they can help.
blocking-basecamp: ? → -
Blake, are you reviewing this case now? 
We need this to be ready soon. We have a code drop on Dec 21th and captive portal should be included.
Flags: needinfo?(mrbkap)
We discussed this on the drivers call today and the conclusion was that Blake is solidly working on v1.0 for now and won't have time to review this given that it's a v1.1 feature.

So what we'll have to do is to leave this code on a v1.1 branch for now, and we can get it reviewed once we have more resources that can work on v1.1.

In other words, this can't land on mozilla-central for now.
blocking-b2g: --- → shira+
Whiteboard: mno11
rebase to lastest m-c tip, this patch is verified on my local environment and ready for checking in shira branch.
Attachment #681429 - Attachment is obsolete: true
Attachment #681429 - Flags: review?(mrbkap)
Attachment #694182 - Flags: review?(mrbkap)
rebase to latest m-c tip, this patch is verified on my local environment and ready for checking in shira branch.
Attachment #671351 - Attachment is obsolete: true
Attachment #671351 - Flags: review?(mrbkap)
Attachment #694183 - Flags: review?(mrbkap)
Whiteboard: [mno11]
Whiteboard: [mno11] → [mno11] [triaged:1/18]
Hi mrbkap, have time review this patch? or should I switch the reviewer back to thinker?
Flags: needinfo?(mrbkap)
No longer blocks: 820287
:mrbkap, Shira build is being planned for another round of new feature testing with nightly builds from the v1-train/mozilla-b2g18 branches next week. It will be great if you can spare some time to do the review this week so testing progress won't be blocked. Or anyone else can do the review? Thanks
bsmith is going to do the review here.
Flags: needinfo?(mrbkap)
Comment on attachment 694182 [details] [diff] [review]
Part 1 - captive portal detection service implementation v5.3 (rebase to m-c tip)

Change reviewer back to Thinker.
Attachment #694182 - Flags: review?(tlee)
Attachment #694182 - Flags: review?(mrbkap)
Attachment #694182 - Flags: review?(bsmith)
Comment on attachment 694183 [details] [diff] [review]
Part 2 - captive portal detection service test cases, v2.1 (rebase to latest m-c)

Change reviewer back to Thinker.
Attachment #694183 - Flags: review?(tlee)
Attachment #694183 - Flags: review?(mrbkap)
Attachment #694183 - Flags: review?(bsmith)
Comment on attachment 694182 [details] [diff] [review]
Part 1 - captive portal detection service implementation v5.3 (rebase to m-c tip)

Review of attachment 694182 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/captivedetect/captivedetect.js
@@ +214,5 @@
> +       this._runNextRequest();
> +    }
> +
> +  },
> +

Please check this senario:
1. Cleint calls abort *immidiately* after finishPreparation, which means it decide to abort log-in.
2. A event reciever recieves kOpenCaptivePortalLoginEvent after kAbortCaptivePortalLoginEvent

Call "abort" after "finishPreparation" means I don't want to log in now, abort it. From event reciever perspective, it thinks user want to log-in, since it recieve "log-in event" after "abort event".
Want to make sure everyone knows that Shira is targeting 2/15 code freeze. 
Shira+ bugs needs more attention for unassigned, needinfo?,  review?
Full regression test is planned on 2/6. Need to have shira+ bugs landed by the end of 2/5 to be covered in full regression testing. Thanks
(In reply to Thinker Li [:sinker] from comment #24)
> Without captive portal detection, installed apps may be swept out from the
> offline cache if the device connect to a WIFI hot spot with captive portal. 
> The updater will get a page of captive portal when trying to retrieve
> offline cache manifest, and swept out all pages already in the offline
> cache.  And, then, the swept apps may never come back without a
> reinstallation(install after an uninstalling).

Did you actually observe this behavior? The behavior you describe should not happen because:

1. Offline cache is transactional; we are only supposed to be throwing away the old version of the app after we've completely downloaded the new version.
2. The check for the offline app cache manifest checks to see if the manifest starts with the line "CACHE MANIFEST", which most captive portal pages won't contain.
3. Most captive portals respond to every request with a 302 redirect to their website, and 3xx redirects cause the offline cache update process to abort, precisely to avoid corruption from captive portals.

When you send the request to the captive portal site, you need to use the LOAD_ANONYMOUS flag so that we avoid tracking cookies and other such things.

Is there a design document of any kind for this feature? I have the following questions:
1. Who will host the captive portal detection website? Mozilla or the carrier or the OEM?
2. What is the UI design for the captive portal detection? Are we simply opening up a new tab in the default browser to do the login?
Try run for 230b2fdc7805 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=230b2fdc7805
Results (out of 3 total builds):
    success: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/schien@mozilla.com-230b2fdc7805
(In reply to Brian Smith (:bsmith) from comment #43)
> Is there a design document of any kind for this feature? I have the
I don't have a design document in hand, but I could put the related information on MozillaWiki for tracking this feature.
> following questions:
> 1. Who will host the captive portal detection website? Mozilla or the
> carrier or the OEM?
Carrier and OEM whoever owns the branding should host the website IMO.
> 2. What is the UI design for the captive portal detection? Are we simply
> opening up a new tab in the default browser to do the login?
The latest UI spec I knew is in http://people.mozilla.com/~lco/Captive_Portal_B2G/ . We'll open up a new tab in browser while manually connecting to a login-required wifi ap, and use notification instead while re-entering the coverage of a login-required wifi ap.
(In reply to Shih-Chiang Chien [:schien] from comment #45)
> (In reply to Brian Smith (:bsmith) from comment #43)
> > following questions:
> > 1. Who will host the captive portal detection website? Mozilla or the
> > carrier or the OEM?
> Carrier and OEM whoever owns the branding should host the website IMO.

require product input on this. Seems to make sense if Mozilla is hosting as well
Flags: needinfo?(ffos-product)
Comment on attachment 694182 [details] [diff] [review]
Part 1 - captive portal detection service implementation v5.3 (rebase to m-c tip)

Review of attachment 694182 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/captivedetect/captivedetect.js
@@ +57,5 @@
> +function LoginObserver(captivePortalDetector) {
> +  const LOGIN_OBSERVER_STATE_DETACHED = 0;
> +  const LOGIN_OBSERVER_STATE_IDLE = 1;
> +  const LOGIN_OBSERVER_STATE_BURST = 2;
> +  const LOGIN_OBSERVER_STATE_COOLDOWN = 3;

please explain these states.

@@ +209,5 @@
> +    debug('abort for ' + aInterfaceName);
> +    this._removeRequest(aInterfaceName);
> +
> +    // Continue next pending reqeust if the ongoing one has been aborted.
> +    if (!this._runningRequest) {

Maybe you could put all _runNextRequest() in _removeRequest() since you had done a lot actions in that function.  It would be more simple for readers.

@@ +216,5 @@
> +
> +  },
> +
> +  finishPreparation: function finishPreparation(aInterfaceName) {
> +    debug('finish preparation phase for interface "' + aInterfaceName + '"');

Please leave a comment to explain what you want the preparation callback to do.  For example, routing, dns, and something else.

@@ +272,5 @@
> +      if (status >= 300 && status <= 399) {
> +        // canonical website has been redirect to unknown location
> +        self._startLogin();
> +      } else {
> +        callback();

retry?!

@@ +330,5 @@
> +      this._applyDetection();
> +    }
> +  },
> +
> +  _addRequest: function _addRequest(request) {

Is there any redundant request for the same interface in the queue?

::: services/captivedetect/nsICaptivePortalDetector.idl
@@ +15,5 @@
> +
> +  /**
> +   * Invoke callbacks after captive portal detection finished.
> +   */
> +  void invoke(in bool success);

Please change the name to complete() or alike.

@@ +33,5 @@
> +  /**
> +   * Abort captive portal detection for specific network interface.
> +   * @param ifname The name of network interface, must be unique.
> +   */
> +  void abort(in wstring ifname);

Explain what is different between abort() and cancelLogin().  We need better names.
Attachment #694182 - Flags: review?(tlee) → review-
(In reply to Shih-Chiang Chien [:schien] from comment #45)
> The latest UI spec I knew is in
> http://people.mozilla.com/~lco/Captive_Portal_B2G/ . We'll open up a new tab
> in browser while manually connecting to a login-required wifi ap, and use
> notification instead while re-entering the coverage of a login-required wifi
> ap.

This is correct. The notification only occurs when the user enters a saved network that requires a captive portal login (because all other saved networks are either open, or already has a password saved)
rebase to m-c tip and update patch according to the change in Part 1.
Attachment #694183 - Attachment is obsolete: true
Attachment #694183 - Flags: review?(tlee)
Attachment #710063 - Flags: review?(tlee)
Try run for f2e5b941233c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f2e5b941233c
Results (out of 9 total builds):
    success: 7
    warnings: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/schien@mozilla.com-f2e5b941233c
disable caching for polling captive-portal-detection web page.
Attachment #710062 - Attachment is obsolete: true
Attachment #710062 - Flags: review?(tlee)
Attachment #710106 - Flags: review?(tlee)
Comment on attachment 710062 [details] [diff] [review]
Part 1 - captive portal detection service implementation v6 (rebase to m-c tip)

Review of attachment 710062 [details] [diff] [review]:
-----------------------------------------------------------------

Some minor changes.

::: services/captivedetect/captivedetect.js
@@ +33,5 @@
> +      }
> +      if (xhr.status === 200) {
> +        self.onsuccess(xhr.responseText);
> +      } else {
> +        self.onfailure(xhr.status);

This is very misleading. The status in 3xx is not a failure.

@@ +55,5 @@
> +}
> +
> +function LoginObserver(captivePortalDetector) {
> +  const LOGIN_OBSERVER_STATE_DETACHED = 0; /* no login procedure being observed */
> +  const LOGIN_OBSERVER_STATE_IDLE = 1; /* login procedure is started, no ongoing network activity */

How about to change this to "Found no any network activity since start monitoring"?

@@ +57,5 @@
> +function LoginObserver(captivePortalDetector) {
> +  const LOGIN_OBSERVER_STATE_DETACHED = 0; /* no login procedure being observed */
> +  const LOGIN_OBSERVER_STATE_IDLE = 1; /* login procedure is started, no ongoing network activity */
> +  const LOGIN_OBSERVER_STATE_BURST = 2; /* network activity is detected, probably cause by login procedure */
> +  const LOGIN_OBSERVER_STATE_COOLDOWN = 3; /* waiting for login procedure finished */

It is quiet for a while, and wait for a more prior to make sure really quiet.

@@ +66,5 @@
> +  let _timer = Cc['@mozilla.org/timer;1'].createInstance(Ci.nsITimer);
> +  let _activityDistributor = Cc['@mozilla.org/network/http-activity-distributor;1']
> +                               .getService(Ci.nsIHttpActivityDistributor);
> +
> +  let _verificationFinished = function _verificationFinished() {

These function and variables are not exposed to outside, please remove the prefix '_'.

@@ +73,5 @@
> +      _state = LOGIN_OBSERVER_STATE_IDLE;
> +    }
> +  };
> +
> +  let _verify = function _verify() {

This is a bad name.  How about "CheckPageContent"?  So as _verificationFinished -> "PageCheckingDone".

@@ +82,5 @@
> +    urlFetcher.ontimeout = _verificationFinished;
> +    urlFetcher.onerror = _verificationFinished;
> +    urlFetcher.onsuccess = function (content) {
> +      if (captivePortalDetector._validateContent(content)) {
> +        captivePortalDetector._executeCallback(true);

Since function '_executeCallback' and '_validateContent' are exposed, please remove the prefix '_'.

@@ +124,5 @@
> +        switch (_state) {
> +          case LOGIN_OBSERVER_STATE_IDLE:
> +            _timer.initWithCallback(this,
> +                                    captivePortalDetector._pollingTime,
> +                                    _timer.TYPE_ONE_SHOT);

Suggest to move this calling to attach() to make sure it is always called.  It is risky to depend on that attach() is always called before sending the login message.  Some day, some one may change the code and the relied condition no more stand.

@@ +261,5 @@
> +    let self = this;
> +
> +    let urlFetcher = new URLFetcher(this._canonicalSiteURL, this._maxWaitingTime);
> +
> +    let callback = this._executeCallback.bind(this, true);

How about 'requestDone'? It is more clear.
update patch according to comment 55
Attachment #710106 - Attachment is obsolete: true
Attachment #710106 - Flags: review?(tlee)
Attachment #710484 - Flags: review?(tlee)
update wording according the offline discussion with Thinker.
Attachment #710484 - Attachment is obsolete: true
Attachment #710484 - Flags: review?(tlee)
Attachment #710563 - Flags: review?(tlee)
patch update for following reason.
1. correctly abort URLFetcher if LoginObserver is detached in VERIFYING state.
2. minor modification for code readability.
Attachment #710563 - Attachment is obsolete: true
Attachment #710563 - Flags: review?(tlee)
Attachment #710574 - Flags: review?(tlee)
update patch according to offline discussion with Thinker.
Attachment #710063 - Attachment is obsolete: true
Attachment #710063 - Flags: review?(tlee)
Attachment #710579 - Flags: review?(tlee)
Attachment #710136 - Attachment is obsolete: true
Attachment #710136 - Flags: review?(tlee)
Attachment #710587 - Flags: review?(tlee)
Attachment #710137 - Attachment is obsolete: true
Attachment #710137 - Flags: review?(tlee)
Attachment #710588 - Flags: review?(tlee)
Comment on attachment 710574 [details] [diff] [review]
Part 1 - captive portal detection service implementation v8

Review of attachment 710574 [details] [diff] [review]:
-----------------------------------------------------------------

Great job!
Attachment #710574 - Flags: review?(tlee) → review+
Comment on attachment 710587 [details] [diff] [review]
(b2g18) Part 1 - captive portal detection service implementation

Review of attachment 710587 [details] [diff] [review]:
-----------------------------------------------------------------

rubber stamp since it the same as another Part 1 patch.
Attachment #710587 - Flags: review?(tlee) → review+
Comment on attachment 710574 [details] [diff] [review]
Part 1 - captive portal detection service implementation v8

Stick to checkin-needed ;)
Attachment #710574 - Flags: checkin?
Comment on attachment 710574 [details] [diff] [review]
Part 1 - captive portal detection service implementation v8

Actually, setting checkin? was fine on this (since only one patch is landing currently). However, still set checkin-needed so it shows up in my queries :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/35d48d9d80d5
Attachment #710574 - Flags: checkin+
Whiteboard: [mno11] [triaged:1/18] → [mno11] [triaged:1/18] [leave open]
Hi,

a fresh build from the current b2g18 source yields this in my logcat :

E/GeckoConsole(  771): [JavaScript Error: "self._runningRequest is null" {file: "jar:file:///system/b2g/omni.ja!/components/captivedetect.js" line: 301}]

So that you know.
(In reply to Julien Wajsberg [:julienw] from comment #68)
> Hi,
> 
> a fresh build from the current b2g18 source yields this in my logcat :
> 
> E/GeckoConsole(  771): [JavaScript Error: "self._runningRequest is null"
> {file: "jar:file:///system/b2g/omni.ja!/components/captivedetect.js" line:
> 301}]
> 
> So that you know.

I filed bug 839399 for this JS error.
Why did this land in /services? /services is its own module and requires r+ from {mconnor, rnewman, gps}. While I may have missed something while on PTO, it appears none of us were involved in landing this in /services.
In addition, the configure changes (and any non-trivial makefile changes) must have build peer review before this relands please.
Comment on attachment 710574 [details] [diff] [review]
Part 1 - captive portal detection service implementation v8

This is a || review as I don't who is the best to do this. We must land this by Friday Feb. 15
Attachment #710574 - Flags: review?(rnewman)
Attachment #710574 - Flags: review?(mconnor)
Attachment #710574 - Flags: review?(gps)
Do we still need product input here?  Let me know if we need some clarification, thanks.
Flags: needinfo?(ffos-product)
Fabrice offered to fix.
Assignee: schien → fabrice
Attachment #710574 - Flags: review?(rnewman)
Attachment #710574 - Flags: review?(mconnor)
Attachment #710574 - Flags: review?(gps)
Fabrice could you confirm these patch have landed ?
Could you create a follow-up for the tests cases and mark this as fixed if accurate ?
Flags: needinfo?(fabrice)
Nothing has landed yet. I need to update all the patches and get them reviewed by toolkit peers.
Flags: needinfo?(fabrice)
After discussing with Gavin and Justin, I moved this code into toolkit/component/captivedetect

Gregory, can you check the configure.in changes?

thanks all!
Attachment #710574 - Attachment is obsolete: true
Attachment #710587 - Attachment is obsolete: true
Attachment #710587 - Flags: review?
Attachment #713669 - Flags: review?(gps)
Attachment #713669 - Flags: review?(gavin.sharp)
Attachment #713669 - Flags: review?(dolske)
Attached patch tests patch, "rebased" (obsolete) — Splinter Review
The test patch, also moved to toolkit/components/captivedetect
Attachment #710579 - Attachment is obsolete: true
Attachment #710588 - Attachment is obsolete: true
Attachment #710579 - Flags: review?(tlee)
Attachment #710588 - Flags: review?(tlee)
Attachment #713674 - Flags: review?(gavin.sharp)
Attachment #713674 - Flags: review?(dolske)
Comment on attachment 713669 [details] [diff] [review]
implem patch, moved to toolkit/components

Review of attachment 713669 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +8438,5 @@
> +dnl Build Captive Portal Detector if required
> +AC_SUBST(MOZ_SERVICES_CAPTIVEDETECT)
> +if test -n "$MOZ_SERVICES_CAPTIVEDETECT"; then
> +  AC_DEFINE(MOZ_SERVICES_CAPTIVEDETECT)
> +fi

MOZ_SERVICES is kinda/sorta reserved for /services foo. How about MOZ_CAPTIVEDETECT?

::: toolkit/components/captivedetect/Makefile.in
@@ +21,5 @@
> +  CaptivePortalDetectComponents.manifest \
> +  captivedetect.js \
> +  $(NULL)
> +
> +PREF_JS_EXPORTS := $(srcdir)/services-captivedetect.js

I'm not sure "services" is appropriate on the filename any more.

Also, I /think/ PREF_JS_EXPORTS may not be fully suitable after the latest app and platform split. There is a pit I keep falling into with prefs files and can't remember what it is. Sync is kinda special for hysterical raisins.
Attachment #713669 - Flags: review?(gps) → feedback?(mh+mozilla)
Comment on attachment 713669 [details] [diff] [review]
implem patch, moved to toolkit/components

Review of attachment 713669 [details] [diff] [review]:
-----------------------------------------------------------------

Is there a patch somewhere which actually makes something call this code? Bug #? (This patch adds a component, but it's only ever invoked by the tests.)

I gather Windows and OS X have system APIs for doing these checks. Not relevant for B2G (and certainly not in this patch), but down the road I think we'd want to consider a common Gecko API for the different forms of captive portal detection might be used under the hood.

I haven't looked at captivedetect.js or the .idl much yet, I'd like to give that a quick skim next. (Submitting this right now for the other changes/questions.)

::: b2g/installer/package-manifest.in
@@ +284,5 @@
>  #ifdef MOZ_SERVICES_SYNC
>  @BINPATH@/components/services-crypto.xpt
>  #endif
>  @BINPATH@/components/services-crypto-component.xpt
> +#ifdef MOZ_SERVICES_CAPTIVEDETECT

You should also remove the "SERVICES" bit from the ifdef, since this isn't actually related to Services' stuff (like Sync).

TBH, I'm not even sure this small component needs an ifdef / configure flag. It could just only be included in B2G's package-manifest. Or, even if you include it in Firefox, there's nothing that would be invoking it... Although I suppose addons could start using it, and it's not clear if this is production-ready yet.

::: toolkit/components/captivedetect/Makefile.in
@@ +17,5 @@
> +  nsICaptivePortalDetector.idl \
> +  $(NULL)
> +
> +EXTRA_COMPONENTS = \
> +  CaptivePortalDetectComponents.manifest \

Hmm. I see other components that have no references to the manifest files other than what's in package-manifest.in. But other things do (old cruft?).

@@ +21,5 @@
> +  CaptivePortalDetectComponents.manifest \
> +  captivedetect.js \
> +  $(NULL)
> +
> +PREF_JS_EXPORTS := $(srcdir)/services-captivedetect.js

I'd not call this "services-foo.js". Better as "foo-prefs.js".

Better yet, just move the contents into modules/libpref/src/init/all.js in an ifdef wrapper. No need to scatter small additional pref files around.

::: toolkit/components/captivedetect/services-captivedetect.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +pref("services.captivedetect.canonicalURL", "http://localhost/test.html");

I... don't see how this is going to work. If localhost is ever a captive portal, you've got problems. :)

Presumably this should be a http://foo.mozilla.com/some/static/content URL? Is there a bug on file for making that service exist?

@@ +5,5 @@
> +pref("services.captivedetect.canonicalURL", "http://localhost/test.html");
> +pref("services.captivedetect.canonicalContent", "true");
> +pref("services.captivedetect.maxWaitingTime", 5000);
> +pref("services.captivedetect.pollingTime", 3000);
> +pref("services.captivedetect.maxRetryCount", 5);

None of these prefs should be "services." prefixed. Just "captivedetect.*" is fine.
Attachment #713669 - Flags: review?(gavin.sharp)
Attachment #713669 - Flags: review?(dolske)
Attachment #713669 - Flags: review-
Driveby wearing service hat, mainly pointing out for the benefit of reviewers:


> > +pref("services.captivedetect.canonicalURL", "http://localhost/test.html");
> 
> I... don't see how this is going to work. If localhost is ever a captive
> portal, you've got problems. :)
> 
> Presumably this should be a http://foo.mozilla.com/some/static/content URL?
> Is there a bug on file for making that service exist?

Please ensure that this doesn't happen:

http://www.tuaw.com/2012/09/19/apple-web-page-outage-inadvertantly-disabled-ios-6-device-wifi-n/

Unless you're committing to a high-nines georedundant canonical URL (and probably even then), you need to make sure that the strategy when it's unreachable isn't unworkable, as Apple's was.
(In reply to Justin Dolske [:Dolske] from comment #82)
> Comment on attachment 713669 [details] [diff] [review]
> implem patch, moved to toolkit/components
> 
> Review of attachment 713669 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there a patch somewhere which actually makes something call this code?
> Bug #? (This patch adds a component, but it's only ever invoked by the
> tests.)

The b2g parts landed in bug 780793. I'm considering backing it out if I can't finish this one.
Attached patch rebased impl v2 (obsolete) — Splinter Review
Addressing review comments.
Attachment #713669 - Attachment is obsolete: true
Attachment #713669 - Flags: feedback?(mh+mozilla)
Attachment #713719 - Flags: review?(gps)
Attachment #713719 - Flags: review?(dolske)
Attached patch rebased tests v2 (obsolete) — Splinter Review
I had to change the pref names.
Attachment #713674 - Attachment is obsolete: true
Attachment #713674 - Flags: review?(gavin.sharp)
Attachment #713674 - Flags: review?(dolske)
Attachment #713720 - Flags: review?(dolske)
Attached patch rebased tests v2 (obsolete) — Splinter Review
I forgot to qref my last change.
Attachment #713720 - Attachment is obsolete: true
Attachment #713720 - Flags: review?(dolske)
Attachment #713730 - Flags: review?(dolske)
Blocks: 841243
(In reply to Justin Dolske [:Dolske] from comment #82)
> Comment on attachment 713669 [details] [diff] [review]
> implem patch, moved to toolkit/components
> 
> Review of attachment 713669 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there a patch somewhere which actually makes something call this code?
> Bug #? (This patch adds a component, but it's only ever invoked by the
> tests.)
> 
> I gather Windows and OS X have system APIs for doing these checks. Not
> relevant for B2G (and certainly not in this patch), but down the road I
> think we'd want to consider a common Gecko API for the different forms of
> captive portal detection might be used under the hood.
> 
> I haven't looked at captivedetect.js or the .idl much yet, I'd like to give
> that a quick skim next. (Submitting this right now for the other
> changes/questions.)
> 
> ::: b2g/installer/package-manifest.in
> @@ +284,5 @@
> >  #ifdef MOZ_SERVICES_SYNC
> >  @BINPATH@/components/services-crypto.xpt
> >  #endif
> >  @BINPATH@/components/services-crypto-component.xpt
> > +#ifdef MOZ_SERVICES_CAPTIVEDETECT
> 
> You should also remove the "SERVICES" bit from the ifdef, since this isn't
> actually related to Services' stuff (like Sync).
> 
> TBH, I'm not even sure this small component needs an ifdef / configure flag.
> It could just only be included in B2G's package-manifest. Or, even if you
> include it in Firefox, there's nothing that would be invoking it... Although
> I suppose addons could start using it, and it's not clear if this is
> production-ready yet.
> 
> ::: toolkit/components/captivedetect/Makefile.in
> @@ +17,5 @@
> > +  nsICaptivePortalDetector.idl \
> > +  $(NULL)
> > +
> > +EXTRA_COMPONENTS = \
> > +  CaptivePortalDetectComponents.manifest \
> 
> Hmm. I see other components that have no references to the manifest files
> other than what's in package-manifest.in. But other things do (old cruft?).
> 
> @@ +21,5 @@
> > +  CaptivePortalDetectComponents.manifest \
> > +  captivedetect.js \
> > +  $(NULL)
> > +
> > +PREF_JS_EXPORTS := $(srcdir)/services-captivedetect.js
> 
> I'd not call this "services-foo.js". Better as "foo-prefs.js".
> 
> Better yet, just move the contents into modules/libpref/src/init/all.js in
> an ifdef wrapper. No need to scatter small additional pref files around.
> 
> ::: toolkit/components/captivedetect/services-captivedetect.js
> @@ +1,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +pref("services.captivedetect.canonicalURL", "http://localhost/test.html");
> 
> I... don't see how this is going to work. If localhost is ever a captive
> portal, you've got problems. :)

This pref is supposed to be overridden by the preference of products.  For example, b2g.js.

> 
> Presumably this should be a http://foo.mozilla.com/some/static/content URL?
> Is there a bug on file for making that service exist?
> 
> @@ +5,5 @@
> > +pref("services.captivedetect.canonicalURL", "http://localhost/test.html");
> > +pref("services.captivedetect.canonicalContent", "true");
> > +pref("services.captivedetect.maxWaitingTime", 5000);
> > +pref("services.captivedetect.pollingTime", 3000);
> > +pref("services.captivedetect.maxRetryCount", 5);
> 
> None of these prefs should be "services." prefixed. Just "captivedetect.*"
> is fine.

(In reply to Richard Newman [:rnewman] from comment #83)
> Driveby wearing service hat, mainly pointing out for the benefit of
> reviewers:
> 
> 
> > > +pref("services.captivedetect.canonicalURL", "http://localhost/test.html");
> > 
> > I... don't see how this is going to work. If localhost is ever a captive
> > portal, you've got problems. :)
> > 
> > Presumably this should be a http://foo.mozilla.com/some/static/content URL?
> > Is there a bug on file for making that service exist?
> 
> Please ensure that this doesn't happen:
> 
> http://www.tuaw.com/2012/09/19/apple-web-page-outage-inadvertantly-disabled-
> ios-6-device-wifi-n/
> 
> Unless you're committing to a high-nines georedundant canonical URL (and
> probably even then), you need to make sure that the strategy when it's
> unreachable isn't unworkable, as Apple's was.

This pref is supposed to be overridden by the pref of a production.  For now, it does not affect functions of networking.  Once the page is unreachable, the detector just treats the network like no captive portal there without doing anything further.
Fabrice: Could you also merge the change in bug #839399 into your update?
> ::: toolkit/components/captivedetect/services-captivedetect.js
> @@ +1,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +pref("services.captivedetect.canonicalURL", "http://localhost/test.html");
> 
> I... don't see how this is going to work. If localhost is ever a captive
> portal, you've got problems. :)
> 
> Presumably this should be a http://foo.mozilla.com/some/static/content URL?
> Is there a bug on file for making that service exist?
> 
Using a non-existed localhost URL is to turn off the captive portal detection. Deciding the default value for this URL depends on how we hosting the static page. Currently we haven't decided whether Mozilla or our partners should be responsible for hosting the pinging page.
I've created a pining page on people.mozilla.org for QA process and we'll override the canonical URL/content in user prefs at runtime.
user_pref("services.captivedetect.canonicalURL", "http://people.mozilla.org/~schien/test.txt");
user_pref("services.captivedetect.canonicalContent", "true\n");
(In reply to Shih-Chiang Chien [:schien] from comment #90)

> Using a non-existed localhost URL is to turn off the captive portal
> detection.

Why not just have missing pref mean that? localhost with a path is quite misleading. You can have individual applications specify a pref value.
(In reply to Richard Newman [:rnewman] from comment #91)
> (In reply to Shih-Chiang Chien [:schien] from comment #90)
> 
> > Using a non-existed localhost URL is to turn off the captive portal
> > detection.
> 
> Why not just have missing pref mean that? localhost with a path is quite
> misleading. You can have individual applications specify a pref value.

I think we agree: https://bugzilla.mozilla.org/show_bug.cgi?id=841243#c3
Attached patch rebased impl v3 (obsolete) — Splinter Review
I made small changes to the implementation to bail out when no canonical url is set.
Attachment #713719 - Attachment is obsolete: true
Attachment #713719 - Flags: review?(gps)
Attachment #713719 - Flags: review?(dolske)
Attachment #714016 - Flags: review?(gps)
Attachment #714016 - Flags: review?(dolske)
Comment on attachment 714016 [details] [diff] [review]
rebased impl v3

Review of attachment 714016 [details] [diff] [review]:
-----------------------------------------------------------------

Bug 780793 still leaves me a bit confused. I guess B2G already has a captive portal UI implementation, and this is just changing when/how it's called?

It would be nice to s/services-captivedetect.xpt/captivedetect.xpt/, too... I don't know if the services people care about that or not. (gps?)

r+ with the removal of services-captivedetect.js references. I only gave captivedetect.js a light skim. I still have some questions about it but since it's not being enabled or shipped in Firefox, I'll get out of the way of your impending schedule deadline.

::: b2g/installer/package-manifest.in
@@ +591,5 @@
>  @BINPATH@/@PREF_DIR@/healthreport-prefs.js
>  #endif
> +#ifdef MOZ_CAPTIVEDETECT
> +@BINPATH@/@PREF_DIR@/services-captivedetect.js
> +#endif

This file no longer exists.

::: browser/installer/package-manifest.in
@@ +586,5 @@
>  @BINPATH@/@PREF_DIR@/healthreport-prefs.js
>  #endif
> +#ifdef MOZ_CAPTIVEDETECT
> +@BINPATH@/@PREF_DIR@/services-captivedetect.js
> +#endif

No longer exists.

::: browser/installer/removed-files.in
@@ +965,5 @@
>    defaults/pref/firefox-branding.js
>    defaults/pref/firefox.js
>    defaults/pref/firefox-l10n.js
>    defaults/pref/services-sync.js
> +  defaults/pref/services-captivedetect.js

Gone etc etc. ;)

::: toolkit/components/captivedetect/nsICaptivePortalDetector.idl
@@ +10,5 @@
> +{
> +  /**
> +   * Preparation for network interface before captive portal detection started.
> +   */
> +  void prepare();

I can't tell what this method is actually for. Bug 780793 has a CaptivePortalDetectionHelper implementing it, but just calls back to nsICaptivePortalDetector's finishPreparation(). Is there a plan to actually do something with this?

(I ask mainly because it would be nice to tag the callback interface with [function], so that JS implementations can just provide a simple callback function, ala addEventListener).
Attachment #714016 - Flags: review?(dolske) → review+
Comment on attachment 713730 [details] [diff] [review]
rebased tests v2

rs=me
Attachment #713730 - Flags: review?(dolske) → review+
(In reply to Justin Dolske [:Dolske] from comment #94)

> It would be nice to s/services-captivedetect.xpt/captivedetect.xpt/, too...
> I don't know if the services people care about that or not. (gps?)

In favor of that change.
Comment on attachment 714016 [details] [diff] [review]
rebased impl v3

Review of attachment 714016 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #714016 - Flags: review?(gps) → review+
(In reply to Justin Dolske [:Dolske] from comment #94)
> Comment on attachment 714016 [details] [diff] [review]
> rebased impl v3
> 
> Review of attachment 714016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Bug 780793 still leaves me a bit confused. I guess B2G already has a captive
> portal UI implementation, and this is just changing when/how it's called?
> 
> It would be nice to s/services-captivedetect.xpt/captivedetect.xpt/, too...
> I don't know if the services people care about that or not. (gps?)
> 
> r+ with the removal of services-captivedetect.js references. I only gave
> captivedetect.js a light skim. I still have some questions about it but
> since it's not being enabled or shipped in Firefox, I'll get out of the way
> of your impending schedule deadline.
> 
> ::: b2g/installer/package-manifest.in
> @@ +591,5 @@
> >  @BINPATH@/@PREF_DIR@/healthreport-prefs.js
> >  #endif
> > +#ifdef MOZ_CAPTIVEDETECT
> > +@BINPATH@/@PREF_DIR@/services-captivedetect.js
> > +#endif
> 
> This file no longer exists.
> 
> ::: browser/installer/package-manifest.in
> @@ +586,5 @@
> >  @BINPATH@/@PREF_DIR@/healthreport-prefs.js
> >  #endif
> > +#ifdef MOZ_CAPTIVEDETECT
> > +@BINPATH@/@PREF_DIR@/services-captivedetect.js
> > +#endif
> 
> No longer exists.
> 
> ::: browser/installer/removed-files.in
> @@ +965,5 @@
> >    defaults/pref/firefox-branding.js
> >    defaults/pref/firefox.js
> >    defaults/pref/firefox-l10n.js
> >    defaults/pref/services-sync.js
> > +  defaults/pref/services-captivedetect.js
> 
> Gone etc etc. ;)
> 
> ::: toolkit/components/captivedetect/nsICaptivePortalDetector.idl
> @@ +10,5 @@
> > +{
> > +  /**
> > +   * Preparation for network interface before captive portal detection started.
> > +   */
> > +  void prepare();
> 
> I can't tell what this method is actually for. Bug 780793 has a
> CaptivePortalDetectionHelper implementing it, but just calls back to
> nsICaptivePortalDetector's finishPreparation(). Is there a plan to actually
> do something with this?

For some environment,for example multi-hosts, it need extra-configuration; DNS, host routing, .. etc, before doing detection.  That is why it is here.  For now, it is not much useful since only support the detection for WIFI.  But, I guess it is soon, we will have the support for 3G data too.  (Ya~ It is weir, but somewhere on the earth use it.)  While we have both WIFI and 3G data, a multi-hosts environment, we need these extra-configuration doing before detection by NIC.
We shouldn't implement something because "somewhere on earth uses it". That would lead to way too much to do :)

We should implement captive portal detection for 3g if it's either easier to enable it everywhere, and it doesn't have a performance cost, or if it's something that's common enough to our users that they would benefit from it.

Keeping in mind that we should as soon as possible make it so that applications don't get the "online" signal until we've detected that there is no captive portal in the way after connecting to a network. Right now a lot of websites break because they assume that getting the "online" event means that they are able to connect to their home servers without any captive portals in the way.

So doing the captive portal detection will likely delay getting online by a few seconds once things are implemented properly.
replace the 'service' with 'toolkit' in contract id.
Attachment #714016 - Attachment is obsolete: true
Attachment #714431 - Flags: review?(gps)
Attachment #714431 - Flags: review?(dolske)
1. update test case according to the contract id change in Part 1.
2. change Makefile and test folder layout, which follows the style in toolkit/components folder.
Attachment #713730 - Attachment is obsolete: true
Attachment #714433 - Flags: review?(dolske)
Comment on attachment 714431 [details] [diff] [review]
Part 1 - rebased impl v4

Bugzilla's intradiff sucks, so I'm assuming nothing in the idl/js changed. Everything else looks fine.
Attachment #714431 - Flags: review?(gps)
Attachment #714431 - Flags: review?(dolske)
Attachment #714431 - Flags: review+
Attachment #714433 - Flags: review?(dolske) → review+
So.... One late breaking thought.

There's a serious privacy issue with how this works. The XHR that's made every time (presumably) a wifi connection is started could be used by the owner of the canonicalURL to implement user tracking. Just set a cookie, and any time that user goes online the site operator will know about  it.

There may also be some worrysome edge cases along the lines of being logged in to http://carrier.net (via HTTP auth or cookie), connecting to evil wifi, and having the captive portal able to steal your auth/cookie because canonicalURL == carrier.net. (Yes, they should have used https/STS, but...)

Thankfully, there is a trivial workaround for this.

When you set the various flags on the XHR, also set nsIRequest.LOAD_ANONYMOUS. That's already used in a variety of places doing similar things.
Comment on attachment 714431 [details] [diff] [review]
Part 1 - rebased impl v4

Review of attachment 714431 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/installer/package-manifest.in
@@ +497,5 @@
>  #endif
> +#ifdef MOZ_CAPTIVEDETECT
> ++@BINPATH@/components/CaptivePortalDetectComponents.manifest
> ++@BINPATH@/components/captivedetect.js
> ++#endif

looks like copy&paste error...let's fix it.
(In reply to Shih-Chiang Chien [:schien] from comment #107)
> Comment on attachment 714431 [details] [diff] [review]
> Part 1 - rebased impl v4
> 
> Review of attachment 714431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/installer/package-manifest.in
> @@ +497,5 @@
> >  #endif
> > +#ifdef MOZ_CAPTIVEDETECT
> > ++@BINPATH@/components/CaptivePortalDetectComponents.manifest
> > ++@BINPATH@/components/captivedetect.js
> > ++#endif
> 
> looks like copy&paste error...let's fix it.

Arrg. That totally make sense with the failures we saw
(In reply to Justin Dolske [:Dolske] from comment #106)
> ... also set nsIRequest.LOAD_ANONYMOUS ...

For anyone playing along at home, this landed with f86bcd71d357.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #113)
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/ecdb53e21d4b
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/089b6e51feb4
> 
> https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d3bcc05af408
> https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f6b38063d26f

Thanks for the uplift!

> Not sure why this is [leave open], but ok...

No idea either, I think we can close.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [mno11] [triaged:1/18] [leave open] → [mno11] [triaged:1/18]
Follow up bug on captive portal detection server hosting
Bug 842953 - Captive portal detection server hosting
Blocks: 562917
You need to log in before you can comment on or make changes to this bug.