Only record fallback metrics on successful requests.

Only record fallback metrics on successful requests.

While this still counts spurious fallbacks, it won't count connections to https
URLs which never succeeded at all. Hopefully this'll be slightly more accurate.

BUG=459690
Committed: https://crrev.com/ca9d6916c5fedf6f0ee73dfc397cb0f65ad326b9
Cr-Commit-Position: refs/heads/master@{#328177}

[davidben, 2015-05-01 17:16:28]

davidben@chromium.org changed reviewers:
+ asvitkine@chromium.org, rsleevi@chromium.org

[davidben, 2015-05-01 17:16:29]

I also want to record the cause of the fallback, but that'll take more work to
plumb up and will probably need to be a separate histogram anyway.

[Alexei Svitkine (slow), 2015-05-01 17:41:31]

lgtm % nit

https://codereview.chromium.org/1116063006/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1116063006/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:1570: (host.size() == 10 ||
host[host.size() - 11] == '.')) {
Nit: Can you just use EndsWith()?

[davidben, 2015-05-01 22:50:54]

https://codereview.chromium.org/1116063006/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1116063006/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:1570: (host.size() == 10 ||
host[host.size() - 11] == '.')) {
On 2015/05/01 17:41:30, Alexei Svitkine wrote:
> Nit: Can you just use EndsWith()?

I just copied that from the original code. :-) Switched it to EndsWith.

[Ryan Sleevi, 2015-05-01 23:57:55]

https://codereview.chromium.org/1116063006/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1116063006/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:789: if (request_->url.SchemeIs("https"))
BUG: Why not WSS? Why not SchemeIsCryptographic?

https://codereview.chromium.org/1116063006/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:1542: };
Do you need to document the (hopefully obvious, but not intrinisically) fact
that this is used for UMA and must remain stable?

https://codereview.chromium.org/1116063006/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:1572: }
Is this even worthwhile with the fallback SCSV? We should never succeed to such
a host. Can't this be removed now?

[davidben, 2015-05-02 00:22:09]

https://codereview.chromium.org/1116063006/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1116063006/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:789: if (request_->url.SchemeIs("https"))
On 2015/05/01 23:57:55, Ryan Sleevi wrote:
> BUG: Why not WSS? Why not SchemeIsCryptographic?

Done.

https://codereview.chromium.org/1116063006/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:1542: };
On 2015/05/01 23:57:55, Ryan Sleevi wrote:
> Do you need to document the (hopefully obvious, but not intrinisically) fact
> that this is used for UMA and must remain stable?

Done.

https://codereview.chromium.org/1116063006/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:1572: }
On 2015/05/01 23:57:55, Ryan Sleevi wrote:
> Is this even worthwhile with the fallback SCSV? We should never succeed to
such
> a host. Can't this be removed now?

I'm actually especially interested in a fixed version of these metrics now that
FALLBACK_SCSV exists. :-) With this fix, these metrics will never see a spurious
fallback on Google servers, so we get to find out how many TLS-intolerant MITM
proxies there are[*].

[*] It'll still be an overestimate because there may exist spurious fallbacks on
tolerant MITMs that don't implement FALLBACK_SCSV.

[Ryan Sleevi, 2015-05-02 00:37:34]

LGTM % nit

https://codereview.chromium.org/1116063006/diff/60001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1116063006/diff/60001/net/http/http_network_t...
net/http/http_network_transaction.cc:1567: // caused by network middleware
rather than buggy HTTPS servers.
Update this comment to reflect your explanation then?

[davidben, 2015-05-04 17:51:06]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-05-04 17:51:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1116063006/#ps80001
(title: "adjust comment")

[davidben, 2015-05-04 17:51:17]

https://codereview.chromium.org/1116063006/diff/60001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1116063006/diff/60001/net/http/http_network_t...
net/http/http_network_transaction.cc:1567: // caused by network middleware
rather than buggy HTTPS servers.
On 2015/05/02 00:37:34, Ryan Sleevi wrote:
> Update this comment to reflect your explanation then?

Done.

[commit-bot: I haz the power, 2015-05-04 17:51:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1116063006/80001

[commit-bot: I haz the power, 2015-05-04 20:18:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-04 20:19:44]

Patchset 5 (id:??) landed as
https://crrev.com/ca9d6916c5fedf6f0ee73dfc397cb0f65ad326b9
Cr-Commit-Position: refs/heads/master@{#328177}