QUIC - Don't close the QUIC connection if server config update message

QUIC - Don't close the QUIC connection if server config update message
is not handled. Added a histogram to track how often this happens.

R=rch@chromium.org,rjshade@chromium.org,  asvitkine@chromium.org

Committed: https://crrev.com/78c0c1b60221c2c521f86e26e999e3819df49c9c
Cr-Commit-Position: refs/heads/master@{#295824}

[ramant (doing other things), 2014-09-19 01:11:22]

rtenneti@chromium.org changed reviewers:
+ asvitkine@chromium.org

[ramant (doing other things), 2014-09-19 01:14:19]

Patchset #1 (id:1) has been deleted

[ramant (doing other things), 2014-09-19 01:18:51]

Hi Ryan and Robbie,
  When server sends a SCUP (server config update), if client doesn't handle it,
we were closing the connection (DoInitializeServerConfigUpdate doesn't change
the state and it remains STATE_IDLE. STATE_IDLE closes the connection).

  We don't handle the message if it is a https server_id and there is no cache
entry for it or the the cached entry doesn't have a signature.

  I have never encountered this situation during my tests. I thought it is safer
to ignore the such server config update messages than close the connection.
Added a histogram to track such errors.

  What do you think of this change?

thanks
raman

[Alexei Svitkine (slow), 2014-09-19 15:14:17]

I also suggest filing a crbug for this change since its apparently changing
behavior in addition to the histogram.

https://codereview.chromium.org/583063002/diff/20001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/583063002/diff/20001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:463:
UMA_HISTOGRAM_COUNTS("Net.QuicNumServerConfigUpdateMessagesIgnored", 1);
This would probably be better served with an UMA_HISTOGRAM_BOOOLEAN() since
you're always logging a 1 (equivalent to true).

Once you change it to a boolean, you probably also want to add a normalizing
factor - i.e. what does the absolute number of these really mean? Why not count
the number of server config updates as the "false" value of the histogram and
then you can get a percentage by comparing the two... Or have "false" log when
it's _not_ ignored.

[ramant (doing other things), 2014-09-19 17:00:05]

Thanks very much Alexei. Made the changes you suggested.

https://codereview.chromium.org/583063002/diff/20001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/583063002/diff/20001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:463:
UMA_HISTOGRAM_COUNTS("Net.QuicNumServerConfigUpdateMessagesIgnored", 1);
On 2014/09/19 15:14:17, Alexei Svitkine wrote:
> This would probably be better served with an UMA_HISTOGRAM_BOOOLEAN() since
> you're always logging a 1 (equivalent to true).
> 
> Once you change it to a boolean, you probably also want to add a normalizing
> factor - i.e. what does the absolute number of these really mean? Why not
count
> the number of server config updates as the "false" value of the histogram and
> then you can get a percentage by comparing the two... Or have "false" log when
> it's _not_ ignored.

Done.

[Alexei Svitkine (slow), 2014-09-19 17:08:04]

Looks good, just a couple last nits. Thanks!

https://codereview.chromium.org/583063002/diff/40001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/583063002/diff/40001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:464:
UMA_HISTOGRAM_BOOLEAN("Net.QuicNumServerConfigUpdateMessagesIgnored",
Nit: How about naming this Net.QuicNumServerConfig.UpdateMessagesIgnored (which
is a bit easier to read and lets you later group related metrics together under
the 2nd level category).

https://codereview.chromium.org/583063002/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/583063002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:16149: +<histogram
name="Net.QuicNumServerConfigUpdateMessagesIgnored" enum="Boolean">
Nit: Can you add a BooleanIgnored and use that for the enum?

[ramant (doing other things), 2014-09-19 17:17:07]

https://codereview.chromium.org/583063002/diff/40001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/583063002/diff/40001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:464:
UMA_HISTOGRAM_BOOLEAN("Net.QuicNumServerConfigUpdateMessagesIgnored",
On 2014/09/19 17:08:04, Alexei Svitkine wrote:
> Nit: How about naming this Net.QuicNumServerConfig.UpdateMessagesIgnored
(which
> is a bit easier to read and lets you later group related metrics together
under
> the 2nd level category).

Done.

https://codereview.chromium.org/583063002/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/583063002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:16149: +<histogram
name="Net.QuicNumServerConfigUpdateMessagesIgnored" enum="Boolean">
On 2014/09/19 17:08:04, Alexei Svitkine wrote:
> Nit: Can you add a BooleanIgnored and use that for the enum?

Done.

[Alexei Svitkine (slow), 2014-09-19 17:18:00]

LGTM, thanks!

[Ryan Hamilton, 2014-09-19 18:16:13]

lgtm

[ramant (doing other things), 2014-09-19 20:12:01]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-09-19 20:26:07]

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

[commit-bot: I haz the power, 2014-09-19 20:26:10]

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

[commit-bot: I haz the power, 2014-09-19 20:32:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-19 20:32:15]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/58410)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ramant (doing other things), 2014-09-19 21:06:16]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-09-19 21:06:57]

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

[commit-bot: I haz the power, 2014-09-19 21:07:03]

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

[commit-bot: I haz the power, 2014-09-19 21:57:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-19 21:57:23]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[ramant (doing other things), 2014-09-19 23:36:00]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-09-19 23:36:28]

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

[commit-bot: I haz the power, 2014-09-20 00:07:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-20 00:07:49]

Failed to apply the patch.

[commit-bot: I haz the power, 2014-09-20 00:07:55]

Committed patchset #4 (id:80001) as 87cc54d5b80c8d62b5d81005f587a3dd2188a473

[commit-bot: I haz the power, 2014-09-20 00:08:57]

Patchset 4 (id:??) landed as
https://crrev.com/78c0c1b60221c2c521f86e26e999e3819df49c9c
Cr-Commit-Position: refs/heads/master@{#295824}