Implements new, more robust design for communicating between SSLConnectJobs.

net/socket/ssl_session_cache_openssl.cc

Index: net/socket/ssl_session_cache_openssl.cc
diff --git a/net/socket/ssl_session_cache_openssl.cc b/net/socket/ssl_session_cache_openssl.cc
index d16bb8d6325ec6b105da7219d4e9ea9e5f6f4828..12a8c63d5bb76b992a73bb047bebd88d1b724031 100644
--- a/net/socket/ssl_session_cache_openssl.cc
+++ b/net/socket/ssl_session_cache_openssl.cc
@@ -236,6 +236,34 @@ class SSLSessionCacheOpenSSLImpl {
     return SSL_set_session(ssl, session) == 1;
   }
 
+  bool SSLSessionIsInCache(const std::string& cache_key) const {
+    base::AutoLock locked(lock_);
+    KeyIndex::const_iterator it = key_index_.find(cache_key);
+    if (it == key_index_.end())
+      return false;
+    return true;
+  }
+
+  void NotifyOnSessionAdded(SSL* ssl, const base::Closure& callback) {
+    // Add this SSL* to the SSLtoCallbackMap.
+    std::pair<const base::Closure&, int> temp(callback, 0);
+    ssl_to_callback_map_.insert(SSLToCallbackMap::value_type(ssl, temp));

[wtc, 2014/07/08 01:25:43]

This can be written using the associative array notation"

  ssl_to_callback_map_[ssl] = temp;

Nit: use a more meaningful variable name such as "entry" or "item" instead of
"temp".

[mshelley, 2014/07/09 19:51:02]

I can't use this notation because the callback cannot use the default assignment
operator.

+  }
+
+  // Determines if the session for |ssl| is in the cache, and calls the
+  // appropriate callback if that is the case.
+  void CheckIfSessionAdded(SSL* ssl) {
+    SSLToCallbackMap::iterator it = ssl_to_callback_map_.find(ssl);
+    if (it == ssl_to_callback_map_.end())
+      return;
+    // Increment the session's completion count.
+    (it->second.second)++;

[wtc, 2014/07/08 01:25:43]

Nit: you can omit the parentheses. I know it's not clear whether the struct
member operators (-> and .) or ++ have precedence. You should memorize that
struct member operators have the highest priority.

[mshelley, 2014/07/09 19:51:02]

Done.

+    if (it->second.second == 2) {
+      // The session has been MarkedAsGood and Added, so it can be used.

[wtc, 2014/07/08 01:25:43]

Please document that these can occur in either order.

[mshelley, 2014/07/09 19:51:02]

Done.

+      it->second.first.Run();

[wtc, 2014/07/08 01:25:43]

Should we remove the entry for |ssl| from ssl_to_callback_map_ after running the
callback? Perhaps it is safer to just let the SSLClientSocketOpenSSL destructor
remove the entry?

[mshelley, 2014/07/09 19:51:02]

Done.

+    }
+  }
+
   void MarkSSLSessionAsGood(SSL* ssl) {
     SSL_SESSION* session = SSL_get_session(ssl);
     if (!session)
@@ -244,6 +272,8 @@ class SSLSessionCacheOpenSSLImpl {
     // Mark the session as good, allowing it to be used for future connections.
     SSL_SESSION_set_ex_data(
         session, GetSSLSessionExIndex(), reinterpret_cast<void*>(1));
+
+    CheckIfSessionAdded(ssl);
   }
 
   // Flush all entries from the cache.
@@ -265,6 +295,9 @@ class SSLSessionCacheOpenSSLImpl {
   typedef base::hash_map<std::string, MRUSessionList::iterator> KeyIndex;
   // Type for a dictionary from SessionId values to key index nodes.
   typedef base::hash_map<SessionId, KeyIndex::iterator> SessionIdIndex;
+  // Type for a map from SSL* to associated callbacks
+  typedef std::map<SSL*, std::pair<const base::Closure&, int> >

[wtc, 2014/07/08 01:25:43]

I think we may need to use "base::Closure" instead of "const base::Closure&" in
the std::pair.

A reference is really just syntactic sugar for a pointer. I think it is safer to
for the std::pair to hold a copy of the base::Closure object rather than a
reference. It seems that base::Closure just have two data members: a
scoped_refptr and a function pointer. So it is cheap to copy base::Closure.

[mshelley, 2014/07/09 19:51:02]

Done.

+      SSLToCallbackMap;
 
   // Return the key associated with a given session, or the empty string if
   // none exist. This shall only be used for debugging.
@@ -343,6 +376,7 @@ class SSLSessionCacheOpenSSLImpl {
   // should not decrement its reference count after completion.
   static int NewSessionCallbackStatic(SSL* ssl, SSL_SESSION* session) {
     GetCache(ssl->ctx)->OnSessionAdded(ssl, session);
+    GetCache(ssl->ctx)->CheckIfSessionAdded(ssl);
     return 1;
   }
 
@@ -466,10 +500,11 @@ class SSLSessionCacheOpenSSLImpl {
 
   SSL_CTX* ctx_;
   SSLSessionCacheOpenSSL::Config config_;
+  SSLToCallbackMap ssl_to_callback_map_;
 
   // method to get the index which can later be used with SSL_CTX_get_ex_data()
   // or SSL_CTX_set_ex_data().
-  base::Lock lock_;  // Protects access to containers below.
+  mutable base::Lock lock_;  // Protects access to containers below.
 
   MRUSessionList ordering_;
   KeyIndex key_index_;
@@ -499,6 +534,16 @@ bool SSLSessionCacheOpenSSL::SetSSLSessionWithKey(
   return impl_->SetSSLSessionWithKey(ssl, cache_key);
 }
 
+bool SSLSessionCacheOpenSSL::SSLSessionIsInCache(
+    const std::string& cache_key) const {
+  return impl_->SSLSessionIsInCache(cache_key);
+}
+
+void SSLSessionCacheOpenSSL::NotifyOnSessionAdded(SSL* ssl,
+                                                  const base::Closure& cb) {
+  impl_->NotifyOnSessionAdded(ssl, cb);
+}
+
 void SSLSessionCacheOpenSSL::MarkSSLSessionAsGood(SSL* ssl) {
   return impl_->MarkSSLSessionAsGood(ssl);
 }