Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1892033002:
Cert - protobufs to serialize and deserialize CertVerifierCache. (Closed)
|
Created:
4 years, 8 months ago by ramant (doing other things) Modified:
4 years, 6 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, cbentzel+watch_chromium.org, Ryan Hamilton Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCert - protobufs to serialize and deserialize CertVerifierCache.
Patch Set 1 : #
Total comments: 14
Patch Set 2 : #Patch Set 3 : Unnest CertVerifierCacheIterator class to avoid forward declaring nested structs #Patch Set 4 : delete scoped_ptr and use unique_ptr #
Total comments: 20
Patch Set 5 : #
Total comments: 26
Depends on Patchset: Messages
Total messages: 36 (17 generated)
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892033002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892033002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892033002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892033002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892033002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892033002/40001
Patchset #1 (id:20001) has been deleted
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/1892033002/diff/40001/net/cert/cert_verifier_... File net/cert/cert_verifier_cache_persister.h (right): https://codereview.chromium.org/1892033002/diff/40001/net/cert/cert_verifier_... net/cert/cert_verifier_cache_persister.h:45: // Update |proto_request_param| with data from |verifier_->cache_|'s Doesn't seem like you need these as private methods; they don't access any instance data. Instead, they should be part of the unnamed namespace of the .cc https://codereview.chromium.org/1892033002/diff/40001/net/cert/multi_threaded... File net/cert/multi_threaded_cert_verifier.h (right): https://codereview.chromium.org/1892033002/diff/40001/net/cert/multi_threaded... net/cert/multi_threaded_cert_verifier.h:74: friend class CertVerifierCachePersister; The design I suggested was so we could explicitly avoid this. We do not want the two coupled - that's a circular dependency. https://codereview.chromium.org/1892033002/diff/40001/net/cert/multi_threaded... net/cert/multi_threaded_cert_verifier.h:101: RequestParams(); I'd like to avoid this, because it leaves RequestParams in an undefined state. https://codereview.chromium.org/1892033002/diff/40001/net/cert/multi_threaded... net/cert/multi_threaded_cert_verifier.h:156: class NET_EXPORT CertVerifierCacheIterator { NAMING: This is highly redundant naming; just Iterator is sufficient https://codereview.chromium.org/1892033002/diff/40001/net/cert/proto/cert_ver... File net/cert/proto/cert_verification.proto (right): https://codereview.chromium.org/1892033002/diff/40001/net/cert/proto/cert_ver... net/cert/proto/cert_verification.proto:11: message CertVerificationSHA1HashValue { SHA256 https://codereview.chromium.org/1892033002/diff/40001/net/cert/proto/cert_ver... net/cert/proto/cert_verification.proto:18: repeated CertVerificationSHA1HashValue hash_values = 3; Note: You don't have to store the full SHA-1/SHA-256 hash. The only requirement here is that whatever IDs you assign to cross-referenced entries are stable. That is, you could just as well assign uint32_ts here (4 bytes rather than 20/32-bytes), and monotonically increase the count for each new certificate added. For experimentation, it's fine, but I'm sure you'll end up needing to optimize this away. For example, when building the list to serialize, you could build an internal BST of all the entries (for fast insertion and determining if an entry is already present), and once you've built that for all of the certs in all of the cached entries, number each of the nodes and just serialize the number. Or you could be less efficient at serialization time, but extremely efficient at deserialization and w/r/t memory, by just doing std::vector<std::string> serialized_certs; std::string encoded; CHECK(GetDEREncoded(os_cert_handle, &encoded)); const auto& it = find(serialized_certs.begin(), serialized_certs.end(), encoded); if (it != serialized_certs.end()) { return std::distance(serialized_certs.begin(), it); // Watch out for distance_type here, it's signed; } else { serialized_certs.push_back(encoded); return serialized_certs.size(); // but size is unsigned } There's a lot of ways you can do this. https://codereview.chromium.org/1892033002/diff/40001/net/cert/proto/cert_ver... net/cert/proto/cert_verification.proto:27: // be substantially different. In the event of a verification failure, this I'm not sure what you're trying to say about things being different here. Certainly, don't document the statements about the CertVerifierCache
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:60001) has been deleted
Hi Ryan, This CL is a first cut. Implemented Iterator similar to TransportStatePersister. You had in mind an API design. Would like to implement that if you don't mind suggesting again. I could make all the methods private and make CertVerifierCachePersister a friend class of MultiThreadedCertVerifier (we could forward declare CertVerifierCachePersister to avoid including the .h file. Is it better if either David Ben or some one in security group takes over this CL? thanks raman https://codereview.chromium.org/1892033002/diff/40001/net/cert/cert_verifier_... File net/cert/cert_verifier_cache_persister.h (right): https://codereview.chromium.org/1892033002/diff/40001/net/cert/cert_verifier_... net/cert/cert_verifier_cache_persister.h:45: // Update |proto_request_param| with data from |verifier_->cache_|'s On 2016/04/16 00:36:15, Ryan Sleevi wrote: > Doesn't seem like you need these as private methods; they don't access any > instance data. Instead, they should be part of the unnamed namespace of the .cc Done. https://codereview.chromium.org/1892033002/diff/40001/net/cert/multi_threaded... File net/cert/multi_threaded_cert_verifier.h (right): https://codereview.chromium.org/1892033002/diff/40001/net/cert/multi_threaded... net/cert/multi_threaded_cert_verifier.h:74: friend class CertVerifierCachePersister; On 2016/04/16 00:36:15, Ryan Sleevi wrote: > The design I suggested was so we could explicitly avoid this. We do not want the > two coupled - that's a circular dependency. Done. https://codereview.chromium.org/1892033002/diff/40001/net/cert/multi_threaded... net/cert/multi_threaded_cert_verifier.h:101: RequestParams(); On 2016/04/16 00:36:15, Ryan Sleevi wrote: > I'd like to avoid this, because it leaves RequestParams in an undefined state. Done. https://codereview.chromium.org/1892033002/diff/40001/net/cert/multi_threaded... net/cert/multi_threaded_cert_verifier.h:156: class NET_EXPORT CertVerifierCacheIterator { On 2016/04/16 00:36:15, Ryan Sleevi wrote: > NAMING: This is highly redundant naming; just Iterator is sufficient Done. https://codereview.chromium.org/1892033002/diff/40001/net/cert/proto/cert_ver... File net/cert/proto/cert_verification.proto (right): https://codereview.chromium.org/1892033002/diff/40001/net/cert/proto/cert_ver... net/cert/proto/cert_verification.proto:11: message CertVerificationSHA1HashValue { On 2016/04/16 00:36:15, Ryan Sleevi wrote: > SHA256 Done. https://codereview.chromium.org/1892033002/diff/40001/net/cert/proto/cert_ver... net/cert/proto/cert_verification.proto:18: repeated CertVerificationSHA1HashValue hash_values = 3; On 2016/04/16 00:36:15, Ryan Sleevi wrote: > Note: You don't have to store the full SHA-1/SHA-256 hash. The only requirement > here is that whatever IDs you assign to cross-referenced entries are stable. > > That is, you could just as well assign uint32_ts here (4 bytes rather than > 20/32-bytes), and monotonically increase the count for each new certificate > added. For experimentation, it's fine, but I'm sure you'll end up needing to > optimize this away. > > For example, when building the list to serialize, you could build an internal > BST of all the entries (for fast insertion and determining if an entry is > already present), and once you've built that for all of the certs in all of the > cached entries, number each of the nodes and just serialize the number. Or you > could be less efficient at serialization time, but extremely efficient at > deserialization and w/r/t memory, by just doing > > std::vector<std::string> serialized_certs; > std::string encoded; > CHECK(GetDEREncoded(os_cert_handle, &encoded)); > const auto& it = find(serialized_certs.begin(), serialized_certs.end(), > encoded); > if (it != serialized_certs.end()) { > return std::distance(serialized_certs.begin(), it); // Watch out for > distance_type here, it's signed; > } else { > serialized_certs.push_back(encoded); > return serialized_certs.size(); // but size is unsigned > } > > There's a lot of ways you can do this. Made this changes for certificates. RequestParams has hash_values which is computed data, We serialize/deserilaize that data. https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/multi_thr... https://codereview.chromium.org/1892033002/diff/40001/net/cert/proto/cert_ver... net/cert/proto/cert_verification.proto:27: // be substantially different. In the event of a verification failure, this On 2016/04/16 00:36:15, Ryan Sleevi wrote: > I'm not sure what you're trying to say about things being different here. > Certainly, don't document the statements about the CertVerifierCache Done.
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892033002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892033002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892033002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892033002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... File net/cert/multi_threaded_cert_verifier.h (right): https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.h:74: // |cache_| is updated. This is not a helpful comment, because it provides no details about what these parameters are, how they should be set, or even why this method exists. It goes directly to talking about implementation details, but that shouldn't be done for a public header and public method. // Caches |result| as the result for |hostname|, with the error code // of |error|, which was previously obtained by calling |Verify()| // with |flags| at |start_time|, for the certificate whose ordered // chain was |hash_values|, which was completed at // |verification_time|, and should expire by |expiration_time|. // If it returns true, subsequent calls to |Verify()| will return this // result, if it is before |expiration_time| and matches the // |hostname| and |flags|. (If you read the above, you'll see how it's unclear the difference between |start_time| and |verification_time|. I don't consider the above comment complete, but mostly wanted to explore how to be clearer in the comment). It's also unclear, from this, why you return a bool result, but I'm less picky about that. Just that it's not clear *why* someone should care about that (just for metrics?). That one I'm not picky about documenting, other than https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.h:102: PersistCacheExpiredEntry); This (and line 87) improperly creates the circular dependency, except via tests, because now this code depends on knowing how it's used. You should be testing the interface, not the implementation. https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.h:115: std::vector<SHA1HashValue>& hash_values_arg, Don't pass by non-const reference. I realize you did this to swap, but you need to take it explicitly, and std::move. At your call site, if you're trying to optimize, you should std::move() into that. https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.h:148: struct NET_EXPORT_PRIVATE CacheValidityPeriod { Why? https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.h:156: struct NET_EXPORT_PRIVATE CacheExpirationFunctor { Why? https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.h:213: class NET_EXPORT_PRIVATE CertVerifierCacheIterator { I suggested several times that this be a member. MultiThreadedCertVerifier::Iterator To be more explicit: CertVerifierCacheIterator is extremely confusing with CertVerifierCache::Iterator, which this is not, and has no fundamental relation to. https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... File net/cert/multi_threaded_cert_verifier_unittest.cc (right): https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier_unittest.cc:119: EXPECT_FALSE(iterator.HasNext()); No. Please add a new test. https://codereview.chromium.org/1892033002/diff/120001/net/cert/proto/cert_ve... File net/cert/proto/cert_verification.proto (right): https://codereview.chromium.org/1892033002/diff/120001/net/cert/proto/cert_ve... net/cert/proto/cert_verification.proto:16: // certificate verification request. BUG: Please don't couple your protobuf like this to some other file. Describe what this proto is and does, in isolation to all other files. https://codereview.chromium.org/1892033002/diff/120001/net/cert/proto/cert_ve... net/cert/proto/cert_verification.proto:20: repeated CertVerificationSHA256HashValue hash_values = 3; Surely this isn't correct. The MultiThreadedCertVerifier header says SHA-1 hash values, but your API suggest it's SHA-256. https://codereview.chromium.org/1892033002/diff/120001/net/cert/proto/cert_ve... net/cert/proto/cert_verification.proto:65: optional CertVerificationCacheValidityPeriod cache_validity_period = 3; I don't feel comfortable reviewing protobuf files for proto style, but this mix of required and optional feels fundamentally wrong. I thought protobufs were discouraged from having required fields (period). Certainly, the liberal use of required here defeats the primary purpose of why we're using protobufs to begin with - to provide some possibility of compatibility/versioning.
Patchset #5 (id:140001) has been deleted
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892033002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892033002/160001
Thanks Ryan for the comments. PTAL. https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... File net/cert/multi_threaded_cert_verifier.h (right): https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.h:74: // |cache_| is updated. On 2016/04/29 23:33:21, Ryan Sleevi wrote: > This is not a helpful comment, because it provides no details about what these > parameters are, how they should be set, or even why this method exists. It goes > directly to talking about implementation details, but that shouldn't be done for > a public header and public method. > > // Caches |result| as the result for |hostname|, with the error code > // of |error|, which was previously obtained by calling |Verify()| > // with |flags| at |start_time|, for the certificate whose ordered > // chain was |hash_values|, which was completed at > // |verification_time|, and should expire by |expiration_time|. > // If it returns true, subsequent calls to |Verify()| will return this > // result, if it is before |expiration_time| and matches the > // |hostname| and |flags|. > > > (If you read the above, you'll see how it's unclear the difference > between |start_time| and |verification_time|. I don't consider the > above comment complete, but mostly wanted to explore how to be clearer > in the comment). > > It's also unclear, from this, why you return a bool result, but I'm less picky > about that. Just that it's not clear *why* someone should care about that (just > for metrics?). That one I'm not picky about documenting, other than Many many thanks for the above comments. Done. https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.h:102: PersistCacheExpiredEntry); On 2016/04/29 23:33:21, Ryan Sleevi wrote: > This (and line 87) improperly creates the circular dependency, except via tests, > because now this code depends on knowing how it's used. You should be testing > the interface, not the implementation. Done. https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.h:115: std::vector<SHA1HashValue>& hash_values_arg, On 2016/04/29 23:33:21, Ryan Sleevi wrote: > Don't pass by non-const reference. I realize you did this to swap, but you need > to take it explicitly, and std::move. > > At your call site, if you're trying to optimize, you should std::move() into > that. Done. https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.h:148: struct NET_EXPORT_PRIVATE CacheValidityPeriod { On 2016/04/29 23:33:21, Ryan Sleevi wrote: > Why? Deleted them. Changed the tests to test the interface and not the implementation. https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.h:156: struct NET_EXPORT_PRIVATE CacheExpirationFunctor { On 2016/04/29 23:33:21, Ryan Sleevi wrote: > Why? Deleted them. Changed the tests to test the interface and not the implementation. https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.h:213: class NET_EXPORT_PRIVATE CertVerifierCacheIterator { On 2016/04/29 23:33:21, Ryan Sleevi wrote: > I suggested several times that this be a member. > > MultiThreadedCertVerifier::Iterator > > To be more explicit: CertVerifierCacheIterator is extremely confusing with > CertVerifierCache::Iterator, which this is not, and has no fundamental relation > to. Sincere apologies. Undid this change. Was trying to avoid forward declaration of struct's required by CertVerifierCache typedef. Done. https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... File net/cert/multi_threaded_cert_verifier_unittest.cc (right): https://codereview.chromium.org/1892033002/diff/120001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier_unittest.cc:119: EXPECT_FALSE(iterator.HasNext()); On 2016/04/29 23:33:21, Ryan Sleevi wrote: > No. Please add a new test. Done. https://codereview.chromium.org/1892033002/diff/120001/net/cert/proto/cert_ve... File net/cert/proto/cert_verification.proto (right): https://codereview.chromium.org/1892033002/diff/120001/net/cert/proto/cert_ve... net/cert/proto/cert_verification.proto:16: // certificate verification request. On 2016/04/29 23:33:21, Ryan Sleevi wrote: > BUG: Please don't couple your protobuf like this to some other file. Describe > what this proto is and does, in isolation to all other files. Removed references to the MultiThreadedCertVerifier's data structures from this file. Done. https://codereview.chromium.org/1892033002/diff/120001/net/cert/proto/cert_ve... net/cert/proto/cert_verification.proto:20: repeated CertVerificationSHA256HashValue hash_values = 3; On 2016/04/29 23:33:21, Ryan Sleevi wrote: > Surely this isn't correct. The MultiThreadedCertVerifier header says SHA-1 hash > values, but your API suggest it's SHA-256. My fault. I have misunderstood the earlier comments about SHA256 to mean that I should use SHA256. Changed them back to SHA1. Done. https://codereview.chromium.org/1892033002/diff/120001/net/cert/proto/cert_ve... net/cert/proto/cert_verification.proto:65: optional CertVerificationCacheValidityPeriod cache_validity_period = 3; On 2016/04/29 23:33:21, Ryan Sleevi wrote: > I don't feel comfortable reviewing protobuf files for proto style, but this mix > of required and optional feels fundamentally wrong. I thought protobufs were > discouraged from having required fields (period). Certainly, the liberal use of > required here defeats the primary purpose of why we're using protobufs to begin > with - to provide some possibility of compatibility/versioning. Changed all the fields to optional fields.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi rsleevi, If you have sometime, could you please take another look? thanks raman
https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... File net/cert/cert_verifier_cache_persister.cc (right): https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... net/cert/cert_verifier_cache_persister.cc:234: DVLOG(1) << "Invalid CertVerificationCacheEntry"; All of these DVLOGS seem unnecessarily verbose and useless. I'm very anti-DVLOG, but if there's a use case you're trying to address, perhaps you could explain? DVLOGs are only for developer builds - but also show up extensively in our test bots - so why do these add particular value? Are these signs of programmer error? If so, doesn't DCHECK make more sense? Or are they benign, ignorable errors? If so, why DVLOG at all? https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... File net/cert/cert_verifier_cache_persister_unittest.cc (right): https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... net/cert/cert_verifier_cache_persister_unittest.cc:84: callback.callback(), &request, BoundNetLog()); What are you testing here? That MTCV caches? Its API contract doesn't guarantee it caches failures. https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... net/cert/cert_verifier_cache_persister_unittest.cc:95: EXPECT_FALSE(persister.LoadCache(std::string())); Separate test https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... net/cert/cert_verifier_cache_persister_unittest.cc:98: EXPECT_FALSE(persister.LoadCache(std::string("junk"))); Separate test https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... net/cert/cert_verifier_cache_persister_unittest.cc:101: EXPECT_FALSE(persister.LoadCache(data)); Separate test https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... net/cert/cert_verifier_cache_persister_unittest.cc:103: // Create a new Verifier and restoring the data into it, should succeed. Separate test https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... File net/cert/multi_threaded_cert_verifier.cc (right): https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.cc:559: if (hostname.empty() || hash_values.size() == 0 || .empty() https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.cc:564: DVLOG(1) << "Invalid data for: " << hostname; Why DVLOG? Is it a programmer error? If so, why not DCHECK? If it's an acceptable condition, why DVLOG? https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.cc:570: DVLOG(1) << "Cache is full"; Necessary? https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier.cc:578: DVLOG(1) << "Already exists in the cache for " << key.hostname; Necessary? https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... File net/cert/multi_threaded_cert_verifier_unittest.cc (right): https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier_unittest.cc:76: for (size_t i = 0; i < size; ++i) { alternative: return std::equal(a.begin(), a.end(), b.begin(), [](const HashValue& lhs, const HashValue& rhs) { return lhs.Equals(rhs)}); Although if we got rid of the .Equals() overloads and made it overload==, which is now legal per https://google.github.io/styleguide/cppguide.html#Operator_Overloading , this could just be EXPECT_EQ(a, b) in the code below. https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier_unittest.cc:643: // Invalid hostname. Separate https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier_unittest.cc:649: // Invalid hash_values. Separate https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier_unittest.cc:655: // Invalid start_time. Separate https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier_unittest.cc:661: // Corrupted start_time. Separate https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier_unittest.cc:668: // Corrupted verification_time. Separate https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier_unittest.cc:675: // Corrupted expired entry. Separate https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier_unittest.cc:682: // Expired entry. Separate https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier_unittest.cc:688: // Adding a duplicate entry. Separate https://codereview.chromium.org/1892033002/diff/160001/net/cert/multi_threade... net/cert/multi_threaded_cert_verifier_unittest.cc:695: // Save the data before clearing the cache. Separate
Made the relevant changes to protobuf code in https://codereview.chromium.org/2021433004/ rsleevi made the other changes in CLs: 1994353002, 2000503002, 1991653002 and 1999733002. We thought it is better to upload as a new CL. Closing this issue. https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... File net/cert/cert_verifier_cache_persister.cc (right): https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... net/cert/cert_verifier_cache_persister.cc:234: DVLOG(1) << "Invalid CertVerificationCacheEntry"; On 2016/05/13 20:18:12, Ryan Sleevi wrote: > All of these DVLOGS seem unnecessarily verbose and useless. > > I'm very anti-DVLOG, but if there's a use case you're trying to address, perhaps > you could explain? DVLOGs are only for developer builds - but also show up > extensively in our test bots - so why do these add particular value? Are these > signs of programmer error? If so, doesn't DCHECK make more sense? Or are they > benign, ignorable errors? If so, why DVLOG at all? Deleted the DVLOGs. They are all ignorable errors. Done. https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... File net/cert/cert_verifier_cache_persister_unittest.cc (right): https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... net/cert/cert_verifier_cache_persister_unittest.cc:84: callback.callback(), &request, BoundNetLog()); On 2016/05/13 20:18:12, Ryan Sleevi wrote: > What are you testing here? That MTCV caches? > > Its API contract doesn't guarantee it caches failures. +1. It is unnecessary. Done. https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... net/cert/cert_verifier_cache_persister_unittest.cc:95: EXPECT_FALSE(persister.LoadCache(std::string())); On 2016/05/13 20:18:12, Ryan Sleevi wrote: > Separate test Done. https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... net/cert/cert_verifier_cache_persister_unittest.cc:98: EXPECT_FALSE(persister.LoadCache(std::string("junk"))); On 2016/05/13 20:18:12, Ryan Sleevi wrote: > Separate test Done. https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... net/cert/cert_verifier_cache_persister_unittest.cc:101: EXPECT_FALSE(persister.LoadCache(data)); On 2016/05/13 20:18:12, Ryan Sleevi wrote: > Separate test Done. https://codereview.chromium.org/1892033002/diff/160001/net/cert/cert_verifier... net/cert/cert_verifier_cache_persister_unittest.cc:103: // Create a new Verifier and restoring the data into it, should succeed. On 2016/05/13 20:18:12, Ryan Sleevi wrote: > Separate test Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
