+++ This bug was initially created as a clone of Bug #1320088 +++ [Enviroment:] OS'es: Ubuntu 16.04 Windows 10 x 64 NOT repro - Mac OSX 10.10 and Mac 10.12 Versions tested on: 53.0a1 20170118030214 52.0a2 20170118004007 [Description]: Captive Portal detection fails when FF was already started in a no connection environment at the time Wifi connection is enabled. [Steps]: 1. Make sure you have no active internet connections. 2. Start FF. 3. Enable wifi and attempt connecting to a Captive portal SSID. (do not finalize the connection by actually logging in). 4. Focus FF or try to refresh any page. [Actual Result]: The Captive Portal detection doesn't happen. If you open a new page and try to access a new link, the captive portal canonical will be loaded. [Expected Result]: Detection should be consistent.

Thanks for the updated STR. I have now been able to reproduce the issue. I will try to come up with a fix soon.

Snatched your try-build from above Valentin and gave it a run on Ubuntu 16.04. The detection is successful now, but it needs over 1' do do it.

That's somewhat to be expected. The timer is set for 60s by default. Absent any redirects to local IP addresses, or certificate errors, the only way to detect it would be by using the timer. We should also trigger a detection when connecting to the network. I'm currently looking into if that works properly or not.

(In reply to Valentin Gosu [:valentin] from comment #4) > That's somewhat to be expected. The timer is set for 60s by default. > Absent any redirects to local IP addresses, or certificate errors, the only > way to detect it would be by using the timer. > We should also trigger a detection when connecting to the network. I'm > currently looking into if that works properly or not. After Wi-fi is connected: - if i open google.com, it instantly redirects to the canonical link; no detection happens - if i try opening facebook.com, it will eventually load the SSL error page; from the timing point of view, the SSL page is usually loaded before the CP detection happens. In all the above cases, seems like there is no difference in timing in either of the 3 cases: 1. SSL page loads (when attempting to open facebook.com) 2. redirect to canonical (when attempting to open google.com) 3. just focus the browser and do nothing.

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

Comment on attachment 8829382 [details] [diff] [review] Do not cancel timer when captive portal request times out Wrong reviewer :)

FYI, this is the last blocker for enabling captive portal in 52, so please handle it with the appropriate urgency :)

If Patrick is not able to review this by tomorrow I will find another reviewer, and make sure we land this ASAP. Thanks!

Comment on attachment 8829382 [details] [diff] [review] Do not cancel timer when captive portal request times out Review of attachment 8829382 [details] [diff] [review]: ----------------------------------------------------------------- this is better. thanks

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a937980f2ddbffffc40f6af68d4a511db29f098 Bug 1332271 - Do not cancel timer when captive portal request times out r=mcmanus

Comment on attachment 8829382 [details] [diff] [review] Do not cancel timer when captive portal request times out Approval Request Comment [Feature/Bug causing the regression]: Preexisting bug in captive portal detection [User impact if declined]: Failure to detect captive portal in some cases [Is this code covered by automated tests?]: No. Tested manually. [Has the fix been verified in Nightly?]: Try push has been tested. [Needs manual test from QE? If yes, steps to reproduce]: Yes. See comment 0. It would also be useful to run all other test cases to ensure no regressions, and no other bugs pop up. [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: There is a low risk this change might expose other bugs in the CP implementation. [Why is the change risky/not risky?]: We should take the patch, as it fixes an obvious bug in the CP detection, and is necessary for the initial roll-out. [String changes made/needed]: None.

Flagging for QE verification, hopefully Adrian can take care of that.

Comment on attachment 8829382 [details] [diff] [review] Do not cancel timer when captive portal request times out captive portal fix, aurora53+, beta52+

I just did a quick pass over the detection for Win 10 and Ubuntu 16.04. Same behavior as in comment 4. Detection is now successful with comment 0 steps, but there is still the 50-60 seconds delay before connecting the wifi and feedback from FF. My concern would be that this is the case in which the users have the browser already opened, which means that users will see the network connected and no apparent feedback from FF (~1 minute delay) and this becomes a bit confusing when you experience FF response in the other detection cases. It might be that maybe I'm also pushing this a bit too far, but I would still expect to have a consistent user experience, although I'm not sure if there is any easy technical solution for this. Valentin, any thoughts on improving the above case? Julien, I still want to do a regression testing for the other detection scenarios after Valentin's answer, so I'll leave the NI on for me.

(In reply to Adrian Florinescu [:AdrianSV] [PTO jan 27 -feb 01] from comment #17) > Valentin, any thoughts on improving the above case? I filed bug 1334520 and posted a patch to trigger immediate captive portal detection.

I'll keep the NI on on me, waiting for bug 1334520 to get uplifted to 52 before verifying this one and marking as such.

Since bug 1334520 has already been uplifted, all the detection scenarios (comment 17) can be now verified on Aurora and Beta. For this purpose redirecting the NI to Camelia.

Verified fixed on Windows 10 x64 and Ubuntu 14.04 x64 using Firefox 52 Beta 4 (buildID: 20170206101855) and latest Aurora 53.0a2 (2017-02-08): detection is now successful.