QUIC - Fix QUIC enabled experiment spending significantly more time verifying certs on Android

QUIC - Fix QUIC enabled experiment spending significantly more time verifying certs on Android.

Get computed  CertVerifier::VerifyFlags from SSLConfig from
HttpStreamFactoryImpl::Job and pass them to QUIC's
CertVerifier code. With this change, when
SSLClientSocket[NSS|OpenSSL] code tries  to verify the
code, certs are already cached and we don't verify certs
twice.

BUG=502509
[email protected]

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

[ramant (doing other things), 2015-06-20 00:40:36]

[email protected] changed reviewers:
+ [email protected]

[ramant (doing other things), 2015-06-20 00:41:29]

Patchset #1 (id:1) has been deleted

[ramant (doing other things), 2015-06-20 01:01:39]

Patchset #1 (id:20001) has been deleted

[ramant (doing other things), 2015-06-20 01:03:22]

https://codereview.chromium.org/1197823002/diff/40001/net/quic/quic_client_se...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1197823002/diff/40001/net/quic/quic_client_se...
net/quic/quic_client_session.cc:113: dict->SetInteger("cert_verify_flags",
cert_verify_flags);
hi Ryan,
  Is there any value for logging this flag? If not, will delete.
thanks
raman

[Ryan Hamilton, 2015-06-20 02:24:51]

Looks great, modulo a gotcha I noticed :/

https://codereview.chromium.org/1197823002/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1197823002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.cc:808: &proxy_ssl_config_, true /* is a
proxy server */);
nit: I know you didn't write this comment, but as long as you're here, can you
change the comment to be /*is_proxy=*/ here and below to be consistent with
other similar comments.

https://codereview.chromium.org/1197823002/diff/40001/net/quic/crypto/proof_v...
File net/quic/crypto/proof_verifier.h (right):

https://codereview.chromium.org/1197823002/diff/40001/net/quic/crypto/proof_v...
net/quic/crypto/proof_verifier.h:84: int cert_verify_flags,
Doh! I didn't think about this when we spoke. This is a shared interface and we
should keep the API consistent between internal and external. Instead of adding
|cert_verify_flags| to VerifyProof(), can you add it to
ProofVerifyContextChromium instead?

https://codereview.chromium.org/1197823002/diff/40001/net/quic/crypto/proof_v...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://codereview.chromium.org/1197823002/diff/40001/net/quic/crypto/proof_v...
net/quic/crypto/proof_verifier_chromium.cc:391: chromium_context->net_log));
In much the same way we get the net_log from the context, I think we should
probably be able to get the flags.

https://codereview.chromium.org/1197823002/diff/40001/net/quic/quic_client_se...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1197823002/diff/40001/net/quic/quic_client_se...
net/quic/quic_client_session.cc:113: dict->SetInteger("cert_verify_flags",
cert_verify_flags);
seems like it can't hurt, I guess, but I don't feel strongly either way. Are
they logged in the cert verify net-internals entry?

https://codereview.chromium.org/1197823002/diff/40001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1197823002/diff/40001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:65: const SSLConfig& ssl_config,
I don't feel strongly about this either, but you could consider simply passing
in the flags here, thus moving the SSLConfig -> flags logic up to
HttpStreamFactoryImpl::Job. Perhaps that's nice because QUIC doesn't *Actually*
use SSL. On the other hand it doesn't really matter so totally feel free to keep
this here.

[ramant (doing other things), 2015-06-21 01:13:24]

Thanks very much Ryan for the comments. Made all the changes. PTAL.

https://codereview.chromium.org/1197823002/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1197823002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.cc:808: &proxy_ssl_config_, true /* is a
proxy server */);
On 2015/06/20 02:24:51, Ryan Hamilton wrote:
> nit: I know you didn't write this comment, but as long as you're here, can you
> change the comment to be /*is_proxy=*/ here and below to be consistent with
> other similar comments.  

Done.

https://codereview.chromium.org/1197823002/diff/40001/net/quic/crypto/proof_v...
File net/quic/crypto/proof_verifier.h (right):

https://codereview.chromium.org/1197823002/diff/40001/net/quic/crypto/proof_v...
net/quic/crypto/proof_verifier.h:84: int cert_verify_flags,
On 2015/06/20 02:24:51, Ryan Hamilton wrote:
> Doh! I didn't think about this when we spoke. This is a shared interface and
we
> should keep the API consistent between internal and external. Instead of
adding
> |cert_verify_flags| to VerifyProof(), can you add it to
> ProofVerifyContextChromium instead?

Good point. Done.

https://codereview.chromium.org/1197823002/diff/40001/net/quic/crypto/proof_v...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://codereview.chromium.org/1197823002/diff/40001/net/quic/crypto/proof_v...
net/quic/crypto/proof_verifier_chromium.cc:391: chromium_context->net_log));
On 2015/06/20 02:24:51, Ryan Hamilton wrote:
> In much the same way we get the net_log from the context, I think we should
> probably be able to get the flags.

Done.

https://codereview.chromium.org/1197823002/diff/40001/net/quic/quic_client_se...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1197823002/diff/40001/net/quic/quic_client_se...
net/quic/quic_client_session.cc:113: dict->SetInteger("cert_verify_flags",
cert_verify_flags);
On 2015/06/20 02:24:51, Ryan Hamilton wrote:
> seems like it can't hurt, I guess, but I don't feel strongly either way. Are
> they logged in the cert verify net-internals entry?

Goof point. They are not logged in cert verify (would be good to log there so
that we could verify why we didn't get a cache hit. Will make that change there
too in the next CL when we move cert_verify_flags into SSLConfig)

https://codereview.chromium.org/1197823002/diff/40001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1197823002/diff/40001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:65: const SSLConfig& ssl_config,
On 2015/06/20 02:24:51, Ryan Hamilton wrote:
> I don't feel strongly about this either, but you could consider simply passing
> in the flags here, thus moving the SSLConfig -> flags logic up to
> HttpStreamFactoryImpl::Job. Perhaps that's nice because QUIC doesn't
*Actually*
> use SSL. On the other hand it doesn't really matter so totally feel free to
keep
> this here.

+1 to the above idea.

Done.

[Ryan Hamilton, 2015-06-21 20:13:36]

LGTM! Looks great. Thanks for doing this.

https://codereview.chromium.org/1197823002/diff/40001/net/quic/quic_client_se...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1197823002/diff/40001/net/quic/quic_client_se...
net/quic/quic_client_session.cc:113: dict->SetInteger("cert_verify_flags",
cert_verify_flags);
On 2015/06/21 01:13:23, ramant wrote:
> On 2015/06/20 02:24:51, Ryan Hamilton wrote:
> > seems like it can't hurt, I guess, but I don't feel strongly either way. Are
> > they logged in the cert verify net-internals entry?
> 
> Goof point. They are not logged in cert verify (would be good to log there so
> that we could verify why we didn't get a cache hit. Will make that change
there
> too in the next CL when we move cert_verify_flags into SSLConfig)

SGTM!

https://codereview.chromium.org/1197823002/diff/60001/net/quic/crypto/proof_v...
File net/quic/crypto/proof_verifier_chromium.h (right):

https://codereview.chromium.org/1197823002/diff/60001/net/quic/crypto/proof_v...
net/quic/crypto/proof_verifier_chromium.h:47: explicit
ProofVerifyContextChromium(int cert_verify_flags,
nit: no need to make this explicit.

[ramant (doing other things), 2015-06-21 22:09:02]

Thanks very much Ryan,
raman

https://codereview.chromium.org/1197823002/diff/60001/net/quic/crypto/proof_v...
File net/quic/crypto/proof_verifier_chromium.h (right):

https://codereview.chromium.org/1197823002/diff/60001/net/quic/crypto/proof_v...
net/quic/crypto/proof_verifier_chromium.h:47: explicit
ProofVerifyContextChromium(int cert_verify_flags,
On 2015/06/21 20:13:36, Ryan Hamilton wrote:
> nit: no need to make this explicit.

Done.

[ramant (doing other things), 2015-06-21 22:09:14]

The CQ bit was checked by [email protected]

[ramant (doing other things), 2015-06-21 22:09:14]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected]
Link to the patchset: https://codereview.chromium.org/1197823002/#ps80001
(title: "removed explicit")

[commit-bot: I haz the power, 2015-06-21 22:09:19]

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

[commit-bot: I haz the power, 2015-06-21 23:59:57]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-06-22 00:00:50]

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