1313577 - Forget references to captive portal tab once login has succeeded or aborted

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

If there is an origin change in the captive portal tab opened by CaptivePortalWatcher, it should no longer be considered a captive portal tab - i.e. for example if the "Show Login Page" in the notification bar is clicked, we should open a brand new captive portal tab.

Comment 1

[Dirkjan Ochtman (:djc)]

This seems like a pretty bad problem -- I have a captive portal that does a nice redirect to a conference-specific page (on a different origin) on successful login, but this now means it's really hard to get Firefox online again.

Comment 2

[Sam Hall]

As soon as network connectivity is detected the captive portal tab should be forgotten. I'd consider it a bonus feature if Firefox simply closes the tab actually. I know that's not going to help Dirkjan, but as a user who really cares what the captive portal wants me to do next after I've authenticated.

Currently, if your captive portal session times out the Firefox captive portal detection button is useless. It just activates the old tab without reloading the page or anything. So the user is just looking at a page of cute kittens with no idea what Firefox is trying to do. If you're lucky, the user might think to refresh the page and get the captive portal login again, or the SSL error page.

It's a shame because the captive portal detection feature is 90% there. Fix this and it's perfect.

I've just had a look at the source https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-captivePortal.js

Line 162 there is a comment "// We're free of the portal!"

Perhaps just replace that comment with...

this._captivePortalTab = null;

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

It's been a while since I looked at captive portal stuff but I'm feeling inspired to try and take a look at this soon. Needinfo'ing myself for tracking purposes.

Comment 4

[Sam Hall]

I had another look at the code, and it seem the logical place to forget _captivePortalTab would be in the _captivePortalGone() method.

Comment 5

[Sam Hall]

While I have someone's attention on this topic, I read this in the design document on GitHub "In order to prevent many tabs being open, if an existing captive portal is already open overwrite it when a new captive portal is detected.". This seems to be where some of this trouble has stemmed from.

I'm not sure the "too many tabs" issue ever got resolved in a sane way. For example: If I press the button for the first time and I'm presented with "Captive Portal 1" login screen but I do not login then move to another network and press the button, I believe I would currently be presented with "Captive Portal 1" login screen which is not useful at all.

What should happen is that user presses the button and if the tab exists it gets focus and also it's reloaded with the URL resulting from the Location header in the response from the request for canonicalURL (or simply try to load canonicalURL).

This minor enhancement coupled with forgetting _captivePortalTab after authentication would solve some problems with the current implementation. I don't think you have to worry about detecting origin change, because if the user has managed to navigate elsewhere on the net or wanted to remain on the page that the captive portal landed them on, that should be up to them. After all, if that page were not useful to them they would close the tab or navigated elsewhere by now.

Dirkjan mentions he actually wants users to use his captive portal site after they have authenticated and it's possible to imagine the user could have un-submitted form data they don't want to lose when triggering a new captive portal login attempt for instance. In such cases, it wouldn't make sense to force the user to reuse the old captive portal tab at all.

The issue outlined in the statement I quoted is probably just a little unrefined. The design document I'm referring to is here... https://github.com/vtsatskin/FX-Captive-Portals-Design/blob/master/design/spec.md

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

I'm going to consolidate and triage captive portal bugs in March.

Comment 7

[:prathiksha]

Attachment: Bug 1313577 - Forget references to the captive portal tab once login has succeeded or aborted. r?johannh

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33747fc9e28f
Forget references to the captive portal tab once login has succeeded or aborted. r=johannh

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/33747fc9e28f

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

Prathiksha, I don't think the patch in this bug fixes the issue that's described. Note that setting the captive portal tab binding to null has little to no effect since the reference is weak [1].

The purpose of this bug was to stop treating the captive portal tab as a captive portal tab, if the portal page redirected to some third-party site. This is hard to detect and at the moment I don't have a solution in mind.

Could you chime in here and re-open the bug if necessary?

[1] https://searchfox.org/mozilla-central/rev/f36cb2af46edd2659f446b7acdb2154e230ee445/browser/base/content/browser-captivePortal.js#288

Comment 11

[:prathiksha]

(In reply to Nihanth Subramanya [:nhnt11] from comment #10)

Prathiksha, I don't think the patch in this bug fixes the issue that's described. Note that setting the captive portal tab binding to null has little to no effect since the reference is weak [1].

We were a little unclear on what this redirecting to a third party website behavior is when we were triaging this bug. Are you saying that we want to forget the captive portal tab if it redirects to a different origin on successful login? What does "forget" mean here? Should we forget the _captivePortalTab object reference or does forget mean "removeTab" in this context?

If a user currently logs in successfully in a captive portal and if the captive portal tab redirects to, say, a different website after login, we don't want to remember this existing captive portal tab that is not closed yet (the reference is still held, isn't it?) in the event that the user switches their network in the same session and tries to login again. We want to avoid pointing them to the old captive portal tab in this case. Same with abort. I think my patch was meant to fix that issue.

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to :prathiksha from comment #11)

(In reply to Nihanth Subramanya [:nhnt11] from comment #10)

Prathiksha, I don't think the patch in this bug fixes the issue that's described. Note that setting the captive portal tab binding to null has little to no effect since the reference is weak [1].

We were a little unclear on what this redirecting to a third party website behavior is when we were triaging this bug. Are you saying that we want to forget the captive portal tab if it redirects to a different origin on successful login? What does "forget" mean here? Should we forget the _captivePortalTab object reference or does forget mean "removeTab" in this context?

Ah! I can clarify with an example scenario: the user opens a captive portal tab by clicking the Open Login Page button in the notification. Then, they get distracted by something IRL, and when they come back to their machine in a few mins, they promptly try to navigate to their favorite cat gif website in the currently focused captive portal login page tab. At this point, the tab is no longer the captive portal tab. Our UI will not open a new captive portal tab but just focus this one, which isn't very helpful. I didn't think further than this point for more possible anomalies.

So by "forget" I just mean, treat that tab as an ordinary one, which would indeed mean killing our reference to it.

If a user currently logs in successfully in a captive portal and if the captive portal tab redirects to, say, a different website after login, we don't want to remember this existing captive portal tab that is not closed yet (the reference is still held, isn't it?) in the event that the user switches their network in the same session and tries to login again. We want to avoid pointing them to the old captive portal tab in this case. Same with abort. I think my patch was meant to fix that issue.

This is an excellent catch - thanks!. I didn't try to repro or verify it, but I'm pretty sure you're right and your fix should do the trick.

Considering the validity of both the bug that was fixed and the original description, want to clone this into a new bug to track the old issue and rename this one to reflect what was actually fixed?

Comment 13

[:prathiksha]

Considering the validity of both the bug that was fixed and the original description, want to clone this into a new bug to track the old issue and rename this one to reflect what was actually fixed?

Yeah. Thanks for clarifying :)