Adding DiskBasedCertCache to HttpCache (+UMA).

net/http/http_cache_transaction.cc

Index: net/http/http_cache_transaction.cc
diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index bf791118be88b704966c7d3a886d5fcc4c924f63..a2e48432fbb866e5e3ecd3d384ee14c2ef079415 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -32,6 +32,7 @@
 #include "net/base/upload_data_stream.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/disk_cache/disk_cache.h"
+#include "net/http/disk_based_cert_cache.h"
 #include "net/http/http_network_session.h"
 #include "net/http/http_request_info.h"
 #include "net/http/http_response_headers.h"
@@ -47,6 +48,107 @@ using base::TimeTicks;
 
 namespace {
 
+// Stores data relevant to the statistics of writing entire certificate

[wtc, 2014/07/08 00:14:03]

Nit: you also use this struct for reading certificate chains. So it's not just
"writing".

+// chains using DiskBasedCertCache. |num_pending_ops| is the number
+// of certificates in the chain that have pending operations in the
+// DiskBasedCertCache. |start_time| is the time that the read and write
+// commands began being issued to the DiskBasedCertCache.
+// TODO(brandonsalmon): Remove this when it is no longer necessary to
+// collect data.
+struct SharedCCData {

[wtc, 2014/07/08 00:14:04]

Don't use the abbreviation "CC" in the name. See the Style Guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...

Without looking at the rest of the file, I didn't know whether "CC" stands for
certificate chain or certificate cache.

You can name this struct "SharedChainData".

+  SharedCCData(int num, TimeTicks time)

[wtc, 2014/07/08 00:14:03]

These two parameters are poorly named. You can try:

num => num_ops or pending_ops

time => start

Note: I remember it is actually legal to name these parameters num_pending_ops
and start_time. You can also try that.

+      : num_pending_ops(num), start_time(time) {}
+  int num_pending_ops;
+  TimeTicks start_time;
+};
+
+typedef base::RefCountedData<SharedCCData> RefCountedSharedCCData;

[wtc, 2014/07/08 00:14:04]

This isn't how a ref-counted type is defined. Use
net/http/http_response_headers.h as an example:

https://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_response_heade...

+
+// Used to obtain a cache entry key for an OSCertHandle.
+// TODO(brandonsalmon): Remove this when cache keys are stored
+// and no longer have to be recomputed to retrieve the OSCertHandle
+// from the disk.
+std::string GetCacheKeyToCert(

[wtc, 2014/07/08 00:14:03]

Nit: GetCacheKeyToCert => GetCacheKeyForCert

To guarantee that this file gets the exact cache key used by the
CertBasedDiskCache, you should instead have CertBasedDiskCache expose its cache
key function, with a similar TODO comment.

[Ryan Sleevi, 2014/07/08 00:34:18]

I recommended Brandon not do this, precisely because it would have forced a more
explicit coupling. The current method (recomputing by hand) more practically
reflects the on-disk storage (in that if we do change the cache method, existing
entries won't be updated)

+    const net::X509Certificate::OSCertHandle cert_handle) {

[wtc, 2014/07/08 00:14:03]

This const is not necessary.

+  net::SHA1HashValue fingerprint =
+      net::X509Certificate::CalculateFingerprint(cert_handle);
+
+  return "cert:" +
+         base::HexEncode(fingerprint.data, arraysize(fingerprint.data));
+}
+
+// |dist_from_root| indicates the position of the read certificate in the
+// certificate chain, 0 indicating it is the leaf. |is_leaf| indicates

[wtc, 2014/07/08 00:14:03]

Typo: 0 indicating it is the leaf => 0 indicating it is the root

Also fix line 117.

+// whether or not the read certificate was the leaf of the chain.
+// |shared_chain_data| contains data shared by each certificate in
+// the chain.
+void OnCertReadIOComplete(
+    int dist_from_root,
+    bool is_leaf,
+    scoped_refptr<RefCountedSharedCCData> shared_chain_data,

[wtc, 2014/07/08 00:14:04]

I think you can declare this as

[Ryan Sleevi, 2014/07/08 00:34:18]

Incomplete?

passing "const scoped_refptr<RefCountedSharedCCData>& shared_chain_data" is the
recommended way, though.

non-const, non-ref for scoped_ptr
const, ref for scoped_refptr

[wtc, 2014/07/08 01:54:42]

Yes, this comment is incomplete. Please ignore it.

+    net::X509Certificate::OSCertHandle cert_handle) {
+  // If |num_pending_ops| is one, this was the last pending read operation
+  // for this chain of certificates. The total time to write the chain

[wtc, 2014/07/08 00:14:03]

write the chain => read the chain ?

+  // can be calculated by subtracting the starting time from Now().
+  (shared_chain_data->data).num_pending_ops--;

[wtc, 2014/07/08 00:14:03]

Omit the parentheses.

+  if (!shared_chain_data->data.num_pending_ops) {
+    const TimeDelta read_chain_wait =
+        TimeTicks::Now() - shared_chain_data->data.start_time;
+    UMA_HISTOGRAM_TIMES("DiskBasedCertCache.ChainReadTime", read_chain_wait);

[wtc, 2014/07/08 00:14:03]

IMPORTANT: use a local histogram (HISTOGRAM_TIMES) instead of UMA_HISTOGRAM for
now. Switch to UMA_HISTOGRAM when this code is ready.

[Ryan Sleevi, 2014/07/08 00:34:18]

wtc: Why do you suggest this? Part of the field trial is to gather performance
metrics, and it's reason for field trial is to avoid regressing (due to induced
additional I/O to read cache entries)

[wtc, 2014/07/08 01:54:42]

I didn't know you suggested the field trial.

This CL looked incomplete to me, which is why I suggested using local histograms
first to avoid sending statistics to UMA prematurely.

+  }
+
+  bool success = (cert_handle != NULL);
+  UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.ReadSuccessTotal", success);
+
+  if (is_leaf)
+    UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.ReadSuccessLeaf", success);
+
+  if (dist_from_root == 0)
+    UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.ReadSuccessRoot", success);

[wtc, 2014/07/08 00:14:03]

We should not need to cache the root certificate because trusted root
certificates are usually built in to the crypto library.

[Ryan Sleevi, 2014/07/08 00:34:18]

We do cache the root certs today, precisely because root certificates change,
but the UI status does not (it remains fixed in time).

That is, if you validated a connection at time T0, to root R, and then at T5,
removed the root R, you should still display a correct chain at T10.

Considering the rate of NSS root changes as the timeline for deprecating
1024-bit roots consider, this is not out of the realm of possibility, especially
for a large number of sites (Verisign/Symantec roots)

+  else if (dist_from_root == 1)
+    UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.ReadSuccessInt1", success);
+  else if (dist_from_root == 2)
+    UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.ReadSuccessInt2", success);

[wtc, 2014/07/08 00:14:03]

BUG: The Int1 and Int2 names imply these certificates are intermediate CA certs.
But they can be a leaf certificate. To fix this, you need to return from the
function if is_leaf is true, on line 104. (Remember to add curly braces to that
if statement.)

Make the same change to lines 139-140 in OnCertWriteIOComplete.

An alternative is to use the dist_from_leaf count instead, and count the
intermediate CA certificates from the leaf. This also allows us to omit is_leaf
because it is simply dist_from_leaf == 0.

+  else
+    UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.ReadSuccessIntN", success);
+}
+
+// |dist_from_root| indicates the position of the written certificate in the
+// certificate chain, 0 indicating it is the leaf. |is_leaf| indicates
+// whether or not the written certificate was the leaf of the chain.
+// |shared_chain_data| contains data shared by each certificate in
+// the chain.
+void OnCertWriteIOComplete(
+    int dist_from_root,
+    bool is_leaf,
+    scoped_refptr<RefCountedSharedCCData> shared_chain_data,

[Ryan Sleevi, 2014/07/08 00:34:18]

const-ref

+    const std::string& key) {
+  // If |num_pending_ops| is one, this was the last pending write operation
+  // for this chain of certificates. The total time to write the chain
+  // can be calculated by subtracting the starting time from Now().
+  shared_chain_data->data.num_pending_ops--;
+  if (!shared_chain_data->data.num_pending_ops) {
+    const TimeDelta write_chain_wait =
+        TimeTicks::Now() - shared_chain_data->data.start_time;
+    UMA_HISTOGRAM_TIMES("DiskBasedCertCache.ChainWriteTime", write_chain_wait);
+  }
+
+  bool success = (key != std::string());

[wtc, 2014/07/08 00:14:03]

key != std::string() => !key.empty()

+  UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.WriteSuccessTotal", success);
+
+  if (is_leaf)
+    UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.WriteSuccessLeaf", success);
+
+  if (dist_from_root == 0)
+    UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.WriteSuccessRoot", success);
+  else if (dist_from_root == 1)
+    UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.WriteSuccessInt1", success);
+  else if (dist_from_root == 2)
+    UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.WriteSuccessInt2", success);
+  else
+    UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.WriteSuccessIntN", success);
+}
+
 // From http://tools.ietf.org/html/draft-ietf-httpbis-p6-cache-21#section-6
 //      a "non-error response" is one with a 2xx (Successful) or 3xx
 //      (Redirection) status code.
@@ -1495,6 +1597,37 @@ int HttpCache::Transaction::DoCacheReadResponseComplete(int result) {
     return OnCacheReadError(result, true);
   }
 
+  // CertCache() will be null if the CertCacheTrial field trial is disabled.
+  if (cache_->CertCache() && response_.ssl_info.is_valid()) {
+    std::string key =
+        GetCacheKeyToCert(response_.ssl_info.cert->os_cert_handle());
+    X509Certificate::OSCertHandles intermediates =

[wtc, 2014/07/08 00:14:03]

Declare this as const X509Certificate::OSCertHandles& (note the '&') to avoid
the copying of X509Certificate::OSCertHandles.

[Ryan Sleevi, 2014/07/08 00:34:18]

Correct.

So it's unambiguous: const-ref OSCertHandles (plural), by-value OSCertHandle
(singular)

+        response_.ssl_info.cert->GetIntermediateCertificates();
+    int dist_from_root = intermediates.size();
+
+    SharedCCData temp(intermediates.size() + 1, TimeTicks::Now());
+    scoped_refptr<RefCountedSharedCCData> shared_chain_data(
+        new RefCountedSharedCCData(temp));
+    cache_->CertCache()->Get(key,

[wtc, 2014/07/08 00:14:04]

IMPORTANT: Add new states to wait for the completion of Get() and Set(). Are you
planning to do this in a separate CL?

[Ryan Sleevi, 2014/07/08 00:34:18]

wtc: I recommended this path to Brandon. Could you elaborate on your design
concerns?

[wtc, 2014/07/08 01:54:42]

Eventually we will need to wait for the completion of Get() and Set(). I was
asking if Brandon plans to do that in a separate CL.

+                             base::Bind(&OnCertReadIOComplete,
+                                        dist_from_root,
+                                        true /* is leaf */,
+                                        shared_chain_data));

[wtc, 2014/07/08 00:14:03]

IMPORTANT: rather than using a heap-allocated SharedCCData struct, I would just
consider adding a data member of the SharedCCData type to this class, and
passing a WeakPtr of this object to base::Bind here. I haven't thought about
this carefully, so this may not work.

[Ryan Sleevi, 2014/07/08 00:34:18]

It's unclear to me what you're trying to solve.

This path is preferred when binding a common structure to multiple callbacks.

[wtc, 2014/07/08 01:54:42]

Eventually this class will need to wait for the completion of Get() and Set().
So the GetCallback and SetCallback will need a pointer to this object at that
time. In that case we might as well store num_pending_ops and start_time inside
this class.

Again, I haven't thought about this carefully, so what I suggested may not work.

+
+    for (X509Certificate::OSCertHandles::iterator it = intermediates.begin();
+         it != intermediates.end();
+         ++it) {
+      --dist_from_root;
+      key = GetCacheKeyToCert(*it);
+      cache_->CertCache()->Get(key,
+                               base::Bind(&OnCertReadIOComplete,
+                                          dist_from_root,
+                                          false /* is not leaf */,
+                                          shared_chain_data));
+    }
+    DCHECK_EQ(0, dist_from_root);
+  }
+
   // Some resources may have slipped in as truncated when they're not.
   int current_size = entry_->disk_entry->GetDataSize(kResponseContentIndex);
   if (response_.headers->GetContentLength() == current_size)
@@ -2327,6 +2460,32 @@ int HttpCache::Transaction::WriteResponseInfoToEntry(bool truncated) {
     return OK;
   }
 
+  // CertCache() will be null if the CertCacheTrial field trial is disabled.
+  if (cache_->CertCache() && response_.ssl_info.is_valid()) {
+    X509Certificate::OSCertHandles intermediates =
+        response_.ssl_info.cert->GetIntermediateCertificates();
+    int dist_from_root = intermediates.size();
+    SharedCCData temp(intermediates.size() + 1, TimeTicks::Now());
+    scoped_refptr<RefCountedSharedCCData> shared_chain_data(
+        new RefCountedSharedCCData(temp));
+    cache_->CertCache()->Set(response_.ssl_info.cert->os_cert_handle(),
+                             base::Bind(&OnCertWriteIOComplete,
+                                        dist_from_root,
+                                        true /* is leaf */,
+                                        shared_chain_data));
+    for (X509Certificate::OSCertHandles::iterator it = intermediates.begin();
+         it != intermediates.end();
+         ++it) {
+      --dist_from_root;
+      cache_->CertCache()->Set(*it,
+                               base::Bind(&OnCertWriteIOComplete,
+                                          dist_from_root,
+                                          false /* is not leaf */,
+                                          shared_chain_data));
+    }
+    DCHECK_EQ(0, dist_from_root);
+  }
+
   // When writing headers, we normally only write the non-transient
   // headers; when in record mode, record everything.
   bool skip_transient_headers = (cache_->mode() != RECORD);