Closed Bug 1402879 Opened 2 years ago Closed 2 years ago

Small change to TFO and a bigger change to the telemetry.

Categories

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

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- disabled
firefox59 --- fixed

People

(Reporter: dragana, Assigned: dragana)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file)

Changes:

1) If backup connection has already started we do not need to close it. (This used to be like this earlier and during my look for TFO bug I have removed it, but there is no reason to remove it)

2) Telemetry:
I notice that we have a high TFO_NOT_TRIED value ~18%. I have change that to get more precise data. I also notice that we could report some TFO errors like TFO_FAILED_CONNECTION_REFUSED, etc. twice through a socket that is triggered by nsSocketTransport::RecoverFromError and by the backup connection. I have split FO_FAILED_BACKUP_CONNECTION into 3 errors.
Attachment #8911925 - Flags: review?(mcmanus)
Whiteboard: [necko-triaged]
Priority: -- → P2
Comment on attachment 8911925 [details] [diff] [review]
bug_tfo_small_changes.patch

Nick can you please take a look. Thanks
Attachment #8911925 - Flags: review?(mcmanus) → review?(hurley)
Comment on attachment 8911925 [details] [diff] [review]
bug_tfo_small_changes.patch

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

Looks good to me. A few things that would be nice to have noted below.

::: netwerk/base/TCPFastOpenLayer.h
@@ +52,4 @@
>    TFO_FAILED_CONNECTION_REFUSED,
>    TFO_FAILED_NET_TIMEOUT,
>    TFO_FAILED_UNKNOW_ERROR,
> +  // The following 3 are set when backup connection finishes before the primary

nit: 4? (I could be wrong, but there are 4 TFO_FAILED_BACKUP_CONNECTION_foo things below here)

::: netwerk/base/nsSocketTransport2.cpp
@@ +2233,5 @@
>              PROsfd osfd = PR_FileDesc2NativeHandle(fd);
>              BOOL option = 0;
>              int len = sizeof(option);
>              PRInt32 rv = getsockopt((SOCKET)osfd, IPPROTO_TCP, TCP_FASTOPEN, (char*)&option, &len);
> +            if (!rv && !option) {

!rv is not the same as (rv != 0), so I'm confused by this change. This is fine if it's intended (and reading the change in the TFO_foo below, it very well may be), but I wanted to call it out just in case.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +2419,5 @@
> +            if ((mFastOpenStatus >= TFO_FAILED_BACKUP_CONNECTION_TFO_NOT_TRIED) &&
> +                (mFastOpenStatus <= TFO_FAILED_BACKUP_CONNECTION_TFO_DATA_COOKIE_NOT_ACCEPTED)) {
> +                mFastOpenStatus = TFO_FAILED_BACKUP_CONNECTION_NO_TFO_FAILED_TOO;
> +            } else {
> +                mFastOpenStatus = tfoStatus + 7;

Would be nice to have a comment explaining what the +7 does and why it's 7 (and when you might want to change that, say if the list of TFO_foo things changes in a particular way).

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +4234,5 @@
> +            mFastOpenStatus = TFO_FAILED_BACKUP_CONNECTION_TFO_DATA_SENT;
> +        } else {
> +            // This is TFO_DATA_COOKIE_NOT_ACCEPTED (I think this cannot
> +            // happened, because the primary connection will be already
> +            // connected or in recovery and mFastOpenInProgress==false).

Maybe a MOZ_ASSERT(false)? I doubt it'll ever hit since we probably don't do TFO on tests, but since you seem to think it won't happen, might be good to at least try to see if you're wrong :)

::: toolkit/components/telemetry/Histograms.json
@@ +2466,4 @@
>      "expires_in_version": "61",
>      "kind": "enumerated",
> +    "n_values": 32,
> +    "description": "When a http connection is closed, track whether or not TCP Fast Open was used: 0=TFO_NOT_SET, 1=TFO_UNKNOWN, 2=TFO_DISABLED, 3=TFO_DISABLED_CONNECT, 4=TFO_NOT_TRIED, 5=TFO_TRIED, 6=TFO_DATA_SENT, 7=TFO_DATA_COOKIE_NOT_ACCEPTED, 8=TFO_FAILED_CONNECTION_REFUSED, 9=TFO_FAILED_NET_TIMEOUT, 10=TFO_FAILED_UNKNOW_ERROR, 11=TFO_FAILED_BACKUP_CONNECTION_TFO_NOT_TRIED, 12=TFO_FAILED_BACKUP_CONNECTION_TFO_TRIED, 13=TFO_FAILED_BACKUP_CONNECTION_TFO_DATA_SENT, 14=TFO_FAILED_BACKUP_CONNECTION_TFO_DATA_COOKIE_NOT_ACCEPTED, 15=TFO_FAILED_CONNECTION_REFUSED_NO_TFO_FAILED_TOO, 16=TFO_FAILED_NET_TIMEOUT__NO_TFO_FAILED_TOO, 17=TFO_FAILED_UNKNOW_ERROR_NO_TFO_FAILED_TOO, 18=TFO_FAILED_BACKUP_CONNECTION_NO_TFO_FAILED_TOO, 19=TFO_BACKUP_CONN",

Maybe add something in here to point people to the code with more explanations about the various error codes.
Attachment #8911925 - Flags: review?(hurley) → review+
Pushed by dd.mozilla@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce42f336f82c
Make small changes to TFO: telemetry and use of backup socket that already has started. r-mcmanus
https://hg.mozilla.org/mozilla-central/rev/ce42f336f82c
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.