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& cb) {
+    // Add this SSL* to the SSLtoCallbackMap.
+    std::pair<const base::Closure&, int> temp(cb, 0);
+    ssl_to_callback_map_.insert(SSLtoCallbackMap::value_type(ssl, temp));

[Ryan Sleevi, 2014/06/26 01:47:16]

Note: You can just use

ssl_to_callback_map_[ssl] = temp; here

[mshelley, 2014/07/01 02:35:23]

So I tried that initially, but received "cannot use default assignment operator"
error -- I assume because I'm mapping to a pair? 
On 2014/06/26 01:47:16, Ryan Sleevi wrote:

+  }
+
+  // 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;  // this shouldn't actually happen ever

[Ryan Sleevi, 2014/06/26 01:47:16]

So then add a NOTREACHED() here.

Why shouldn't it happen? Is that actually part of your SSLSessionCacheOpenSSL
*contract* (eg: it MUST never happen), or is it just an implementation detail of
the Pool?

[mshelley, 2014/07/01 02:35:23]

Oops -- honestly I put that statement/comment in while I was mocking out these
functions and forgot to delete it. The way things are implemented now this
actually can happen. 

On 2014/06/26 01:47:16, Ryan Sleevi wrote:

+
+    // Increment the session's completion count.
+    it->second.second++;
+    if (it->second.second == 2) {
+      // The session has been MarkedAsGood and Added, so it can be used.
+      it->second.first.Run();
+    }
+  }
+
   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/06/27 00:36:50]

What is the "int" in the std::pair?

[mshelley, 2014/07/01 02:35:23]

The int is used to determine whether or not the session has completed. In order
for a connection to technically be complete, it must have passed through
MarkSSLSessionAsGood as well as NewSessionCallbackStatic; each time this happens
I increment the int so that I can see when both methods have been called. 

On 2014/06/27 00:36:50, wtc wrote:

+      SSLtoCallbackMap;

[wtc, 2014/06/27 00:36:50]

Nit: the "to" in "SSLtoCallbackMap" should be capitalized: SSLToCallbackMap.

[mshelley, 2014/07/01 02:35:23]

Done.

 
   // 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) {
     GetCache(ctx)->OnSessionRemoved(session);
   }
 
   // Called by OpenSSL to generate a new session ID. This happens during a
@@ skipping 106 matching lines @@
     DLOG(ERROR) << "Couldn't generate unique session ID of " << id_len
                 << "bytes after " << kMaxTries << " tries.";
     return false;
   }
 
   SSL_CTX* ctx_;
   SSLSessionCacheOpenSSL::Config config_;
 
   // 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_;
 
+  SSLtoCallbackMap ssl_to_callback_map_;

[wtc, 2014/06/27 00:36:50]

Is this member protected by |lock_|?

[mshelley, 2014/07/01 02:35:23]

No it is not...sorry didn't notice the comment above. I'll move this. 

On 2014/06/27 00:36:50, wtc wrote:

+
   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