Merge internal change: 63891842 - QuicServerId changes

Merge internal change: 63891842

Begin internal description:

+ Use QuicServerId tuple (host, port, is_https) instead of
  server_hostname, while creating QuicClientSession,
  QuicCryptoClientStream, QuicCryptoClientConfig, QuicClient,
  QuicTestClient, etc objects.

+ QuicServerId is used as the key to access QUIC server
  config information from all caches.

+ Added couple of new unit tests for HostPortPair class.

End internal description


Most of the above changes were already in chromium. After the code
review in the internal source tree, this CL required merging.

The following are the chromium specific changes:
+ Search and replace:
  QuicSessionKey -> QuicServerId
  server_key     -> server_id
  session_key    -> server_id

+ Added unit tests for QuicServerId for privacy mode
  combination with host, port, is_https.

R=rch@chromium.org

[Ryan Hamilton, 2014-03-31 15:45:43]

lgtm

https://codereview.chromium.org/218923002/diff/1/net/base/host_port_pair.cc
File net/base/host_port_pair.cc (right):

https://codereview.chromium.org/218923002/diff/1/net/base/host_port_pair.cc#n...
net/base/host_port_pair.cc:53: LOG(DFATAL) << "Host has a null char: " << host_;
nit: how about DLOG_IF(DFATAL) ...?

https://codereview.chromium.org/218923002/diff/1/net/quic/quic_server_id_test.cc
File net/quic/quic_server_id_test.cc (right):

https://codereview.chromium.org/218923002/diff/1/net/quic/quic_server_id_test...
net/quic/quic_server_id_test.cc:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
Nice test!

[ramant (doing other things), 2014-03-31 18:40:31]

https://codereview.chromium.org/218923002/diff/1/net/base/host_port_pair.cc
File net/base/host_port_pair.cc (right):

https://codereview.chromium.org/218923002/diff/1/net/base/host_port_pair.cc#n...
net/base/host_port_pair.cc:53: LOG(DFATAL) << "Host has a null char: " << host_;
On 2014/03/31 15:45:43, Ryan Hamilton wrote:
> nit: how about DLOG_IF(DFATAL) ...? 

Fixed in
https://codereview.chromium.org/216713003/diff2/20001:40001/net/base/host_por...

Done.

https://codereview.chromium.org/218923002/diff/1/net/quic/quic_server_id_test.cc
File net/quic/quic_server_id_test.cc (right):

https://codereview.chromium.org/218923002/diff/1/net/quic/quic_server_id_test...
net/quic/quic_server_id_test.cc:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
On 2014/03/31 15:45:43, Ryan Hamilton wrote:
> Nice test!

Thanks very much.

[ramant (doing other things), 2014-04-01 03:19:56]

https://codereview.chromium.org/218923002/diff/1/net/base/host_port_pair.cc
File net/base/host_port_pair.cc (right):

https://codereview.chromium.org/218923002/diff/1/net/base/host_port_pair.cc#n...
net/base/host_port_pair.cc:53: LOG(DFATAL) << "Host has a null char: " << host_;
On 2014/03/31 15:45:43, Ryan Hamilton wrote:
> nit: how about DLOG_IF(DFATAL) ...? 

Hi Ryan,
  In net/spdy and net/quic, we are using LOG_IF(DFATAL. EXPECT_DFATAL fails if
we use DLOG_IF on CQ bots. Thus switched to using LOG_IF(DFATAL. Would
appreciate suggestions. thanks.

[wtc, 2014-04-01 22:10:58]

Patch set 1 LGTM.

Nit: the first two lines of the CL's description doesn't make sense to someone
unfamiliar with the history of this CL:

  Porting changes from chromium CL - 192583004 into internal source tree.

  https://codereview.chromium.org/192583004/

I suggest rewriting them or perhaps just deleting them. In addition, for this
particular CL, it would be better to say "Merge internal change: 63891842"
first, and either indent or add clear begin and and markers to the internal CL's
description.

https://codereview.chromium.org/218923002/diff/1/net/base/host_port_pair.cc
File net/base/host_port_pair.cc (right):

https://codereview.chromium.org/218923002/diff/1/net/base/host_port_pair.cc#n...
net/base/host_port_pair.cc:51: // TODO(rtenneti): Add support for |host| to have
'\0'.

Can you explain why |host| may have '\0'?

[ramant (doing other things), 2014-04-02 02:34:45]

https://codereview.chromium.org/218923002/diff/1/net/base/host_port_pair.cc
File net/base/host_port_pair.cc (right):

https://codereview.chromium.org/218923002/diff/1/net/base/host_port_pair.cc#n...
net/base/host_port_pair.cc:51: // TODO(rtenneti): Add support for |host| to have
'\0'.
On 2014/04/01 22:10:58, wtc wrote:
> 
> Can you explain why |host| may have '\0'?

avd mentioned if host is constructed in code where the host user-input, then an
attacker could embed \0 in the string.

Should we delete this code/comment?

The following is a comment from avd:

If HostPortPair class is used in the server environment (could attacker provider
a host string with \0 embedded in it)
The problem is that the Host string is user-input.  I as an attacker can trigger
this DCHECK by constructing specific packets.