Gather metrics classifying the cause of the TLS fallback.

Gather metrics classifying the cause of the TLS fallback.

This CL classifies the following fallback triggers in BoringSSL ports:

- ClientHello intolerance.
- Microsoft's buggy AES-GCM implementation.
- CertificateVerify was rejected (some servers broke TLS 1.2 client auth).
- Resuming a session at the wrong version.
- F5's buggy NPN implementation.
- Other.

It also records how often each error code triggers the fallback.

Initial numbers for the fixed metrics are actually rather encouraging,
but put this in place for the same release if larger populations' numbers
are worse and we'd need to carve out large swathes of intolerance with
less problematic workarounds.

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

[davidben, 2015-05-13 19:22:59]

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

[davidben, 2015-05-13 19:23:00]

This is slightly buggy without
https://boringssl-review.googlesource.com/#/c/4733/, but sending it out for
review now so that can happen in parallel.

[Ryan Sleevi, 2015-05-13 22:26:37]

Overall this approach works for me. A few bugs though

https://codereview.chromium.org/1127623005/diff/60001/net/socket/client_socke...
File net/socket/client_socket_handle.cc (right):

https://codereview.chromium.org/1127623005/diff/60001/net/socket/client_socke...
net/socket/client_socket_handle.cc:77: pending_http_proxy_connection_.reset();
BUG?: You forgot to reset the SSLFailureState here

https://codereview.chromium.org/1127623005/diff/60001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1127623005/diff/60001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:406: return ssl_failure_state_;
BUG: You never reset this in Disconnect()

DOUBLE BUG: Looks like we never reset certificate_verified_ or
handshake_completed_ either. Want to grab easily cleanup and make sure we're
consistent about resetting internal state? Even if the API isn't used, that's
the contract we operate under.

https://codereview.chromium.org/1127623005/diff/60001/net/ssl/ssl_failure_sta...
File net/ssl/ssl_failure_state.h (right):

https://codereview.chromium.org/1127623005/diff/60001/net/ssl/ssl_failure_sta...
net/ssl/ssl_failure_state.h:10: // Gives more information about the state of the
handshake when it failed. This
s/the handshake/TLS handshake/

s/when it failed/when it fails/?

Is this documenting too much about the API? That is, the "gives" verb is a
property of the socket, this enum doesn't give anything.

// Describes the most likely cause for the TLS handshake failure. This is an
// approximation used to classify the causes of TLS version fallback.

or something like that?

https://codereview.chromium.org/1127623005/diff/60001/net/ssl/ssl_failure_sta...
net/ssl/ssl_failure_state.h:36: kSSLFailureNextProto = 6,
style: Newlines between each value would help (eg line 15+16, line 17+18, line
20+21, etc)

[davidben, 2015-05-14 23:24:33]

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

[davidben, 2015-05-14 23:24:34]

+asargent for chrome/browser/extensions
+asvitkine for histograms.xml

https://codereview.chromium.org/1127623005/diff/60001/net/socket/client_socke...
File net/socket/client_socket_handle.cc (right):

https://codereview.chromium.org/1127623005/diff/60001/net/socket/client_socke...
net/socket/client_socket_handle.cc:77: pending_http_proxy_connection_.reset();
On 2015/05/13 22:26:36, Ryan Sleevi wrote:
> BUG?: You forgot to reset the SSLFailureState here

Done. (Sigh. Why must we have so many manual "reset state" methods? I much
prefer a "create object anew if reseting universe.)

https://codereview.chromium.org/1127623005/diff/60001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1127623005/diff/60001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:406: return ssl_failure_state_;
On 2015/05/13 22:26:36, Ryan Sleevi wrote:
> BUG: You never reset this in Disconnect()

Done.

> DOUBLE BUG: Looks like we never reset certificate_verified_ or
> handshake_completed_ either. Want to grab easily cleanup and make sure we're
> consistent about resetting internal state? Even if the API isn't used, that's
> the contract we operate under.

Done. (But I have Opinions about this kind of contract and want it to go away...
:-) OpenSSL's SSL* internally has a similar contract and it's super-buggy.)

https://codereview.chromium.org/1127623005/diff/60001/net/ssl/ssl_failure_sta...
File net/ssl/ssl_failure_state.h (right):

https://codereview.chromium.org/1127623005/diff/60001/net/ssl/ssl_failure_sta...
net/ssl/ssl_failure_state.h:10: // Gives more information about the state of the
handshake when it failed. This
On 2015/05/13 22:26:37, Ryan Sleevi wrote:
> s/the handshake/TLS handshake/
> 
> s/when it failed/when it fails/?
> 
> Is this documenting too much about the API? That is, the "gives" verb is a
> property of the socket, this enum doesn't give anything.
> 
> // Describes the most likely cause for the TLS handshake failure. This is an
> // approximation used to classify the causes of TLS version fallback.
> 
> or something like that?

Yeah, your's is much better. DOne.

https://codereview.chromium.org/1127623005/diff/60001/net/ssl/ssl_failure_sta...
net/ssl/ssl_failure_state.h:36: kSSLFailureNextProto = 6,
On 2015/05/13 22:26:37, Ryan Sleevi wrote:
> style: Newlines between each value would help (eg line 15+16, line 17+18, line
> 20+21, etc)

Done.

[Ryan Sleevi, 2015-05-14 23:30:20]

lgtm

https://codereview.chromium.org/1127623005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/socket/tls_socket_unittest.cc (right):

https://codereview.chromium.org/1127623005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/socket/tls_socket_unittest.cc:69:
MOCK_CONST_METHOD0(GetSSLFailureState, net::SSLFailureState());
Not your fault, but a lot of this API is all "Danger Zone" because it fails to
actually return the bits from the Mock, and thus can cause who knows what kind
of side-effects :)

[davidben, 2015-05-14 23:35:06]

(For completeness, when this CL lands, the prerequisite[*] BoringSSL roll will
be in. https://codereview.chromium.org/1126233010/ )

[*] Actually. I suspect it'll Just Work without it, but if we're calling
SSL_get_current_cipher before the handshake completes, let's make the semantics
right first.

[Alexei Svitkine (slow), 2015-05-15 14:39:37]

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

https://codereview.chromium.org/1127623005/diff/80001/net/http/http_network_t...
net/http/http_network_transaction.cc:1489: net::GetAllErrorCodesForUma());
It's more efficient to use UMA_HISTOGRAM_SPARSE_SLOWLY() in this case, instead
of allocating an array to hold all possible bucket values.

https://codereview.chromium.org/1127623005/diff/80001/net/ssl/ssl_failure_sta...
File net/ssl/ssl_failure_state.h (right):

https://codereview.chromium.org/1127623005/diff/80001/net/ssl/ssl_failure_sta...
net/ssl/ssl_failure_state.h:15: kSSLFailureNone = 0,
Nit: Chromium code prefers SSL_FAILURE_NONE style enum naming convention.

[asargent_no_longer_on_chrome, 2015-05-15 17:50:38]

c/b/e lgtm

[davidben, 2015-05-15 18:00:24]

https://codereview.chromium.org/1127623005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/socket/tls_socket_unittest.cc (right):

https://codereview.chromium.org/1127623005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/socket/tls_socket_unittest.cc:69:
MOCK_CONST_METHOD0(GetSSLFailureState, net::SSLFailureState());
On 2015/05/14 23:30:20, Ryan Sleevi wrote:
> Not your fault, but a lot of this API is all "Danger Zone" because it fails to
> actually return the bits from the Mock, and thus can cause who knows what kind
> of side-effects :)

Acknowledged. (I'm assuming there's nothing really to be done here? I could
switch this and most of the rest to concrete no-op implementations.)

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

https://codereview.chromium.org/1127623005/diff/80001/net/http/http_network_t...
net/http/http_network_transaction.cc:1489: net::GetAllErrorCodesForUma());
On 2015/05/15 14:39:36, Alexei Svitkine wrote:
> It's more efficient to use UMA_HISTOGRAM_SPARSE_SLOWLY() in this case, instead
> of allocating an array to hold all possible bucket values.

Done.

In that case should we switch all code that currently uses
net::GetAllErrorCodesForUma() or is that incompatible? There's a few places. I
can do it in a follow-up if SPARSE_SLOWLY is preferred.

https://code.google.com/p/chromium/codesearch#search/&q=GetAllErrorCodesForUm...

https://codereview.chromium.org/1127623005/diff/80001/net/ssl/ssl_failure_sta...
File net/ssl/ssl_failure_state.h (right):

https://codereview.chromium.org/1127623005/diff/80001/net/ssl/ssl_failure_sta...
net/ssl/ssl_failure_state.h:15: kSSLFailureNone = 0,
On 2015/05/15 14:39:36, Alexei Svitkine wrote:
> Nit: Chromium code prefers SSL_FAILURE_NONE style enum naming convention.

Done. (I'd wondered what was up with the mix of styles...)

[Alexei Svitkine (slow), 2015-05-15 18:05:22]

lgtm

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

https://codereview.chromium.org/1127623005/diff/80001/net/http/http_network_t...
net/http/http_network_transaction.cc:1489: net::GetAllErrorCodesForUma());
On 2015/05/15 18:00:23, David Benjamin wrote:
> On 2015/05/15 14:39:36, Alexei Svitkine wrote:
> > It's more efficient to use UMA_HISTOGRAM_SPARSE_SLOWLY() in this case,
instead
> > of allocating an array to hold all possible bucket values.
> 
> Done.
> 
> In that case should we switch all code that currently uses
> net::GetAllErrorCodesForUma() or is that incompatible? There's a few places. I
> can do it in a follow-up if SPARSE_SLOWLY is preferred.
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=GetAllErrorCodesForUm...

Yes, preferably all the places should be switched.

[davidben, 2015-05-15 20:29:52]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-05-15 20:29:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org, rsleevi@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1127623005/#ps120001
(title: "missing header")

[commit-bot: I haz the power, 2015-05-15 20:31:50]

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

[commit-bot: I haz the power, 2015-05-15 22:18:50]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-18 11:28:23]

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