1332271 - [Captive Portal] Detection fails when FF is open (in no internet connection) prior to engaging Wifi

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Adrian Florinescu [:aflorinescu]]

+++ 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.

Comment 1

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

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

Comment 2

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b13dc6d4fb7

Comment 3

[Adrian Florinescu [:aflorinescu]]

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.

Comment 4

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

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.

Comment 5

[Adrian Florinescu [:aflorinescu]]

(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.

Comment 6

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Attachment: Do not cancel timer when captive portal request times out

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 7

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

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

Wrong reviewer :)

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 9

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

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

Comment 10

[Patrick McManus [:mcmanus]]

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

Comment 11

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

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

Comment 12

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

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.

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7a937980f2dd

Comment 14

[Julien Cristau [:jcristau]]

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

Comment 15

[Julien Cristau [:jcristau]]

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

captive portal fix, aurora53+, beta52+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c81058f0cbf8

Comment 17

[Adrian Florinescu [:aflorinescu]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f7f886a58559

Comment 19

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

(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.

Comment 20

[Adrian Florinescu [:aflorinescu]]

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.

Comment 21

[Adrian Florinescu [:aflorinescu]]

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.

Comment 22

[Camelia Badau [:cbadau], Desktop QA]

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.