Improving and adding an in-memory MRU cache to DiskBasedCertCache.

net/http/disk_based_cert_cache.cc

Index: net/http/disk_based_cert_cache.cc
diff --git a/net/http/disk_based_cert_cache.cc b/net/http/disk_based_cert_cache.cc
index b6c5b7e2731b5ede6215fefe0f2ffceba679f5e6..9aa2189656d8ef2c0ff5ecb5158a15ba32f5f568 100644
--- a/net/http/disk_based_cert_cache.cc
+++ b/net/http/disk_based_cert_cache.cc
@@ -207,7 +207,6 @@ int DiskBasedCertCache::WriteWorker::DoOpen() {
 
 int DiskBasedCertCache::WriteWorker::DoOpenComplete(int rv) {
   if (rv < 0) {
-    state_ = STATE_NONE;
     return rv;
   }

[wtc, 2014/07/01 02:11:31]

Nit: omit the curly braces in these simple if statements.

   state_ = STATE_WRITE;
@@ -219,7 +218,6 @@ int DiskBasedCertCache::WriteWorker::DoWrite() {
   bool encoded = X509Certificate::GetDEREncoded(cert_handle_, &write_data);
 
   if (!encoded) {
-    state_ = STATE_NONE;
     return ERR_FAILED;
   }
 
@@ -238,7 +236,6 @@ int DiskBasedCertCache::WriteWorker::DoWrite() {
 }
 
 int DiskBasedCertCache::WriteWorker::DoWriteComplete(int rv) {
-  state_ = STATE_NONE;
   if (rv < io_buf_len_)
     return ERR_FAILED;
 
@@ -287,7 +284,7 @@ class DiskBasedCertCache::ReadWorker {
   // regardless of success or failure.
   ReadWorker(disk_cache::Backend* backend,
              const std::string& key,
-             const base::Closure& cleanup_callback);
+             const GetCallback& cleanup_callback);
 
   ~ReadWorker();
 
@@ -335,15 +332,14 @@ class DiskBasedCertCache::ReadWorker {
   scoped_refptr<IOBuffer> buffer_;
   int io_buf_len_;
 
-  base::Closure cleanup_callback_;
+  GetCallback cleanup_callback_;
   std::vector<GetCallback> user_callbacks_;
   CompletionCallback io_callback_;
 };
 
-DiskBasedCertCache::ReadWorker::ReadWorker(
-    disk_cache::Backend* backend,
-    const std::string& key,
-    const base::Closure& cleanup_callback)
+DiskBasedCertCache::ReadWorker::ReadWorker(disk_cache::Backend* backend,
+                                           const std::string& key,
+                                           const GetCallback& cleanup_callback)
     : backend_(backend),
       cert_handle_(NULL),
       key_(key),
@@ -419,7 +415,6 @@ int DiskBasedCertCache::ReadWorker::DoOpen() {
 
 int DiskBasedCertCache::ReadWorker::DoOpenComplete(int rv) {
   if (rv < 0) {
-    state_ = STATE_NONE;
     return rv;
   }
   state_ = STATE_READ;
@@ -435,7 +430,6 @@ int DiskBasedCertCache::ReadWorker::DoRead() {
 }
 
 int DiskBasedCertCache::ReadWorker::DoReadComplete(int rv) {
-  state_ = STATE_NONE;
   if (rv < io_buf_len_)
     return ERR_FAILED;
 
@@ -457,7 +451,7 @@ void DiskBasedCertCache::ReadWorker::RunCallbacks() {
 }
 
 void DiskBasedCertCache::ReadWorker::Finish(int rv) {
-  cleanup_callback_.Run();
+  cleanup_callback_.Run(cert_handle_);
   cleanup_callback_.Reset();
   RunCallbacks();
   delete this;
@@ -475,7 +469,7 @@ DiskBasedCertCache::ReadWorker::~ReadWorker() {
 }
 
 DiskBasedCertCache::DiskBasedCertCache(disk_cache::Backend* backend)
-    : backend_(backend), weak_factory_(this) {
+    : backend_(backend), mru_cert_cache_(30), weak_factory_(this) {

[wtc, 2014/07/01 02:11:31]

Define a constant with the value 30 near the top of this file.

   DCHECK(backend_);
 }
 
@@ -495,6 +489,15 @@ DiskBasedCertCache::~DiskBasedCertCache() {
 void DiskBasedCertCache::Get(const std::string& key, const GetCallback& cb) {
   DCHECK(!key.empty());
 
+  // If the handle is already in the MRU cache, just return that (via callback).
+  // Note, this will also bring the cert_handle to the front
+  // of the regency list in the MRU cache.

[wtc, 2014/07/01 02:11:31]

1. Typo: regency => recency

2. Move some of the words to the previous line.

+  MRUCertCache::iterator mru_it = mru_cert_cache_.Get(key);
+  if (mru_it != mru_cert_cache_.end()) {
+    cb.Run(mru_it->second);
+    return;
+  }
+
   ReadWorkerMap::iterator it = read_worker_map_.find(key);
 
   if (it == read_worker_map_.end()) {
@@ -518,6 +521,15 @@ void DiskBasedCertCache::Set(const X509Certificate::OSCertHandle cert_handle,
   DCHECK(cert_handle);
   std::string key = GetCacheKeyToCert(cert_handle);
 
+  // If |cert_handle| already exists in the MRU cache, there is no need to
+  // re-write it to the disk cache. This will also bring |cert_handle|
+  // to the front of the regency list in the MRU cache.

[wtc, 2014/07/01 02:11:31]

Typo: regency => recency

+  if (mru_cert_cache_.Get(key) != mru_cert_cache_.end()) {
+    cb.Run(key);
+    return;
+  }
+
+  mru_cert_cache_.Put(key, X509Certificate::DupOSCertHandle(cert_handle));

[wtc, 2014/07/01 02:11:31]

A comment that explains why it is advantageous and correct to add the cert to
the MRU cache now should be added.

[brandonsalmon, 2014/07/01 18:19:29]

I no longer think this is that advantageous. (There are many write requests
simultaneously, but in those cases the user callback will just be added to the
WriteWorker).

I moved this to the FinishedWriteOperation function. 

   WriteWorkerMap::iterator it = write_worker_map_.find(key);
 
   if (it == write_worker_map_.end()) {
@@ -540,7 +552,11 @@ void DiskBasedCertCache::FinishedWriteOperation(const std::string& key) {
   write_worker_map_.erase(key);
 }
 
-void DiskBasedCertCache::FinishedReadOperation(const std::string& key) {
+void DiskBasedCertCache::FinishedReadOperation(
+    const std::string& key,
+    X509Certificate::OSCertHandle cert_handle) {
+  if (cert_handle)
+    mru_cert_cache_.Put(key, X509Certificate::DupOSCertHandle(cert_handle));
   read_worker_map_.erase(key);
 }