Persist the server config that is received via kSCUP.

Persist the server config that is received via kSCUP.

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

Committed: https://crrev.com/c584fb92ddd78cb2c2a36b90f0d7be30227d3cff
Cr-Commit-Position: refs/heads/master@{#295582}

[ramant (doing other things), 2014-07-30 19:46:36]

Persisted server config if proof is valid after chromium receives STK and server
config update.

If proof is not valid (when we receive STK update), we could set proof to be
valid for insecure QUIC. Talking with rjshade if server sends kSCUP (server
config updates) for insecure QUIC or not.

        if (!cached->proof_valid()) {
          if (!server_id_.is_https()) {
            // We don't check the certificates for insecure QUIC connections.
            SetCachedProofValid(cached);

[wtc, 2014-07-31 00:43:06]

https://codereview.chromium.org/427313002/diff/1/net/quic/quic_crypto_client_...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/427313002/diff/1/net/quic/quic_crypto_client_...
net/quic/quic_crypto_client_stream.cc:162: if (cached->proof_valid()) {

If we have access to the proof verifier here, we should verify the proof (for an
https server) that is not yet verified.

[ramant (doing other things), 2014-08-01 19:52:22]

https://codereview.chromium.org/427313002/diff/1/net/quic/quic_crypto_client_...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/427313002/diff/1/net/quic/quic_crypto_client_...
net/quic/quic_crypto_client_stream.cc:162: if (cached->proof_valid()) {
On 2014/07/31 00:43:06, wtc wrote:
> 
> If we have access to the proof verifier here, we should verify the proof (for
an
> https server) that is not yet verified.

Base on internal CL: 71770356, it looks like  we are not getting certs and proof
when we receive kSCUP message.

Would like to talk to you about async proof verification (may be we could
separate out the state machine for proof verification).

Will hold off checking this change until we merge 71770356. rjshade@: what do
you think? Or is there any advantage to add a TODO for proof verification and
check-in this change for proof's that are already valid.

[chromium-reviews, 2014-08-01 19:54:37]

The intention is to send certs and proofs in the SCUP. cl/71770356 is
attempting to do this.

On Fri, Aug 1, 2014 at 3:52 PM,  <rtenneti@chromium.org> wrote:
>
>
https://codereview.chromium.org/427313002/diff/1/net/quic/quic_crypto_client_...
> File net/quic/quic_crypto_client_stream.cc (right):
>
>
https://codereview.chromium.org/427313002/diff/1/net/quic/quic_crypto_client_...
> net/quic/quic_crypto_client_stream.cc:162: if (cached->proof_valid()) {
> On 2014/07/31 00:43:06, wtc wrote:
>
>> If we have access to the proof verifier here, we should verify the
>
> proof (for an
>>
>> https server) that is not yet verified.
>
>
> Base on internal CL: 71770356, it looks like  we are not getting certs
> and proof when we receive kSCUP message.
>
> Would like to talk to you about async proof verification (may be we
> could separate out the state machine for proof verification).
>
> Will hold off checking this change until we merge 71770356. rjshade@:
> what do you think? Or is there any advantage to add a TODO for proof
> verification and check-in this change for proof's that are already
> valid.
>
> https://codereview.chromium.org/427313002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ramant (doing other things), 2014-09-05 02:12:38]

Patchset #2 (id:20001) has been deleted

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

Patchset #2 (id:40001) has been deleted

[Ryan Hamilton, 2014-09-15 22:37:56]

I think this looks pretty good. One issue we discussed offline is to make sure
that we don't confused the cert used during the handshake with the one pushed in
SCUP and hence become confused about what cert a page should be associated with.

One suggestion about refactoring the loop.

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

https://codereview.chromium.org/427313002/diff/60001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:503: next_state_ =
STATE_VERIFY_PROOF_COMPLETE;
Since we have to conditionally set next_state_ in these methods, I wonder if it
wouldn't be easier to just have a single DoLoop (or DoHandshakeLoop) method
which is capable of handling any of the state transitions, instead of a
HandshakeLoop and a ProofLoop?

https://codereview.chromium.org/427313002/diff/60001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:544: return QUIC_PROOF_INVALID;
possibly add a histogram here when the handshake has been confirmed to see how
often (never?) this happens.

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

Patchset #3 (id:80001) has been deleted

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

Hi Ryan,
  Made the changes, we have discussed. PTAL.

thanks
raman

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

https://codereview.chromium.org/427313002/diff/60001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:503: next_state_ =
STATE_VERIFY_PROOF_COMPLETE;
On 2014/09/15 22:37:56, Ryan Hamilton wrote:
> Since we have to conditionally set next_state_ in these methods, I wonder if
it
> wouldn't be easier to just have a single DoLoop (or DoHandshakeLoop) method
> which is capable of handling any of the state transitions, instead of a
> HandshakeLoop and a ProofLoop?

Done.

https://codereview.chromium.org/427313002/diff/60001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:544: return QUIC_PROOF_INVALID;
On 2014/09/15 22:37:56, Ryan Hamilton wrote:
> possibly add a histogram here when the handshake has been confirmed to see how
> often (never?) this happens.

Done.

[Ryan Hamilton, 2014-09-17 15:52:42]

this change LGTM. I'd prefer to see the states simplified, but I'd be happy to
see that in a follow up. (It would also be great to refactor the state machine
so that each STATE has a DoState method and to use the conventions that we
typically use in our state machines. But such a cleanup would only make sense in
a followup)

https://codereview.chromium.org/427313002/diff/100001/net/quic/quic_crypto_cl...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/427313002/diff/100001/net/quic/quic_crypto_cl...
net/quic/quic_crypto_client_stream.cc:467: break;
Since the implementation of these two states are basically the same thing, I
think we should merge the two states.

DoProofVerify will always transition to STATE_VERIFY_PROOF_COMPLETE

DoProofVerifyComplete will conditionally transition to the next state based on
handshake_confirmed().

What do you think?

[ramant (doing other things), 2014-09-17 22:10:05]

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

[ramant (doing other things), 2014-09-17 22:13:03]

rtenneti@chromium.org changed reviewers:
- wtc@chromium.org

[ramant (doing other things), 2014-09-17 22:13:04]

+agl@: would appreciate your comments.
+asvitkine@: for histograms.xml changes.

https://codereview.chromium.org/427313002/diff/100001/net/quic/quic_crypto_cl...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/427313002/diff/100001/net/quic/quic_crypto_cl...
net/quic/quic_crypto_client_stream.cc:467: break;
On 2014/09/17 15:52:41, Ryan Hamilton wrote:
> Since the implementation of these two states are basically the same thing, I
> think we should merge the two states.
> 
> DoProofVerify will always transition to STATE_VERIFY_PROOF_COMPLETE
> 
> DoProofVerifyComplete will conditionally transition to the next state based on
> handshake_confirmed().
> 
> What do you think?

Done.

[ramant (doing other things), 2014-09-18 03:30:59]

Patchset #4 (id:120001) has been deleted

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

Patchset #4 (id:140001) has been deleted

[ramant (doing other things), 2014-09-18 03:42:09]

Patchset #4 (id:160001) has been deleted

[ramant (doing other things), 2014-09-18 03:46:10]

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

[ramant (doing other things), 2014-09-18 03:52:13]

Hi Ryan and Ricardo,
  Made a small change to keep disk cache entry open for multiple writes to
happen. Previously we were closing the entry after write is complete. Ricardo
and I have seen, when we tried to write again, we get IO_PENDING or FAILED error
and we weren't writing again.

  This change keeps the cache entry to be opened and we close it when the
QuicServerInfo is deleted.

  The following is an example of multiple writes to disk cache entry:

t= 90479 [st=    0] +DISK_CACHE_ENTRY_IMPL  [dt=31980]
                     --> created = true
                     --> key = "quicserverinfo:http://www.google.com:80"
t= 90523 [st=   44]   +ENTRY_WRITE_DATA  [dt=0]
                       --> buf_len = 216
                       --> index = 0
                       --> offset = 0
                       --> truncate = true
t= 90523 [st=   44]   -ENTRY_WRITE_DATA
                       --> bytes_copied = 216
t= 93436 [st= 2957]   +ENTRY_WRITE_DATA  [dt=1]
                       --> buf_len = 248
                       --> index = 0
                       --> offset = 0
                       --> truncate = true
t= 93437 [st= 2958]   -ENTRY_WRITE_DATA
                       --> bytes_copied = 248
t=122458 [st=31979]    ENTRY_CLOSE
t=122459 [st=31980] -DISK_CACHE_ENTRY_IMPL

PTAL.

[Alexei Svitkine (slow), 2014-09-18 13:41:01]

lgtm

[agl, 2014-09-18 19:11:47]

agl@chromium.org changed reviewers:
+ agl@chromium.org

[agl, 2014-09-18 19:11:48]

lgtm

[rjshade, 2014-09-18 19:42:51]

lgtm, thanks for doing this and looking forward to seeing the results

https://codereview.chromium.org/427313002/diff/180001/net/http/disk_cache_bas...
File net/http/disk_cache_based_quic_server_info.cc (right):

https://codereview.chromium.org/427313002/diff/180001/net/http/disk_cache_bas...
net/http/disk_cache_based_quic_server_info.cc:227: entry_ = data_shim_->entry;
Chromium style guide doesn't require {} around single line bodies?

[Ryan Hamilton, 2014-09-18 19:52:52]

Still LGTM

https://codereview.chromium.org/427313002/diff/180001/net/http/disk_cache_bas...
File net/http/disk_cache_based_quic_server_info.cc (right):

https://codereview.chromium.org/427313002/diff/180001/net/http/disk_cache_bas...
net/http/disk_cache_based_quic_server_info.cc:227: entry_ = data_shim_->entry;
On 2014/09/18 19:42:51, rjshade wrote:
> Chromium style guide doesn't require {} around single line bodies?

Nope; Chrome's unofficial net/ style guide prohibits them :>

[ramant (doing other things), 2014-09-18 21:05:59]

The CQ bit was checked by rtenneti@chromium.org

[ramant (doing other things), 2014-09-18 21:07:16]

On 2014/09/18 19:52:52, Ryan Hamilton wrote:
> Still LGTM
> 
>
https://codereview.chromium.org/427313002/diff/180001/net/http/disk_cache_bas...
> File net/http/disk_cache_based_quic_server_info.cc (right):
> 
>
https://codereview.chromium.org/427313002/diff/180001/net/http/disk_cache_bas...
> net/http/disk_cache_based_quic_server_info.cc:227: entry_ = data_shim_->entry;
> On 2014/09/18 19:42:51, rjshade wrote:
> > Chromium style guide doesn't require {} around single line bodies?
> 
> Nope; Chrome's unofficial net/ style guide prohibits them :>

Many many thanks Adam, Alexei, Robbie and Ryan for code review,
raman

[commit-bot: I haz the power, 2014-09-18 21:08:13]

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

[commit-bot: I haz the power, 2014-09-18 22:19:33]

Committed patchset #4 (id:180001) as 6e75c2235199be33b0e598360473721cb5e09460

[commit-bot: I haz the power, 2014-09-18 22:20:15]

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

[rvargas (doing something else), 2014-09-19 00:27:31]

lgtm