Cache certificate verification results in memory.

Cache certificate verification results in memory.

R=agl
BUG=63357
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69414

[wtc, 2010-12-11 02:42:12]

Please review this CL in the following order.

1. cert_verifier.{h,cc}
   cert_verifier_unittest.cc

The code is primarily based on agl's dnsrr_resolver.{h,cc},
with the API based on eroman's host_resolver.h.

2. ssl_host_info.{h,cc}
   ssl_client_socket_nss.{h,cc}

Please check how I replaced the old CertVerifier class
member by a SingleRequestCertVerifier.

3. io_thread.cc
   connection_tester.cc
   service_url_request_context.cc
   metrics_service.cc

These files create new net::CertVerifier objects.  I
guess this means chrome will have multiple certificate
verification caches.  If so, this needs more work.

4. The rest of the files.

Mostly adding new cert_verifier function arguments.

[willchan no longer on Chromium, 2010-12-13 09:30:53]

Drive by comments :)

http://codereview.chromium.org/5386001/diff/52001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/5386001/diff/52001/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:48: scoped_ptr<net::CertVerifier> cert_verifier;
I think you should put |cert_verifier| below |dnsrr_resolver| because, at least
in my mind, it's conceivable that |cert_verifier| may eventually depend on
|dnsrr_resolver|.

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc
File net/base/cert_verifier.cc (right):

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
net/base/cert_verifier.cc:88: class CertVerifierRequest {
Should this be within the anonymous namespace?  I actually wonder if it's really
better to use the typedef void* RequestHandle.  We could just make this a nested
class within CertVerifier and rename it to CertVerifier::Request.  In
CertVerifier, we could forward declare class Request in CertVerifier and typedef
Request* RequestHandle.  Any attempts to dereference the handle would lead to a
compile error.  The advantage of this approach is avoiding a reinterpret_cast<>,
which always scares me a little bit.

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
net/base/cert_verifier.cc:126: CertVerifier* cert_verifier)
I think you should just pass in a callback.  Then you don't need to friend
CertVerifierWorker.  And it's not like CertVerifierWorker should be calling
anything else on CertVerifier, it really should just be HandleResult().  If you
don't want to have too many parameters, you could change HandleResult to take a
const CertVerifierWorker&, and add const accessors for all the relevant data.

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
net/base/cert_verifier.cc:137: DCHECK_EQ(MessageLoop::current(), origin_loop_);
You may want to consider using ThreadChecker instead.  It uses PlatformThreadId
rather than MessageLoop::current(), which is generally speaking more accurate.

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
net/base/cert_verifier.cc:188: // We assume that the origin loop outlives the
CertVerifier. If the
See my note below for avoiding this complexity by using MessageLoopProxy.

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
net/base/cert_verifier.cc:214: MessageLoop* const origin_loop_;
Note that if an object is leaked (not canceled properly), for example, an
SSLClientSocket, then origin_loop_ may refer to a deleted object.  This may
cause crashes like http://code.google.com/p/chromium/issues/detail?id=42275.  I
solved it there by using a MessageLoopProxy instead.  That will make the leak
not cause crashes, although it's still a leak and therefore a bug we should fix.

Note that using the MessageLoopProxy will also eliminate the need for the
complication wrt the above code in Finish().  The code is very tricky, because
|lock_| actually is locking more than just |canceled|.  It's also trying to make
sure that |origin_loop_| is valid.  If you used the MessageLoopProxy, then you
wouldn't have to worry about that.  Then you could do the natural thing of just
having |lock_| synchronize access to |canceled_| so you can store it in the
local variable |canceled|.

Note that you will have to check the value of MessageLoopProxy::PostTask() to
see if it got posted successfully.  If not, then you need to delete |this|.

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
net/base/cert_verifier.cc:217: Lock lock_;
You should document what |lock_| locks.  In particular, it seems to primarily be
|canceled_|.

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
net/base/cert_verifier.cc:290: if (!callback || !verify_result ||
hostname.empty()) {
You should probably add a NOTREACHED().  These represent coding bugs, therefore
we should crash in debug mode.

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
net/base/cert_verifier.cc:326: inflight_.insert(std::make_pair(key, job));
Should this be moved after the conditional?  So we don't insert unless the
worker started successfully?

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
net/base/cert_verifier.cc:332: return ERR_FAILED;  // TODO(wtc): Log an error
message.
Should this be ERR_UNEXPECTED?

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h
File net/base/cert_verifier.h (right):

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h#ne...
net/base/cert_verifier.h:27: class CachedCertVerifyResult {
I think this should be a struct.  Otherwise, please make the member variables
private and use accessors.

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h#ne...
net/base/cert_verifier.h:50: class CertVerifier : public NonThreadSafe {
Have you thought about monitoring the NetworkChangeNotifier?  We should probably
be aborting the requests on network changes, right?  And do we need to flush the
cache too?

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h#ne...
net/base/cert_verifier.h:99: int Verify(X509Certificate* cert,
I don't know enough about this area, but it seems to be that if we're verifying
a cert, then we shouldn't be modifying the cert, so |cert| should be a const
X509Certificate&.  Just a thought for you to consider.  I don't know it well
enough to say if this is possible.  Just sounds like verification of a cert
should be a const operation for the cert.

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h#ne...
net/base/cert_verifier.h:104: RequestHandle* out_req);
I think you should be passing in a BoundNetLog for logging purposes now.  And
you should add a bunch of new logging events so you can see in
about:net-internals when CertVerifier joins requests or has cache hits or what
not.  Because if caching is broken in some way and someone reports a bug, you'll
want to be able to tell from about:net-internals that it's a cache issue.

Btw, now that you're caching, do we need to do something special wrt load flags?
 Eric can speak better to this.  We should be consistent with the HostResolver. 
I coulda sworn that it did something to force a lookup when a load flag was set,
but I don't see anything like that now.  I just want to comment about that here
so the right people are considering this issue.

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h#ne...
net/base/cert_verifier.h:130: sizeof(cert_fingerprint.data)) == 0 &&
arraysize

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h#ne...
net/base/cert_verifier.h:141: sizeof(cert_fingerprint.data));
arraysize

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h#ne...
net/base/cert_verifier.h:184: // certificate verification is cancelled, and the
completion callback will
Cancelled is the British spelling.  American English uses canceled.  Down with
the Brits! :P

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier_unit...
File net/base/cert_verifier_unittest.cc (right):

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier_unit...
net/base/cert_verifier_unittest.cc:16: 
Can this code go into an anonymous namespace?

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier_unit...
net/base/cert_verifier_unittest.cc:28: class CertVerifierTest : public
testing::Test {
If we don't need the fixture, can we just use TEST?

http://codereview.chromium.org/5386001/diff/52001/net/http/disk_cache_based_s...
File net/http/disk_cache_based_ssl_host_info.cc (right):

http://codereview.chromium.org/5386001/diff/52001/net/http/disk_cache_based_s...
net/http/disk_cache_based_ssl_host_info.cc:21:
http_cache->network_layer()->GetSession()->cert_verifier()),
Can we just pass in the CertVerifier explicitly?  It feels "cleaner" to me.

http://codereview.chromium.org/5386001/diff/52001/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/5386001/diff/52001/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool_unittest.cc:198: scoped_ptr<CertVerifier>
cert_verifier_;
Can this be of type CertVerifier rather than scoped_ptr<CertVerifier>?

http://codereview.chromium.org/5386001/diff/52001/net/socket/client_socket_po...
File net/socket/client_socket_pool_manager.h (right):

http://codereview.chromium.org/5386001/diff/52001/net/socket/client_socket_po...
net/socket/client_socket_pool_manager.h:10: #define
NET_SOCKET_CLIENT_SOCKET_POOL_MANAGER_H_
Thanks for all the cleanup in this file!  I feel a bit stupid for those silly
mistakes.

http://codereview.chromium.org/5386001/diff/52001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/5386001/diff/52001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_unittest.cc:47: scoped_ptr<net::CertVerifier>
cert_verifier_;
Should this just be a CertVerifier rather than scoped_ptr<CertVerifier>?

http://codereview.chromium.org/5386001/diff/52001/net/tools/fetch/fetch_clien...
File net/tools/fetch/fetch_client.cc (right):

http://codereview.chromium.org/5386001/diff/52001/net/tools/fetch/fetch_clien...
net/tools/fetch/fetch_client.cc:143: scoped_ptr<net::CertVerifier>
cert_verifier(new net::CertVerifier);
Why not just do CertVerifier cert_verifier?  Do you need the scoped_ptr?

[willchan no longer on Chromium, 2010-12-13 09:38:26]

Oops, I forgot to mention the main reason I looked at this.  I was curious
about your thoughts on the worker pool job/worker paradigm.  I was looking
to create a common utility class for this.  Please take a look at
http://codereview.chromium.org/5710002/ where I create a base::WorkerPoolJob
and use it for DirectoryLister and make HostResolverImpl use it.  Maybe
you'd like to review it too?  The one area where it majorly differs I think
is that it uses refcounting like HostResolverImpl does.  I need to think
further about it, but I might actually agree that
DISABLE_RUNNABLE_METHOD_REFCOUNT is the way to go.  I think we should
consolidate implementations because this is traditionally an area that is
buggy, especially in shutdown.

On Mon, Dec 13, 2010 at 1:30 AM, <willchan@chromium.org> wrote:

> Drive by comments :)
>
>
>
> http://codereview.chromium.org/5386001/diff/52001/chrome/browser/io_thread.h
> File chrome/browser/io_thread.h (right):
>
>
>
http://codereview.chromium.org/5386001/diff/52001/chrome/browser/io_thread.h#...
> chrome/browser/io_thread.h:48: scoped_ptr<net::CertVerifier>
> cert_verifier;
> I think you should put |cert_verifier| below |dnsrr_resolver| because,
> at least in my mind, it's conceivable that |cert_verifier| may
> eventually depend on |dnsrr_resolver|.
>
> http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc
> File net/base/cert_verifier.cc (right):
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
> net/base/cert_verifier.cc:88: class CertVerifierRequest {
> Should this be within the anonymous namespace?  I actually wonder if
> it's really better to use the typedef void* RequestHandle.  We could
> just make this a nested class within CertVerifier and rename it to
> CertVerifier::Request.  In CertVerifier, we could forward declare class
> Request in CertVerifier and typedef Request* RequestHandle.  Any
> attempts to dereference the handle would lead to a compile error.  The
> advantage of this approach is avoiding a reinterpret_cast<>, which
> always scares me a little bit.
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
> net/base/cert_verifier.cc:126: CertVerifier* cert_verifier)
> I think you should just pass in a callback.  Then you don't need to
> friend CertVerifierWorker.  And it's not like CertVerifierWorker should
> be calling anything else on CertVerifier, it really should just be
> HandleResult().  If you don't want to have too many parameters, you
> could change HandleResult to take a const CertVerifierWorker&, and add
> const accessors for all the relevant data.
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
> net/base/cert_verifier.cc:137: DCHECK_EQ(MessageLoop::current(),
> origin_loop_);
> You may want to consider using ThreadChecker instead.  It uses
> PlatformThreadId rather than MessageLoop::current(), which is generally
> speaking more accurate.
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
> net/base/cert_verifier.cc:188: // We assume that the origin loop
> outlives the CertVerifier. If the
> See my note below for avoiding this complexity by using
> MessageLoopProxy.
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
> net/base/cert_verifier.cc:214: MessageLoop* const origin_loop_;
> Note that if an object is leaked (not canceled properly), for example,
> an SSLClientSocket, then origin_loop_ may refer to a deleted object.
> This may cause crashes like
> http://code.google.com/p/chromium/issues/detail?id=42275.  I solved it
> there by using a MessageLoopProxy instead.  That will make the leak not
> cause crashes, although it's still a leak and therefore a bug we should
> fix.
>
> Note that using the MessageLoopProxy will also eliminate the need for
> the complication wrt the above code in Finish().  The code is very
> tricky, because |lock_| actually is locking more than just |canceled|.
> It's also trying to make sure that |origin_loop_| is valid.  If you used
> the MessageLoopProxy, then you wouldn't have to worry about that.  Then
> you could do the natural thing of just having |lock_| synchronize access
> to |canceled_| so you can store it in the local variable |canceled|.
>
> Note that you will have to check the value of
> MessageLoopProxy::PostTask() to see if it got posted successfully.  If
> not, then you need to delete |this|.
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
> net/base/cert_verifier.cc:217: Lock lock_;
> You should document what |lock_| locks.  In particular, it seems to
> primarily be |canceled_|.
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
> net/base/cert_verifier.cc:290: if (!callback || !verify_result ||
> hostname.empty()) {
> You should probably add a NOTREACHED().  These represent coding bugs,
> therefore we should crash in debug mode.
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
> net/base/cert_verifier.cc:326: inflight_.insert(std::make_pair(key,
> job));
> Should this be moved after the conditional?  So we don't insert unless
> the worker started successfully?
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
> net/base/cert_verifier.cc:332: return ERR_FAILED;  // TODO(wtc): Log an
> error message.
> Should this be ERR_UNEXPECTED?
>
> http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h
> File net/base/cert_verifier.h (right):
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h#ne...
> net/base/cert_verifier.h:27: class CachedCertVerifyResult {
> I think this should be a struct.  Otherwise, please make the member
> variables private and use accessors.
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h#ne...
> net/base/cert_verifier.h:50: class CertVerifier : public NonThreadSafe {
> Have you thought about monitoring the NetworkChangeNotifier?  We should
> probably be aborting the requests on network changes, right?  And do we
> need to flush the cache too?
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h#ne...
> net/base/cert_verifier.h:99: int Verify(X509Certificate* cert,
> I don't know enough about this area, but it seems to be that if we're
> verifying a cert, then we shouldn't be modifying the cert, so |cert|
> should be a const X509Certificate&.  Just a thought for you to consider.
>  I don't know it well enough to say if this is possible.  Just sounds
> like verification of a cert should be a const operation for the cert.
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h#ne...
> net/base/cert_verifier.h:104: RequestHandle* out_req);
> I think you should be passing in a BoundNetLog for logging purposes now.
>  And you should add a bunch of new logging events so you can see in
> about:net-internals when CertVerifier joins requests or has cache hits
> or what not.  Because if caching is broken in some way and someone
> reports a bug, you'll want to be able to tell from about:net-internals
> that it's a cache issue.
>
> Btw, now that you're caching, do we need to do something special wrt
> load flags?  Eric can speak better to this.  We should be consistent
> with the HostResolver.  I coulda sworn that it did something to force a
> lookup when a load flag was set, but I don't see anything like that now.
>  I just want to comment about that here so the right people are
> considering this issue.
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h#ne...
> net/base/cert_verifier.h:130: sizeof(cert_fingerprint.data)) == 0 &&
> arraysize
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h#ne...
> net/base/cert_verifier.h:141: sizeof(cert_fingerprint.data));
> arraysize
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h#ne...
> net/base/cert_verifier.h:184: // certificate verification is cancelled,
> and the completion callback will
> Cancelled is the British spelling.  American English uses canceled.
> Down with the Brits! :P
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier_unit...
> File net/base/cert_verifier_unittest.cc (right):
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier_unit...
> net/base/cert_verifier_unittest.cc:16:
> Can this code go into an anonymous namespace?
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier_unit...
> net/base/cert_verifier_unittest.cc:28: class CertVerifierTest : public
> testing::Test {
> If we don't need the fixture, can we just use TEST?
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/http/disk_cache_based_s...
> File net/http/disk_cache_based_ssl_host_info.cc (right):
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/http/disk_cache_based_s...
> net/http/disk_cache_based_ssl_host_info.cc:21:
> http_cache->network_layer()->GetSession()->cert_verifier()),
> Can we just pass in the CertVerifier explicitly?  It feels "cleaner" to
> me.
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/http/http_proxy_client_...
> File net/http/http_proxy_client_socket_pool_unittest.cc (right):
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/http/http_proxy_client_...
> net/http/http_proxy_client_socket_pool_unittest.cc:198:
> scoped_ptr<CertVerifier> cert_verifier_;
> Can this be of type CertVerifier rather than scoped_ptr<CertVerifier>?
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/socket/client_socket_po...
> File net/socket/client_socket_pool_manager.h (right):
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/socket/client_socket_po...
> net/socket/client_socket_pool_manager.h:10: #define
> NET_SOCKET_CLIENT_SOCKET_POOL_MANAGER_H_
> Thanks for all the cleanup in this file!  I feel a bit stupid for those
> silly mistakes.
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/socket/ssl_client_socke...
> File net/socket/ssl_client_socket_unittest.cc (right):
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/socket/ssl_client_socke...
> net/socket/ssl_client_socket_unittest.cc:47:
> scoped_ptr<net::CertVerifier> cert_verifier_;
> Should this just be a CertVerifier rather than scoped_ptr<CertVerifier>?
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/tools/fetch/fetch_clien...
> File net/tools/fetch/fetch_client.cc (right):
>
>
>
http://codereview.chromium.org/5386001/diff/52001/net/tools/fetch/fetch_clien...
> net/tools/fetch/fetch_client.cc:143: scoped_ptr<net::CertVerifier>
> cert_verifier(new net::CertVerifier);
> Why not just do CertVerifier cert_verifier?  Do you need the scoped_ptr?
>
>
> http://codereview.chromium.org/5386001/
>

[agl, 2010-12-13 16:25:04]

I expect that there will be a another round to address willchan's comments. I
agree that there will be multiple caches in the current code, although I
encourage you to land this change without addressing that. In my experience,
these changes bitrot fast and merges are an any easy to introduce bugs.

http://codereview.chromium.org/5386001/diff/52001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/5386001/diff/52001/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:48: scoped_ptr<net::CertVerifier> cert_verifier;
On 2010/12/13 09:30:53, willchan wrote:
> I think you should put |cert_verifier| below |dnsrr_resolver| because, at
least
> in my mind, it's conceivable that |cert_verifier| may eventually depend on
> |dnsrr_resolver|.

I actually wouldn't worry about this. I don't want cert_verifier to depend on
dnsrr_resolver.

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc
File net/base/cert_verifier.cc (right):

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
net/base/cert_verifier.cc:63: static const unsigned kMaxCacheEntries = 64;
My feeling is that this is probably a little low? Given the complexity of modern
sites, we could flush a cache of this size from a single navigation. I feel that
256, 512 etc would be a better balance.

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
net/base/cert_verifier.cc:217: Lock lock_;
On 2010/12/13 09:30:53, willchan wrote:
> You should document what |lock_| locks.  In particular, it seems to primarily
be
> |canceled_|.

I used to do this by indenting the protected variables, i.e.:

Lock lock_;
  bool canceled_;

Although reviewers always thought that it was a typo.

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.cc#n...
net/base/cert_verifier.cc:326: inflight_.insert(std::make_pair(key, job));
On 2010/12/13 09:30:53, willchan wrote:
> Should this be moved after the conditional?  So we don't insert unless the
> worker started successfully?

I think, originally, my code could reenter this object, thus this order. But
that's no longer the case so reordering is safe.

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h
File net/base/cert_verifier.h (right):

http://codereview.chromium.org/5386001/diff/52001/net/base/cert_verifier.h#ne...
net/base/cert_verifier.h:50: class CertVerifier : public NonThreadSafe {
On 2010/12/13 09:30:53, willchan wrote:
> Have you thought about monitoring the NetworkChangeNotifier?  We should
probably
> be aborting the requests on network changes, right?  And do we need to flush
the
> cache too?

I can imagine that OCSP/CRL/AIA requests get guillotined on a network change and
time out. However, the validity of a certificate shouldn't change based on the
current network.

(If you were thinking of DNS based stuff, then I think that this should be
layered under that.)

[wtc, 2010-12-16 03:14:15]

Please review Patch Set 4.  You can just review the diffs
between Patch Sets 2 and 4 for the three primary files:
  cert_verifier.h
  cert_verifier.cc
  cert_verifier_unittest.cc

agl: I increased the cache size from 64 to 256.

willchan: thank you for your thoughtful drive-by review.
I went through all of your comments and made the simple
changes.  I will save the complicated changes for a
follow-up CL.  The most important comment of yours is the
use of MessageLoopProxy. I believe the current code won't
crash in the presence of leaked requests provided that the
global CertVerifier object is deleted before the origin loop
goes away.  I think this is ensured by the IOThread class.

The CertVerifier destructor calls Cancel() on all the
pending requests and suppress their user callbacks.  This
is why the code won't crash as long as IOThread's global
CertVerifer object is deleted before the IO thread's
message loop goes away.

[agl, 2011-01-06 18:23:30]

Have looked at the differences from patch set 2 for those three files. LGTM.
Again, if this hasn't landed yet then I fear for the bit-rot :)