Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(306)

Issue 20047002: net: make QUIC ProofVerifier more generic. (Closed)

Created:
7 years, 5 months ago by agl
Modified:
7 years, 4 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, wtc, Ryan Hamilton
Visibility:
Public.

Description

net: make QUIC ProofVerifier more generic. The QUIC ProofVerifier code is currently rather Chromium specific: it's using weak pointers, base::Bind etc. This change wraps abstractions around things so that the code is more portable again. It also solves an issue where, when a QUIC connection is canceled while a verification is running, the verification can write into free memory. I went back and forth on this change a bit. It effectively reinvents weak pointers and callbacks in order not to use the Chromium versions of these things. This is unfortunate but it is desirable to minimise the amount of skew between different copies of the QUIC code. In the end, the duplicate didn't seem so bad to me. Weak pointers are replaced with an explicit callback interface for the ProofVerifier. The QUIC client stream implements a Cancel method so that it can cope with being deleted while a proof verification is still running. The need to store a CertVerifyDetails is taken care of with an abstract class for wrapping "implementation specific" results from a verification. There is still Chromium-specific code that casts it to a Chromium object, but that's unavoidable somewhere. (Although it's not clear that the QUIC client stream is the best place for it.) BUG=none Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213862

Patch Set 1 #

Total comments: 20

Patch Set 2 : Addressing rch's comments. #

Patch Set 3 : ... #

Patch Set 4 : Forgot proof_verifier.cc #

Patch Set 5 : clang-style fixes #

Patch Set 6 : Add export tag for unittest. #

Patch Set 7 : Can't use a token called "ERROR" on Windows. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+332 lines, -199 lines) Patch
M net/net.gyp View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M net/quic/crypto/crypto_handshake.h View 3 chunks +6 lines, -6 lines 0 comments Download
M net/quic/crypto/crypto_handshake.cc View 2 chunks +6 lines, -6 lines 0 comments Download
M net/quic/crypto/proof_test.cc View 1 2 3 4 5 6 3 chunks +91 lines, -76 lines 1 comment Download
M net/quic/crypto/proof_verifier.h View 1 2 3 4 5 6 1 chunk +52 lines, -16 lines 0 comments Download
A + net/quic/crypto/proof_verifier.cc View 1 2 3 1 chunk +5 lines, -5 lines 0 comments Download
M net/quic/crypto/proof_verifier_chromium.h View 1 3 chunks +17 lines, -12 lines 0 comments Download
M net/quic/crypto/proof_verifier_chromium.cc View 1 2 3 4 5 6 7 chunks +40 lines, -25 lines 0 comments Download
M net/quic/quic_crypto_client_stream.h View 1 2 3 4 4 chunks +34 lines, -17 lines 0 comments Download
M net/quic/quic_crypto_client_stream.cc View 1 2 3 4 5 6 9 chunks +67 lines, -26 lines 0 comments Download
M net/tools/quic/test_tools/quic_test_client.cc View 1 2 3 4 5 6 2 chunks +13 lines, -10 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
agl
I've been doing too much OpenSSL coding recently, so may have silly style mistakes. There's ...
7 years, 5 months ago (2013-07-23 15:27:05 UTC) #1
ramant (doing other things)
lgtm
7 years, 5 months ago (2013-07-23 18:03:13 UTC) #2
ramant (doing other things)
On 2013/07/23 18:03:13, ramant wrote: > lgtm Will apply this patch in my windows env ...
7 years, 5 months ago (2013-07-23 18:03:57 UTC) #3
Ryan Hamilton
This is a nice cleanup, agl. Thanks for taking it on! I have a number ...
7 years, 5 months ago (2013-07-23 18:40:50 UTC) #4
agl
https://codereview.chromium.org/20047002/diff/1/net/quic/crypto/proof_verifier.h File net/quic/crypto/proof_verifier.h (right): https://codereview.chromium.org/20047002/diff/1/net/quic/crypto/proof_verifier.h#newcode41 net/quic/crypto/proof_verifier.h:41: // |details| is owned by the callback and the ...
7 years, 5 months ago (2013-07-23 21:04:56 UTC) #5
Ryan Hamilton
LGTM! https://codereview.chromium.org/20047002/diff/1/net/quic/crypto/proof_verifier_chromium.h File net/quic/crypto/proof_verifier_chromium.h (right): https://codereview.chromium.org/20047002/diff/1/net/quic/crypto/proof_verifier_chromium.h#newcode75 net/quic/crypto/proof_verifier_chromium.h:75: std::string error_details_; On 2013/07/23 21:04:57, agl wrote: > ...
7 years, 5 months ago (2013-07-23 21:19:16 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/agl@chromium.org/20047002/14001
7 years, 5 months ago (2013-07-24 15:47:55 UTC) #7
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 5 months ago (2013-07-24 16:03:09 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/agl@chromium.org/20047002/43001
7 years, 5 months ago (2013-07-24 16:08:52 UTC) #9
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 5 months ago (2013-07-24 16:28:14 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/agl@chromium.org/20047002/32004
7 years, 5 months ago (2013-07-24 16:55:09 UTC) #11
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 5 months ago (2013-07-24 17:26:40 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/agl@chromium.org/20047002/43014
7 years, 5 months ago (2013-07-24 17:55:32 UTC) #13
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 5 months ago (2013-07-24 18:38:52 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/agl@chromium.org/20047002/84001
7 years, 5 months ago (2013-07-25 16:43:32 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/agl@chromium.org/20047002/84001
7 years, 5 months ago (2013-07-26 01:50:03 UTC) #16
commit-bot: I haz the power
Change committed as 213862
7 years, 5 months ago (2013-07-26 12:30:01 UTC) #17
ramant (doing other things)
7 years, 4 months ago (2013-07-29 17:04:23 UTC) #18
Message was sent while issue was closed.
https://codereview.chromium.org/20047002/diff/84001/net/quic/crypto/proof_tes...
File net/quic/crypto/proof_test.cc (right):

https://codereview.chromium.org/20047002/diff/84001/net/quic/crypto/proof_tes...
net/quic/crypto/proof_test.cc:293: verifier.get(), hostname, server_config,
certs, signature, true);
nit: I was shy to ask this before. Is it good to test cert_verify_result in
these tests?

IMO, these unit tests cover getting the data from cache (synchronously),
asynchronously.

 ASSERT_FALSE(IsCertStatusError(cert_verify_result.cert_status));

Powered by Google App Engine
This is Rietveld 408576698