Refactor X509Certificate caching to cache the OS handle, rather than the X509Certificate

net/base/x509_certificate_unittest.cc

Index: net/base/x509_certificate_unittest.cc
diff --git a/net/base/x509_certificate_unittest.cc b/net/base/x509_certificate_unittest.cc
index e87ab05331ad17c706f99b0c0058560d7e390049..83ebbd40f94c143354ec1430f455c96129f09e15 100644
--- a/net/base/x509_certificate_unittest.cc
+++ b/net/base/x509_certificate_unittest.cc
@@ -428,7 +428,6 @@ TEST(X509CertificateTest, IntermediateCARequireExplicitPolicy) {
   intermediates.push_back(intermediate_cert->os_cert_handle());
   scoped_refptr<X509Certificate> cert_chain =
       X509Certificate::CreateFromHandle(server_cert->os_cert_handle(),
-                                        X509Certificate::SOURCE_FROM_NETWORK,
                                         intermediates);
 
   int flags = 0;
@@ -439,7 +438,7 @@ TEST(X509CertificateTest, IntermediateCARequireExplicitPolicy) {
   root_certs->Clear();
 }
 
-// Tests X509Certificate::Cache via X509Certificate::CreateFromHandle.  We
+// Tests X509CertificateCache via X509Certificate::CreateFromHandle.  We
 // call X509Certificate::CreateFromHandle several times and observe whether
 // it returns a cached or new X509Certificate object.
 //
@@ -448,57 +447,49 @@ TEST(X509CertificateTest, IntermediateCARequireExplicitPolicy) {
 // come from the network.
 TEST(X509CertificateTest, Cache) {
   X509Certificate::OSCertHandle google_cert_handle;
+  X509Certificate::OSCertHandle thawte_cert_handle;
 
-  // Add a certificate from the source SOURCE_LONE_CERT_IMPORT to our
-  // certificate cache.
+  // Add a single certificate to the certificate cache.
   google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
       reinterpret_cast<const char*>(google_der), sizeof(google_der));
   scoped_refptr<X509Certificate> cert1(X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_LONE_CERT_IMPORT,
-      X509Certificate::OSCertHandles()));
+      google_cert_handle, X509Certificate::OSCertHandles()));
   X509Certificate::FreeOSCertHandle(google_cert_handle);
 
-  // Add a certificate from the same source (SOURCE_LONE_CERT_IMPORT).  This
-  // should return the cached certificate (cert1).
+  // Add the same certificate, but as a new handle.
   google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
       reinterpret_cast<const char*>(google_der), sizeof(google_der));
   scoped_refptr<X509Certificate> cert2(X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_LONE_CERT_IMPORT,
-      X509Certificate::OSCertHandles()));
+      google_cert_handle, X509Certificate::OSCertHandles()));
   X509Certificate::FreeOSCertHandle(google_cert_handle);
 
-  EXPECT_EQ(cert1, cert2);
+  // A new X509Certificate should be returned.
+  EXPECT_NE(cert1.get(), cert2.get());
+  // But both instances should share the underlying OS certificate handle.
+  EXPECT_EQ(cert1->os_cert_handle(), cert2->os_cert_handle());
+  EXPECT_TRUE(cert1->HasIntermediateCertificates(
+      cert2->GetIntermediateCertificates()));
 
-  // Add a certificate from the network.  This should kick out the original
-  // cached certificate (cert1) and return a new certificate.
+  // Add the same certificate, but this time with an intermediate. This
+  // should result in the intermediate being cached. Note that this is not
+  // a legitimate chain, but is suitable for testing.
   google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
       reinterpret_cast<const char*>(google_der), sizeof(google_der));
+  thawte_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
+      reinterpret_cast<const char*>(thawte_der), sizeof(thawte_der));
+  X509Certificate::OSCertHandles intermediates;
+  intermediates.push_back(thawte_cert_handle);
   scoped_refptr<X509Certificate> cert3(X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
-      X509Certificate::OSCertHandles()));
-  X509Certificate::FreeOSCertHandle(google_cert_handle);
-
-  EXPECT_NE(cert1, cert3);
-
-  // Add one certificate from each source.  Both should return the new cached
-  // certificate (cert3).
-  google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
-      reinterpret_cast<const char*>(google_der), sizeof(google_der));
-  scoped_refptr<X509Certificate> cert4(X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
-      X509Certificate::OSCertHandles()));
-  X509Certificate::FreeOSCertHandle(google_cert_handle);
-
-  EXPECT_EQ(cert3, cert4);
-
-  google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
-      reinterpret_cast<const char*>(google_der), sizeof(google_der));
-  scoped_refptr<X509Certificate> cert5(X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
-      X509Certificate::OSCertHandles()));
+      google_cert_handle, intermediates));
   X509Certificate::FreeOSCertHandle(google_cert_handle);
+  X509Certificate::FreeOSCertHandle(thawte_cert_handle);
 
-  EXPECT_EQ(cert3, cert5);
+  // Test that the new certificate, even with intermediates, results in the
+  // same underlying handle being used.
+  EXPECT_EQ(cert1->os_cert_handle(), cert3->os_cert_handle());
+  // Though they use the same OS handle, the intermediates should be different.
+  EXPECT_FALSE(cert1->HasIntermediateCertificates(
+      cert3->GetIntermediateCertificates()));
 }
 
 TEST(X509CertificateTest, Pickle) {
@@ -511,13 +502,8 @@ TEST(X509CertificateTest, Pickle) {
 
   X509Certificate::OSCertHandles intermediates;
   intermediates.push_back(thawte_cert_handle);
-  // Faking SOURCE_LONE_CERT_IMPORT so that when the pickled certificate is
-  // read, it successfully evicts |cert| from the X509Certificate::Cache.
-  // This will be fixed when http://crbug.com/49377 is fixed.
   scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(
-      google_cert_handle,
-      X509Certificate::SOURCE_LONE_CERT_IMPORT,
-      intermediates);
+      google_cert_handle, intermediates);
 
   X509Certificate::FreeOSCertHandle(google_cert_handle);
   X509Certificate::FreeOSCertHandle(thawte_cert_handle);
@@ -573,7 +559,6 @@ TEST(X509CertificateTest, Policy) {
   EXPECT_TRUE(policy.HasDeniedCert());
 }
 
-#if defined(OS_MACOSX) || defined(OS_WIN)
 TEST(X509CertificateTest, IntermediateCertificates) {
   scoped_refptr<X509Certificate> webkit_cert(
       X509Certificate::CreateFromBytes(
@@ -594,8 +579,7 @@ TEST(X509CertificateTest, IntermediateCertificates) {
       reinterpret_cast<const char*>(google_der), sizeof(google_der));
   X509Certificate::OSCertHandles intermediates1;
   scoped_refptr<X509Certificate> cert1;
-  cert1 = X509Certificate::CreateFromHandle(
-      google_handle, X509Certificate::SOURCE_FROM_NETWORK, intermediates1);
+  cert1 = X509Certificate::CreateFromHandle(google_handle, intermediates1);
   EXPECT_TRUE(cert1->HasIntermediateCertificates(intermediates1));
   EXPECT_FALSE(cert1->HasIntermediateCertificate(
       webkit_cert->os_cert_handle()));
@@ -605,8 +589,7 @@ TEST(X509CertificateTest, IntermediateCertificates) {
   intermediates2.push_back(webkit_cert->os_cert_handle());
   intermediates2.push_back(thawte_cert->os_cert_handle());
   scoped_refptr<X509Certificate> cert2;
-  cert2 = X509Certificate::CreateFromHandle(
-      google_handle, X509Certificate::SOURCE_FROM_NETWORK, intermediates2);
+  cert2 = X509Certificate::CreateFromHandle(google_handle, intermediates2);
 
   // The cache should have stored cert2 'cause it has more intermediates:
   EXPECT_NE(cert1, cert2);
@@ -619,20 +602,9 @@ TEST(X509CertificateTest, IntermediateCertificates) {
   EXPECT_FALSE(cert2->HasIntermediateCertificate(
       paypal_cert->os_cert_handle()));
 
-  // Create object with 1 intermediate:
-  X509Certificate::OSCertHandles intermediates3;
-  intermediates2.push_back(thawte_cert->os_cert_handle());
-  scoped_refptr<X509Certificate> cert3;
-  cert3 = X509Certificate::CreateFromHandle(
-      google_handle, X509Certificate::SOURCE_FROM_NETWORK, intermediates3);
-
-  // The cache should have returned cert2 'cause it has more intermediates:
-  EXPECT_EQ(cert3, cert2);
-
   // Cleanup
   X509Certificate::FreeOSCertHandle(google_handle);
 }
-#endif
 
 #if defined(OS_MACOSX)
 TEST(X509CertificateTest, IsIssuedBy) {