Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
154933003:
Persist server's crypto config data to disk cache for 0-RTT (Closed)
|
Created:
6 years, 10 months ago by ramant (doing other things) Modified:
6 years, 10 months ago Reviewers:
wtc CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPersist server's crypto config data to disk cache for 0-RTT.
+ Changed QuicCryptoClientStream's state machine to wait for the read of disk cache data before we sent CHLO.
+ This code is not hooked up yet because QuicServerInfoFactory is not yet hooked up. I have tested the code by manually creating it.
This CL was coded by wtc/raman
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252310
Patch Set 1 #Patch Set 2 : Update with TOT and cleanup of the code #Patch Set 3 : Update with TOT and handle multiple tabs loading same URL #
Total comments: 52
Patch Set 4 : Deleted unused data_loaded_ member #
Total comments: 38
Patch Set 5 : Fix comments for Patch Set 4 #
Total comments: 28
Patch Set 6 : Merge with trunk #
Total comments: 4
Patch Set 7 : Undid bad upload (had problems with git merge) #
Total comments: 14
Patch Set 8 : Fixed comments in patch set 7 #
Total comments: 8
Messages
Total messages: 21 (0 generated)
Hi Wan-Teh, This is first cut at persisting/reading the QUIC server information. Would appreciate if you coudl take a quick look. thanks very much raman
Review comments on patch set 3: Raman, overall it is good. I'd like to review it again. Here are the comments from my first pass. Thanks. https://codereview.chromium.org/154933003/diff/490002/net/http/disk_cache_bas... File net/http/disk_cache_based_quic_server_info.cc (right): https://codereview.chromium.org/154933003/diff/490002/net/http/disk_cache_bas... net/http/disk_cache_based_quic_server_info.cc:80: return ERR_INVALID_ARGUMENT; Nit: we may want to use a different error code. Can you explain why you want to allow user_callback_ to be null? https://codereview.chromium.org/154933003/diff/490002/net/http/disk_cache_bas... net/http/disk_cache_based_quic_server_info.cc:204: new_data_.clear(); Another good place to do new_data_.clear() is in DoSetDone, because we can also go to the SET_DONE state from DoCreateOrOpenComplete. https://codereview.chromium.org/154933003/diff/490002/net/http/disk_cache_bas... File net/http/disk_cache_based_quic_server_info_unittest.cc (right): https://codereview.chromium.org/154933003/diff/490002/net/http/disk_cache_bas... net/http/disk_cache_based_quic_server_info_unittest.cc:107: state = quic_server_info->mutable_state(); 1. We don't need mutable_state() here. We can just use state() (read only). 2. Nit: add a blank line before this line. https://codereview.chromium.org/154933003/diff/490002/net/http/disk_cache_bas... net/http/disk_cache_based_quic_server_info_unittest.cc:116: state = quic_server_info->mutable_state(); Remove this line. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:171: need_to_persist_ = true; IMPORTANT: is this a bug? It seems that we should only persist if the proof is valid, but not if the proof is invalid. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:231: if (proof_valid() || !signature().empty()) { 1. Why do you test !signature().empty()? 2. Since we are in the same class, we should use the members proof_valid_, signature_, quic_server_info_ directly, rather than their getter methods. Please check all the new code you wrote for this issue. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:246: } I think we should be able to assign a std::vector directly: certs_ = state.certs_; https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:250: void QuicCryptoClientConfig::CachedState::SaveQuicServerInfo() { IMPORTANT: these two methods show a duplication of data in CachedState and QuicServerInfo. We should eliminate the duplication of data. This may mean combining CachedState and QuicServerInfo into one. For example, CachedState can implement the QuicServerInfo interface. Other solutions are possible. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:252: !need_to_persist_) { It seems that if we set need_to_persist_ appropriately, we just need to test !need_to_persist_ here. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:270: } An assignment should work: state->certs_ = certs_; https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... File net/quic/crypto/quic_crypto_client_config.h (right): https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.h:100: // |server_config_sig_| fields of |this| from |quic_server_info_|. Nit: remove "of |this|". That is implied. Nit: the comment should mention quic_server_info_ will read from the disk cache. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... File net/quic/crypto/quic_server_info.cc (right): https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:7: #include "base/bind.h" I think this header can be removed. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:10: using std::string; I think we should remove this "using std::string" statement because this file is only used in Chromium. If you think we should add "using std::string", then we should remove all the "std::" from "std::string" in this file. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:14: } // namespace 1. Nit: kQuicCryptoConfigVersion can be of a smaller type, such as int or unsigned int. It will be pickled as an int, so I think int is appropriate. 2. Nit: add blank lines, like this: namespace { const size_t kQuicCryptoConfigVersion = 1; } // namespace https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:50: data_loaded_ = false; I recommend moving this to the beginning of ParseInner. This way, by looking at only the ParseInner function, we know when we set data_loaded_ to false and when we set it to true. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:70: if (!p.ReadInt(&iter, &version) || version < 0) { You should check version != kQuicCryptoConfigVersion instead of (or in addition to) version < 0. We only support kQuicCryptoConfigVersion. version != kQuicCryptoConfigVersion should get a different log message "Unsupported version". https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:115: !p.WriteInt(state_.certs_.size())) { This probably should be p.WriteUInt32 since state_.certs_.size() is unsigned. It would also be nice to check for overflow: ... state_.certs_.size() > std::numeric_limits<uint32>::max() || !p.WriteUint32(state_.certs_.size())) { If we use WriteInt, then we should check: ... state_.certs_.size() > std::numeric_limits<int>::max() || !p.WriteInt(state_.certs_.size())) { Need to include <limits> for std::numeric_limits. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:116: return ""; Nit: it may be better to return std::string(). https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... File net/quic/crypto/quic_server_info.h (right): https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.h:60: std::string server_config_; // A serialized handshake message. 1. Please add a comment to document this class matches QuicClientCryptoConfig::CachedState. 2. Should we also persist server_config_id_? https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.h:64: // order. Nit: it may be better to swap server_config_sig_ and certs_ so that they are declared in the same order as in QuicClientCryptoConfig::CachedState. For convenience we can still serialize server_config_sig_ before certs_. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.h:94: base::WeakPtrFactory<QuicServerInfo> weak_factory_; This member is not being used. https://codereview.chromium.org/154933003/diff/490002/net/quic/quic_crypto_cl... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/154933003/diff/490002/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:180: } I think this should look like lines 274-283. In particular, we should call cached->SetProofValid() if crypto_config_->proof_verifier() is null. https://codereview.chromium.org/154933003/diff/490002/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:333: cached->SaveQuicServerInfo(); This can be refined. We only need to call cached->SaveQuicServerInfo() if the server info comes from a REJ message, rather than from the disk cache. https://codereview.chromium.org/154933003/diff/490002/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:415: int QuicCryptoClientStream::DoLoadQuicServerInfo( This function should be split into two state: STATE_LOAD_QUIC_SERVER_INFO and STATE_LOAD_QUIC_SERVER_INFO_COMPLETE. https://codereview.chromium.org/154933003/diff/490002/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:431: return rv; We may want to return OK in this case. We should add a comment that it is fine to proceed to STATE_SEND_CHLO when we cannot load from the disk cache. We should log a warning message in that case. https://codereview.chromium.org/154933003/diff/490002/net/quic/quic_crypto_cl... File net/quic/quic_crypto_client_stream.h (right): https://codereview.chromium.org/154933003/diff/490002/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.h:94: int DoLoadQuicServerInfo(QuicCryptoClientConfig::CachedState* cached); Right now STATE_LOAD_QUIC_SERVER_INFO is the only state that has a DoXXX function. Please add a TODO comment to convert the other states of the state machine into DoXXX functions. Alternatively, just inline this function into DoHandshakeLoop.
Hi Wan-Teh, Made all the changes. Add IsDataReady method to check if we could save or not (and also if data is parsed or not). thanks raman https://codereview.chromium.org/154933003/diff/490002/net/http/disk_cache_bas... File net/http/disk_cache_based_quic_server_info.cc (right): https://codereview.chromium.org/154933003/diff/490002/net/http/disk_cache_bas... net/http/disk_cache_based_quic_server_info.cc:80: return ERR_INVALID_ARGUMENT; On 2014/02/07 00:54:11, wtc wrote: > > Nit: we may want to use a different error code. > > Can you explain why you want to allow user_callback_ to be null? If user_callback_ not null, then there is a pending callback. Overwriting a previous user_callback_ with a new callback doesn't seem to be good. Thought, if we give an error, then caller knows WaitForDataReady has failed and can handle the error condition, instead of overwriting previous caller's callback. Would appreciate if you could recommend a better error code. https://codereview.chromium.org/154933003/diff/490002/net/http/disk_cache_bas... net/http/disk_cache_based_quic_server_info.cc:204: new_data_.clear(); On 2014/02/07 00:54:11, wtc wrote: > > Another good place to do new_data_.clear() is in DoSetDone, because we can also > go to the SET_DONE state from DoCreateOrOpenComplete. Done. https://codereview.chromium.org/154933003/diff/490002/net/http/disk_cache_bas... File net/http/disk_cache_based_quic_server_info_unittest.cc (right): https://codereview.chromium.org/154933003/diff/490002/net/http/disk_cache_bas... net/http/disk_cache_based_quic_server_info_unittest.cc:107: state = quic_server_info->mutable_state(); On 2014/02/07 00:54:11, wtc wrote: > > 1. We don't need mutable_state() here. We can just use state() (read only). > > 2. Nit: add a blank line before this line. Done. https://codereview.chromium.org/154933003/diff/490002/net/http/disk_cache_bas... net/http/disk_cache_based_quic_server_info_unittest.cc:116: state = quic_server_info->mutable_state(); On 2014/02/07 00:54:11, wtc wrote: > > Remove this line. Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:171: need_to_persist_ = true; On 2014/02/07 00:54:11, wtc wrote: > > IMPORTANT: is this a bug? It seems that we should only persist if the proof is > valid, but not if the proof is invalid. Added need_to_persist_ = true in SetProof and only when certs have changed. WDYT? https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:231: if (proof_valid() || !signature().empty()) { On 2014/02/07 00:54:11, wtc wrote: > > 1. Why do you test !signature().empty()? > > 2. Since we are in the same class, we should use the members proof_valid_, > signature_, quic_server_info_ directly, rather than their getter methods. > > Please check all the new code you wrote for this issue. Made the changes to access members directly. > Why do you test !signature().empty()? Was worried about the race condition, that multiple requests for the same URL were going on at the same time. For the first request, we are waiting for the data to be read from disk cache. The second request might have sent CHLO (without certs) and server has sent back REJ message with certs and we are in the process of verifying proof. Meanwhile first request has finished reading the data from the disk and trying to update the data with what it has read. This change says, we will use the data from disk cache only if the cache object doesn't have data. What do you think? https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:246: } On 2014/02/07 00:54:11, wtc wrote: > > I think we should be able to assign a std::vector directly: > > certs_ = state.certs_; Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:250: void QuicCryptoClientConfig::CachedState::SaveQuicServerInfo() { On 2014/02/07 00:54:11, wtc wrote: > > IMPORTANT: these two methods show a duplication of data in CachedState and > QuicServerInfo. We should eliminate the duplication of data. > > This may mean combining CachedState and QuicServerInfo into one. For example, > CachedState can implement the QuicServerInfo interface. Other solutions are > possible. Added a TODO for the above. Possible race condition between reading from disk cache and another request updating the cached state. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:252: !need_to_persist_) { On 2014/02/07 00:54:11, wtc wrote: > > It seems that if we set need_to_persist_ appropriately, we just need to test > !need_to_persist_ here. Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:270: } On 2014/02/07 00:54:11, wtc wrote: > > An assignment should work: > > state->certs_ = certs_; Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... File net/quic/crypto/quic_crypto_client_config.h (right): https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.h:100: // |server_config_sig_| fields of |this| from |quic_server_info_|. On 2014/02/07 00:54:11, wtc wrote: > > Nit: remove "of |this|". That is implied. > > Nit: the comment should mention quic_server_info_ will read from the disk cache. Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... File net/quic/crypto/quic_server_info.cc (right): https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:7: #include "base/bind.h" On 2014/02/07 00:54:11, wtc wrote: > > I think this header can be removed. Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:10: using std::string; On 2014/02/07 00:54:11, wtc wrote: > > I think we should remove this "using std::string" statement because this file is > only used in Chromium. > > If you think we should add "using std::string", then we should remove all the > "std::" from "std::string" in this file. Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:14: } // namespace On 2014/02/07 00:54:11, wtc wrote: > > 1. Nit: kQuicCryptoConfigVersion can be of a smaller type, such as int or > unsigned int. It will be pickled as an int, so I think int is appropriate. > > 2. Nit: add blank lines, like this: > > namespace { > > const size_t kQuicCryptoConfigVersion = 1; > > } // namespace Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:50: data_loaded_ = false; On 2014/02/07 00:54:11, wtc wrote: > > I recommend moving this to the beginning of ParseInner. This way, by looking at > only the ParseInner function, we know when we set data_loaded_ to false and when > we set it to true. Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:70: if (!p.ReadInt(&iter, &version) || version < 0) { On 2014/02/07 00:54:11, wtc wrote: > > You should check version != kQuicCryptoConfigVersion instead of (or in addition > to) version < 0. We only support kQuicCryptoConfigVersion. > > version != kQuicCryptoConfigVersion should get a different log message > "Unsupported version". Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:115: !p.WriteInt(state_.certs_.size())) { On 2014/02/07 00:54:11, wtc wrote: > > This probably should be p.WriteUInt32 since state_.certs_.size() is unsigned. It > would also be nice to check for overflow: > > ... > state_.certs_.size() > std::numeric_limits<uint32>::max() || > !p.WriteUint32(state_.certs_.size())) { > > If we use WriteInt, then we should check: > > ... > state_.certs_.size() > std::numeric_limits<int>::max() || > !p.WriteInt(state_.certs_.size())) { > > Need to include <limits> for std::numeric_limits. Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:115: !p.WriteInt(state_.certs_.size())) { On 2014/02/07 00:54:11, wtc wrote: > > This probably should be p.WriteUInt32 since state_.certs_.size() is unsigned. It > would also be nice to check for overflow: > > ... > state_.certs_.size() > std::numeric_limits<uint32>::max() || > !p.WriteUint32(state_.certs_.size())) { > > If we use WriteInt, then we should check: > > ... > state_.certs_.size() > std::numeric_limits<int>::max() || > !p.WriteInt(state_.certs_.size())) { > > Need to include <limits> for std::numeric_limits. Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... File net/quic/crypto/quic_server_info.h (right): https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.h:60: std::string server_config_; // A serialized handshake message. On 2014/02/07 00:54:11, wtc wrote: > > 1. Please add a comment to document this class matches > QuicClientCryptoConfig::CachedState. > > 2. Should we also persist server_config_id_? server_config_id_ is not used. Added a comment to delete that member. Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.h:64: // order. On 2014/02/07 00:54:11, wtc wrote: > > Nit: it may be better to swap server_config_sig_ and certs_ so that they are > declared in the same order as in QuicClientCryptoConfig::CachedState. > > For convenience we can still serialize server_config_sig_ before certs_. Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.h:94: base::WeakPtrFactory<QuicServerInfo> weak_factory_; On 2014/02/07 00:54:11, wtc wrote: > > This member is not being used. Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/quic_crypto_cl... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/154933003/diff/490002/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:180: } On 2014/02/07 00:54:11, wtc wrote: > > I think this should look like lines 274-283. In particular, we should call > cached->SetProofValid() if crypto_config_->proof_verifier() is null. Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:333: cached->SaveQuicServerInfo(); On 2014/02/07 00:54:11, wtc wrote: > > This can be refined. We only need to call cached->SaveQuicServerInfo() if the > server info comes from a REJ message, rather than from the disk cache. ACK (and changed the code). We set need_to_persist_ to true only when we get REJ message. SaveQuicServerInfo saves the data if need_to_persist_ is true. https://codereview.chromium.org/154933003/diff/490002/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:415: int QuicCryptoClientStream::DoLoadQuicServerInfo( On 2014/02/07 00:54:11, wtc wrote: > > This function should be split into two state: STATE_LOAD_QUIC_SERVER_INFO and > STATE_LOAD_QUIC_SERVER_INFO_COMPLETE. Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:431: return rv; On 2014/02/07 00:54:11, wtc wrote: > > We may want to return OK in this case. We should add a comment that it is fine > to proceed to STATE_SEND_CHLO when we cannot load from the disk cache. We should > log a warning message in that case. Done. https://codereview.chromium.org/154933003/diff/490002/net/quic/quic_crypto_cl... File net/quic/quic_crypto_client_stream.h (right): https://codereview.chromium.org/154933003/diff/490002/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.h:94: int DoLoadQuicServerInfo(QuicCryptoClientConfig::CachedState* cached); On 2014/02/07 00:54:11, wtc wrote: > > Right now STATE_LOAD_QUIC_SERVER_INFO is the only state that has a DoXXX > function. > > Please add a TODO comment to convert the other states of the state machine into > DoXXX functions. Alternatively, just inline this function into DoHandshakeLoop. Added a TODO. Done.
Review comments on patch set 4: Raman, thanks for the new patch set. I suggest some changes. https://codereview.chromium.org/154933003/diff/970001/net/http/disk_cache_bas... File net/http/disk_cache_based_quic_server_info.cc (right): https://codereview.chromium.org/154933003/diff/970001/net/http/disk_cache_bas... net/http/disk_cache_based_quic_server_info.cc:80: return ERR_INVALID_ARGUMENT; 1. Please add a comment to explain why we need to check for a non-null user_callback_. 2. The ERR_INVALID_ARGUMENT error code is not ideal. Ideally we should use an error code that says "busy" or "in use". If this code is temporary, it is fine to use ERR_INVALID_ARGUMENT. https://codereview.chromium.org/154933003/diff/970001/net/http/disk_cache_bas... File net/http/disk_cache_based_quic_server_info_unittest.cc (right): https://codereview.chromium.org/154933003/diff/970001/net/http/disk_cache_bas... net/http/disk_cache_based_quic_server_info_unittest.cc:114: EXPECT_EQ(cert_a, state1.certs_[0]); Nit: move line 110 right before this line because they are all related to state1.certs_. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_cr... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:120: server_config_ = server_config.as_string(); It seems that should set need_to_persist_ to true here because we changed server_config_ from an external source (as opposed to the disk cache). https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:155: need_to_persist_ = true; This doesn't seem like the right place to set need_to_persist to true because the proof for a server config is optional. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:237: if (server_config_valid_ || !server_config_sig_.empty()) { It seems that we should not test server_config_valid_. It should be enough to just test !server_config_.empty(). Testing !server_config_sig_.empty() is not ideal because server config signature is optional. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:243: if (state.certs_.empty()) { It is better to test state.server_config_.empty() instead, because state.certs_ may be empty in a valid State (if we didn't request a proof). https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:268: state->Clear(); This state->Clear() call is not necessary because we are going to set all members of |state| next. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:274: quic_server_info_->Persist(); We should set need_to_persist_ to false after the Persist() call. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_se... File net/quic/crypto/quic_server_info.cc (right): https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:69: if (!p.ReadInt(&iter, &version) || version < 0) { I think we don't need to test if version < 0. We can just rely on the version != kQuicCryptoConfigVersion test below. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:94: if (!p.ReadInt(&iter, &num_certs) || num_certs < 0) { num_certs should have the uint32 type to match the Serialize() method: uint32 num_certs; if (!p.ReadUInt32(&iter, &num_certs)) { ... } for (uint32 i = 0; i < num_certs; i++) { ... https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_se... File net/quic/crypto/quic_server_info.h (right): https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.h:47: // Returns true is data is loaded from disk cache and ready (WaitForDataReady Typo: is data is => if data is https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.h:69: std::string server_config_sig_; // A signature of |server_config_|. These members should follow the naming convention of struct members (without a trailing underscore). https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:190: } I think this (lines 181-190) should be moved to DoLoadQuicServerInfoComplete. https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:343: cached->SaveQuicServerInfo(); We should also call cached->SaveQuicServerInfo() when we don't need to verify the server config (i.e., when there is no proof verifier). It seems that whenever we call cached->SetProofValid(), we should call cached->SaveQuicServerInfo(). https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:421: DCHECK_EQ(STATE_LOAD_QUIC_SERVER_INFO, next_state_); next_state_ should be STATE_LOAD_QUIC_SERVER_INFO_COMPLETE. https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:427: } Lines 423-427 should be deleted. Since DoHandshakeLoop takes a pointer, we can't pass |result| to DoHandshakeLoop directly, so we'll need to store |result| in a class member if DoHandshakeLoop needs to know about |result|. In this case, it is OK to just drop |result| because DoLoadQuicServerInfoComplete has an alternative way to check that (cached->quic_server_info()->IsDataReady()). https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:443: int rv = quic_server_info->WaitForDataReady( We probably should save the generation count of |cached| here, and compare with it in DoLoadQuicServerInfoComplete. https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:449: next_state_ = STATE_LOAD_QUIC_SERVER_INFO; This should be STATE_LOAD_QUIC_SERVER_INFO_COMPLETE. https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:463: if (!cached->quic_server_info()->IsDataReady()) { We probably need to check the generation count here.
Many many thanks Wan-Teh. Made all the changes and verified the code. PTAL. https://codereview.chromium.org/154933003/diff/970001/net/http/disk_cache_bas... File net/http/disk_cache_based_quic_server_info.cc (right): https://codereview.chromium.org/154933003/diff/970001/net/http/disk_cache_bas... net/http/disk_cache_based_quic_server_info.cc:80: return ERR_INVALID_ARGUMENT; On 2014/02/11 01:01:45, wtc wrote: > > 1. Please add a comment to explain why we need to check for a non-null > user_callback_. > > 2. The ERR_INVALID_ARGUMENT error code is not ideal. Ideally we should use an > error code that says "busy" or "in use". If this code is temporary, it is fine > to use ERR_INVALID_ARGUMENT. Done. https://codereview.chromium.org/154933003/diff/970001/net/http/disk_cache_bas... File net/http/disk_cache_based_quic_server_info_unittest.cc (right): https://codereview.chromium.org/154933003/diff/970001/net/http/disk_cache_bas... net/http/disk_cache_based_quic_server_info_unittest.cc:114: EXPECT_EQ(cert_a, state1.certs_[0]); On 2014/02/11 01:01:45, wtc wrote: > > Nit: move line 110 right before this line because they are all related to > state1.certs_. Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_cr... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:120: server_config_ = server_config.as_string(); On 2014/02/11 01:01:45, wtc wrote: > > It seems that should set need_to_persist_ to true here because we changed > server_config_ from an external source (as opposed to the disk cache). Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:155: need_to_persist_ = true; On 2014/02/11 01:01:45, wtc wrote: > > This doesn't seem like the right place to set need_to_persist to true because > the proof for a server config is optional. Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:237: if (server_config_valid_ || !server_config_sig_.empty()) { On 2014/02/11 01:01:45, wtc wrote: > > It seems that we should not test server_config_valid_. > > It should be enough to just test !server_config_.empty(). Testing > !server_config_sig_.empty() is not ideal because server config signature is > optional. Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:243: if (state.certs_.empty()) { On 2014/02/11 01:01:45, wtc wrote: > > It is better to test state.server_config_.empty() instead, because state.certs_ > may be empty in a valid State (if we didn't request a proof). Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:268: state->Clear(); On 2014/02/11 01:01:45, wtc wrote: > > This state->Clear() call is not necessary because we are going to set all > members of |state| next. Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:274: quic_server_info_->Persist(); On 2014/02/11 01:01:45, wtc wrote: > > We should set need_to_persist_ to false after the Persist() call. Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_se... File net/quic/crypto/quic_server_info.cc (right): https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:69: if (!p.ReadInt(&iter, &version) || version < 0) { On 2014/02/11 01:01:45, wtc wrote: > > I think we don't need to test if version < 0. We can just rely on the version != > kQuicCryptoConfigVersion test below. Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.cc:94: if (!p.ReadInt(&iter, &num_certs) || num_certs < 0) { On 2014/02/11 01:01:45, wtc wrote: > > num_certs should have the uint32 type to match the Serialize() method: > > uint32 num_certs; > if (!p.ReadUInt32(&iter, &num_certs)) { > ... > } > > for (uint32 i = 0; i < num_certs; i++) { > ... Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_se... File net/quic/crypto/quic_server_info.h (right): https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.h:47: // Returns true is data is loaded from disk cache and ready (WaitForDataReady On 2014/02/11 01:01:45, wtc wrote: > > Typo: is data is => if data is Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/crypto/quic_se... net/quic/crypto/quic_server_info.h:69: std::string server_config_sig_; // A signature of |server_config_|. On 2014/02/11 01:01:45, wtc wrote: > > These members should follow the naming convention of struct members (without a > trailing underscore). Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:190: } On 2014/02/11 01:01:45, wtc wrote: > > I think this (lines 181-190) should be moved to DoLoadQuicServerInfoComplete. Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:343: cached->SaveQuicServerInfo(); On 2014/02/11 01:01:45, wtc wrote: > > We should also call cached->SaveQuicServerInfo() when we don't need to verify > the server config (i.e., when there is no proof verifier). > > It seems that whenever we call cached->SetProofValid(), we should call > cached->SaveQuicServerInfo(). Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:421: DCHECK_EQ(STATE_LOAD_QUIC_SERVER_INFO, next_state_); On 2014/02/11 01:01:45, wtc wrote: > > next_state_ should be STATE_LOAD_QUIC_SERVER_INFO_COMPLETE. Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:427: } On 2014/02/11 01:01:45, wtc wrote: > > Lines 423-427 should be deleted. > > Since DoHandshakeLoop takes a pointer, we can't pass |result| to DoHandshakeLoop > directly, so we'll need to store |result| in a class member if DoHandshakeLoop > needs to know about |result|. In this case, it is OK to just drop |result| > because DoLoadQuicServerInfoComplete has an alternative way to check that > (cached->quic_server_info()->IsDataReady()). Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:443: int rv = quic_server_info->WaitForDataReady( On 2014/02/11 01:01:45, wtc wrote: > > We probably should save the generation count of |cached| here, and compare with > it in DoLoadQuicServerInfoComplete. Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:449: next_state_ = STATE_LOAD_QUIC_SERVER_INFO; On 2014/02/11 01:01:45, wtc wrote: > > This should be STATE_LOAD_QUIC_SERVER_INFO_COMPLETE. Done. https://codereview.chromium.org/154933003/diff/970001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:463: if (!cached->quic_server_info()->IsDataReady()) { On 2014/02/11 01:01:45, wtc wrote: > > We probably need to check the generation count here. Done.
Patch set 5 LGTM. Please ask me to explain the comments in quic_crypto_client_config.cc and quic_crypto_client_stream.cc. Some of the issues are quite complicated. Thanks. https://codereview.chromium.org/154933003/diff/1500001/net/http/disk_cache_ba... File net/http/disk_cache_based_quic_server_info.cc (right): https://codereview.chromium.org/154933003/diff/1500001/net/http/disk_cache_ba... net/http/disk_cache_based_quic_server_info.cc:80: // pending callback(|user_callback_|). Nit: add a space before the opening parenthesis '('. https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:248: server_config_ = state.server_config; I now think we should call SetServerConfig() to set server_config_ rather than setting server_config_ directly. SetServerConfig() will check for expiry and call SetProofInvalid(), which increments the generation counter. We'll need to make sure that need_to_persist_ should be set to false here, but for the other call to SetServerConfig() we need to set need_to_persist_ to true. Perhaps we need to add a new method for setting need_to_persist_ https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... File net/quic/crypto/quic_crypto_client_config.h (right): https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.h:71: // calls SaveQuicServerInfo() to persist the server config information to Nit: it is not necessary to mention it calls SaveQuicServerInfo. Just say "It persists the server config information to disk cache." https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.h:111: // TODO(rtenneti): |server_config_id_| is not being used, delete it. You can delete this TODO comment now because I just deleted |server_config_id_|. https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:78: proof_verify_callback_(NULL) { We should initialize quic_server_info_data_ready_result_ (to either OK or ERR_UNEXPECTED) in the constructor. https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:424: generation_counter_ = cached->generation_counter(); I think we should set next_state_ to STATE_LOAD_QUIC_SERVER_INFO_COMPLETE here, and remove all the subsequent assignments of next_state_. https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:449: return OK; I think lines 440-449 can be replaced by: if (rv != ERR_IO_PENDING) { quic_server_info_data_ready_result_ = rv; } return rv; It should be fine to return an |rv| of error because the return value of this function is ignored unless it is ERR_IO_PENDING. https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:452: void QuicCryptoClientStream::DoLoadQuicServerInfoComplete( The logic of this method probably should be (omitting the log messages): next_state_ = STATE_SEND_CHLO; if (generation_counter_ == cached->generation_counter()) { if (quic_server_info_data_ready_result_ != OK) { // It is ok to proceed to STATE_SEND_CHLO when we cannot // load QuicServerInfo from the disk cache. DCHECK that |cached| is empty. return; } cached->LoadQuicServerInfo(); } if (cached->proof_valid()) { return; } ProofVerifier* verifier = crypto_config_->proof_verifier(); ... https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... File net/quic/quic_crypto_client_stream.h (right): https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.h:130: int quic_server_info_data_ready_result_; 1. Please list this member after cert_verify_result_. Otherwise it sits in between the members related to proof verification. 2. Nit: this member name could be shorter. For example, "disk_cache_load_result_".
More comments on patch set 5: https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:237: void QuicCryptoClientConfig::CachedState::LoadQuicServerInfo() { This method needs to return a bool status. https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:238: if (!server_config_.empty()) { This should become a DCHECK: DCHECK(server_config_.empty()); Also DCHECK(quic_server_info_->IsDataReady()). https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:245: return; We should return false. https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:248: server_config_ = state.server_config; If SetServerConfig() fails, we should return false. https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:452: void QuicCryptoClientStream::DoLoadQuicServerInfoComplete( void QuicCryptoClientStream::DoLoadQuicServerInfoComplete( QuicCryptoClientConfig::CachedState* cached) { next_state_ = STATE_SEND_CHLO; if (generation_counter_ != cached->generation_counter()) { // Someone else has already saved a server config received // from the network. return; } if (quic_server_info_data_ready_result_ != OK || !cached->LoadQuicServerInfo()) { // It is ok to proceed to STATE_SEND_CHLO when we cannot // load QuicServerInfo from the disk cache. DCHECK that cached.server_config_ is empty. return; } ProofVerifier* verifier = crypto_config_->proof_verifier(); if (!verifier) { // If no verifier is set then we don't check the certificates. cached->SetProofValid(); } else if (!cached->signature().empty()) { next_state_ = STATE_VERIFY_PROOF; } }
This is the code Wan-Teh and I had worked together for the last couple of weeks. IMO, we have identified all the remaining TODO items and we think it is good design to save and restore server config data from the disk cache. This CL doesn't enable this code yet. https://codereview.chromium.org/154933003/diff/1500001/net/http/disk_cache_ba... File net/http/disk_cache_based_quic_server_info.cc (right): https://codereview.chromium.org/154933003/diff/1500001/net/http/disk_cache_ba... net/http/disk_cache_based_quic_server_info.cc:80: // pending callback(|user_callback_|). On 2014/02/13 23:48:43, wtc wrote: > > Nit: add a space before the opening parenthesis '('. Done. https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:237: void QuicCryptoClientConfig::CachedState::LoadQuicServerInfo() { On 2014/02/14 01:46:13, wtc wrote: > > This method needs to return a bool status. Done. https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:238: if (!server_config_.empty()) { On 2014/02/14 01:46:13, wtc wrote: > > This should become a DCHECK: > DCHECK(server_config_.empty()); > > Also DCHECK(quic_server_info_->IsDataReady()). Done. https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:245: return; On 2014/02/14 01:46:13, wtc wrote: > > We should return false. Done. https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:248: server_config_ = state.server_config; On 2014/02/13 23:48:43, wtc wrote: > > I now think we should call SetServerConfig() to set server_config_ rather than > setting server_config_ directly. SetServerConfig() will check for expiry and > call SetProofInvalid(), which increments the generation counter. > > We'll need to make sure that need_to_persist_ should be set to false here, but > for the other call to SetServerConfig() we need to set need_to_persist_ to true. > Perhaps we need to add a new method for setting need_to_persist_ Done. https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:248: server_config_ = state.server_config; On 2014/02/13 23:48:43, wtc wrote: > > I now think we should call SetServerConfig() to set server_config_ rather than > setting server_config_ directly. SetServerConfig() will check for expiry and > call SetProofInvalid(), which increments the generation counter. > > We'll need to make sure that need_to_persist_ should be set to false here, but > for the other call to SetServerConfig() we need to set need_to_persist_ to true. > Perhaps we need to add a new method for setting need_to_persist_ Done. https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... File net/quic/crypto/quic_crypto_client_config.h (right): https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.h:71: // calls SaveQuicServerInfo() to persist the server config information to On 2014/02/13 23:48:43, wtc wrote: > > Nit: it is not necessary to mention it calls SaveQuicServerInfo. Just say "It > persists the server config information to disk cache." Done. https://codereview.chromium.org/154933003/diff/1500001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.h:111: // TODO(rtenneti): |server_config_id_| is not being used, delete it. On 2014/02/13 23:48:43, wtc wrote: > > You can delete this TODO comment now because I just deleted |server_config_id_|. Done. https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:78: proof_verify_callback_(NULL) { On 2014/02/13 23:48:43, wtc wrote: > > We should initialize quic_server_info_data_ready_result_ (to either OK or > ERR_UNEXPECTED) in the constructor. Done. https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:424: generation_counter_ = cached->generation_counter(); On 2014/02/13 23:48:43, wtc wrote: > > I think we should set next_state_ to STATE_LOAD_QUIC_SERVER_INFO_COMPLETE here, > and remove all the subsequent assignments of next_state_. Done. https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:449: return OK; On 2014/02/13 23:48:43, wtc wrote: > > I think lines 440-449 can be replaced by: > > if (rv != ERR_IO_PENDING) { > quic_server_info_data_ready_result_ = rv; > } > return rv; > > It should be fine to return an |rv| of error because the return value of this > function is ignored unless it is ERR_IO_PENDING. Done. https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:452: void QuicCryptoClientStream::DoLoadQuicServerInfoComplete( On 2014/02/13 23:48:43, wtc wrote: > > The logic of this method probably should be (omitting the log messages): > > next_state_ = STATE_SEND_CHLO; > > if (generation_counter_ == cached->generation_counter()) { > if (quic_server_info_data_ready_result_ != OK) { > // It is ok to proceed to STATE_SEND_CHLO when we cannot > // load QuicServerInfo from the disk cache. > DCHECK that |cached| is empty. > return; > } > cached->LoadQuicServerInfo(); > } > > if (cached->proof_valid()) { > return; > } > > ProofVerifier* verifier = crypto_config_->proof_verifier(); > ... > Done. https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:452: void QuicCryptoClientStream::DoLoadQuicServerInfoComplete( On 2014/02/14 01:46:13, wtc wrote: > > void QuicCryptoClientStream::DoLoadQuicServerInfoComplete( > QuicCryptoClientConfig::CachedState* cached) { > next_state_ = STATE_SEND_CHLO; > > if (generation_counter_ != cached->generation_counter()) { > // Someone else has already saved a server config received > // from the network. > return; > } > > if (quic_server_info_data_ready_result_ != OK || > !cached->LoadQuicServerInfo()) { > // It is ok to proceed to STATE_SEND_CHLO when we cannot > // load QuicServerInfo from the disk cache. > DCHECK that cached.server_config_ is empty. > return; > } > > ProofVerifier* verifier = crypto_config_->proof_verifier(); > if (!verifier) { > // If no verifier is set then we don't check the certificates. > cached->SetProofValid(); > } else if (!cached->signature().empty()) { > next_state_ = STATE_VERIFY_PROOF; > } > } As we had talked implemented the second comment. https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... File net/quic/quic_crypto_client_stream.h (right): https://codereview.chromium.org/154933003/diff/1500001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.h:130: int quic_server_info_data_ready_result_; On 2014/02/13 23:48:43, wtc wrote: > > 1. Please list this member after cert_verify_result_. Otherwise it sits in > between the members related to proof verification. > > 2. Nit: this member name could be shorter. For example, > "disk_cache_load_result_". Done.
Review comments on patch set 6: I will review this again carefully tomorrow. https://codereview.chromium.org/154933003/diff/2530001/net/quic/crypto/quic_c... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/154933003/diff/2530001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:52: state->SetConfigData(&server_config_, It is a little dangerous for quic_server_info_ to point directly at the members of CachedState because quic_server_info_->Persist() does not complete right away. When quic_server_info_ actually writes the data to the disk cache, the members of CachedState may have changed. https://codereview.chromium.org/154933003/diff/2530001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:243: // of data. I think you should remove this TODO comment now. https://codereview.chromium.org/154933003/diff/2530001/net/quic/crypto/quic_s... File net/quic/crypto/quic_server_info.h (right): https://codereview.chromium.org/154933003/diff/2530001/net/quic/crypto/quic_s... net/quic/crypto/quic_server_info.h:64: void SetConfigData(std::string* server_config, This function name is confusing.
Hi Wan-Teh, Please ignore the previous upload. I was running into http server 500 errors while uploading the previous patch. PTAL of this patch. thanks raman https://codereview.chromium.org/154933003/diff/2530001/net/quic/crypto/quic_c... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/154933003/diff/2530001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:52: state->SetConfigData(&server_config_, On 2014/02/19 02:24:11, wtc wrote: > > It is a little dangerous for quic_server_info_ to point directly at the members > of CachedState because quic_server_info_->Persist() does not complete right > away. When quic_server_info_ actually writes the data to the disk cache, the > members of CachedState may have changed. This change shouldn't have been uploaded. I was having issues uploading the changes. Let me upload again. Sorry about that.
Patch set 7 LGTM. https://codereview.chromium.org/154933003/diff/2720001/net/quic/crypto/quic_c... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/154933003/diff/2720001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:226: server_config_ = other.server_config_; I think this method should also bump the generation counter. It seems wrong to do that by calling SetServerConfig here because SetServerConfig will also set server_config_valid_ to false. So we may need to just do ++generation_counter_. https://codereview.chromium.org/154933003/diff/2720001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:273: // save anything. This seems wrong. This means we may not save the data in CachedState to the disk cache. We should fix this problem (maybe later). https://codereview.chromium.org/154933003/diff/2720001/net/quic/crypto/quic_c... File net/quic/crypto/quic_crypto_client_config.h (right): https://codereview.chromium.org/154933003/diff/2720001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.h:106: // the data from disk cache successfully. We should also document how |now| is used. https://codereview.chromium.org/154933003/diff/2720001/net/quic/quic_crypto_c... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/154933003/diff/2720001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:424: disk_cache_load_result_ = OK; This probably should be initialized to ERR_UNEXPECTED. We should never use the value we assign here. Alternatively, just remove this line. https://codereview.chromium.org/154933003/diff/2720001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:434: // config. This comment ("We always call WaitForDataReady, ...") is hard to understand. You should point out that quic_server_info->Persist requires quic_server_info to be ready. In "we might have already loaded server config from a canonical config", change "loaded server config" to "initialized |cached|". Change "a canonical config" to "the cached state for a canonical hostname". https://codereview.chromium.org/154933003/diff/2720001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:453: // information from canonical server config)? If |cached| becomes non-empty but the generation counter is the same, there is a bug. The code that fills |cached| needs to bump the generation counter. I guess the bug is in CachedState::InitializeFrom? Once the bug in CachedState::InitializeFrom is fixed, I think it should be sufficient to just check generation_counter_ != cached->generation_counter(). On the other hand, it seems fine to just check !cached->IsEmpty() because that's really what we want to check: if someone else already saved a server config, we don't want to overwrite it. Also, it someone else saved a server config and then cleared it (so cached->IsEmpty() is true, but the generation counter changed), we still want to load from QuicServerInfo. In summary, I think we can just check !cached->IsEmpty() and remove the code related to generation_counter_ from this method and the DoLoadQuicServerInfo method. https://codereview.chromium.org/154933003/diff/2720001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:456: // Someone else has already saved a server config received from the network. This comment should say "from the network or the canonical server config".
Thanks much wtc. Made the changes you have suggested. https://codereview.chromium.org/154933003/diff/2720001/net/quic/crypto/quic_c... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/154933003/diff/2720001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:226: server_config_ = other.server_config_; On 2014/02/19 22:53:28, wtc wrote: > > I think this method should also bump the generation counter. It seems wrong to > do that by calling SetServerConfig here because SetServerConfig will also set > server_config_valid_ to false. So we may need to just do ++generation_counter_. Done. https://codereview.chromium.org/154933003/diff/2720001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:273: // save anything. On 2014/02/19 22:53:28, wtc wrote: > > This seems wrong. This means we may not save the data in CachedState to the disk > cache. We should fix this problem (maybe later). Added a TODO. thanks. https://codereview.chromium.org/154933003/diff/2720001/net/quic/crypto/quic_c... File net/quic/crypto/quic_crypto_client_config.h (right): https://codereview.chromium.org/154933003/diff/2720001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.h:106: // the data from disk cache successfully. On 2014/02/19 22:53:28, wtc wrote: > > We should also document how |now| is used. Done. https://codereview.chromium.org/154933003/diff/2720001/net/quic/quic_crypto_c... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/154933003/diff/2720001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:424: disk_cache_load_result_ = OK; On 2014/02/19 22:53:28, wtc wrote: > > This probably should be initialized to ERR_UNEXPECTED. We should never use the > value we assign here. > > Alternatively, just remove this line. Done. https://codereview.chromium.org/154933003/diff/2720001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:434: // config. On 2014/02/19 22:53:28, wtc wrote: > > This comment ("We always call WaitForDataReady, ...") is hard to understand. > > You should point out that quic_server_info->Persist requires quic_server_info to > be ready. > > In "we might have already loaded server config from a canonical config", change > "loaded server config" to "initialized |cached|". Change "a canonical config" to > "the cached state for a canonical hostname". Done. https://codereview.chromium.org/154933003/diff/2720001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:453: // information from canonical server config)? On 2014/02/19 22:53:28, wtc wrote: > > If |cached| becomes non-empty but the generation counter is the same, there is a > bug. The code that fills |cached| needs to bump the generation counter. I guess > the bug is in CachedState::InitializeFrom? > > Once the bug in CachedState::InitializeFrom is fixed, I think it should be > sufficient to just check generation_counter_ != cached->generation_counter(). > > On the other hand, it seems fine to just check !cached->IsEmpty() because that's > really what we want to check: if someone else already saved a server config, we > don't want to overwrite it. Also, it someone else saved a server config and then > cleared it (so cached->IsEmpty() is true, but the generation counter changed), > we still want to load from QuicServerInfo. > > In summary, I think we can just check !cached->IsEmpty() and remove the code > related to generation_counter_ from this method and the DoLoadQuicServerInfo > method. Done. https://codereview.chromium.org/154933003/diff/2720001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:456: // Someone else has already saved a server config received from the network. On 2014/02/19 22:53:28, wtc wrote: > > This comment should say "from the network or the canonical server config". Done.
The CQ bit was checked by rtenneti@chromium.org
The CQ bit was unchecked by rtenneti@chromium.org
Will make further changes in another CL. On 2014/02/20 02:34:06, ramant wrote: > Thanks much wtc. Made the changes you have suggested. > > https://codereview.chromium.org/154933003/diff/2720001/net/quic/crypto/quic_c... > File net/quic/crypto/quic_crypto_client_config.cc (right): > > https://codereview.chromium.org/154933003/diff/2720001/net/quic/crypto/quic_c... > net/quic/crypto/quic_crypto_client_config.cc:226: server_config_ = > other.server_config_; > On 2014/02/19 22:53:28, wtc wrote: > > > > I think this method should also bump the generation counter. It seems wrong to > > do that by calling SetServerConfig here because SetServerConfig will also set > > server_config_valid_ to false. So we may need to just do > ++generation_counter_. > > Done. > > https://codereview.chromium.org/154933003/diff/2720001/net/quic/crypto/quic_c... > net/quic/crypto/quic_crypto_client_config.cc:273: // save anything. > On 2014/02/19 22:53:28, wtc wrote: > > > > This seems wrong. This means we may not save the data in CachedState to the > disk > > cache. We should fix this problem (maybe later). > > Added a TODO. thanks. > > https://codereview.chromium.org/154933003/diff/2720001/net/quic/crypto/quic_c... > File net/quic/crypto/quic_crypto_client_config.h (right): > > https://codereview.chromium.org/154933003/diff/2720001/net/quic/crypto/quic_c... > net/quic/crypto/quic_crypto_client_config.h:106: // the data from disk cache > successfully. > On 2014/02/19 22:53:28, wtc wrote: > > > > We should also document how |now| is used. > > Done. > > https://codereview.chromium.org/154933003/diff/2720001/net/quic/quic_crypto_c... > File net/quic/quic_crypto_client_stream.cc (right): > > https://codereview.chromium.org/154933003/diff/2720001/net/quic/quic_crypto_c... > net/quic/quic_crypto_client_stream.cc:424: disk_cache_load_result_ = OK; > On 2014/02/19 22:53:28, wtc wrote: > > > > This probably should be initialized to ERR_UNEXPECTED. We should never use the > > value we assign here. > > > > Alternatively, just remove this line. > > Done. > > https://codereview.chromium.org/154933003/diff/2720001/net/quic/quic_crypto_c... > net/quic/quic_crypto_client_stream.cc:434: // config. > On 2014/02/19 22:53:28, wtc wrote: > > > > This comment ("We always call WaitForDataReady, ...") is hard to understand. > > > > You should point out that quic_server_info->Persist requires quic_server_info > to > > be ready. > > > > In "we might have already loaded server config from a canonical config", > change > > "loaded server config" to "initialized |cached|". Change "a canonical config" > to > > "the cached state for a canonical hostname". > > Done. > > https://codereview.chromium.org/154933003/diff/2720001/net/quic/quic_crypto_c... > net/quic/quic_crypto_client_stream.cc:453: // information from canonical server > config)? > On 2014/02/19 22:53:28, wtc wrote: > > > > If |cached| becomes non-empty but the generation counter is the same, there is > a > > bug. The code that fills |cached| needs to bump the generation counter. I > guess > > the bug is in CachedState::InitializeFrom? > > > > Once the bug in CachedState::InitializeFrom is fixed, I think it should be > > sufficient to just check generation_counter_ != cached->generation_counter(). > > > > On the other hand, it seems fine to just check !cached->IsEmpty() because > that's > > really what we want to check: if someone else already saved a server config, > we > > don't want to overwrite it. Also, it someone else saved a server config and > then > > cleared it (so cached->IsEmpty() is true, but the generation counter changed), > > we still want to load from QuicServerInfo. > > > > In summary, I think we can just check !cached->IsEmpty() and remove the code > > related to generation_counter_ from this method and the DoLoadQuicServerInfo > > method. > > Done. > > https://codereview.chromium.org/154933003/diff/2720001/net/quic/quic_crypto_c... > net/quic/quic_crypto_client_stream.cc:456: // Someone else has already saved a > server config received from the network. > On 2014/02/19 22:53:28, wtc wrote: > > > > This comment should say "from the network or the canonical server config". > > Done.
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/154933003/2970001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/154933003/2970001
Message was sent while issue was closed.
Change committed as 252310
Message was sent while issue was closed.
Comments on patch set 8: please address these in a new CL. Thanks! https://codereview.chromium.org/154933003/diff/2970001/net/quic/crypto/quic_c... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/154933003/diff/2970001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:231: ++generation_counter_; I verified that whenever we change (set, modify, or clear) server_config_, we bump the generation counter. I noticed that we set and clear server_config_ and scfg_ at the same time. Do you know why CachedState::InitializeFrom doesn't copy scfg_ from |other|? https://codereview.chromium.org/154933003/diff/2970001/net/quic/quic_crypto_c... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/154933003/diff/2970001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:431: // quic_server_info->Persist requires quic_server_info to be ready. We might Thanks for updating the comment. The new comment is still confusing because readers may wonder why the comment tells me Persist requires quic_server_info to be ready. Maybe something like this? We may need to call quic_server_info->Persist later. quic_server_info->Persist requires quic_server_info to be ready, so we always call WaitForDataReady, even though we might have initialized |cached| config from the cached state for a canonical hostname. Does this say what you intended to say originally? https://codereview.chromium.org/154933003/diff/2970001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:450: // cached->IsEmpty() is true, but the generation counter changed), we still We should delete ", but the generation counter changed" because the code is no longer checking the generation counter. https://codereview.chromium.org/154933003/diff/2970001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:454: // or the canonical server config. We only need one of these comments, but not both. I would probably keep the first one and delete the second one.
Message was sent while issue was closed.
https://codereview.chromium.org/154933003/diff/2970001/net/quic/crypto/quic_c... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/154933003/diff/2970001/net/quic/crypto/quic_c... net/quic/crypto/quic_crypto_client_config.cc:231: ++generation_counter_; On 2014/02/20 20:06:16, wtc wrote: > > I verified that whenever we change (set, modify, or clear) server_config_, we > bump the generation counter. > > I noticed that we set and clear server_config_ and scfg_ at the same time. Do > you know why CachedState::InitializeFrom doesn't copy scfg_ from |other|? We lazily initialize it in GetServerConfig (which parses server_config_ and updates the scfg_ if it not NULL). https://codereview.chromium.org/154933003/diff/2970001/net/quic/quic_crypto_c... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/154933003/diff/2970001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:431: // quic_server_info->Persist requires quic_server_info to be ready. We might On 2014/02/20 20:06:16, wtc wrote: > > Thanks for updating the comment. The new comment is still confusing because > readers may wonder why the comment tells me Persist requires quic_server_info to > be ready. > > Maybe something like this? > > We may need to call quic_server_info->Persist later. > quic_server_info->Persist requires quic_server_info > to be ready, so we always call WaitForDataReady, even > though we might have initialized |cached| config from > the cached state for a canonical hostname. > > Does this say what you intended to say originally? Many many thanks for the new comments (they read lot better). Done in CL https://codereview.chromium.org/174533002. https://codereview.chromium.org/154933003/diff/2970001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:450: // cached->IsEmpty() is true, but the generation counter changed), we still On 2014/02/20 20:06:16, wtc wrote: > > We should delete ", but the generation counter changed" because the code is no > longer checking the generation counter. Done in CL https://codereview.chromium.org/174533002 https://codereview.chromium.org/154933003/diff/2970001/net/quic/quic_crypto_c... net/quic/quic_crypto_client_stream.cc:454: // or the canonical server config. On 2014/02/20 20:06:16, wtc wrote: > > We only need one of these comments, but not both. I would probably keep the > first one and delete the second one. Done in CL https://codereview.chromium.org/174533002 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
