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

Status: RESOLVED FIXED

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

Description

[Dragana Damjanovic [:dragana]]

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.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_tfo_small_changes.patch

Comment 2

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6b6b433dfa4f52fd8664a19a4089bc4a966a352

Comment 3

[Dragana Damjanovic [:dragana]]

Comment on attachment 8911925 [details] [diff] [review]
bug_tfo_small_changes.patch

Nick can you please take a look. Thanks

Comment 4

[u408661]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ce42f336f82c