This CL corrects a bug in which the OnHandshakeComplete callback for an ssl session was never called

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 239 matching lines @@
         SSL_SESSION_get_ex_data(session, GetSSLSessionExIndex());
 
     return session_is_good;
   }
 
   void SetSessionAddedCallback(SSL* ssl, const base::Closure& callback) {
     // Add this SSL* to the SSLtoCallbackMap.
     ssl_to_callback_map_.insert(SSLToCallbackMap::value_type(
         ssl, CallbackAndCompletionCount(callback, 0)));
   }
 

[davidben, 2014/07/29 19:16:01]

The completion count thing is a little non-obvious on first read. Maybe add a
comment like:

// CheckIfSessionFinished is called twice per connection, once in
MarkSSLSessionAsGood, when the certificate has been verified, and once when the
handshake has completed. On the second call, the connection's handshake
completion callback is run.

[mshelley, 2014/08/05 23:17:11]

Done.

-  // Determines if the session for |ssl| is in the cache, and calls the
-  // appropriate callback if that is the case.
-  void CheckIfSessionFinished(SSL* ssl) {
+  void CheckIfSessionFinished(const 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.
     if (++it->second.count == 2) {
       // The session has been MarkedAsGood and Added, so it can be used.
       // These two events can occur in either order.
       base::Closure callback = it->second.callback;
       ssl_to_callback_map_.erase(it);
       callback.Run();
@@ skipping 46 matching lines @@
     int count;
   };
 
   // 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*, CallbackAndCompletionCount> SSLToCallbackMap;
+  typedef std::map<const SSL*, CallbackAndCompletionCount> 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 59 matching lines @@
   }
 
   // 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) {
     SSLSessionCacheOpenSSLImpl* cache = GetCache(ssl->ctx);
     cache->OnSessionAdded(ssl, session);
-    cache->CheckIfSessionFinished(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 103 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_;

[davidben, 2014/07/29 19:16:01]

Now that the completion hook comes from SSLClientSocketOpenSSL, instead of
having the callback live in ssl_session_cache_openssl.cc, perhaps the callback
and state could live in SSLClientSocketOpenSSL. Then you don't need the map. So
you'd call CheckIfSessionFinished right after calling MarkSessionAsGood in
DoVerifyComplete and then also call it directly in OnSessionFinishedCallback.
Might be simpler; you wouldn't have to go through this session cache object at
all.

I'll defer to Ryan as to whether that should be done here or in a follow-up.

[mshelley, 2014/08/05 23:17:11]

Done.

 
   // method to get the index which can later be used with SSL_CTX_get_ex_data()
   // or SSL_CTX_set_ex_data().
   mutable base::Lock lock_;  // Protects access to containers below.
 
   MRUSessionList ordering_;
   KeyIndex key_index_;
   SessionIdIndex id_index_;
 
   size_t expiration_check_;
@@ skipping 18 matching lines @@
     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::CheckIfSessionFinished(const SSL* ssl) {
+  impl_->CheckIfSessionFinished(ssl);
+}
+
 void SSLSessionCacheOpenSSL::RemoveSessionAddedCallback(SSL* ssl) {
   impl_->RemoveSessionAddedCallback(ssl);
 }
 
 void SSLSessionCacheOpenSSL::SetSessionAddedCallback(SSL* ssl,
                                                      const base::Closure& cb) {
   impl_->SetSessionAddedCallback(ssl, cb);
 }
 
 void SSLSessionCacheOpenSSL::MarkSSLSessionAsGood(SSL* ssl) {
   return impl_->MarkSSLSessionAsGood(ssl);
 }
 
 void SSLSessionCacheOpenSSL::Flush() { impl_->Flush(); }
 
 }  // namespace net