Closed Bug 1352273 Opened 3 years ago Closed 3 years ago

Reset mFastOpenConsecutiveFailureCounter upon cleared captive portal event

Categories

(Core :: Networking: HTTP, enhancement)

55 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file)

No description provided.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
I need a little help here.
mFastOpenConsecutiveFailureCounter is incremented every time TCP fastOpen fails until it reaches certain number and then we disable TCP Fast Open.

I want to reset mFastOpenConsecutiveFailureCounter when captive portal detection has happened.

Valentin, can you  tell me what is the right place to do this? I do not know captive portal code.
Thanks!!!
Flags: needinfo?(valentin.gosu)
You can watch the observer notifications to see when that happens. See CaptivePortalService::Observe
The topic will be "captive-portal-login" when we detect a captive portal, and "captive-portal-login-success" when we have successfully logged into the captive portal.
Flags: needinfo?(valentin.gosu)
Depends on: 1188435
Attachment #8860849 - Flags: review?(mcmanus)
Attachment #8860849 - Flags: review?(mcmanus) → review?(valentin.gosu)
Attachment #8860849 - Flags: review?(valentin.gosu) → review?(mcmanus)
Comment on attachment 8860849 [details] [diff] [review]
bug_1352273_v1.patch

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

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +473,5 @@
>          obsService->AddObserver(this,
>                                  "net:current-toplevel-outer-content-windowid",
>                                  true);
>  
> +        if (mFastOpenSupported) {

just doublecheck that mFastOpenSupported is not racy against this init.

@@ -640,5 @@
>          mFastOpenConsecutiveFailureCounter++;
>          if (mFastOpenConsecutiveFailureCounter == mFastOpenConsecutiveFailureLimit) {
>              LOG(("nsHttpHandler::IncrementFastOpenConsecutiveFailureCounter - "
>                   "Fast open failed too many times"));
> -            SetFastOpenNotSupported();

I presume the runtime check involves looking at the counter and the limit and not just "isSupported()" ?
Attachment #8860849 - Flags: review?(mcmanus) → review+
Comment on attachment 8860849 [details] [diff] [review]
bug_1352273_v1.patch

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

It looks good overall. I think we need answers to Patrick's questions before we land it.
Also, I'm wondering if it would be a good idea to disable TFO when we detect a captive portal via the "captive-portal-login" notification?
(In reply to Patrick McManus [:mcmanus] from comment #4)
> Comment on attachment 8860849 [details] [diff] [review]
> bug_1352273_v1.patch
> 
> Review of attachment 8860849 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpHandler.cpp
> @@ +473,5 @@
> >          obsService->AddObserver(this,
> >                                  "net:current-toplevel-outer-content-windowid",
> >                                  true);
> >  
> > +        if (mFastOpenSupported) {
> 
> just doublecheck that mFastOpenSupported is not racy against this init.

It is not racy: mFastOpenSupported is set in the constructor of nsHttpHandler and in some cases during the first PR_Connect. This is nsHttpHandler::Init function  and we should not have sockets at this point.

> 
> @@ -640,5 @@
> >          mFastOpenConsecutiveFailureCounter++;
> >          if (mFastOpenConsecutiveFailureCounter == mFastOpenConsecutiveFailureLimit) {
> >              LOG(("nsHttpHandler::IncrementFastOpenConsecutiveFailureCounter - "
> >                   "Fast open failed too many times"));
> > -            SetFastOpenNotSupported();
> 
> I presume the runtime check involves looking at the counter and the limit
> and not just "isSupported()" ?

Yes, I have change that. We check mFastOpenSupported and counter and flags that correspond to the TFO pref.
(In reply to Valentin Gosu [:valentin] (Vacation until May 1st) from comment #5)
> Comment on attachment 8860849 [details] [diff] [review]
> bug_1352273_v1.patch
> 
> Review of attachment 8860849 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks good overall. I think we need answers to Patrick's questions before
> we land it.
> Also, I'm wondering if it would be a good idea to disable TFO when we detect
> a captive portal via the "captive-portal-login" notification?

you mean disable it between captive-portal-login and captive-portal-login-success? A fallback socket should fix this if needed, but if you think that we could have a problem I can change that.
(In reply to Dragana Damjanovic [:dragana] from comment #7)
> (In reply to Valentin Gosu [:valentin] (Vacation until May 1st) from comment
> #5)
> > Comment on attachment 8860849 [details] [diff] [review]
> > bug_1352273_v1.patch
> > 
> > Review of attachment 8860849 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > It looks good overall. I think we need answers to Patrick's questions before
> > we land it.
> > Also, I'm wondering if it would be a good idea to disable TFO when we detect
> > a captive portal via the "captive-portal-login" notification?
> 
> you mean disable it between captive-portal-login and
> captive-portal-login-success? A fallback socket should fix this if needed,
> but if you think that we could have a problem I can change that.

I'm just wondering how many captive portal networks would benefit from having it enabled. I assume very few at first, but indeed that may change in the future, and in the case of captive portals that whitelist some services such as gmail, or wikipedia, it could already work. So, you're right, there's probably no need to worry about that.
Pushed by dd.mozilla@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d38863c19d5
Restart the TFO failure counter if a captive portal is detected.r=mcmanus
https://hg.mozilla.org/mozilla-central/rev/2d38863c19d5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.