Log QUIC public reset client address mismatch to a histogram.

Log QUIC public reset client address mismatch to a histogram.

Remove the client_address member from QuicCryptoNegotiatedParameters.
Replace it by the client_address_ member of QuicConnectionLogger.

Add kCADR support to CryptoHandshakeMessage::DebugStringInternal.

R=jar@chromium.org,rch@chromium.org
BUG=none
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251258

[wtc, 2014-02-13 02:04:10]

https://codereview.chromium.org/159143007/diff/90001/net/quic/crypto/crypto_s...
File net/quic/crypto/crypto_server_test.cc (left):

https://codereview.chromium.org/159143007/diff/90001/net/quic/crypto/crypto_s...
net/quic/crypto/crypto_server_test.cc:346: }

It seems that this piece of code (lines 340-346) is piggybacking on the
ReplayProtection test to check the kVER tag in a ServerHello message, so I did
the same thing, rather than adding a new unit test.

https://codereview.chromium.org/159143007/diff/90001/net/quic/quic_connection...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/159143007/diff/90001/net/quic/quic_connection...
net/quic/quic_connection_logger.cc:206: kAddressAndPortMatch_v4_v4 = 0,

Is it correct for the histogram enumeration to start from 0?

https://codereview.chromium.org/159143007/diff/90001/net/quic/quic_connection...
net/quic/quic_connection_logger.cc:262:
HISTOGRAM_ENUMERATION("Net.QuicSession.PublicResetAddressMismatch",

Is this histogram name OK? All the histograms in this file are under
Net.QuicSession, so I also put this one there.

[jar (doing other things), 2014-02-13 03:24:48]

After the one typo, and perhaps the comment: LGTM

Thanks!!  I'm looking forward to see this data! :-)

https://codereview.chromium.org/159143007/diff/90001/net/quic/quic_connection...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/159143007/diff/90001/net/quic/quic_connection...
net/quic/quic_connection_logger.cc:206: kAddressAndPortMatch_v4_v4 = 0,
On 2014/02/13 02:04:10, wtc wrote:
> 
> Is it correct for the histogram enumeration to start from 0?

Perfectly correct.

https://codereview.chromium.org/159143007/diff/90001/net/quic/quic_connection...
net/quic/quic_connection_logger.cc:228: // TODO(wtc): should we report these two
cases?
This is fine IMO.  You are seemingly talking to and older server, that does not
support the feature... so you can't (yet) report the results in the histogram. 

Suggest deleting the comment (or adding a comment that the server doesn't
support this... so we can't report).

https://codereview.chromium.org/159143007/diff/90001/net/quic/quic_connection...
net/quic/quic_connection_logger.cc:262:
HISTOGRAM_ENUMERATION("Net.QuicSession.PublicResetAddressMismatch",
On 2014/02/13 02:04:10, wtc wrote:
> 
> Is this histogram name OK? All the histograms in this file are under
> Net.QuicSession, so I also put this one there.

Critical is the "UMA" prefix, as otherwise the histograms won't upload (in
retrospect.. I shouldn't have made UMA upload the default).  You need to use:

UMA_HISTOGRAM_ENUMERATION()

The actual histogram name looks just fine.

[Ryan Hamilton, 2014-02-13 04:44:04]

Looks good... one suggestion.

https://codereview.chromium.org/159143007/diff/90001/net/quic/quic_connection...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/159143007/diff/90001/net/quic/quic_connection...
net/quic/quic_connection_logger.cc:223: kAddressMismatch_v6_v4 = 11,
An alternative to consider which might make the code simpler would be for sample
to be effectively a bit mask encoded as:

<address_matches><port_matches><reset_is_v4><shlo_is_v4>

I think this would allow you to incrementally build up sample via:

if (first_ipv4)
  sample |= 1 << 0;
if (second_ipv4)
  sample |= 1 << 1;
if (server_hello_address.address() == public_reset_address.address())
  sample |= 1 << 2;
if (server_hello_address.port() == public_reset_address.port())
  sample |= 1 << 4;

Or something like that...

[wtc, 2014-02-14 00:53:56]

Please review patch set 4.

I made all the changes you suggested. Thanks!

https://codereview.chromium.org/159143007/diff/90001/net/quic/quic_connection...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/159143007/diff/90001/net/quic/quic_connection...
net/quic/quic_connection_logger.cc:223: kAddressMismatch_v6_v4 = 11,
On 2014/02/13 04:44:05, Ryan Hamilton wrote:
> An alternative to consider which might make the code simpler would be for
sample
> to be effectively a bit mask encoded as:
> 
> <address_matches><port_matches><reset_is_v4><shlo_is_v4>
> 
> I think this would allow you to incrementally build up sample via:
> 
> if (first_ipv4)
>   sample |= 1 << 0;
> if (second_ipv4)
>   sample |= 1 << 1;
> if (server_hello_address.address() == public_reset_address.address())
>   sample |= 1 << 2;
> if (server_hello_address.port() == public_reset_address.port())
>   sample |= 1 << 4;
> 
> Or something like that...

Done. (Not in the way you suggested though.)

https://codereview.chromium.org/159143007/diff/90001/net/quic/quic_connection...
net/quic/quic_connection_logger.cc:228: // TODO(wtc): should we report these two
cases?
On 2014/02/13 03:24:48, jar wrote:
> This is fine IMO.  You are seemingly talking to and older server, that does
not
> support the feature... so you can't (yet) report the results in the histogram.

> 
> Suggest deleting the comment (or adding a comment that the server doesn't
> support this... so we can't report).

Done.

https://codereview.chromium.org/159143007/diff/90001/net/quic/quic_connection...
net/quic/quic_connection_logger.cc:262:
HISTOGRAM_ENUMERATION("Net.QuicSession.PublicResetAddressMismatch",
On 2014/02/13 03:24:48, jar wrote:
> On 2014/02/13 02:04:10, wtc wrote:
> > 
> > Is this histogram name OK? All the histograms in this file are under
> > Net.QuicSession, so I also put this one there.
> 
> Critical is the "UMA" prefix, as otherwise the histograms won't upload (in
> retrospect.. I shouldn't have made UMA upload the default).  You need to use:
> 
> UMA_HISTOGRAM_ENUMERATION()
> 
> The actual histogram name looks just fine.
> 

Done.

[Ryan Hamilton, 2014-02-14 01:52:04]

lgtm

[wtc, 2014-02-14 02:04:16]

The CQ bit was checked by wtc@chromium.org

[commit-bot: I haz the power, 2014-02-14 02:06:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/159143007/180001

[commit-bot: I haz the power, 2014-02-14 04:27:33]

Change committed as 251258