Ensure X509Certificate::OSCertHandles are safe to be used on both UI and IO threads on Win

net/base/x509_certificate.h

Index: net/base/x509_certificate.h
diff --git a/net/base/x509_certificate.h b/net/base/x509_certificate.h
index 208d8f61455786b57649746dffaf0d64f0a0b57c..0b46dd222482a1e09eb1a9c5926a5b5519444127 100644
--- a/net/base/x509_certificate.h
+++ b/net/base/x509_certificate.h
@@ -27,8 +27,10 @@
 
 #include "base/synchronization/lock.h"
 #elif defined(USE_OPENSSL)
+#include <openssl/safestack.h>
 // Forward declaration; real one in <x509.h>
-struct x509_st;
+typedef struct x509_st X509;
+PREDECLARE_STACK_OF(X509);
 typedef struct x509_store_st X509_STORE;
 #elif defined(USE_NSS)
 // Forward declaration; real one in <cert.h>
@@ -55,20 +57,41 @@ typedef std::vector<scoped_refptr<X509Certificate> > CertificateList;
 class NET_API X509Certificate
     : public base::RefCountedThreadSafe<X509Certificate> {
  public:
-  // A handle to the certificate object in the underlying crypto library.
-  // We assume that OSCertHandle is a pointer type on all platforms and
-  // NULL is an invalid OSCertHandle.
+  // An OSCertHandle is a handle to the certificate object in the underlying
+  // crypto library. We assume that OSCertHandle is a pointer type on all
+  // platforms and that NULL represents an invalid OSCertHandle.
+  //
+  // An OSCertListHandle is a handle to the underlying crypto library that

[wtc, 2011/10/04 00:26:34]

Add "the object in" before "the underlying crypto library".

+  // represents a collection of certificates, with one of the certificates
+  // marked as an identity certificate and the remaining certificates marked

[wtc, 2011/10/04 00:26:34]

I suggest changing "an identity certificate" to
"a primary certificate" (or "the primary certificate"?).
The meaning of "identity certificate" is not clear here.

[Ryan Sleevi, 2011/10/04 03:38:07]

Does the explanation on line 53-54 provide the necessary context?

[wtc, 2011/10/04 18:00:51]

This is fine.  My complaint about this comment is that
every certificate we deal with here is an identity certificate,
so we need a way to distinguish the cert represented by
os_cert_handle_ and the other certs used for chain building.
This is why I suggested "primary" vs. "supplementary".

+  // as supplementary certificates for path building. Like OSCertHandle, it
+  // is assumed to be a pointer type on all platforms and that NULL
+  // represents an invalid OSCertListHandle.
+  //
+  // It should be noted that depending on the underlying cryptographic

[wtc, 2011/10/04 00:26:34]

Nit: for brevity, change "It should be noted that" to
"Note: " or "Note that", or just remove it.

+  // library, an OSCertHandle or OSCertListHandle may not be thread-safe.

[wtc, 2011/10/04 00:26:34]

Please add a comment to motivate OSCertListHandle.  I think
it is for thread safety.  Since OSCertListHandle may not be
thread-safe, we create an OSCertListHandle for each thread
that needs one.  Do I understand this correctly?

 #if defined(OS_WIN)
   typedef PCCERT_CONTEXT OSCertHandle;
+  // Though the same type as an OSCertHandle, a unique HCERTSTORE member is
+  // used for the certificate containing just the subset of related

[wtc, 2011/10/04 00:26:34]

Add "store" after "certificate".

Change "related" to "supplementary".

+  // certificates.
+  typedef PCCERT_CONTEXT OSCertListHandle;
 #elif defined(OS_MACOSX)
   typedef SecCertificateRef OSCertHandle;
+  typedef CFArrayRef OSCertListHandle;
 #elif defined(USE_OPENSSL)
   typedef struct x509_st* OSCertHandle;

[wtc, 2011/10/04 00:26:34]

Change "struct x509_st" to X509.

+  typedef STACK_OF(X509)* OSCertListHandle;
 #elif defined(USE_NSS)
   typedef struct CERTCertificateStr* OSCertHandle;
+  // TODO(rsleevi): With NSS, it is not currently necessary to use a
+  // separate type, because of how certificate path building/verification is
+  // implemented.
+  typedef OSCertHandle OSCertListHandle;
 #else
   // TODO(ericroman): not implemented
   typedef void* OSCertHandle;
+  typedef void* OSCertListHandle;

[wtc, 2011/10/04 00:26:34]

Nit: it would be nice to use the same style of typedef
when OSCertListHandle is the same as OSCertHandle.

On line 78 and line 94, we duplicate the typedef.
On line 90, we define OSCertListHandle as OSCertHandle.

 #endif
 
   typedef std::vector<OSCertHandle> OSCertHandles;
@@ -235,6 +258,12 @@ class NET_API X509Certificate
   // Returns true if I already contain all the given intermediate certs.
   bool HasIntermediateCertificates(const OSCertHandles& certs);
 
+  // Returns a new OSCertListHandle representing the certificate and any
+  // associated intermediates, or NULL on failure. Ownership is transferred

[wtc, 2011/10/04 00:26:34]

Nit: intermediates => intermediate certificates
                 or   intermediate CA certificates

+  // to the caller and may be released by calling FreeOSCertListHandle()
+  // with the returned value.
+  OSCertListHandle CreateOSCertListHandle() const;
+
 #if defined(OS_MACOSX)
   // Does this certificate's usage allow SSL client authentication?
   bool SupportsSSLClientAuth() const;
@@ -345,6 +374,9 @@ class NET_API X509Certificate
   // Frees (or releases a reference to) an OS certificate handle.
   static void FreeOSCertHandle(OSCertHandle cert_handle);
 
+  // Frees (or releases a reference to) an OS certificate list handle.
+  static void FreeOSCertListHandle(OSCertListHandle cert_list);

[wtc, 2011/10/04 00:26:34]

cert_list => cert_list_handle

+
   // Calculates the SHA-1 fingerprint of the certificate.  Returns an empty
   // (all zero) fingerprint on failure.
   static SHA1Fingerprint CalculateFingerprint(OSCertHandle cert_handle);