Disk Based Certificate Cache Implementation

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
new file mode 100644
index 0000000000000000000000000000000000000000..844ebcc8b7cf833d79050c6648ad80fb605d10d2
--- /dev/null
+++ b/net/http/disk_based_cert_cache.cc
@@ -0,0 +1,512 @@
+// Copyright (c) 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/http/disk_based_cert_cache.h"
+
+#include <vector>
+
+#include "base/bind.h"
+#include "base/callback_helpers.h"
+#include "base/memory/ref_counted.h"
+#include "base/stl_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/disk_cache/disk_cache.h"
+
+namespace net {
+
+namespace {
+
+// Used to obtain a unique cache key for a certificate in the form of
+// "cert:<hash>".
+std::string GetCacheKeyToCert(const X509Certificate::OSCertHandle cert_handle) {
+  SHA1HashValue fingerprint =
+      X509Certificate::CalculateFingerprint(cert_handle);
+
+  return "cert:" +
+         base::HexEncode(fingerprint.data, arraysize(fingerprint.data));
+}
+
+}  // namespace
+
+// WriteWorkers represent pending Set jobs in the DiskBasedCertCache. Each
+// certificate requested to be cached is assigned a Writeworker on a one-to-one
+// basis. The same certificate should not have multiple WriteWorkers at the same
+// time; instead, add a user_callback_ to the existing WriteWorker.

[wtc, 2014/06/21 00:43:12]

Typo: user_callback_ => user callback

+class DiskBasedCertCache::WriteWorker {
+ public:
+  // |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 ReadWorker,
+  // regardless of success or failure.
+  WriteWorker(disk_cache::Backend* backend,
+              const std::string& key,
+              const X509Certificate::OSCertHandle cert_handle,

[wtc, 2014/06/21 00:43:13]

Nit: this 'const' isn't necessary.

+              const base::Closure& cleanup_callback);
+
+  ~WriteWorker();
+
+  // Writes the given certificate to the cache. On completion, will invoke all
+  // user callbacks.
+  void Start();
+
+  // Adds a callback to the set of callbacks to be notified when this

[wtc, 2014/06/21 00:43:12]

Nit: to be notified => to run

+  // WriteWorker finishes processing.
+  void AddCallback(const SetCallback& user_callback);
+
+  // Signals the WriteWorker to finish early. The WriteWorker will be destroyed

[wtc, 2014/06/21 00:43:13]

Nit: finish early => abort early ?

+  // upon the completion of any pending callbacks. User callbacks will be
+  // invoked with an empty string.
+  void Cancel();
+
+ private:
+  enum WriteState {
+    STATE_CREATE_OR_OPEN,
+    STATE_FINISH_CREATE_OR_OPEN,
+    STATE_START_WRITE,

[wtc, 2014/06/21 00:43:12]

Nit: this state would be named STATE_WRITE in our convention.

+    STATE_FINISH_WRITE,

[wtc, 2014/06/21 00:43:12]

Our naming convention for the STATE_FINISH_XXX state is STATE_XXX_COMPLETE.

Please rename the corresponding DoFinishXXX() functions to DoXXXComplete().

+    STATE_WRITE_NONE

[wtc, 2014/06/21 00:43:13]

Nit: This state can be named simply STATE_NONE.

+  };
+
+  void OnIOComplete(int rv);
+  int DoLoop(int rv);
+  int DoCreateOrOpen();
+  int DoFinishCreateOrOpen(int rv);
+  int DoStartWrite();
+  int DoFinishWrite(int rv);
+  void Finish(int rv);
+
+  // invokes all of the |user_callbacks_|

[wtc, 2014/06/21 00:43:12]

Nit: capitalize "Invokes".

+  void CallCallbacks(int rv);
+
+  disk_cache::Backend* backend_;
+  const X509Certificate::OSCertHandle cert_handle_;
+  std::string key_;
+  bool canceled_;
+
+  disk_cache::Entry* entry_;
+  WriteState state_;
+  bool create_failed_;
+  scoped_refptr<IOBuffer> buffer;

[wtc, 2014/06/21 00:43:12]

This member should be named buffer_, with a trailing '_'.

+
+  base::Closure cleanup_callback_;
+  std::vector<SetCallback> user_callbacks_;
+  CompletionCallback io_callback_;
+
+  base::WeakPtrFactory<WriteWorker> weak_factory_;
+};
+
+DiskBasedCertCache::WriteWorker::WriteWorker(
+    disk_cache::Backend* backend,
+    const std::string& key,
+    X509Certificate::OSCertHandle cert_handle,
+    const base::Closure& cleanup_callback)
+    : backend_(backend),
+      cert_handle_(cert_handle),
+      key_(key),
+      canceled_(false),
+      entry_(NULL),
+      state_(STATE_CREATE_OR_OPEN),

[wtc, 2014/06/21 00:43:12]

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.

+      create_failed_(false),
+      cleanup_callback_(cleanup_callback),
+      weak_factory_(this) {
+  io_callback_ =
+      base::Bind(&WriteWorker::OnIOComplete, weak_factory_.GetWeakPtr());
+}
+
+void DiskBasedCertCache::WriteWorker::Start() {
+  int rv = DoLoop(OK);
+
+  if (state_ != STATE_WRITE_NONE)

[wtc, 2014/06/21 00:43:12]

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.

+    return;
+
+  Finish(rv);
+}
+
+void DiskBasedCertCache::WriteWorker::AddCallback(
+    const SetCallback& user_callback) {
+  user_callbacks_.push_back(user_callback);
+}
+
+void DiskBasedCertCache::WriteWorker::OnIOComplete(int rv) {
+  if (canceled_) {
+    Finish(ERR_FAILED);
+    return;
+  }
+
+  rv = DoLoop(rv);
+
+  if (state_ != STATE_WRITE_NONE)
+    return;
+
+  Finish(rv);
+}
+
+int DiskBasedCertCache::WriteWorker::DoLoop(int rv) {
+  do {
+    switch (state_) {
+      case STATE_CREATE_OR_OPEN:
+        rv = DoCreateOrOpen();
+        break;
+      case STATE_FINISH_CREATE_OR_OPEN:
+        rv = DoFinishCreateOrOpen(rv);
+        break;
+      case STATE_START_WRITE:
+        rv = DoStartWrite();
+        break;
+      case STATE_FINISH_WRITE:
+        rv = DoFinishWrite(rv);
+        break;
+      case STATE_WRITE_NONE:
+        break;
+    }
+  } while (rv != ERR_IO_PENDING && state_ != STATE_WRITE_NONE);
+
+  return rv;
+}
+
+int DiskBasedCertCache::WriteWorker::DoCreateOrOpen() {
+  state_ = STATE_FINISH_CREATE_OR_OPEN;
+
+  if (create_failed_)
+    return backend_->OpenEntry(key_, &entry_, io_callback_);
+
+  return backend_->CreateEntry(key_, &entry_, io_callback_);
+}
+
+int DiskBasedCertCache::WriteWorker::DoFinishCreateOrOpen(int rv) {
+  // ERR_FAILED implies create entry failed. In order to avoid trying
+  // to open the entry multiple times, the flag create_failed_ is set and
+  // checked.
+  if (rv < 0 && !create_failed_) {
+    create_failed_ = true;

[wtc, 2014/06/21 00:43:12]

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.

+    state_ = STATE_CREATE_OR_OPEN;
+    return OK;
+  }
+  if (rv < 0) {
+    state_ = STATE_WRITE_NONE;
+    return rv;
+  }
+
+  state_ = STATE_START_WRITE;
+  return OK;
+}
+
+int DiskBasedCertCache::WriteWorker::DoStartWrite() {
+  std::string write_data;
+  bool encoded = X509Certificate::GetDEREncoded(cert_handle_, &write_data);
+
+  if (!encoded) {
+    state_ = STATE_WRITE_NONE;
+    return ERR_FAILED;
+  }
+
+  buffer = new IOBuffer(write_data.size());
+  memcpy(buffer->data(), write_data.data(), write_data.size());
+
+  state_ = STATE_FINISH_WRITE;
+
+  return entry_->WriteData(0 /* index */,
+                           0 /* offset */,
+                           buffer,
+                           write_data.size(),
+                           io_callback_,
+                           true /* truncate */);
+}
+
+int DiskBasedCertCache::WriteWorker::DoFinishWrite(int rv) {
+  if (rv < 0) {
+    state_ = STATE_WRITE_NONE;
+    return rv;
+  }
+  state_ = STATE_WRITE_NONE;
+  return OK;

[wtc, 2014/06/21 00:43:12]

The function can be simply

  state_ = STATE_WRITE_NONE;
  return rv;

+}
+
+void DiskBasedCertCache::WriteWorker::CallCallbacks(int rv) {
+  for (std::vector<SetCallback>::iterator it = user_callbacks_.begin();
+       it != user_callbacks_.end();
+       ++it) {
+    it->Run((rv >= 0) ? key_ : std::string());

[wtc, 2014/06/21 00:43:12]

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.

+    it->Reset();
+  }
+}
+
+void DiskBasedCertCache::WriteWorker::Finish(int rv) {
+  cleanup_callback_.Run();
+  cleanup_callback_.Reset();
+  CallCallbacks(rv);
+  delete this;
+}
+
+void DiskBasedCertCache::WriteWorker::Cancel() {
+  canceled_ = true;
+}
+
+DiskBasedCertCache::WriteWorker::~WriteWorker() {
+  if (entry_)
+    entry_->Close();
+}
+
+// ReadWorkers represent pending Get jobs in the DiskBasedCertCache. Each
+// certificate requested to be retrieved from the cache is assigned a ReadWorker
+// on a one-to-one basis. The same |key| should not have multiple ReadWorkers
+// at the same time; instead, call AddCallback to add a user_callback_ to
+// the the existing ReadWorker.
+class DiskBasedCertCache::ReadWorker {
+ public:
+  // |backend| is the backend to read |certificate| from, using
+  // |key| as the key for the disk_cache::Entry.
+  // |cleanup_callback| is called to clean up this ReadWorker,
+  // regardless of success or failure.
+  ReadWorker(disk_cache::Backend* backend,
+             const std::string& key,
+             const base::Closure& cleanup_callback);
+
+  ~ReadWorker();
+
+  // Reads the given certificate from the cache. On completion, will invoke all
+  // user callbacks.
+  void Start();
+
+  // Adds a callback to the set of callbacks to be notified when this
+  // ReadWorker finishes processing.
+  void AddCallback(const GetCallback& user_callback);
+
+  // Signals the ReadWorker to finish early. The ReadWorker will be destroyed
+  // upon the completion of any pending callbacks. User callbacks will be
+  // invoked with a NULL cert handle.
+  void Cancel();
+
+ private:
+  enum ReadState {
+    STATE_READ_OPEN,
+    STATE_START_READ,
+    STATE_FINISH_READ,
+    STATE_READ_NONE
+  };
+
+  void OnIOComplete(int rv);
+  int DoLoop(int rv);
+  int DoOpen();
+  int DoStartRead(int rv);
+  int DoFinishRead(int rv);
+  void Finish(int rv);
+
+  // invokes all of |user_callbacks_|
+  void CallCallbacks(int rv);
+
+  disk_cache::Backend* backend_;
+  X509Certificate::OSCertHandle cert_handle_;
+  std::string key_;
+  bool canceled_;
+
+  disk_cache::Entry* entry_;
+
+  ReadState state_;
+  int entry_size_;
+  scoped_refptr<IOBuffer> buffer;
+
+  base::Closure cleanup_callback_;
+  std::vector<GetCallback> user_callbacks_;
+  CompletionCallback io_callback_;
+  base::WeakPtrFactory<ReadWorker> weak_factory_;
+};
+
+DiskBasedCertCache::ReadWorker::ReadWorker(
+    disk_cache::Backend* backend,
+    const std::string& key,
+    const base::Closure& cleanup_callback)
+    : backend_(backend),
+      cert_handle_(NULL),
+      key_(key),
+      canceled_(false),
+      entry_(NULL),
+      state_(STATE_READ_OPEN),
+      entry_size_(0),
+      cleanup_callback_(cleanup_callback),
+      weak_factory_(this) {
+  io_callback_ =
+      base::Bind(&ReadWorker::OnIOComplete, weak_factory_.GetWeakPtr());
+}
+
+void DiskBasedCertCache::ReadWorker::Start() {
+  int rv = DoLoop(OK);
+
+  if (state_ != STATE_READ_NONE)
+    return;
+
+  Finish(rv);
+}
+
+void DiskBasedCertCache::ReadWorker::AddCallback(
+    const GetCallback& user_callback) {
+  user_callbacks_.push_back(user_callback);
+}
+
+void DiskBasedCertCache::ReadWorker::OnIOComplete(int rv) {
+  if (canceled_) {
+    Finish(ERR_FAILED);
+    return;
+  }
+
+  rv = DoLoop(rv);
+
+  if (state_ != STATE_READ_NONE)
+    return;
+
+  Finish(rv);
+}
+
+int DiskBasedCertCache::ReadWorker::DoLoop(int rv) {
+  do {
+    switch (state_) {
+      case STATE_READ_OPEN:
+        rv = DoOpen();
+        break;
+      case STATE_START_READ:
+        rv = DoStartRead(rv);
+        break;
+      case STATE_FINISH_READ:
+        rv = DoFinishRead(rv);
+        break;
+      case STATE_READ_NONE:
+        break;
+    }
+  } while (rv != ERR_IO_PENDING && state_ != STATE_READ_NONE);
+
+  return rv;
+}
+
+int DiskBasedCertCache::ReadWorker::DoOpen() {
+  state_ = STATE_START_READ;
+  return backend_->OpenEntry(key_, &entry_, io_callback_);
+}
+
+int DiskBasedCertCache::ReadWorker::DoStartRead(int rv) {
+  if (rv < 0) {
+    state_ = STATE_READ_NONE;
+    return rv;
+  }

[wtc, 2014/06/21 00:43:12]

Nit: we usually use a STATE_FINISH_OPEN state to handle the result of the
backend_->OpenEntry() call.

+
+  entry_size_ = entry_->GetDataSize(0 /* index */);
+  state_ = STATE_FINISH_READ;
+  buffer = new IOBuffer(entry_size_);
+  return entry_->ReadData(
+      0 /* index */, 0 /* offset */, buffer, entry_size_, io_callback_);
+}
+
+int DiskBasedCertCache::ReadWorker::DoFinishRead(int rv) {
+  if (rv < 0) {
+    state_ = STATE_READ_NONE;
+    return rv;
+  }
+
+  state_ = STATE_READ_NONE;
+
+  cert_handle_ =
+      X509Certificate::CreateOSCertHandleFromBytes(buffer->data(), entry_size_);
+
+  DCHECK(cert_handle_);
+  return OK;
+}
+
+void DiskBasedCertCache::ReadWorker::CallCallbacks(int rv) {
+  for (std::vector<GetCallback>::iterator it = user_callbacks_.begin();
+       it != user_callbacks_.end();
+       ++it) {
+    it->Run((rv >= 0) ? cert_handle_ : NULL);
+    it->Reset();
+  }
+}
+
+void DiskBasedCertCache::ReadWorker::Finish(int rv) {
+  cleanup_callback_.Run();
+  cleanup_callback_.Reset();
+  CallCallbacks(rv);
+
+  delete this;
+}
+
+void DiskBasedCertCache::ReadWorker::Cancel() {
+  canceled_ = true;
+}
+
+DiskBasedCertCache::ReadWorker::~ReadWorker() {
+  if (entry_)
+    entry_->Close();
+  if (cert_handle_)
+    X509Certificate::FreeOSCertHandle(cert_handle_);
+}
+
+DiskBasedCertCache::DiskBasedCertCache(disk_cache::Backend* backend)
+    : backend_(backend), weak_factory_(this) {
+  DCHECK(backend_);
+}
+
+DiskBasedCertCache::~DiskBasedCertCache() {
+  for (WriteWorkerMap::iterator it = write_worker_map_.begin();
+       it != write_worker_map_.end();
+       ++it)
+    it->second->Cancel();

[wtc, 2014/06/21 00:43:12]

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?

[brandonsalmon, 2014/06/21 02:21:32]

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"

+  for (ReadWorkerMap::iterator it = read_worker_map_.begin();
+       it != read_worker_map_.end();
+       ++it)
+    it->second->Cancel();
+}
+
+void DiskBasedCertCache::Get(const std::string& key, const GetCallback& cb) {
+  DCHECK(!key.empty());
+
+  ReadWorkerMap::iterator it = read_worker_map_.find(key);
+
+  if (it == read_worker_map_.end()) {
+    ReadWorker* worker =
+        new ReadWorker(backend_,
+                       key,
+                       base::Bind(&DiskBasedCertCache::FinishedReadOperation,
+                                  weak_factory_.GetWeakPtr(),
+                                  key));
+    read_worker_map_[key] = worker;
+    worker->AddCallback(cb);
+    worker->Start();
+  } else {
+    it->second->AddCallback(cb);
+  }
+}
+
+void DiskBasedCertCache::Set(const X509Certificate::OSCertHandle cert_handle,
+                             const SetCallback& cb) {
+  DCHECK(!cb.is_null());
+  DCHECK(cert_handle);
+  std::string key = GetCacheKeyToCert(cert_handle);
+
+  WriteWorkerMap::iterator it = write_worker_map_.find(key);
+
+  if (it == write_worker_map_.end()) {
+    WriteWorker* worker =
+        new WriteWorker(backend_,
+                        key,
+                        cert_handle,
+                        base::Bind(&DiskBasedCertCache::FinishedWriteOperation,
+                                   weak_factory_.GetWeakPtr(),
+                                   key));
+    write_worker_map_[key] = worker;
+    worker->AddCallback(cb);
+    worker->Start();
+  } else {
+    it->second->AddCallback(cb);
+  }
+}
+
+void DiskBasedCertCache::FinishedWriteOperation(const std::string& key) {
+  write_worker_map_.erase(key);
+}
+
+void DiskBasedCertCache::FinishedReadOperation(const std::string& key) {
+  read_worker_map_.erase(key);
+}
+
+}  // namespace net