QUIC - Start the process for reading crypto config data from disk cache

QUIC - Start the process for reading crypto config data from disk cache
as early as possible (even before host resokution has started).

Create QuicServerInfo in QuicStreamFactory and pass the ownership
to QuicCryptoClientConfig::CachedState.

This is a parital release. Will be hooking up persisting and
reading of the data (including waiting for the disk read to finish)
in the next release.

R=wtc@chromium.org

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

[wtc, 2014-01-30 20:14:26]

Patch set 1: thanks for the CL. The changes to quic_stream_factory.{h,cc} still
need work.

https://codereview.chromium.org/149413008/diff/1/net/quic/crypto/quic_crypto_...
File net/quic/crypto/quic_crypto_client_config.h (right):

https://codereview.chromium.org/149413008/diff/1/net/quic/crypto/quic_crypto_...
net/quic/crypto/quic_crypto_client_config.h:34:
CachedState(scoped_ptr<QuicServerInfo> quic_server_info);

Please add "explicit" because this constructor takes one argument.

https://codereview.chromium.org/149413008/diff/1/net/quic/crypto/quic_crypto_...
net/quic/crypto/quic_crypto_client_config.h:119: explicit
QuicCryptoClientConfig(scoped_ptr<QuicServerInfo> quic_server_info);

Just to make sure I understand correctly: this change implies the
QuicCryptoClientConfig object can only handle one server hostname, even though
it is originally designed to handle multiple server hostnames (which is why
cached_states_ is a std::map). Correct?

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

https://codereview.chromium.org/149413008/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.cc:334: quic_server_info_->Start();

It is wrong to set quic_server_info_ here, because it will be overwritten the
next time QuicStreamFactory::Create is called.

We need to call quic_server_info_factory_->GetForHost elsewhere. Alternatively,
we need to call GetOrCreateCryptoConfig here so that we can pass the new
QuicServerInfo to the QuicCryptoClientConfig constructor.

https://codereview.chromium.org/149413008/diff/1/net/quic/quic_stream_factory.h
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/149413008/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.h:240: // |quic_server_info_| is used to fetching
crypto config information from disk

Nit: to fetching => to fetch

https://codereview.chromium.org/149413008/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.h:242: scoped_ptr<QuicServerInfo>
quic_server_info_;

It seems wrong for the QuicStreamFactory class to have a QuicServerInfo member,
because QuicServerInfo is only good for one server hostname.

[ramant (doing other things), 2014-02-02 19:35:09]

Hi Wan-Teh,
  Changed the code/API slightly. Will upload the other CL that reads/writes data
to disk cache asap.

 PTAL.

thanks
raman

https://codereview.chromium.org/149413008/diff/1/net/quic/crypto/quic_crypto_...
File net/quic/crypto/quic_crypto_client_config.h (right):

https://codereview.chromium.org/149413008/diff/1/net/quic/crypto/quic_crypto_...
net/quic/crypto/quic_crypto_client_config.h:34:
CachedState(scoped_ptr<QuicServerInfo> quic_server_info);
On 2014/01/30 20:14:26, wtc wrote:
> 
> Please add "explicit" because this constructor takes one argument.

Done.

https://codereview.chromium.org/149413008/diff/1/net/quic/crypto/quic_crypto_...
net/quic/crypto/quic_crypto_client_config.h:119: explicit
QuicCryptoClientConfig(scoped_ptr<QuicServerInfo> quic_server_info);
On 2014/01/30 20:14:26, wtc wrote:
> 
> Just to make sure I understand correctly: this change implies the
> QuicCryptoClientConfig object can only handle one server hostname, even though
> it is originally designed to handle multiple server hostnames (which is why
> cached_states_ is a std::map). Correct?

Thanks for the catch. Deleted this member.

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

https://codereview.chromium.org/149413008/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.cc:334: quic_server_info_->Start();
On 2014/01/30 20:14:26, wtc wrote:
> 
> It is wrong to set quic_server_info_ here, because it will be overwritten the
> next time QuicStreamFactory::Create is called.
> 
> We need to call quic_server_info_factory_->GetForHost elsewhere.
Alternatively,
> we need to call GetOrCreateCryptoConfig here so that we can pass the new
> QuicServerInfo to the QuicCryptoClientConfig constructor.

Modified the code to create the crypto config and start the async read of crypto
config from disk.
Done.

https://codereview.chromium.org/149413008/diff/1/net/quic/quic_stream_factory.h
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/149413008/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.h:240: // |quic_server_info_| is used to fetching
crypto config information from disk
On 2014/01/30 20:14:26, wtc wrote:
> 
> Nit: to fetching => to fetch

Deleted this code.

https://codereview.chromium.org/149413008/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.h:242: scoped_ptr<QuicServerInfo>
quic_server_info_;
On 2014/01/30 20:14:26, wtc wrote:
> 
> It seems wrong for the QuicStreamFactory class to have a QuicServerInfo
member,
> because QuicServerInfo is only good for one server hostname.

Thanks for catching this. Deleted this member from this class.

Done.

[wtc, 2014-02-04 01:37:02]

Patch set 2 LGTM.

Please review the CL's description and make sure it describes the current code.
Note that there is a typo "resokution".

https://codereview.chromium.org/149413008/diff/20001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_client_config.cc (right):

https://codereview.chromium.org/149413008/diff/20001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.cc:239: DCHECK(it ==
cached_states_.end());

Nit: this should be

  DCHECK_EQ(cached_states_.find(server_hostname), cached_states_.end());

or

  DCHECK(cached_states_.find(server_hostname) == cached_states_.end());

https://codereview.chromium.org/149413008/diff/20001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.cc:259:
cached_states_.insert(make_pair(server_hostname, cached));

It seems that you should use the new CreateCachedState method here.

https://codereview.chromium.org/149413008/diff/20001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_client_config.h (right):

https://codereview.chromium.org/149413008/diff/20001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.h:125: // Create returns a CachedState
for the given hostname.

Typo: Create => CreateCachedState

https://codereview.chromium.org/149413008/diff/20001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.h:127: // disk for a QUIC server
(server_hostname).

Nit: quic_server_info_factory itself doesn't fetch crypto config, right? I think
it is the QuicServerInfo that does that.

https://codereview.chromium.org/149413008/diff/20001/net/quic/quic_stream_fac...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/149413008/diff/20001/net/quic/quic_stream_fac...
net/quic/quic_stream_factory.cc:332: DCHECK(crypto_config);

Please add a comment to explain why we are doing this, because we ignore the
return value of GetOrCreateCryptoConfig.

[ramant (doing other things), 2014-02-04 19:23:53]

Hi Wan-Teh,
  Updated the comments and changed |CreateCachedState| to |Create| (ala
LookuoOrCreate). PTAL. thanks.

https://codereview.chromium.org/149413008/diff/20001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_client_config.cc (right):

https://codereview.chromium.org/149413008/diff/20001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.cc:239: DCHECK(it ==
cached_states_.end());
On 2014/02/04 01:37:02, wtc wrote:
> 
> Nit: this should be
> 
>   DCHECK_EQ(cached_states_.find(server_hostname), cached_states_.end());
> 
> or
> 
>   DCHECK(cached_states_.find(server_hostname) == cached_states_.end());

Done.

https://codereview.chromium.org/149413008/diff/20001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.cc:259:
cached_states_.insert(make_pair(server_hostname, cached));
On 2014/02/04 01:37:02, wtc wrote:
> 
> It seems that you should use the new CreateCachedState method here.

In chrome, we call LookupOrCreate() after we call Create().

For unittests of quic_stream_factory and quic_crypto_client_stream, we call
LookupOrCreate without calling Create(). For unittests quic_server_info_ is
NULL.

Done.

https://codereview.chromium.org/149413008/diff/20001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_client_config.h (right):

https://codereview.chromium.org/149413008/diff/20001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.h:125: // Create returns a CachedState
for the given hostname.
On 2014/02/04 01:37:02, wtc wrote:
> 
> Typo: Create => CreateCachedState

Changed the name of method to |Create|.

https://codereview.chromium.org/149413008/diff/20001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.h:127: // disk for a QUIC server
(server_hostname).
On 2014/02/04 01:37:02, wtc wrote:
> 
> Nit: quic_server_info_factory itself doesn't fetch crypto config, right? I
think
> it is the QuicServerInfo that does that.

Updated the comments. Would appreciate your comments of lines 125-128.

https://codereview.chromium.org/149413008/diff/20001/net/quic/quic_stream_fac...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/149413008/diff/20001/net/quic/quic_stream_fac...
net/quic/quic_stream_factory.cc:332: DCHECK(crypto_config);
On 2014/02/04 01:37:02, wtc wrote:
> 
> Please add a comment to explain why we are doing this, because we ignore the
> return value of GetOrCreateCryptoConfig.

Added a comment about why we are starting this process early.

Done.

https://codereview.chromium.org/149413008/diff/20001/net/quic/quic_stream_fac...
net/quic/quic_stream_factory.cc:573: all_crypto_configs_[host_port_proxy_pair];
Undid this change. GetOrCreateCryptoConfig is called when we create the Request
before we call CreateSession. The second call to GetOrCreateCryptoConfig
executes the same code as lines 571 through 573.

[wtc, 2014-02-05 00:03:24]

Patch set 3 LGTM. Thanks.

https://codereview.chromium.org/149413008/diff/100001/net/quic/crypto/quic_cr...
File net/quic/crypto/quic_crypto_client_config.cc (right):

https://codereview.chromium.org/149413008/diff/100001/net/quic/crypto/quic_cr...
net/quic/crypto/quic_crypto_client_config.cc:237:
DCHECK(cached_states_.find(server_hostname) == cached_states_.end());

This DCHECK should be moved outside the "if" statement. I think the Create
method can only be called when cached_states_ doesn't have an entry for
server_hostname, right?

https://codereview.chromium.org/149413008/diff/100001/net/quic/crypto/quic_cr...
File net/quic/crypto/quic_crypto_client_config.h (right):

https://codereview.chromium.org/149413008/diff/100001/net/quic/crypto/quic_cr...
net/quic/crypto/quic_crypto_client_config.h:125: // Create returns a CachedState
for the given hostname. It creates CachedState

Nit: add "a" between "creates" and "CachedState".

https://codereview.chromium.org/149413008/diff/100001/net/quic/crypto/quic_cr...
net/quic/crypto/quic_crypto_client_config.h:127: // to create
|quic_server_info_| which is used to fetch crypto config

Nit: change "|quic_server_info_|" to "a QuicServerInfo", because this class
doesn't have a quic_server_info_ member.

https://codereview.chromium.org/149413008/diff/100001/net/quic/crypto/quic_cr...
net/quic/crypto/quic_crypto_client_config.h:134: CachedState*
LookupOrCreate(const std::string& server_hostname);

Nit: it is a little strange that LookupOrCreate doesn't take a
quic_server_info_factory argument, but the closely related Create method does.

[ramant (doing other things), 2014-02-05 00:40:38]

https://codereview.chromium.org/149413008/diff/100001/net/quic/crypto/quic_cr...
File net/quic/crypto/quic_crypto_client_config.cc (right):

https://codereview.chromium.org/149413008/diff/100001/net/quic/crypto/quic_cr...
net/quic/crypto/quic_crypto_client_config.cc:237:
DCHECK(cached_states_.find(server_hostname) == cached_states_.end());
On 2014/02/05 00:03:25, wtc wrote:
> 
> This DCHECK should be moved outside the "if" statement. I think the Create
> method can only be called when cached_states_ doesn't have an entry for
> server_hostname, right?

Done.

https://codereview.chromium.org/149413008/diff/100001/net/quic/crypto/quic_cr...
File net/quic/crypto/quic_crypto_client_config.h (right):

https://codereview.chromium.org/149413008/diff/100001/net/quic/crypto/quic_cr...
net/quic/crypto/quic_crypto_client_config.h:125: // Create returns a CachedState
for the given hostname. It creates CachedState
On 2014/02/05 00:03:25, wtc wrote:
> 
> Nit: add "a" between "creates" and "CachedState".

Done.

https://codereview.chromium.org/149413008/diff/100001/net/quic/crypto/quic_cr...
net/quic/crypto/quic_crypto_client_config.h:127: // to create
|quic_server_info_| which is used to fetch crypto config
On 2014/02/05 00:03:25, wtc wrote:
> 
> Nit: change "|quic_server_info_|" to "a QuicServerInfo", because this class
> doesn't have a quic_server_info_ member.

Done.

https://codereview.chromium.org/149413008/diff/100001/net/quic/crypto/quic_cr...
net/quic/crypto/quic_crypto_client_config.h:134: CachedState*
LookupOrCreate(const std::string& server_hostname);
On 2014/02/05 00:03:25, wtc wrote:
> 
> Nit: it is a little strange that LookupOrCreate doesn't take a
> quic_server_info_factory argument, but the closely related Create method does.

ACK. LookupOrCreate is used by the server code (net/tools/quic). Added a TODO.

[ramant (doing other things), 2014-02-05 00:40:59]

The CQ bit was checked by rtenneti@chromium.org

[ramant (doing other things), 2014-02-05 00:41:04]

The CQ bit was unchecked by rtenneti@chromium.org

[ramant (doing other things), 2014-02-05 02:42:28]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-02-05 03:51:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/149413008/240001

[commit-bot: I haz the power, 2014-02-05 07:38:38]

Change committed as 248905