Implements new, more robust design for communicating between SSLConnectJobs.

net/socket/ssl_session_cache_openssl.cc

 // Copyright 2013 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/socket/ssl_session_cache_openssl.h"
 
 #include <list>
 #include <map>
 
 #include <openssl/rand.h>
@@ skipping 218 matching lines @@
       return false;  // Session has not yet been marked good. Treat as a miss.
 
     // Move to front of MRU list.
     ordering_.push_front(session);
     ordering_.erase(it->second);
     it->second = ordering_.begin();
 
     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)
       return;
 
     // 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.
   void Flush() {
     base::AutoLock lock(lock_);
     id_index_.clear();
     key_index_.clear();
     while (!ordering_.empty()) {
       SSL_SESSION* session = ordering_.front();
       ordering_.pop_front();
       SSL_SESSION_free(session);
     }
   }
 
  private:
   // Type for list of SSL_SESSION handles, ordered in MRU order.
   typedef std::list<SSL_SESSION*> MRUSessionList;
   // Type for a dictionary from unique cache keys to session list nodes.
   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.
   std::string SessionKey(SSL_SESSION* session) {
     if (!session)
       return std::string("<null-session>");
 
     if (session->session_id_length == 0)
       return std::string("<empty-session-id>");
 
@@ skipping 58 matching lines @@
     return reinterpret_cast<SSLSessionCacheOpenSSLImpl*>(result);
   }
 
   // Called by OpenSSL when a new |session| was created and added to a given
   // |ssl| connection. Note that the session's reference count was already
   // incremented before the function is entered. The function must return 1
   // to indicate that it took ownership of the session, i.e. that the caller
   // 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;
   }
 
   // Called by OpenSSL to indicate that a session must be removed from the
   // cache. This happens when SSL_CTX is destroyed.
   static void RemoveSessionCallbackStatic(SSL_CTX* ctx, SSL_SESSION* session) {

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

Should we decrement the session's completion count here?

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

My initial thought would be that it's best to just remove the entry from the map
here. However, I just realized that at this point I no longer have access to the
session's SSL*, which is how I indexed the map. As a result, I can neither
delete the entry nor decrement its completion count at this point.

I believe the only situation in which this could be problematic would be if the
leading connect job's session was added to the cache, and then removed from the
cache before the pending connections following it completed their connections.
My understanding is that this probably wouldn't ever happen, or at least would
happen only rarely?

However, even if this did occur, the result would just be that some of the
pending connections would be unable to use the resumption handshake. 

     GetCache(ctx)->OnSessionRemoved(session);
   }
 
   // Called by OpenSSL to generate a new session ID. This happens during a
   // SSL connection operation, when the SSL object doesn't have a session yet.
   //
   // A session ID is a random string of bytes used to uniquely identify the
   // session between a client and a server.
   //
   // |ssl| is a SSL connection handle. Ignored here.
@@ skipping 97 matching lines @@
       if (id_index_.find(SessionId(id, id_len)) == id_index_.end())
         return true;
     }
     DLOG(ERROR) << "Couldn't generate unique session ID of " << id_len
                 << "bytes after " << kMaxTries << " tries.";
     return false;
   }
 
   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_;
   SessionIdIndex id_index_;
 
   size_t expiration_check_;
 };
 
 SSLSessionCacheOpenSSL::~SSLSessionCacheOpenSSL() { delete impl_; }
 
 size_t SSLSessionCacheOpenSSL::size() const { return impl_->size(); }
 
 void SSLSessionCacheOpenSSL::Reset(SSL_CTX* ctx, const Config& config) {
   if (impl_)
     delete impl_;
 
   impl_ = new SSLSessionCacheOpenSSLImpl(ctx, config);
 }
 
 bool SSLSessionCacheOpenSSL::SetSSLSession(SSL* ssl) {
   return impl_->SetSSLSession(ssl);
 }
 
 bool SSLSessionCacheOpenSSL::SetSSLSessionWithKey(
     SSL* ssl,
     const std::string& cache_key) {
   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);
 }
 
 void SSLSessionCacheOpenSSL::Flush() { impl_->Flush(); }
 
 }  // namespace net