Closed Bug 1426408 Opened 2 years ago Closed 2 years ago

Remove some code for the TCP Fast Open on Windows that is not needed any more

Categories

(Core :: Networking: HTTP, enhancement, P1)

59 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: dragana, Assigned: dragana)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file)

We use to check only the major version for Windows for TFO, i.e. version 10.0.

I did not know at the time, or it was not implemented yet, who to check a subversion, 10.0.16299. Therefore we were relaying on PR_NOT_IMPLEMENTED_ERROR while calling sendTo.

Now we can check for subversion and some code can be removed.
Dragana, can you please explain more in detail why NOT_IMPLEMENTED now can't be returned from sendto() on win?  It's not clear from the descr or the patch.  Thanks.
Flags: needinfo?(dd.mozilla)
Dragana, ping if you need this review soon.  Thanks.
(In reply to Honza Bambas (:mayhemer) from comment #2)
> Dragana, can you please explain more in detail why NOT_IMPLEMENTED now can't
> be returned from sendto() on win?  It's not clear from the descr or the
> patch.  Thanks.

Sorry, I did not answer because this code is really not that important if it is there or not and  I was rethinking if I have not made a mistake (I have implemented that code long time ago). I think it is safe to remove this. What this code was doing:

Not all versions of windows 10 have TFO, I think first 1 or 2 do not have it at all and they do not know socket option TCP_FASTOPEN. PR_SendTo called on tcp socket will start TFO and on windows it will set the socket option TCP_FASTOPEN. If that returns PR_NOT_IMPLEMENTED_ERROR(the real win error is WSAENOPROTOOPT that is transform into PR_NOT_IMPLEMENTED_ERROR) it means that it is an old version of win10 that does not have TFO.

We use to enable TFO for all win10 and check for this error to disable it on the old version. Now we enable it only for the latest version or new so TFO is implemented for sure. We call mFastOpenSupported = IsWindows10BuildOrLater(16299);
Flags: needinfo?(dd.mozilla)
Comment on attachment 8938027 [details] [diff] [review]
bug_remove_tfo_windows_10_not_needed_code.patch

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

thanks!

a side note: I tried to disabled TFO and TFO fallback on my win10 machine with:
netsh int tcp set global fastopen=disabled (+ a system restart, just in case)
netsh int tcp set global fastopenfallback=disabled

(see |netsh int tcp set global /?| output)

and sendto still returns > 0 (success).  hence, we are ok to remove this code apparently, since when TFO is off, this API still behaves as if it's on.
Attachment #8938027 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #5)
> Comment on attachment 8938027 [details] [diff] [review]
> bug_remove_tfo_windows_10_not_needed_code.patch
> 
> Review of attachment 8938027 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> thanks!
> 
> a side note: I tried to disabled TFO and TFO fallback on my win10 machine
> with:
> netsh int tcp set global fastopen=disabled (+ a system restart, just in case)
> netsh int tcp set global fastopenfallback=disabled
> 
> (see |netsh int tcp set global /?| output)
> 
> and sendto still returns > 0 (success).  hence, we are ok to remove this
> code apparently, since when TFO is off, this API still behaves as if it's on.

Thanks for testing it. It is good to have this confirmed.
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5496e22cd86
Remove some code for the TCP fast open for Windows 10 that is not neede any more. r=mayhemer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e5496e22cd86
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.