Closed Bug 1332271 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Networking, defect, critical)

53 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox52 --- verified
firefox53 --- verified
firefox54 --- verified

People

(Reporter: aflorinescu, Assigned: valentin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

User Story

Simplified STR:
1. No Internet connection.
2. Open Firefox.
3. Connect to a Captive Portal SSID.

Attachments

(1 file)

+++ 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.
Assignee: nobody → valentin.gosu
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
Attachment #8829382 - Flags: review?(dburns)
Comment on attachment 8829382 [details] [diff] [review]
Do not cancel timer when captive portal request times out

Wrong reviewer :)
Attachment #8829382 - Flags: review?(dburns) → review?(mcmanus)
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
Attachment #8829382 - Flags: review?(mcmanus) → review+
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.
Attachment #8829382 - Flags: approval-mozilla-beta?
Attachment #8829382 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7a937980f2dd
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Flagging for QE verification, hopefully Adrian can take care of that.
Flags: qe-verify+
Flags: needinfo?(adrian.florinescu)
Comment on attachment 8829382 [details] [diff] [review]
Do not cancel timer when captive portal request times out

captive portal fix, aurora53+, beta52+
Attachment #8829382 - Flags: approval-mozilla-beta?
Attachment #8829382 - Flags: approval-mozilla-beta+
Attachment #8829382 - Flags: approval-mozilla-aurora?
Attachment #8829382 - Flags: approval-mozilla-aurora+
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.
Flags: needinfo?(valentin.gosu)
(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.
Flags: needinfo?(valentin.gosu)
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.
Flags: needinfo?(adrian.florinescu) → needinfo?(camelia.badau)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(camelia.badau)
You need to log in before you can comment on or make changes to this bug.