Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
329733002:
Disk Based Certificate Cache Implementation (Closed)
|
Created:
6 years, 6 months ago by brandonsalmon Modified:
6 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, erikwright+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRelanding new and improved object for the caching of certificate objects on disk.
Fixed memory leaks in the unittests.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280400
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280656
Patch Set 1 #Patch Set 2 : Improved and fixed first test. #
Total comments: 18
Patch Set 3 : Improved flow and added set overwrite functionality. #
Total comments: 12
Patch Set 4 : Added functionality for multiple simultaneous operations. (not properly tested). #Patch Set 5 : Improved unittests, error checking, and improved functionality in workers (although this isn't prop… #
Total comments: 35
Patch Set 6 : Fixed issues with last upload, and changed cleanup_callback_ to be previously bound with the correc… #
Total comments: 27
Patch Set 7 : Workers now delete themselves, and DBCC deletion cancels workers. #
Total comments: 20
Patch Set 8 : Fixed style issues with patch 7. v2 #
Total comments: 32
Patch Set 9 : Fixed isses stated in review of patch 8. #
Total comments: 18
Patch Set 10 : Updated unittests with respect to the review of patch 9. #
Total comments: 20
Patch Set 11 : Minor fixes and implemented size checking on cache read/write. #
Total comments: 12
Patch Set 12 : Updated and improved unittests. #
Total comments: 7
Patch Set 13 : Made changes to unittests, added new test, fixed overwrite bug. #
Total comments: 5
Patch Set 14 : changed unsigned int to size_t, key to cache_key, and added comments. #
Total comments: 3
Patch Set 15 : Fixed issues with patch 14 v2 #
Total comments: 1
Patch Set 16 : Fixed issues with patch 14 and 15 #
Total comments: 9
Patch Set 17 : Added null check for free. #Patch Set 18 : Fixed memory leak by remembering to close entry and free OSCertHandle. #
Total comments: 2
Messages
Total messages: 51 (0 generated)
So, a few comments here.
For next steps, I would focus on supporting multiple requests - and possibly
getting and setting the same key.
This is that PendingOp discussion, where the PendingOp keeps track of the
disk_cache::Entry to be used, the user read/write callbacks, etc. The callbacks
you pass to the disk_cache::Backend will be some helper function, and you would
use base::Passed
for example
void Get(...) {
scoped_ptr<PendingOp> pending_op(new PendingOp);
pending_op->entry = new disk_cache::Entry;
pending_op->read_cb = ...;
pending_op->stuff = other_stuff;
int rv = backend_->OpenEntry(...,
base::Bind(&DiskBasedCertCache::OnEntryOpened, base::Passed(pending_op)));
}
Or something like that. However, I'm pretty sure that above sample code has a
bug, and while I could try to be clever and fix it, I think a better case would
be to look at how the existing consumers of the disk_cache::Backend track
in-flight requests, and adopt the similar mechanics.
https://codereview.chromium.org/329733002/diff/20001/base/base.gyp
File base/base.gyp (right):
https://codereview.chromium.org/329733002/diff/20001/base/base.gyp#newcode24
base/base.gyp:24: {
This looks like an unrelated change from your C++ Codelab.
"git reset --hard HEAD -- base.gyp" (I think), and then adding and committing.
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
File net/cert_cache/disk_based_cert_cache.cc (right):
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
net/cert_cache/disk_based_cert_cache.cc:50: }
A couple common coding conventions:
Handle errors first, so you can short-circuit early
if (rv == ERR_IO_PENDING)
return;
if (rv < 0) {
// TODO(brandonsalmon): implement other error codes.
NOTIMPLEMENTED();
}
DoStartRead();
The use of NOTIMPLEMENTED is handy, because it will make you crash when you hit
it :)
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
net/cert_cache/disk_based_cert_cache.cc:55: buffer = (new
IOBuffer(active_entry_size_));
No need for the extra () around new.
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
net/cert_cache/disk_based_cert_cache.cc:71: if (!user_read_callback_.is_null())
{
if (user_read_callback_.is_null())
return;
active_cert_handle_ ...
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
net/cert_cache/disk_based_cert_cache.cc:75: CHECK(active_cert_handle_ != NULL);
CHECK(active_cert_handle_);
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
net/cert_cache/disk_based_cert_cache.cc:79: callback.Run(active_cert_handle_);
base::ResetAndReturn(&user_read_callback_).Run(active_cert_handle_);
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
net/cert_cache/disk_based_cert_cache.cc:80: reset_state();
It looks like your state machine breaks if user_read_callback_ is null - that
is, they can read one cert, but never read any others?
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
net/cert_cache/disk_based_cert_cache.cc:87: CHECK(active_entry_ == NULL);
CHECK(active_entry_)
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
net/cert_cache/disk_based_cert_cache.cc:96: DoStartWrite();
what about errors?
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
net/cert_cache/disk_based_cert_cache.cc:101: CHECK(active_cert_handle_ != NULL);
CHECK(active_cert_handle_);
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
net/cert_cache/disk_based_cert_cache.cc:137:
CHECK(X509Certificate::GetDEREncoded(active_cert_handle_, &write_data));
DANGER: You never want to put function calls in CHECK/DCHECK, as they may be
optimized out depending on build flags.
Instead
bool encoded = X509Certificate::GetDEREncoded(...);
CHECK(encoded);
However, using CHECK here would be inappropriate - it returns bool because it
CAN fail. So you shouldn't CHECK, but should handle/return.
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
net/cert_cache/disk_based_cert_cache.cc:154: if
(!user_write_callback_.is_null()) {
if (user_write_callback_.is_null())
return;
...
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
net/cert_cache/disk_based_cert_cache.cc:161: void
DiskBasedCertCache::reset_state() {
Naming: This would be ResetState(), not reset_state(), because it's not a
'trivial, inlined function'.
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
File net/cert_cache/disk_based_cert_cache.h (right):
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
net/cert_cache/disk_based_cert_cache.h:28:
base::Callback<void(X509Certificate::OSCertHandle cert_handle)> cb);
Crap, I just realized a problem with this Callback API.
If the Callback doesn't run (eg: because it's bound to a WeakPtr), then we'd end
up leaking a reference to the |cert_handle|.
This doesn't change the API we need to expose, just requires a little trickery
with how we call the callback.
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
net/cert_cache/disk_based_cert_cache.h:55:
base::WeakPtrFactory<DiskBasedCertCache> weak_factory_;
WeakPtrFactory should always be the last member of the class. See
https://code.google.com/p/chromium/issues/detail?id=303818
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
File net/cert_cache/disk_based_cert_cache_unittest.cc (right):
https://codereview.chromium.org/329733002/diff/20001/net/cert_cache/disk_base...
net/cert_cache/disk_based_cert_cache_unittest.cc:19: // Net::CompletionCallback
with non-int parameters. Using this class as a sort
nit: Net:: -> net::
https://codereview.chromium.org/329733002/diff/20001/net/net.gyp
File net/net.gyp (right):
https://codereview.chromium.org/329733002/diff/20001/net/net.gyp#newcode48
net/net.gyp:48: 'target_name': 'disk_based_cert_cache',
You don't need to create a new target for this. You add it to the existing 'net'
target.
https://codereview.chromium.org/329733002/diff/20001/net/net.gypi
File net/net.gypi (right):
https://codereview.chromium.org/329733002/diff/20001/net/net.gypi#newcode356
net/net.gypi:356: 'cert_cache/disk_based_cert_cache.cc',
Don't create a new directory. This probably belongs in net/http, since it's
conceptually tied to how we cache HTTP responses.
Review comments on patch set 3: I only took a quick look, so the comments are all on coding style issues. https://codereview.chromium.org/329733002/diff/40001/net/http/disk_based_cert... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/329733002/diff/40001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:7: #include <string> Nit: since the .h file includes <string>, the .cc file doesn't need to include it again. https://codereview.chromium.org/329733002/diff/40001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:18: #include "net/disk_cache/disk_cache.h" The headers that are included by the .h file don't need to be included again. https://codereview.chromium.org/329733002/diff/40001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:51: // todo(brandonsalmon) implement error handling Our convention is all caps TODO. https://codereview.chromium.org/329733002/diff/40001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:105: return "cert:" + base::HexEncode(fingerprint.data, 20); Avoid the use of 20. Try arraysize(fingerprint.data). https://codereview.chromium.org/329733002/diff/40001/net/http/disk_based_cert... File net/http/disk_based_cert_cache.h (right): https://codereview.chromium.org/329733002/diff/40001/net/http/disk_based_cert... net/http/disk_based_cert_cache.h:6: #define NET_CERT_CACHE_DISK_BASED_CERT_CACHE_H The include guard macro name needs to be updated. https://codereview.chromium.org/329733002/diff/40001/net/http/disk_based_cert... net/http/disk_based_cert_cache.h:30: // Stores |handle| in the cache. If |handle| is successfully stored, |cb| |handle| => |cert_handle| https://codereview.chromium.org/329733002/diff/40001/net/http/disk_based_cert... net/http/disk_based_cert_cache.h:38: // Will have to be updated to accomodate multiple simultaneous operations. Add "TODO(brandonsalmon): " before this line. https://codereview.chromium.org/329733002/diff/40001/net/http/disk_based_cert... net/http/disk_based_cert_cache.h:80: } // namespace This comment needs to say "namespace net". https://codereview.chromium.org/329733002/diff/40001/net/http/disk_based_cert... File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/329733002/diff/40001/net/http/disk_based_cert... net/http/disk_based_cert_cache_unittest.cc:16: namespace { You can put this file in the net namespace, so that you can omit the net:: prefix. https://codereview.chromium.org/329733002/diff/40001/net/http/disk_based_cert... net/http/disk_based_cert_cache_unittest.cc:36: // todo: take in a key. This is slightly cumbersome ( ? since the string must Use all caps TODO, and include your user name. https://codereview.chromium.org/329733002/diff/40001/net/net.gypi File net/net.gypi (right): https://codereview.chromium.org/329733002/diff/40001/net/net.gypi#newcode565 net/net.gypi:565: 'http/disk_based_cert_cache.cc', Nit: list .cc before .h. See the other files as examples. I know conceptually we think of .h (the interface) before .cc (the implementation), but the files are listed in alphabetical order. https://codereview.chromium.org/329733002/diff/40001/saved.txt File saved.txt (right): https://codereview.chromium.org/329733002/diff/40001/saved.txt#newcode3 saved.txt:3: This is still a work in progress and there is a lot more that needs to be done. Remove this file from the git branch.
I improved error checking and got rid of some of the more ugly code in disk_based_cert_cache.cc. I also added a few more unittests, however I still haven't been able to add unittests that correctly incorporate asynchronous actions.
I need to take a more thorough look; I've tried to make a quick pass on style before getting into substance. This is because style issues can sometimes subtly mask substance, hence they tend to be more eye catchy https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:17: //----------------------------------------------------------------------------- vertical whitespace comments apply https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:19: class DiskBasedCertCache::WriteWorker { Document https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:25: SetCallback user_callback); const-refs https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:29: void Start(); Document https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:31: void AddCallback(SetCallback user_callback); const-ref document https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:121: if (state_ == WRITE_NONE) { STYLE: We prefer error-handling/short circuiting as soon as possible if (state_ != WRITE_NONE) return; if (entry) entry->Close(); base::ResetAndReturn(...) https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:124: base::ResetAndReturn(&cleanup_callback_).Run(key_); DANGER: We almost always try to avoid calling callbacks as part of DoLoop, since callbacks may end in the object being deleted, and DoLoop is never an "entry" function (eg: the result of a callback) In this case, your two entry functions are Start and OnIOComplete. Those are the places where you'd want to chain to your next callback https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:129: DCHECK(entry_ == NULL); For == NULL dchecks, just do DCHECK(entry_) https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:135: } No braces on simple { } https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:142: //!create_failed is checked to make sure we only try to open once. When possible, it's better to avoid pronouns (like "we") in comments. The reasoning is listed on https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... You're also missing a space in //!create_failed (it should be "// create_failed) https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:182: CallCallbacks(""); use std::string() for empty strings, rather than "" Believe it or not, it avoids an extra memory copy :) https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:198: base::ResetAndReturn(&(*it)).Run(key_); 1) When would it->is_null be true? Seems like they'd be violating your API contract 2) Can any of these callbacks delete the cache? If so, it seems like deleting the cache would delete the worker, which would delete this, which would cause things to blow up? 3) What if someone, as a result of CallCallbacks, attempted to start a new Write operation? AFAICT, you'd invoke AddCallback(), which would mutate |user_callbacks_|, thus invalidating |it| and causing things to crash! https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:203: weak_factory_.InvalidateWeakPtrs(); You don't need to do this explicitly. WeakPtrFactory's destructor invalidates its issued weak pointers. https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:219: void AddCallback(GetCallback user_callback); Same comments regarding const-refs, comments, etc https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:222: // Types -------------------------------------------------------------------- Same comments regarding vertical whitespace (eg: don't include these block markers) https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:298: base::ResetAndReturn(&cleanup_callback_).Run(key_); Same comments re: entry functions calling callbacks https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:319: buffer = new IOBuffer(entry_size_); Lots of unnecessary vertical whitespace here (and 304, 320) https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:360: : backend_(backend), weak_factory_(this) { Run git-cl format, and I believe it will restructure this as : backend_(backend), weak_factory_(this) { } The one-liner is only supposed to be valid if the entire ctor fits on one line, I believe https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:365: weak_factory_.GetWeakPtr()); You can create these two callbacks in the ctor initializer list, which is more idiomatic in chrome code https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:374: } You can use base/stl_util.h STLDeleteContainerPairSecondPointers(write_worker_map_.begin(), write_worker_map_.end()); https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:385: CHECK(!key.empty()); Use DCHECK() when defining preconditions/post-conditions for API usage. CHECK appears in final Chrome binaries (and thus contributes to size) and should only be used for security-sensitive aspects; otherwise, prefer DCHECK. https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:392: read_worker_map_[key]->Start(); Avoid duplicate accesses to [key], when possible. Here, you'd use std::map::insert() with key to get an std::pair<maptype::iterator, bool>. You'd then pair->first->second->Start(); https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:394: read_worker_map_[key]->AddCallback(cb); Here, you'd it->second->AddCallback(cb); https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:401: CHECK(cert_handle); ditto comments re: DCHECK https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.h (right): https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:16: #include "net/disk_cache/disk_cache.h" As per http://www.chromium.org/developers/coding-style/cpp-dos-and-donts , you can forward declare disk_cache::Backend , and avoid including this header namespace net { namespace disk_cache { class Backend; } // namespace disk_cache class ... { }; } // namespace net https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:23: typedef base::Callback<void(X509Certificate::OSCertHandle cert_handle)> STYLE: In this case, you likely want to ensure your Get/Set pairs are consistently ordered. Your types use Set/Get, your methods use Get/Set, your members use Write/Read, and helper methods use Write/Read https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:33: // Otherwise, |cb| will be called with NULL. Will |cb| ever be called synchronously? Good to document either way. Also, document ownership of the cert_handle (namely, that it's not transferred) https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:34: void Get(std::string& key, GetCallback cb); STYLE: Reference arguments should always be passed as const - "std::string& key" -> "const std::string& key" http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu... STYLE: Callbacks should always be passed as const-references (we generally never pass values by-value, with the exception of scoped_ptr<> https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:38: // std::string, then |handle| was not stored. empty std::string -> empty string https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:39: void Set(const X509Certificate::OSCertHandle cert_handle, SetCallback cb); And with the exception of above, you need to pass the OSCertHandle by value ;) https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:41: base::WeakPtr<DiskBasedCertCache> GetWeakPtr() { DESIGN: Exposing a public member to allow access to a WeakPtr is generally a "code smell". No one should ever ask you for your WeakPtr (nor create one of you) - you should give it to them (via a callback) https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:46: // Types -------------------------------------------------------------------- STYLE: Don't include these block-style comments ( see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Vertic... ) https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:51: typedef base::hash_map<std::string, ReadWorker*> ReadWorkerMap; You use base::hash_map, but you never include the headers that define it. You need to do #include "base/containers/hash_tables.h" https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:54: std::string Key(const X509Certificate::OSCertHandle cert_handle) const; STYLE: Function names are generally of the form of VerbNoun if they're mutating, and always field_name if they're accessors In this case, a better name might be GetCacheKeyForCert() However, this private method doesn't depend on any of the other data members within this class - it's better suited as simply a helper function within an unnamed namespace in the .cc, never even appearing on the class / in the .h https://codereview.chromium.org/329733002/diff/150001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:56: void FinishedReadOperation(const std::string& key); Document (Briefly) what these methods do.
Brandon: Since Ryan reviewed patch set 5 carefully and gave you many comments, I will let you address his comments first, and then review your new patch set.
I fixed the issues with the last upload. Also, I have a couple more things that I didn't include in this upload since I didn't know if they would be appropriate or not.
Ok, I've taken another pass. In this case, I focused only on the specific WriteWorker. Because the ReadWorker is so similar (and I know you're exploring bringing them together), I didn't review that in depth. I also left the body of the Cache alone, since most of my comments regarding the Worker will affect that design. I'm happy to go over these in person if it's easier to explain some of these concerns regarding threading / re-entrancy :) https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:7: #include <list> Curious: Was there a particular reason for choosing <list>? We traditionally use <vector> for ordered sequences like this. <list> is only used if we particularly need qualities that only a list affords (eg: efficient re-ordering) https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:22: // time, instead, add a user_callback_ to the existing WriteWorker. comment nit: // time, instead, add a user_callback_ to the existing WriteWorker. becomes // time; instead, use AddCallback to register a callback to be notified when // the write has completed https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:31: // operation failed. comment style: we generally don't note types in prose, but instead note purpose/use; // |backend| is the backend to store |certificate| in, using // |key| as the key for the disk_cache::Entry. // |cleanup_callback| is called to clean up this WriteWorker, // regardless of success or failure. https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:36: const SetCallback& user_callback); It seems like, for consistency, that the caller should call AddCallback() to add |user_callback|, rather than passing it in the constructor. https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:41: // completion). Comment rewording: // Writes the given certificate to the cache. On completion, will invoke // all user callbacks set. https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:46: // of this worker. Comment rewording: We try to avoid descriptions of how you "should" use it (or how it "is" used), and instead focus on what it does. // Adds |user_callback| to the set of callbacks to be notified when this // WriteWorker has completed processing. https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:66: // given by the user to the DiskBasedCertCache when calling Set). // Invokes all of the |user_callbacks_| https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:67: void CallCallbacks(const std::string& key); Why do you pass |key|, when you have |key_| as a member variable and this is a member method? https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:78: base::Callback<void(void)> cleanup_callback_; the name for this is "base::Closure" - avoids the template specialization. https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:80: CompletionCallback io_callback_; /* used to access the backend */ Delete this comment; Among other things, io_callback_ itself is not used to access the backend. https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:85: // ReadWorkers represent pending get jobs in the DiskBasedCertCache. Each s/get/Get/ https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:89: // the the existing ReadWorker. Similar comment cleanup as above. https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:98: ReadWorker(disk_cache::Backend* backend, Same comment comments as above https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:101: const GetCallback& user_callback); ditto AddCallback https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:147: } This function should be between lines 18/19 in an unnamed namespace https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:171: if (state_ == WRITE_NONE) { if (state_ != WRITE_NONE) return; if (entry_) entry->close; base::ResetAndReturn(&cleanup_callback_).Run(); https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:190: } Ditto cleanup as above. https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:227: if (rv == ERR_FAILED && !create_failed_) { Why combine CREATE_OR_OPEN into one state? It seems like it should be two CREATE -> CREATE_COMPLETE OPEN -> OPEN_COMPLETE if CREATE_COMPLETE succeeded, it goes to START_WRITE if CREATE_COMPLETE failed, it goes to OPEN if OPEN_COMPLETE succeeded, it goes to START_WRITE if OPEN_COMPLETE failed, it goes to WRITE_NONE and fails. https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:232: CallCallbacks(std::string()); DANGER! So the same comments I made on the previous version about the cleanup_callback_ apply here to using CallCallbacks. That is, if you CallCallback, the Set failed, and that leads to the DBCC being deleted, then it's going to delete this worker (while CallCallbacks) is running. You'll then crash within CallCallbacks if you're lucky, or, if not, you'll crash here on line 233 (attempting to write to a member of a deleted object) Assuming you got past 233, when you return ERR_FAILED, you'll end up on line 200, which will break the loop, and you'll end up in either 172/173 or 187/188 - at which point, you'll attempt to access entry_ and crash there too. This is why, in the MultiThreadedCertVerifier example I gave you, the CertVerifierWorker notifies the MTCV to HandleResult, and the MTCV then cleans up its internal state, and *then* delegates to the CertVerifierJob to HandleResult. I think you'll find that any scenario where the DiskBasedCertCache is responsible for deleting a worker is going to result in threading bugs that can crash your application, because the Worker owns the Entry. The Entry *needs* to stay valid until all pending callbacks from the disk_cache::Backend() have been invoked. Using WeakPtr for calls to the Backend doesn't actually do that https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:249: } Let's chat in person how to structure these 'failure' callbacks. It actually works out that both your failure callbacks and your success callbacks can be hermetically handled in Start() / OnIOComplete(), safely, without having to duplicate this logic for errors throughout the WriteWorker. https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:281: base::ResetAndReturn(&(*it)).Run(key_); Same danger regarding the risk of the following: Cache->Set(cert, callback1); worker = New Worker(cert, callback1); .. async stuff happening (Another request) Cache->Set(cert, callback2); worker->AddCallback(callback2); .. async stuff finishes CallCallbacks() Callback1.run(...) Cache->Set(cert, callback3); worker->AddCallback(callback3); // DANGER! I suspect, having read to this point now, you chose <list> to try to allow this; That's a pattern, but one we actually tend to discourage. This is another place where the MultiThreadedCertVerifier setup works in your favour. In this case, when callback1.run(...) causes Cache->Set(...) to be called, you would have already removed the Worker from the Cache's map, so it would use, instead, "new Worker(cert, callback3)" Much easier design to reason about, even if it gets into trickier stuff. https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.h (right): https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:10: #include "base/bind.h" You don't need Bind here; you can move it to the .cc file https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:14: #include "net/base/completion_callback.h" You don't use net::CompletionCallback in the .h. If you do need it, move it to the .cc https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:38: // |cert_handle| is not transferred. I suspect there may have been confusion about my remark about "Document ownership of |cert_handle| is not transferred." That was a directive for you to document eg: // Ownership of |cert_handle| is not transferred, and it will be // invalidated at the end of the callback. Callers that wish to // store a copy should use X509Certificate::DupOSCertHandle https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:66: }; You will want to use DISALLOW_COPY_AND_ASSIGN(DiskBasedCertCache) here, see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C...
I tried to solve the issues with the DiskBasedCertCache being deleted in a callback, as well as with a callback starting new set and get operations. Now, workers delete themselves and DiskBasedCertCache deletion just signals the Workers to cancel (stop working when callbacks are received from the backend. https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:7: #include <list> On 2014/06/18 22:16:03, Ryan Sleevi wrote: > Curious: Was there a particular reason for choosing <list>? > > We traditionally use <vector> for ordered sequences like this. <list> is only > used if we particularly need qualities that only a list affords (eg: efficient > re-ordering) In this circumstance, I could not see any particular reason for using a vector. I switched to using a list because list::push_back guarantees iterator validity (and push_back could conceivably be called by one of the user_callbacks_). https://codereview.chromium.org/329733002/diff/210001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:67: void CallCallbacks(const std::string& key); On 2014/06/18 22:16:03, Ryan Sleevi wrote: > Why do you pass |key|, when you have |key_| as a member variable and this is a > member method? I did this so I could use CallCallbacks(std::string()), however it might be better to just use a boolean flag for errors.
Review comments on patch set 7: I reviewed the .h file carefully. I only reviewed the .cc file partially. I didn't review the _unittest.cc file. High-level comments: I find the logic quite complicated. Perhaps the complexity is inherent and it's hard to avoid it. It's also possible that the code seems complicated to me because it works in a different style from the similar classes in the network stack. However, superficially the code looks very nice and it's clear to me you've put in a lot of thought. I'd like to review the code again carefully tomorrow. Let me send you my comments first. In the .cc file, all of my comments on WriteWorker also apply to ReadWorker. The two classes are very similar, so for brevity I don't repeat my comments. https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:19: std::string GetCacheKeyToCert( Nit: document this function. https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:22: net::X509Certificate::CalculateFingerprint(cert_handle); Nit: you can nest the unnamed namespace inside the net namespace, so that you can omit net::. https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:27: } // namespace Nit: I usually add a blank line after namespace { and a blank line before the closing } // namespace https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:64: WRITE_NONE I suggest adding STATE_ prefix to these states. https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:93: }; Please move the WriteWorker method definitions here, unless Ryan suggested that you declare the two *Worker classes first. https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:123: enum ReadState { OPEN, START_READ, FINISH_READ, READ_NONE }; Nit: I suggest adding STATE_ prefix to these states. Nit: list one state per line. That's our convention. https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:234: if (rv == ERR_FAILED && !create_failed_) { We should test |rv < 0| instead of |rv == ERR_FAILED|, unless |rv| may be ERR_IO_PENDING here. https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:238: } else if (rv < 0) { Nit: In general, omit "else" if the previous block ends with a return statement. https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:240: return ERR_FAILED; IMPORTANT: in general we should report an accurate error code. Here a negative |rv| is the error code, so we should just return |rv|. If you only use ERR_FAILED in this class, essentially only indicating a failure status, then that's fine. https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:283: it++) { Use ++it instead of it++ to increment an iterator, if we don't need the return value of the increment operator. The pre-increment operator returns a reference whereas the post-increment operator returns a copy, so the pre-increment operator is cheaper. (This is a common test for a C++ programmer's proficiency.) https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:284: base::ResetAndReturn(&(*it)).Run((rv >= 0) ? key_ : std::string()); The use of base::ResetAndReturn is wrong. The purpose of base::ResetAndReturn is to set a callback_ member to null and then run the callback. Here the relevant member is user_callbacks_. So we need to clear the entire user_callbacks_ vector. We'll need to do that manually. I believe we usually use swap() to do this: std::vector<SetCallback> callbacks; callbacks.swap(user_callbacks_); std::string key; if (rv >= 0) key = key_; for (std::vector<SetCallback>::iterator it = callbacks.begin(); it != callbacks.end(); ++it) { it->Run(key); } Note how I set |key| outside the for loop. https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:290: entry_->Close(); This kind of cleanup is conventionally done in the destructor. https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:293: delete this; IMPORTANT: the Finish() method must be written very carefully. I think it should first remove the WriteWorker from the write_worker_map_. So we should run cleanup_callback_ first. This way this WriterWorker is no longer reachable from the DiskBasedCertCache. Then it is safe to run the user_callbacks_, which is arbitrary code. The user callbacks cannot possibly reach this WriterWorker now. Finally, delete this. With this analysis, I believe it is NOT necessary to use base::ResetAndReturn. https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:317: base::Bind(&ReadWorker::OnIOComplete, weak_factory_.GetWeakPtr()); Nit: you should be able to initialize io_callback_ in the initializer list if you declare the io_callback_ member after the weak_factory_ member. https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:474: std::pair<WriteWorkerMap::iterator, bool> entry = write_worker_map_.insert( Here, you can just use the associative-array notation: WriteWorker* worker = new WriteWorker(...); write_worker_map_[key] = worker; worker->AddCallback(cb); worker->Start(); https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.h (right): https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:22: class NET_EXPORT_PRIVATE DiskBasedCertCache { Please add a comment to summarize what this class is. https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:36: // a copy should use X509Certificate::DupOSCertHandle. "store a copy" is not clear. I suggest something that "store a reference to the certificate". Add "inside |cb|" after "should use X509Certificate::DupOSCertHandle". https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:50: typedef base::hash_map<std::string, WriteWorker*> WriteWorkerMap; Nit: document what the std::string is. (I guess it's the key for the cache.)
Fixed most of the style issues Wan-Teh mentioned in patch 7. About the complexity: Yes, I agree. The code has gotten a little complicated. Please let me know if you think I missed out on an easier way to structure things, I've thought about it and just haven't thought of a simpler approach. https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:284: base::ResetAndReturn(&(*it)).Run((rv >= 0) ? key_ : std::string()); On 2014/06/20 01:27:06, wtc wrote: > > The use of base::ResetAndReturn is wrong. > > The purpose of base::ResetAndReturn is to set a callback_ member to null and > then run the callback. Here the relevant member is user_callbacks_. So we need > to clear the entire user_callbacks_ vector. We'll need to do that manually. I > believe we usually use swap() to do this: > > std::vector<SetCallback> callbacks; > callbacks.swap(user_callbacks_); > std::string key; > if (rv >= 0) > key = key_; > for (std::vector<SetCallback>::iterator it = callbacks.begin(); > it != callbacks.end(); ++it) { > it->Run(key); > } > > Note how I set |key| outside the for loop. Actually, is it even necessary to do this if it is guaranteed that the callbacks cannot delete the worker? https://codereview.chromium.org/329733002/diff/250001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:317: base::Bind(&ReadWorker::OnIOComplete, weak_factory_.GetWeakPtr()); On 2014/06/20 01:27:06, wtc wrote: > > Nit: you should be able to initialize io_callback_ in the initializer list if > you declare the io_callback_ member after the weak_factory_ member. I'm going to leave this for now. I was talking with Ryan and he said that weak factories should always be last in initialization lists. (although I'm not sure of the reason.)
Review comments on patch set 8: This patch set is much better. I reviewed the .cc file carefully. I only skimmed through the _unittest.cc file and will review it again carefully next Monday. Like last time, for brevity I didn't repeat the comments for ReadWorker. So please assume my comments on WriteWorker also apply to the counterpart code in ReadWorker. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:37: // time; instead, add a user_callback_ to the existing WriteWorker. Typo: user_callback_ => user callback https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:46: const X509Certificate::OSCertHandle cert_handle, Nit: this 'const' isn't necessary. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:55: // Adds a callback to the set of callbacks to be notified when this Nit: to be notified => to run https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:59: // Signals the WriteWorker to finish early. The WriteWorker will be destroyed Nit: finish early => abort early ? https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:68: STATE_START_WRITE, Nit: this state would be named STATE_WRITE in our convention. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:69: STATE_FINISH_WRITE, Our naming convention for the STATE_FINISH_XXX state is STATE_XXX_COMPLETE. Please rename the corresponding DoFinishXXX() functions to DoXXXComplete(). https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:70: STATE_WRITE_NONE Nit: This state can be named simply STATE_NONE. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:81: // invokes all of the |user_callbacks_| Nit: capitalize "Invokes". https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:92: scoped_refptr<IOBuffer> buffer; This member should be named buffer_, with a trailing '_'. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:111: state_(STATE_CREATE_OR_OPEN), Nit: see if you can initialize state_ to STATE_WRITE_NONE in the constructor, and set state_ to STATE_CREATE_OR_OPEN in Start(), before the DoLoop(OK) call. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:122: if (state_ != STATE_WRITE_NONE) In our convention, I think we would check the |rv| instead: if (rv == ERR_IO_PENDING) return; Make the same change to the check on line 141 inside WriteWorker::OnIOComplete. Note: I didn't think about this thoroughly, so my suggested change may be wrong. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:184: create_failed_ = true; Instead of using the create_failed_ boolean, you can refine STATE_CREATE_OR_OPEN into STATE_CREATE and STATE_OPEN, and similarly for STATE_FINISH_CREATE_OR_OPEN. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:225: return OK; The function can be simply state_ = STATE_WRITE_NONE; return rv; https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:232: it->Run((rv >= 0) ? key_ : std::string()); Move (rv >= 0) ? key_ : std::string() outside the loop. std::string key; if (rv >= 0) key = key_; for (...) { it->Run(key); it->Reset(); } user_callbacks_.clear(); Note the user_callbacks_.clear() call I added. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:392: } Nit: we usually use a STATE_FINISH_OPEN state to handle the result of the backend_->OpenEntry() call. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:453: it->second->Cancel(); 1. Add curly braces around the bodies of these two for loops. Even though the body is a one-liner, the "for" part spans three lines, so we still use curly braces in this case. 2. IMPORTANT: after this Cancel() call, we need to make sure the Workers won't try to remove themselves from the worker maps, because the worker maps will have been deleted. Does the use of base::WeakPtr and base::Closure automatically take care of this? https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.h (right): https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:21: // DiskBasedCertClass is used to store and retrieve X509Certificates from the 1. Add a blank line after the "namespace net {" line. 2. Typo: DiskBasedCertClass => DiskBasedCertCache https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:16: #include "testing/gtest/include/gtest/gtest.h" Add a blank line after the last #include statement. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:21: // MockTransactions are required to use the blocking Mock Disk Cache Backend. Nit: Mock Disk Cache Backend => MockDiskCache Backend ? https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:23: // This transaction corresponds to "root_ca_cert.pem" in GetTestCertsDirectory() Nit: add a period at the end of the sentence. Comments should use proper punctuations. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:28: TEST_MODE_NORMAL, NULL, 0}; Nit: our Style Guide does not require aligning the struct members like this. I suggest running "git cl format" and let it format the code for you. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:38: // achieved Nit: "achieved" seems wrong. Did you mean "received" or "obtained"? https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:39: // using CompletionCallback::WaitForResult. Nit: this fits on the previous line. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:47: void Set(const X509Certificate::OSCertHandle cert_handle, Nit: this 'const' isn't necessary. (X509Certificate::OSCertHandle is a pointer.) https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:48: std::string& key, IMPORTANT: our Style Guide requires using pointers instead of references for output parameters. So this shoudl be std::string* key, and the cert_handle parameter of the Get method should be X509Certificate::OSCertHandle* cert_handle, https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:49: CompletionCallback cb) { Nit: our Style Guide recommends against using uncommon abbreviations for variable/parameter names. So where possible, use "callback" instead of "cb". https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:56: void Get(std::string key, This should be const std::string& key, Our Style Guide recommends input parameters should be passed as const references. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:58: CompletionCallback cb) { This probably should also be a const reference. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:88: // ---------------------------------------------------------------------------- Move this horizontal separator line after the "} // namespace" line.
I finished patch 8 based on Wan-Teh's review of patch 7. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:453: it->second->Cancel(); On 2014/06/21 00:43:12, wtc wrote: > > 1. Add curly braces around the bodies of these two for loops. > > Even though the body is a one-liner, the "for" part spans three lines, so we > still use curly braces in this case. > > 2. IMPORTANT: after this Cancel() call, we need to make sure the Workers won't > try to remove themselves from the worker maps, because the worker maps will have > been deleted. > > Does the use of base::WeakPtr and base::Closure automatically take care of this? I believe so, I did check for this earlier. From callback.h line 146: "The callback will not be issued if the object is destroyed at the time" https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:28: TEST_MODE_NORMAL, NULL, 0}; On 2014/06/21 00:43:13, wtc wrote: > > Nit: our Style Guide does not require aligning the struct members like this. I > suggest running "git cl format" and let it format the code for you. In this case I also think this is ugly, however actually git cl format actually does do it this way.
Review comments on patch set 9: I only reviewed the _unittest.cc file. https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:16: #include "testing/gtest/include/gtest/gtest.h" Nit: add a blank line after the header #include statements. https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:37: 0 I suggest adding an initializer for the final field "net::Error return_code". Use "OK" if that field is never used as a byte count. https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:57: // recieved using CompletionCallback::WaitForResult. Nit: recieved => received https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:69: base::Bind(&MockCertCache::FinishSet, Nit: our naming convention for this kind of function seems to be "DidSet" or "OnSetComplete". https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:89: base::ResetAndReturn(&callback).Run(OK); It's not necessary to use ResetAndReturn. You can just do callback.Run(OK); https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:94: const X509Certificate::OSCertHandle handle_retrieved) { Be consistent in using "xxx_received" or "xxx_retrieved" for the last parameter of your FinishXXX functions. https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:119: EXPECT_TRUE(cert.get()); Use ASSERT_TRUE instead of EXPECT_TRUE here, because if |cert| is null, the subsequent code that dereferences |cert| will crash. https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:126: set_callback.WaitForResult(); You should check that this returns OK. https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:128: ASSERT_TRUE(!key.empty()); You should check with EXPECT_EQ that |key| has the right value. Or do you consider that as an implementation detail? https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:143: get_callback.WaitForResult(); You should check that this returns OK. https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:145: ASSERT_EQ(NULL, cert_handle); Nit: you can use EXPECT_EQ here. https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:160: std::string key; Use |key1| and |key2|, and pass &key1 and key2 to the two user.Set() calls. I think the expected result is EXPECT_EQ(key1, key2)? https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:211: EXPECT_TRUE(!X509Certificate::IsSameOSCert(cert1->os_cert_handle(), If there is EXPECT_FALSE, use it. https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:232: ASSERT_TRUE( Use EXPECT_TRUE. https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:236: // Tests result of if a certificate is simultaneously asked to be stored and Nit: the first few words of this comment don't read well... https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:238: // from the cache. Nit: move to the previous line. https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:255: set_callback.WaitForResult(); What is the expected result? https://codereview.chromium.org/329733002/diff/350001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:274: ASSERT_EQ(key, std::string()); 1. We should verify that the expected result of |key| does not depend on the timing. 2. Use EXPECT_EQ. 3. The two arguments of EXPECT_EQ are 'expected' and 'actual', so this should be EXPECT_EQ(std::string(), key);
I made some changes to the unittests in correspondence with Wan-Teh's review of patch set 9. Also edited one comment in the .cc.
I think we're notably closer, as a lot of the design complexity has been reduced. I've mostly focused on style, although tried to highlight a few patterns. I've also made some suggestions, but I suspect they may be better suited for in-person talks to go-over and explain, as well as provide some other examples of net/ code doing it. Let me know if you find any of them confusing, and we can set some time up to discuss in depth. https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:120: base::Bind(&WriteWorker::OnIOComplete, weak_factory_.GetWeakPtr()); In the new design, why would io_callback_ ever need to be bound using a WeakPtr? The WriteWorker should never go out of scope while there is an asynchronous operation still pending on a given Entry. That's because the Entry (line 115) has to stay valid for the duration of the asynchronous op. Did I miss something when reviewing? https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:124: state_ = STATE_CREATE; You should add a DCHECK_EQ(STATE_NONE, state_); This is a form of noting that your precondition is Start() hasn't already been called for this worker (otherwise, you'd end up in a bad state) https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:176: } while (rv != ERR_IO_PENDING && state_ != STATE_NONE); In order to prevent endless loops, we typically initialize such loops as do { State next_state = state_; state_ = STATE_NONE; switch (next_state) { } } while (rv != ERR_IO_PENDING && state_ != STATE_NONE); This ensures that every state transition (in the Do* series) must explicitly set the next state before returning. If any of them fail to do so, the loop will terminate. This will also avoid you needing to duplicate logic on lines 202, 214, 232, for example https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:189: state_ = STATE_OPEN; It would be good to add a comment about why you transition to STATE_OPEN here https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:192: state_ = STATE_WRITE; We usually add a line break between 191 and 192 https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:233: return rv; I don't see any documentation that WriteData() will only invoke |io_callback_| if-and-only-if it writes all of |buf| It seems like you might have situations where it only writes a portion of the data, and you (the caller) are expected to increment offset, the buf pointer, and decrement len. You should reach out to a net/disk_cache OWNER (aka Ricardo and Gavin), ask whether or not this is possible, and if so, will need to update DoWriteComplete so that it sees if it's written all of buffer_ or not. If you're not sure what this would look like, let me know and we can dig in together :) https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:236: void DiskBasedCertCache::WriteWorker::CallCallbacks(int rv) { naming OCD: Since Run() is the method name on Callbacks, this might be clearer as RunCallbacks() [or RunUserCallbacks] https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:245: it->Reset(); You don't need to manually be calling Reset() on these. If you remove that, you can make it a const_iterator, since Run() is a const method on a Callback (but Reset is not) https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:252: cleanup_callback_.Reset(); So it's ok to use base::ResetAndReturn(&cleanup_callback_).Run() here, as a short-hand. I'm not sure if Wan-Teh suggested this wasn't necessary, but it's ok to do, even when you're not running the risk of cleanup_callback_ deleting the object. With the changed design, it might make more sense to rename this to finished_callback or something. https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.h (right): https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:21: // DiskBasedCertCache is used to store and retrieve X509Certificates from the style: newline between 20 & 21 https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:21: // DiskBasedCertCache is used to store and retrieve X509Certificates from the comment nit: You don't actually take X509Certificates, you take cert handles. Perhaps you meant X.509 certificates? https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:22: // cache. Each individual certificate is stored separately from its Certificate s/Certificate/certificate https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:24: // created with a SHA1 hash. comment nit: this last comment feels like it's explaining an implementation detail. It probably fine to omit. https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:32: // used to store the certificates in the cache. Usual comment style is to avoid using the word "constructor" // Initializes a new DiskBasedCertCache that will use |backend|, which // has been previously initialized, to store certificates. https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:61: // hash maps. This last sentence is an implementation detail; we try to omit those from headers. https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:27: "cert:4C005EF1CF45F80D4A5A2BCFB00D4F198121E8D4", Since you end up repeating these cert: strings, you should have them has string constants const char kCertTransaction1Key[] = "cert:..."; const char kCertTransaction2Key[] = "cert:..."; const MockTransaction kCertTransaction1 = { kCertTransaction1Key, .... }; And then update the Get/Set calls to use these constants https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:69: const CompletionCallback& callback) { indentation is wrong for all of these methods https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:250: // does not crash. We should make this test deterministic, and handle cases where both Get succeeds first and where Set succeeds first. This involves a bit more complexity in how the MockCertCache works.
Patch set 10 LGTM. Please wait for Ryan's approval. https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/329733002/diff/310001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:28: TEST_MODE_NORMAL, NULL, 0}; On 2014/06/21 02:21:32, brandonsalmon wrote: > > In this case I also think this is ugly, however actually git cl format actually > does do it this way. If git cl format formatted the code this way, then it's fine. https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:432: DCHECK(cert_handle_); Replace this DCHECK with error handling: if (!cert_handle_) return ERR_FAILED; We should not assume a system call will succeed. https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.h (right): https://codereview.chromium.org/329733002/diff/370001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:20: namespace net { Nit: add a blank line after this line.
https://codereview.chromium.org/329733002/diff/460001/net/http/disk_based_cer... File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/329733002/diff/460001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:96: AddMockTransaction(&kCertTransaction1); Notes from discussion/review: You want to use ScopedMockTransaction, as otherwise this leaves global state around. https://codereview.chromium.org/329733002/diff/460001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:109: EXPECT_FALSE(key.empty()); Notes from discussion/review: You should also verify: 1) That the key was the expected key (kCertTransaction1Key) 2) That the contents for key in backend_ (user.backend_) actually match the expected certificate contents https://codereview.chromium.org/329733002/diff/460001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:124: EXPECT_EQ(NULL, cert_handle); Notes from discussion/review: BUG: cert_handle has been invalidated (the cache's callback ran when your get_callback completed), and so it's not safe to operate on. https://codereview.chromium.org/329733002/diff/460001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:144: user.Set(cert.get()->os_cert_handle(), &key1, set_callback1.callback()); Notes from discussion/review: No need to use "cert.get()->" - just use "cert->" for all places you find the pattern. https://codereview.chromium.org/329733002/diff/460001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:146: user.Set(cert.get()->os_cert_handle(), &key2, set_callback2.callback()); Notes from discussion/review: This isn't matching the description on 127-129 The first value (rv1) may have completed synchronously before you invoke the second set (rv2), so you may have caused two writes, rather than 1 You want to ensure that your test harness ensures that user.Set(key1) won't have completed before you user.Set(key2) https://codereview.chromium.org/329733002/diff/460001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:178: cert.get()->os_cert_handle())); Notes from discussion/review: BUG: As noted, |retrieved_cert_handle| is invalidated by the time you make this check. The reason it doesn't crash is that it happens to be returning the same os_cert_handle as |cert|, and thus because |cert| is kept alive (161), it doesn't crash. However, this isn't part of the API contract of the Dup/Free cycle, so you need to actually be sure to dup/free using your own helper (as discussed) https://codereview.chromium.org/329733002/diff/460001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:180: TEST: You want to have a test where a Get() from a "populated" cache succeeds. The best way to test this is to setup your MockCertCache, then manually use OpenEntry/WriteData/CloseEntry (as part of your test), and then ensure that Get() retrieves that value. https://codereview.chromium.org/329733002/diff/460001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:180: TEST: You want to have a test where a Get() from a "corrupted" cache fails. This is similar to the success case, except you'd just mess up the data, so that X509Certificate::CreateOSCertHandleFromData will fail. https://codereview.chromium.org/329733002/diff/460001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:229: // does not crash. Notes from discussion: You'll want your test harness to deterministically handle whether the Set or Get operation completes first. I believe you may be able to use MockDiskEntry::IgnoreCallbacks(true) to cause the disk cache to suspend all the callbacks, call Set then Get, and then run MockDiskEntry::IgnoreCallbacks(false), which will order the callbacks such that Set happens before Get. Better yet, however, is MockDiskEntry::IgnoreCallbacks(true); base::ScopedClosureRunner runner(base::Bind(&MockDiskEntry::IgnoreCallbacks, false)); You can also set your MockTransaction to use TEST_MODE_SYNC_CACHE_START / _READ / _WRITE to order determinism (eg: if you do SYNC_CACHE_START, the Set() will immediately succeed with OpenEntry()/CreateEntry(), without even waiting for the Callback. https://codereview.chromium.org/329733002/diff/460001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:235: std::string key("cert:4C005EF1CF45F80D4A5A2BCFB00D4F198121E8D4"); Reuse transaction key? https://codereview.chromium.org/329733002/diff/460001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:249: TEST: Use the MockTransactions to alter the sync/async behaviour to ensure things work out well. For example, does your test cache handle synchronous completion for START / READ / WRITE?
I updated and improved the unittests taking into consideration what me and Ryan talked about yesterday. I didn't include the PendingOp/TestBackend classes because I determined that MockDiskEntry::IgnoreCallbacks provided similar functionality.
I think we're almost there. Just a few more comments, which are more style than substance, thankfully :) https://codereview.chromium.org/329733002/diff/460001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/329733002/diff/460001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:180: Did git cl format do these? Normally we'd include whitespace in all the places you removed it. https://codereview.chromium.org/329733002/diff/520001/net/http/disk_based_cer... File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/329733002/diff/520001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:23: // Testing the DiskBasedCertCache requireds constant use of the s/requires/ https://codereview.chromium.org/329733002/diff/520001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:28: const char* name; s/name/file_name/ - since that's what you're naming. https://codereview.chromium.org/329733002/diff/520001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:29: const char* key; s/key/cache_key/ https://codereview.chromium.org/329733002/diff/520001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:30: const int size; Is this supposed to be the size of the file? For sizes of byte ranges/files, we use size_t, not int (the IOBuffer is sort of weird in that respect) Note: We shouldn't bake this (or the contents of the certs) into code. We should obtain it dynamically. https://codereview.chromium.org/329733002/diff/520001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:131: CHECK(encoded); In unittests, you don't want to CHECK, as that can bring down the whole test harness. You'd want to do ASSERT_TRUE(encoded); https://codereview.chromium.org/329733002/diff/520001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:202: ImportCert(&backend, kCert1, false /* not corrupted */); If ImportCert had a failure, this test will continue executing. In order to have the error propagate from subroutines (both this and "CheckCertCached"), you need to wrap them in a GTest macro ASSERT_NO_FATAL_FAILURE(ImportCert(...)); ASSERT_NO_FATAL_FAILURE(CheckCertCached(...)); https://codereview.chromium.org/329733002/diff/520001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:294: cache.Set(cert->os_cert_handle(), set_callback2.callback()); Can you add a note that describes how this is working That is, something like the following (assuming my understanding is correct) // These two operations will be combined into one operation // because the cache is operating asynchronously (e.g. the // first Set() won't proceed until the message loop is pumped, // such as by WaitForResult())
Made some minor changes in response to review of patch 11.
https://codereview.chromium.org/329733002/diff/560001/net/http/disk_based_cer... File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/329733002/diff/560001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:29: const char* key; s/key/cache_key/ https://codereview.chromium.org/329733002/diff/560001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:132: for (unsigned int i = 0; i < write_data.size(); i += 20) s/unsigned int/size_t/ Always use the appropriately sized type, and match the type as it's defined (this will fail to compile on at least one platform) https://codereview.chromium.org/329733002/diff/560001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:132: for (unsigned int i = 0; i < write_data.size(); i += 20) nit: This loop is "dangerous", and were it not test code, would be worth fixing. because |i| is unsigned, it wraps. If .size() were > std::numeric_limits<unsigned int>::max() - 20, this loop would run on forever. These can alternatively be written out as: for (size_t i = 0; i < write_data.size(); ++i) { if (!(i % 20)) ++write_data[i]; } That said, it's pedantic here, because we know it will be <= int (line 137/143), it's just more a "More you know" https://codereview.chromium.org/329733002/diff/560001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:133: write_data[i]++; nit: ++write_data[i] (slightly more performant) https://codereview.chromium.org/329733002/diff/560001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:370: MockDiskEntry::IgnoreCallbacks(true); Document why you're doing these ignore callbacks.
LGTM Note: Before you're able to actually use this in Chromium code, you'll also have to update BUILD.gn. However, let's keep that for a separate, small CL that only updates BUILD.gn
The CQ bit was checked by brandonsalmon@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/brandonsalmon@chromium.org/329733002/5...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
Comments on patch set 14: I looked at the build and test failures. I suggest some solutions below. https://codereview.chromium.org/329733002/diff/570001/net/http/disk_based_cer... File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/329733002/diff/570001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:32: const TestCertMetaData kCert1{"root_ca_cert.pem", This is missing the '=' sign: const TestCertMetaData kCert1 = { "root_ca_cert.pem", "cert:4C005EF1CF45F80D4A5A2BCFB00D4F198121E8D4"}; Line 35 has the same problem. https://codereview.chromium.org/329733002/diff/570001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:47: return {key, "", base::Time(), "", LOAD_NORMAL, "", "", You may need to declare a local MockTransaction variable and return that. Note the '=' sign: MockTransaction transaction = { ... }; return transaction; https://codereview.chromium.org/329733002/diff/570001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:245: EXPECT_EQ(cert->os_cert_handle(), get_callback.cert_handle()); I think you should test EXPECT_TRUE(X509Certificate::IsSameOSCert(get_callback.cert_handle(), cert->os_cert_handle())); as you do elsewhere.
Wan-Teh, I fixed what those issues. Should I recheck the commit button?
Patch set 15 LGTM. Yes, please recheck the Commit box. https://codereview.chromium.org/329733002/diff/600001/net/http/disk_based_cer... File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/329733002/diff/600001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:247: cache.Get(kCert1.cache_key, get_callback.callback()); Hmm... I think we should still call WaitForResult() in the Sync* tests. You can do this in a follow-up CL.
The CQ bit was checked by brandonsalmon@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/brandonsalmon@chromium.org/329733002/6...
The CQ bit was unchecked by brandonsalmon@chromium.org
The CQ bit was checked by brandonsalmon@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/brandonsalmon@chromium.org/329733002/4...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by brandonsalmon@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/brandonsalmon@chromium.org/329733002/6...
a few nits to be considered in the future. https://codereview.chromium.org/329733002/diff/450005/net/http/disk_based_cer... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/329733002/diff/450005/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:49: ~WriteWorker(); nit: shouldn't this be private? https://codereview.chromium.org/329733002/diff/450005/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:210: state_ = STATE_NONE; nit: This should not be needed outside of DoLoop https://codereview.chromium.org/329733002/diff/450005/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:272: DiskBasedCertCache::WriteWorker::~WriteWorker() { nit: follow the same order of methods between declaration and definition https://codereview.chromium.org/329733002/diff/450005/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:434: 0 /* index */, 0 /* offset */, buffer_, io_buf_len_, io_callback_); nit: this is a list of arguments where none of them is long enough to prevent indentation from the function call so they should probably start on the previous line and continue under the first one when reaching 80 cols: ->Foo(bar, stuff, variable, something_else_that_doesnt_fit); https://codereview.chromium.org/329733002/diff/450005/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:507: read_worker_map_[key] = worker; nit: this pattern (map_.find() + map_[]=) searches the map twice. It can be replaced with map_.insert()
https://codereview.chromium.org/329733002/diff/450005/net/http/disk_based_cer... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/329733002/diff/450005/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:210: state_ = STATE_NONE; On 2014/06/27 19:00:09, rvargas wrote: > nit: This should not be needed outside of DoLoop Can you clarify? https://codereview.chromium.org/329733002/diff/450005/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:507: read_worker_map_[key] = worker; On 2014/06/27 19:00:09, rvargas wrote: > nit: this pattern (map_.find() + map_[]=) searches the map twice. It can be > replaced with map_.insert() Does this still apply even when the key isn't found, and .find() returns the end of the map?
https://codereview.chromium.org/329733002/diff/450005/net/http/disk_based_cer... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/329733002/diff/450005/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:210: state_ = STATE_NONE; On 2014/06/27 19:20:15, brandonsalmon wrote: > On 2014/06/27 19:00:09, rvargas wrote: > > nit: This should not be needed outside of DoLoop > > Can you clarify? DoLoop sets state_ to STATE_NONE and then calls the method that implements a given state... so that method doesn't have to set state_ again (to the same) value to indicate that the loop should break. https://codereview.chromium.org/329733002/diff/450005/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:507: read_worker_map_[key] = worker; On 2014/06/27 19:20:14, brandonsalmon wrote: > On 2014/06/27 19:00:09, rvargas wrote: > > nit: this pattern (map_.find() + map_[]=) searches the map twice. It can be > > replaced with map_.insert() > > Does this still apply even when the key isn't found, and .find() returns the end > of the map? I'm not sure I understand the question. If find() returns an entry there is no overhead. However, when find() returns end(), it has to search the map in order to do so, and then the insertion here has to go through the map again to find out where the new entry should go. insert() can do both things with a single lookup. It returns either where the new entry was inserted, or the existing entry. Creating a new worker can be postponed until insert() returns and we know that there was no match.
Message was sent while issue was closed.
Change committed as 280400
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/353183004/ by brandonsalmon@chromium.org. The reason for reverting is: Memory Leak..
Message was sent while issue was closed.
Fixed memory leak.
Message was sent while issue was closed.
Patch set 18 LGTM. To reopen the issue, click the "Edit Issue" link, uncheck the "Closed" box, and click "Update Issue". Watch out for the "Delete Issue" button. Don't click that by mistake. https://codereview.chromium.org/329733002/diff/670001/net/http/disk_based_cer... File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/329733002/diff/670001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:173: entry->Close(); Nit: as defensive programming, it is a good idea to set a pointer to NULL after freeing it. This avoids using a "dangling pointer". You can omit this when you are clearly freeing the pointer at or near the end of a function.
The CQ bit was checked by brandonsalmon@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/brandonsalmon@chromium.org/329733002/6...
https://codereview.chromium.org/329733002/diff/670001/net/http/disk_based_cer... File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/329733002/diff/670001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:173: entry->Close(); On 2014/06/28 14:22:30, wtc wrote: > > Nit: as defensive programming, it is a good idea to set a pointer to NULL after > freeing it. This avoids using a "dangling pointer". You can omit this when you > are clearly freeing the pointer at or near the end of a function. On the other hand, if it crashes due to reuse (something that is likely caused by another path, not the one deleting the object) the crash dump will be harder to debug when things are reset to zero :( (that is to say that I don't particularly follow that practice). If you mean clear the pointer so that other code can know if the pointer is still valid, then absolutely! (and it implies checking for NULL somewhere else... but a better pattern for that case is to use some form of scoped ptr... ScopedEntryPtr in this case)
Message was sent while issue was closed.
Change committed as 280656 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
