Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
328903004:
SSL Connect Job Waiting (Closed)
|
Created:
6 years, 6 months ago by mshelley Modified:
6 years, 5 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionThis CL alters the way in which multiple sockets connect to the same server so that only one complete SSL handshake must be carried out.
This is accomplished by preventing all but one of the parallel connections from proceeding until the first connection has completed. As a result, the remaining connections will be able to us an abbreviated SSL Handshake.
Patch Set 1 : Rough CL that holds back SSLConnectjobs when a connection is already in progress. Note: This is DEF… #Patch Set 2 : Fixed error where next_state_ was not set in WAIT stage, and added a command line flag to enable my… #Patch Set 3 : Added a command line flag that enables my changes. #
Total comments: 56
Patch Set 4 : Fixed various issues #Patch Set 5 : Redesigned cache accessing functions. #
Total comments: 60
Patch Set 6 : Fixed bugs related to memory management and early exits in DoSSLConnectComplete #
Total comments: 39
Patch Set 7 : Fixed memory leak issues. #
Total comments: 19
Patch Set 8 : Fixed commenting issues and prevented PendingJobs from modifying pending_jobs_. #
Total comments: 8
Patch Set 9 : Add separate state for ProcessPendingJobs #
Total comments: 12
Messages
Total messages: 20 (0 generated)
This is definitely unpolished/buggy. I know of one issue right now that only happens the first time I connect to some SSL sites -- I'm working on figuring that out. It has to do with next_state_ being equal to STATE_NONE for some reason when GetLoadState() is called.
Review comments on patch set 3: I haven't reviewed the meat of this CL in ssl_client_socket_openssl.cc yet, so most of my comments are about coding style issues. https://codereview.chromium.org/328903004/diff/100001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/328903004/diff/100001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:504: const char kEnableJobWaiting[] = "enable-job-waiting"; This option should be "enable-ssl-connect-job-waiting" or something like that. It must include "ssl". https://codereview.chromium.org/328903004/diff/100001/chrome/common/chrome_sw... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/328903004/diff/100001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:149: extern const char kEnableJobWaiting[]; This file is not an SSL file, so this variable name needs to include SSL, for example, kEnableSSLConnectJobWaiting. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket.cc:107: // We expect to invoke this method in the SSLClientSocketOpenSSL This is a static method. Virtual methods cannot be static. So if we want to have a SSLClientSocket::InSessionCache method, it needs to be defined like this: bool SSLClientSocket::InSessionCache(const std::string& cache_key) { #if defined(USE_OPENSSL) return SSLClientSocketSSL::InSessionCache(cache_key); #else NOTREACHED(); // You can also use the NOTIMPLEMENTED() macro. return false; #endif } https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:84: static bool InSessionCache(std::string cache_key); The input parameter should be const reference. Please document the method. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:369: bool SSLClientSocketOpenSSL::InSessionCache(std::string cache_key) { Nit: The Chromium convention often (but not always) adds a "// static" comment in front of the definition of a static member or method, so this would look like: // static bool SSLClientSocketOpenSSL::InSessionCache(const std::string& cache_key) { https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:42: class NET_EXPORT SSLClientSocketOpenSSL : public SSLClientSocket { Use NET_EXPORT_PRIVATE if it is exported for net_unittests only. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:60: static bool InSessionCache(std::string cache_key); 1. This method should not be in the "SSLClientSocket implementation" section because it is not a virtual method of SSLClientSocket. 2. The |cache_key| parameter should be a const reference. 3. Please document this method. In particular, the format of |cache_key|. 4. An alternative design is to add a static getter method that returns a pointer to the session cache. But I guess that is overly OpenSSL-specific? (There is no stand-alone session cache class for SSLClientSocketNSS.) https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:21: #include "net/socket/ssl_client_socket_openssl.h" This needs to be put inside #if defined(USE_OPENSSL). Chromium style recommends listing the conditional #include's last, after the cross-platform #include's, so add the following after line 25: #if defined(USE_OPENSSL) #include "net/socket/ssl_client_socket_openssl.h" #endif See the Chromium Coding Style page: http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:120: static bool enable_job_waiting_; This (static) data member should be private. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:138: STATE_WAIT, Nit: this state's name should be more informative. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:186: PendingJobList* pending_; Rename this member pending_job_list_ or pending_jobs_. pending_ is a little confusing because it looks like a boolean member. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_session_... File net/socket/ssl_session_cache_openssl.cc (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_session_... net/socket/ssl_session_cache_openssl.cc:239: bool SessionIsInCache(const std::string& cache_key) { Mark this method const. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_session_... File net/socket/ssl_session_cache_openssl.h (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_session_... net/socket/ssl_session_cache_openssl.h:117: // in the session cache. Nit: the documentation should ideally not refer to the intended user (SSLConnectJob). I think we can just delete this sentence. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_session_... net/socket/ssl_session_cache_openssl.h:119: // Return true iff a cached session was associated with the given cache_key. Nit: put code in ||, for example, |cache_key|. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_session_... net/socket/ssl_session_cache_openssl.h:120: bool SessionIsInCache(const std::string& cache_key); Nit: Mark this method const: bool SessionIsInCache(const std::string& cache_key) const;
Note: When sending reviews, use my "rsleevi" alias (as it suggests) This is just a quick stab at some design-level feedback; I haven't had a chance to review style in depth (I see Wan-Teh has taken a look), nor at the actual implementation logic. Most of this relates to coupling / separation of responsibilities, and is just some off the cuff thoughts. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket.cc:107: // We expect to invoke this method in the SSLClientSocketOpenSSL On 2014/06/13 22:47:24, wtc wrote: > > This is a static method. Virtual methods cannot be static. So if we want to have > a SSLClientSocket::InSessionCache method, it needs to be defined like this: > > bool SSLClientSocket::InSessionCache(const std::string& cache_key) { > #if defined(USE_OPENSSL) > return SSLClientSocketSSL::InSessionCache(cache_key); > #else > NOTREACHED(); // You can also use the NOTIMPLEMENTED() macro. > return false; > #endif > } Note that while Wan-Teh's suggestion is a valid approach, we've tried to avoid coupling SSLClientSocket with it's derived implementations, because it creates a circular dependency. That is, SSLClientSocket would now need SSLClientSocketOpenSSL in order to implement its required features, and SSLClientSocketOpenSSL already needs SSLClientSocket in order to implement its required features. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:84: static bool InSessionCache(std::string cache_key); On 2014/06/13 22:47:24, wtc wrote: > > The input parameter should be const reference. > > Please document the method. First reaction: I generally reaction negatively against new static methods, because they imply global state (in this case, it tightly couples the interface of SSLClientSocket to the idea of a 'global' SSL session cache) However, what we 'want' is a per-profile SSL session cache, so that as profiles are deleted, we evict old sessions. As currently implemented, we actually hold on to the memory for these structures even after the Profile is deleted. The only reason this hasn't killed us on memory usage (yet) is the eviction rate of Chrome, but it's something we should fix. As such, the idea of what session cache a given socket belongs to is likely something that the SSLClientSocketPool would pass to the SSLConnectJob - the Pool would get its value from Params (which would come from the ProfileIOData in Chrome), would pass just that value to the Job, and the Job would pass that to the Socket. That would get rid of the |ssl_session_cache_shard_arg| in the SSLClientSocketContext, in favor of the new structure. The other "coupling smell" is that the computing of |cache_key| is done internal to the SSLClientSocket, as part of Connect() [eg: when it has a host/port], whereas this method expects callers to be able to recreate that same logic. We would likely want to keep that centralized. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:132: callback_( nit: We typically call this io_callback_ https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:220: rv = ERR_IO_PENDING; First Reaction: This is weird to have it as an state transition, since it only serves to set the next transition. Normally, you would have DoCheckForResume() set this state, and return ERR_IO_PENDING there. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:255: if (result == OK) { We try to handle errors before success cases; if (result != OK) return result; if (enable_job_waiting_) next_state_ = STATE_CHECK_FOR_RESUME; else next_state_ = STATE_SSL_CONNECT; return result; https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:285: } Ditto ordering https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:323: next_state_ = STATE_SSL_CONNECT; STYLE: This is now the third time this conditional has been duplicated. It suggests there should be a private helper function, to ensure state transitions are the same (eg: it's the only one that should ever set state = STATE_SSL_CONNECT, presumably) next_state_ = GetNextConnectState() ? https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:338: return OK; This should be ERR_IO_PENDING https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:498: if (enable_job_waiting_) { This should probably all be moved into a helper function; it's a discrete set of functionality tied to this job_waiting_, so centralizing it (and documenting what it does) is a good structural cleanup. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:652: pending_job_list = (*pending_)[group_name]; This is actually somewhat inefficient; it does two scans of the map's red-black-tree You can do std::pair<PendingJobMap::iterator, bool> it = pending_->insert(PendingJobMap::value_type(group_name, new SSLConnectJob::PendingJobList())); pending_job_list = it->first.second; If the PendingJobMap::value_type constructor doesn't work, std::pair<const std::string, SSLConnectJob::PendingJobList*>(...) or PendingJobMap::value_type value(group_name, new SSLConnectJob::PendingJobList()); ... it = pending_->insert(value); https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:654: pending_job_list = p->second; If one portion of an if statement includes braces, the other conditions should too. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:105: SSLConnectJob(const std::string& group_name, New line before the constructor https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:159: void DoSSL(); First reaction: This needs a better (more descriptive) name, preferably one that is self-documenting. e.g.: it's not clear what this means - is this just do the SSL handshake? If so, how does it differ from DoSSLConnect/DoSSLConnectComplete https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:283: typedef std::map<std::string, SSLConnectJob::PendingJobList*> PendingJobMap; You don't need to make this typedef public, because it's an internal implementation detail (that is, the symbol is only used as a private data member) You'd place this between lines 304/305, and then add a newline before the class data members
I corrected all of the more concrete issues. I'm currently looking into correcting the design smells regarding how I access the session cache that Ryan mentioned. https://codereview.chromium.org/328903004/diff/100001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/328903004/diff/100001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:504: const char kEnableJobWaiting[] = "enable-job-waiting"; On 2014/06/13 22:47:24, wtc wrote: > > This option should be "enable-ssl-connect-job-waiting" or something like that. > It must include "ssl". Done. https://codereview.chromium.org/328903004/diff/100001/chrome/common/chrome_sw... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/328903004/diff/100001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:149: extern const char kEnableJobWaiting[]; On 2014/06/13 22:47:24, wtc wrote: > > This file is not an SSL file, so this variable name needs to include SSL, for > example, kEnableSSLConnectJobWaiting. Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket.cc:107: // We expect to invoke this method in the SSLClientSocketOpenSSL On 2014/06/13 22:47:24, wtc wrote: > > This is a static method. Virtual methods cannot be static. So if we want to have > a SSLClientSocket::InSessionCache method, it needs to be defined like this: > > bool SSLClientSocket::InSessionCache(const std::string& cache_key) { > #if defined(USE_OPENSSL) > return SSLClientSocketSSL::InSessionCache(cache_key); > #else > NOTREACHED(); // You can also use the NOTIMPLEMENTED() macro. > return false; > #endif > } Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:84: static bool InSessionCache(std::string cache_key); On 2014/06/13 22:47:24, wtc wrote: > > The input parameter should be const reference. > > Please document the method. Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:369: bool SSLClientSocketOpenSSL::InSessionCache(std::string cache_key) { On 2014/06/13 22:47:24, wtc wrote: > > Nit: The Chromium convention often (but not always) adds a "// static" comment > in front of the definition of a static member or method, so this would look > like: > > // static > bool SSLClientSocketOpenSSL::InSessionCache(const std::string& cache_key) { Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:42: class NET_EXPORT SSLClientSocketOpenSSL : public SSLClientSocket { I think I included NET_EXPORT when I was attempting to solve a linker error with the InSessionCache function, and forgot to take it out after I fixed it. As it turns out I don't think I need it at all. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:60: static bool InSessionCache(std::string cache_key); On 2014/06/13 22:47:24, wtc wrote: > > 1. This method should not be in the "SSLClientSocket implementation" section > because it is not a virtual method of SSLClientSocket. > > 2. The |cache_key| parameter should be a const reference. > > 3. Please document this method. In particular, the format of |cache_key|. > > 4. An alternative design is to add a static getter method that returns a pointer > to the session cache. But I guess that is overly OpenSSL-specific? (There is no > stand-alone session cache class for SSLClientSocketNSS.) Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:21: #include "net/socket/ssl_client_socket_openssl.h" On 2014/06/13 22:47:24, wtc wrote: > > This needs to be put inside #if defined(USE_OPENSSL). > > Chromium style recommends listing the conditional #include's last, after the > cross-platform #include's, so add the following after line 25: > > #if defined(USE_OPENSSL) > #include "net/socket/ssl_client_socket_openssl.h" > #endif > > See the Chromium Coding Style page: > http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:132: callback_( On 2014/06/13 23:24:22, Ryan Sleevi wrote: > nit: We typically call this io_callback_ Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:220: rv = ERR_IO_PENDING; On 2014/06/13 23:24:22, Ryan Sleevi wrote: > First Reaction: This is weird to have it as an state transition, since it only > serves to set the next transition. > > Normally, you would have DoCheckForResume() set this state, and return > ERR_IO_PENDING there. Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:255: if (result == OK) { On 2014/06/13 23:24:22, Ryan Sleevi wrote: > We try to handle errors before success cases; > > if (result != OK) > return result; > > if (enable_job_waiting_) > next_state_ = STATE_CHECK_FOR_RESUME; > else > next_state_ = STATE_SSL_CONNECT; > > return result; Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:285: } On 2014/06/13 23:24:22, Ryan Sleevi wrote: > Ditto ordering Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:323: next_state_ = STATE_SSL_CONNECT; On 2014/06/13 23:24:22, Ryan Sleevi wrote: > STYLE: This is now the third time this conditional has been duplicated. > > It suggests there should be a private helper function, to ensure state > transitions are the same (eg: it's the only one that should ever set state = > STATE_SSL_CONNECT, presumably) > > next_state_ = GetNextConnectState() ? Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:338: return OK; On 2014/06/13 23:24:22, Ryan Sleevi wrote: > This should be ERR_IO_PENDING Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:498: if (enable_job_waiting_) { On 2014/06/13 23:24:22, Ryan Sleevi wrote: > This should probably all be moved into a helper function; it's a discrete set of > functionality tied to this job_waiting_, so centralizing it (and documenting > what it does) is a good structural cleanup. Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:652: pending_job_list = (*pending_)[group_name]; On 2014/06/13 23:24:22, Ryan Sleevi wrote: > This is actually somewhat inefficient; it does two scans of the map's > red-black-tree > > You can do > > std::pair<PendingJobMap::iterator, bool> it = > pending_->insert(PendingJobMap::value_type(group_name, new > SSLConnectJob::PendingJobList())); > pending_job_list = it->first.second; > > > If the PendingJobMap::value_type constructor doesn't work, std::pair<const > std::string, SSLConnectJob::PendingJobList*>(...) > or > > PendingJobMap::value_type value(group_name, new > SSLConnectJob::PendingJobList()); > ... it = pending_->insert(value); Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:654: pending_job_list = p->second; On 2014/06/13 23:24:22, Ryan Sleevi wrote: > If one portion of an if statement includes braces, the other conditions should > too. Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:105: SSLConnectJob(const std::string& group_name, On 2014/06/13 23:24:22, Ryan Sleevi wrote: > New line before the constructor Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:120: static bool enable_job_waiting_; On 2014/06/13 22:47:24, wtc wrote: > > This (static) data member should be private. Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:138: STATE_WAIT, On 2014/06/13 22:47:24, wtc wrote: > > Nit: this state's name should be more informative. Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:159: void DoSSL(); On 2014/06/13 23:24:22, Ryan Sleevi wrote: > First reaction: This needs a better (more descriptive) name, preferably one that > is self-documenting. e.g.: it's not clear what this means - is this just do the > SSL handshake? If so, how does it differ from DoSSLConnect/DoSSLConnectComplete Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:186: PendingJobList* pending_; On 2014/06/13 22:47:24, wtc wrote: > > Rename this member pending_job_list_ or pending_jobs_. pending_ is a little > confusing because it looks like a boolean member. Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:283: typedef std::map<std::string, SSLConnectJob::PendingJobList*> PendingJobMap; On 2014/06/13 23:24:22, Ryan Sleevi wrote: > You don't need to make this typedef public, because it's an internal > implementation detail (that is, the symbol is only used as a private data > member) > > You'd place this between lines 304/305, and then add a newline before the class > data members Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_session_... File net/socket/ssl_session_cache_openssl.cc (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_session_... net/socket/ssl_session_cache_openssl.cc:239: bool SessionIsInCache(const std::string& cache_key) { On 2014/06/13 22:47:24, wtc wrote: > > Mark this method const. Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_session_... File net/socket/ssl_session_cache_openssl.h (right): https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_session_... net/socket/ssl_session_cache_openssl.h:117: // in the session cache. On 2014/06/13 22:47:24, wtc wrote: > > Nit: the documentation should ideally not refer to the intended user > (SSLConnectJob). I think we can just delete this sentence. Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_session_... net/socket/ssl_session_cache_openssl.h:119: // Return true iff a cached session was associated with the given cache_key. On 2014/06/13 22:47:24, wtc wrote: > > Nit: put code in ||, for example, |cache_key|. Done. https://codereview.chromium.org/328903004/diff/100001/net/socket/ssl_session_... net/socket/ssl_session_cache_openssl.h:120: bool SessionIsInCache(const std::string& cache_key); On 2014/06/13 22:47:24, wtc wrote: > > Nit: Mark this method const: > > bool SessionIsInCache(const std::string& cache_key) const; Done.
This patch implements a different method of accessing the session cache that avoids using a static member function (as suggested by Ryan's comments)
Review comments on patch set 5: The CL looks much better. I reviewed the whole CL this time. I believe the more serious problems are all in ssl_client_socket_pool.cc. I marked them with "BUG" or "IMPORTANT". You may want to address those comments first. https://codereview.chromium.org/328903004/diff/180001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/328903004/diff/180001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:503: // maximize the number of full SSL handshakes completed. Nit: Allows => Enables Typo: maximize => minimize Please also move this to the right place in alphabetical order. https://codereview.chromium.org/328903004/diff/180001/chrome/common/chrome_sw... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/328903004/diff/180001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:149: extern const char kEnableSSLConnectJobWaiting[]; Nit: move this constant to the right place in alphabetical order, after we renamed it. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket.cc:10: #include "net/socket/ssl_client_socket_openssl.h" Remove this. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:86: virtual bool InSessionCache(); Nit: this method probably can be declared as const. I think this method should be declared as abstract (= 0) in this header. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:63: // concatenated with its session cache shard. This comment should be moved to ssl_client_socket.h. In a method of a subclass that overrides a method of a base class, the comment is usually omitted. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:64: virtual bool InSessionCache(); Move this method down to the "SSLClientSocket implementation" section. Add the "OVERRIDE" keyword. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:138: bool SSLConnectJob::enable_job_waiting_ = false; Nit: add a // static comment before this line. (This convention is not universally followed in the Chromium source tree though.) Nit: we seem to initialize static data members before the constructors. I didn't find any recommendation about this in the Style Guide. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:152: case STATE_CHECK_FOR_RESUME: Should we also add the STATE_CREATE_SSL_SOCKET state to this switch statement? https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:277: } Nit: omit the curly braces if the if statement's condition and body are just one line. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:345: // If the group is in the cache, continue. This comment should point out that this SSL handshake will be a session resumption handshake. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:358: // If there are no pending jobs, continue Nit: it would be good to elaborate in this comment. Why do we continue in this case? I guess this job would be the leading job? https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:368: // as not to include time spent waiting for an idle socket. This comment was copied here by mistake. Please move it back to line 331, before the line connect_timing_.connect_start = socket_connect_timing.connect_start; https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:403: return ERR_NPN_NEGOTIATION_FAILED; IMPORTANT: Do we need to wake up the pending jobs in this case? We need to check every return statement in the DoSSLConnectComplete function and see what we should do about the pending jobs before returning. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:499: } Nit: omit curly braces. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:508: PendingJobList::iterator i = pending_jobs_->begin(); Nit: name the iterator |it| or |iter|. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:524: else On lines 519 and 524, the else should follow the preceding closing '}'. This requires moving the comments inside the blocks, like this: pending_jobs_->clear(); } else if (i + 1 != pending_jobs_->end()) { // If the connection failed, tell only one job to proceed as the new // leader. ... } else { // If there are no more jobs and the leader failed, delete the entry. pending_jobs_->erase(i); } Note that the final "else" uses curly braces even though its body has only one line. In an if-else-if-else statement, if any branch needs curly braces, all branches should use curly braces. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:525: pending_jobs_->erase(i); Nit: I think this if-else-if-else statement can be simplified. An easy simplification is to combine the second and third cases: if (result == OK) { the first case, unchanged } else { pending_jobs_->erase(i++); if (i != pending_jobs_->end()) (*i)->ResumeSSLConnection(); } If we work harder, this should also work: pending_jobs_->erase(i++); if (result == OK) { for (; i != pending_jobs_->end(); ++i) (*i)->ResumeSSLConnection(); pending_jobs_->clear(); } else if (i != pending_jobs_->end()) { (*i)->ResumeSSLConnection(); } Try to come up with a control structure that's both simple and easy to understand. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:530: next_state_ = STATE_SSL_CONNECT; Please add DCHECK_EQ(next_state_, STATE_CHECK_FOR_RESUME); before this line. But I think we usually handle this by directly setting next_state_ = STATE_SSL_CONNECT; on line 354. Then we don't need to change next_state_ here. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:573: pending_jobs_(new PendingJobMap()) { BUG: this PendingJobMap object is leaked. We should declare this member as a non-pointer in the .h file: PendingJobMap pending_jobs_; https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:653: group_name, new SSLConnectJob::PendingJobList())); IMPORTANT: it seems wrong to create a new SSLConnectJob::PendingJobList every time. It seems that we should first find an existing SSLConnectJob::PendingJobList in pending_jobs_. If none exsts, then create a new one. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:104: typedef std::vector<SSLConnectJob*> PendingJobList; Nit: it would be nice to document this type. For example, are the jobs in the list supposed to be connecting to the same server? Are the jobs in the list suspended? Does the owner of this list need to destroy the jobs? https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:118: PendingJobList* pending); 1. Nit: I suggest naming this parameter "pending_jobs" or "pending_job_list". "pending" sounds like the name of a boolean parameter. 2. I suggest moving this parameter before the "delegate" parameter. By convention, the delegate/callback and net_log parameters are usually listed last in the net code. 3. Please document whether this object owns the pending job list. If not, document that the pending job list must outlive this object. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:126: static NET_EXPORT void EnableJobWaiting(bool enable); Nit: please document this method. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:162: void ResumeSSLConnection(); Please document this method. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:163: State GetNextConnectState(bool enabled); Delete this unused method declaration. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:165: std::string GetSessionCacheKey(); Delete this unused method. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:309: typedef std::map<std::string, SSLConnectJob::PendingJobList*> PendingJobMap; Please document this map. In particular, what is the std::string key. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:319: PendingJobMap* pending_jobs_; Please document who owns pending_jobs_. If SSLClientSocketPool owns it, it probably should be declared as PendingJobMap pending_jobs_; or scoped_ptr<PendingJobMap> pending_jobs_; Nit: the member name probably should have "map" in it. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_session_... File net/socket/ssl_session_cache_openssl.h (right): https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_session_... net/socket/ssl_session_cache_openssl.h:117: bool SessionIsInCache(const std::string& cache_key) const; Nit: add "SSL" before "Session" because the other methods use "SSLSession".
https://codereview.chromium.org/328903004/diff/180001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/328903004/diff/180001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:503: // maximize the number of full SSL handshakes completed. On 2014/06/17 17:44:39, wtc wrote: > > Nit: Allows => Enables > > Typo: maximize => minimize > > Please also move this to the right place in alphabetical order. Done. https://codereview.chromium.org/328903004/diff/180001/chrome/common/chrome_sw... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/328903004/diff/180001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:149: extern const char kEnableSSLConnectJobWaiting[]; On 2014/06/17 17:44:39, wtc wrote: > > Nit: move this constant to the right place in alphabetical order, after we > renamed it. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket.cc:10: #include "net/socket/ssl_client_socket_openssl.h" On 2014/06/17 17:44:39, wtc wrote: > > Remove this. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:86: virtual bool InSessionCache(); On 2014/06/17 17:44:39, wtc wrote: > > Nit: this method probably can be declared as const. > > I think this method should be declared as abstract (= 0) in this header. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:63: // concatenated with its session cache shard. On 2014/06/17 17:44:40, wtc wrote: > > This comment should be moved to ssl_client_socket.h. In a method of a subclass > that overrides a method of a base class, the comment is usually omitted. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:64: virtual bool InSessionCache(); On 2014/06/17 17:44:40, wtc wrote: > > Move this method down to the "SSLClientSocket implementation" section. Add the > "OVERRIDE" keyword. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:138: bool SSLConnectJob::enable_job_waiting_ = false; On 2014/06/17 17:44:40, wtc wrote: > > Nit: add a > // static > comment before this line. (This convention is not universally followed in the > Chromium source tree though.) > > Nit: we seem to initialize static data members before the constructors. I didn't > find any recommendation about this in the Style Guide. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:152: case STATE_CHECK_FOR_RESUME: On 2014/06/17 17:44:40, wtc wrote: > > Should we also add the STATE_CREATE_SSL_SOCKET state to this switch statement? Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:277: } On 2014/06/17 17:44:40, wtc wrote: > > Nit: omit the curly braces if the if statement's condition and body are just one > line. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:345: // If the group is in the cache, continue. On 2014/06/17 17:44:40, wtc wrote: > > This comment should point out that this SSL handshake will be a session > resumption handshake. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:358: // If there are no pending jobs, continue On 2014/06/17 17:44:40, wtc wrote: > > Nit: it would be good to elaborate in this comment. Why do we continue in this > case? I guess this job would be the leading job? Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:368: // as not to include time spent waiting for an idle socket. On 2014/06/17 17:44:40, wtc wrote: > > This comment was copied here by mistake. Please move it back to line 331, before > the line > connect_timing_.connect_start = socket_connect_timing.connect_start; Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:403: return ERR_NPN_NEGOTIATION_FAILED; On 2014/06/17 17:44:40, wtc wrote: > > IMPORTANT: Do we need to wake up the pending jobs in this case? > > We need to check every return statement in the DoSSLConnectComplete function and > see what we should do about the pending jobs before returning. Yes we do -- I believe I can just call the ProcessPendingJobs function to correctly handle it. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:499: } On 2014/06/17 17:44:40, wtc wrote: > > Nit: omit curly braces. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:508: PendingJobList::iterator i = pending_jobs_->begin(); On 2014/06/17 17:44:40, wtc wrote: > > Nit: name the iterator |it| or |iter|. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:524: else On 2014/06/17 17:44:40, wtc wrote: > > On lines 519 and 524, the else should follow the preceding closing '}'. This > requires moving the comments inside the blocks, like this: > > pending_jobs_->clear(); > } else if (i + 1 != pending_jobs_->end()) { > // If the connection failed, tell only one job to proceed as the new > // leader. > ... > } else { > // If there are no more jobs and the leader failed, delete the entry. > pending_jobs_->erase(i); > } > > Note that the final "else" uses curly braces even though its body has only one > line. In an if-else-if-else statement, if any branch needs curly braces, all > branches should use curly braces. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:525: pending_jobs_->erase(i); On 2014/06/17 17:44:40, wtc wrote: > > Nit: I think this if-else-if-else statement can be simplified. An easy > simplification is to combine the second and third cases: > > if (result == OK) { > the first case, unchanged > } else { > pending_jobs_->erase(i++); > if (i != pending_jobs_->end()) > (*i)->ResumeSSLConnection(); > } > > If we work harder, this should also work: > > pending_jobs_->erase(i++); > if (result == OK) { > for (; i != pending_jobs_->end(); ++i) > (*i)->ResumeSSLConnection(); > pending_jobs_->clear(); > } else if (i != pending_jobs_->end()) { > (*i)->ResumeSSLConnection(); > } > > Try to come up with a control structure that's both simple and easy to > understand. I decided to go with your first suggestion. The second suggestion led to an error -- I think because erasing i invalidates i++, but I'm not completely sure. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:530: next_state_ = STATE_SSL_CONNECT; On 2014/06/17 17:44:40, wtc wrote: > > Please add > DCHECK_EQ(next_state_, STATE_CHECK_FOR_RESUME); > before this line. > > But I think we usually handle this by directly setting > next_state_ = STATE_SSL_CONNECT; > on line 354. Then we don't need to change next_state_ here. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:573: pending_jobs_(new PendingJobMap()) { On 2014/06/17 17:44:40, wtc wrote: > > BUG: this PendingJobMap object is leaked. We should declare this member as a > non-pointer in the .h file: > > PendingJobMap pending_jobs_; As you implied in your comment on the .h file, making this a scoped pointer is also an appropriate solution correct? https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:653: group_name, new SSLConnectJob::PendingJobList())); On 2014/06/17 17:44:40, wtc wrote: > > IMPORTANT: it seems wrong to create a new SSLConnectJob::PendingJobList every > time. It seems that we should first find an existing > SSLConnectJob::PendingJobList in pending_jobs_. If none exsts, then create a new > one. If I understand the documentation for std::map::insert correctly, the insert method will first check if there's an existing element with the given key, and if there is it will just return an iterator to that key. If not, then it actually creates a new entry and returns an iterator to that. (http://www.cplusplus.com/reference/map/map/insert/) https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:104: typedef std::vector<SSLConnectJob*> PendingJobList; On 2014/06/17 17:44:41, wtc wrote: > > Nit: it would be nice to document this type. For example, are the jobs in the > list supposed to be connecting to the same server? Are the jobs in the list > suspended? Does the owner of this list need to destroy the jobs? Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:118: PendingJobList* pending); On 2014/06/17 17:44:41, wtc wrote: > > 1. Nit: I suggest naming this parameter "pending_jobs" or "pending_job_list". > "pending" sounds like the name of a boolean parameter. > > 2. I suggest moving this parameter before the "delegate" parameter. By > convention, the delegate/callback and net_log parameters are usually listed last > in the net code. > > 3. Please document whether this object owns the pending job list. If not, > document that the pending job list must outlive this object. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:126: static NET_EXPORT void EnableJobWaiting(bool enable); On 2014/06/17 17:44:41, wtc wrote: > > Nit: please document this method. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:162: void ResumeSSLConnection(); On 2014/06/17 17:44:41, wtc wrote: > > Please document this method. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:163: State GetNextConnectState(bool enabled); On 2014/06/17 17:44:41, wtc wrote: > > Delete this unused method declaration. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:165: std::string GetSessionCacheKey(); On 2014/06/17 17:44:41, wtc wrote: > > Delete this unused method. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:309: typedef std::map<std::string, SSLConnectJob::PendingJobList*> PendingJobMap; On 2014/06/17 17:44:41, wtc wrote: > > Please document this map. In particular, what is the std::string key. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:319: PendingJobMap* pending_jobs_; On 2014/06/17 17:44:41, wtc wrote: > > Please document who owns pending_jobs_. If SSLClientSocketPool owns it, it > probably should be declared as > > PendingJobMap pending_jobs_; > > or > > scoped_ptr<PendingJobMap> pending_jobs_; > > Nit: the member name probably should have "map" in it. Done. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_session_... File net/socket/ssl_session_cache_openssl.h (right): https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_session_... net/socket/ssl_session_cache_openssl.h:117: bool SessionIsInCache(const std::string& cache_key) const; On 2014/06/17 17:44:41, wtc wrote: > > Nit: add "SSL" before "Session" because the other methods use "SSLSession". Done.
Review comments on patch set 6: I reviewed the CL carefully today. I believe the logic is mostly correct. I'd like to go over my review comments with you in person today. High-level comments: 1. All the PendingJobLists are leaked. It doesn't seem easy to come up with a solution quickly. So we should make sure the code that allocate the PendingJobLists is disabled if the feature is disabled. 2. The only flaw in the logic that I spotted (other than the False Start issue I mentioned before) is that the "group_name" for SSLClientSocketPool may be different from the cache key for the SSL session cache. We need to track down where group_name is defined and compare the two strings, or discover this empirically in the debugger. For the key of the PendingJobsMap, we may need to replace group_name with the actually cache key, constructed from host_and_port and ssl_session_cache_shard. https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:573: pending_jobs_(new PendingJobMap()) { On 2014/06/18 18:53:49, mshelley1 wrote: > > As you implied in your comment on the .h file, making this a scoped pointer is > also an appropriate solution correct? Yes, it is. But I suggested a better solution. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket.cc:106: bool SSLClientSocket::InSessionCache() const { Remove the definition of SSLClientSocket::InSessionCache(). Declare SSLClientSocket::InSessionCache() as abstract (= 0) in the .h file. This means we'll need to define InSessionCache() in all subclasses of SSLClientSocket. This is a hassle, but is appropriate in this case because the SSL session cache is very specific to the implementation of SSLClientSocket. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket.cc:109: return false; To suppress your feature, this method actually needs to return true. This is counter-intuitive, so please add a comment. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:88: // concatenated with its session cache shard. Since this method doesn't have a |cache_key| parameter, change "|cache key|" and "|cache_key|" to just "cache key". The "|...|" notation is to simulate a code font, so the text inside the || is a piece of code, usually a variable or function parameter name. "SSLConnectJobs host_and_port name" also needs work. Definitely remove the reference to SSLConnectJob. I think we can just say: The cache key consists of a host_and_port concatenated with a session cache shard. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:89: virtual bool InSessionCache() const; Could you declare this virtual method as abstract (= 0)? https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:65: virtual bool InSessionCache() const OVERRIDE; Nit: move this declaration right before GetSSLCertRequestInfo (line 60). This matches the order in which the methods are defined in the .cc file (a recommendation of the Style Guide) and the order in which the methods are declared in ssl_client_socket.h (the base class). https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:101: // so it must outlive the job. Please move this comment to the .h file. Ideally someone should be able to read just the .h file and understand how to use the class. So API contracts are usually documented in the .h file. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:259: next_state_ = STATE_CREATE_SSL_SOCKET; Nit: keep this method and DoSOCKSConnectComplete (lines 278-282) the same. Pick the form that you find easier to understand. We value consistency highly. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:316: Nit: reverse lines 315 and 316. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:340: } Please confirm that you think lines 325-340 should NOT be moved to DoSSLConnect(). I believe it is correct to leave them here. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:351: // If the group is in the cache, continue with the session resumption Nit: say "If the group's session is in the cache" or "If the group has a session in the cache". https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:361: next_state_ = STATE_CHECK_FOR_RESUME; I think we should change next_state_ to STATE_SSL_CONNECT here, and in ResumeSSLConnection, simply DCHECK that next_state_ is equal to STATE_SSL_CONNECT. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:367: pending_jobs_->push_back(this); I got confused by this before, so a comment might be useful. I didn't understand why we need to add |this| to pending_jobs_ even though |this| is continuing to STATE_SSL_CONNECT. The non-obvious reason is that this will cause other jobs to wait. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:529: pending_jobs_->erase(it); We should figure out how to erase the first job or clear all jobs of pending_jobs_, *before* calling ResumeSSLConnection. This may require copying (or rather, swapping with a temporary PendingJobList variable). You can make this change in a follow-up CL. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:651: SSLConnectJob::PendingJobList* pending_job_list; This code should also be controlled by the enable_job_waiting_ boolean. It would be better to move that static bool member into the SSLClientSocketPool::SSLConnectJobFactory class or even the SSLClientSocketPool class, and expose it to the SSLConnectJob class. As a short-term solution, add a getter method for SSLConnectJob::enable_job_waiting_ and check it here. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:654: group_name, new SSLConnectJob::PendingJobList())); IMPORTANT: there are two memory leaks of PendingJobLists. We should at least fix the first leak before committing this CL. 1. If the map already has an entry for the group_name, the new PendingJobList we create here is immediately leaked. Can you try to come up with a solution? I think you can check the bool (it.second) returned by pending_jobs_map_->insert(), and only allocate a new PendingJobList and store it in it.first->second if the bool (it.second) is true. 2. We need to delete all the PendingJobLists in the map when the map is destroyed. This needs to be done very carefully because some SSLConnectJobs may still be using the PendingJobLists. Please add a TODO comment about this leak. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:655: pending_job_list = it.first->second; Nit: Delete the declaration of pending_job_list on line 651 and declare it here: SSLConnectJob::PendingJobList* pending_job_list = pending_job_list; https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:22: #include "net/socket/ssl_client_socket_openssl.h" Remove this header. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:128: // Allows a command line flag to enable SSLConnectJob waiting Nit: the comment should avoid describing a particular user, so don't mention the command line flag. Just say something like "Enables SSLConnectJob waiting if |enable| is true". https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:323: scoped_ptr<PendingJobMap> pending_jobs_map_; It saves a memory allocation from the heap to declare this as: PendingJobMap pending_jobs_map_; A scoped_ptr is appropriate if the PendingJobMap is constructed by something else, or if we need to delete the PendingJobMap before this object is deleted. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_session_... File net/socket/ssl_session_cache_openssl.cc (right): https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_session_... net/socket/ssl_session_cache_openssl.cc:480: mutable base::Lock lock_; // Protects access to containers below. In general we should avoid the 'mutable' keyword. It is commonly used on members that are a lock or mutex, or a cached value that is computed lazily. So this is fine. But if I understand the code correctly, 'mutable' is necessary only if we also declare the SSLSessionIsInCache method on line 239 as const, right? We should do that.
https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/328903004/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:653: group_name, new SSLConnectJob::PendingJobList())); On 2014/06/18 18:53:49, mshelley1 wrote: > On 2014/06/17 17:44:40, wtc wrote: > > > > IMPORTANT: it seems wrong to create a new SSLConnectJob::PendingJobList every > > time. It seems that we should first find an existing > > SSLConnectJob::PendingJobList in pending_jobs_. If none exsts, then create a > new > > one. > > If I understand the documentation for std::map::insert correctly, the insert > method will first check if there's an existing element with the given key, and > if there is it will just return an iterator to that key. If not, then it > actually creates a new entry and returns an iterator to that. > (http://www.cplusplus.com/reference/map/map/insert/) > So, it's a little complicated: .insert() will always update the value of map[key] to the second argument of your pair if [key] was already in your map, it sets the second argument to true. if [key] was not already in your map, it sets the second argument to false. In both cases, it returns the first argument as an interator to map[key] This means that in the event that map[key] exists, you're *overwriting* it with your "new SSLConnectJob::PendingJobList" - creating a new pending list every time. You still want to do iterator it = map.find(key); if (it == map.end()) { other_it = map.insert(...); it = other_it.first; } pending_job_list = it->second; This ensures you re-use the existing entry (if one exists - your first .find), or, if you have to insert a new entry, you're not having to invoke .find() again to find the entry you *just* inserted.
On 2014/06/19 20:20:43, Ryan Sleevi wrote: > > .insert() will always update the value of map[key] to the second argument of > your pair > > if [key] was already in your map, it sets the second argument to true. > if [key] was not already in your map, it sets the second argument to false. > > In both cases, it returns the first argument as an interator to map[key] > > This means that in the event that map[key] exists, you're *overwriting* it with > your "new SSLConnectJob::PendingJobList" - creating a new pending list every > time. This doesn't seem to match the description and example in http://www.cplusplus.com/reference/map/map/insert/. The second (bool) argument of the returned pair is opposite to what you described. Also, that page seems to say if map[key] exists, the value of map[key] isn't updated. In the example, the attempt to insert std::pair<char,int>('z',500) on line 14 does not change the value of element 'z'; it is still 200. I agree with your suggested correction.
Please confirm my use of a scoped_ptr to prevent pending_jobs_map from being leaked https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket.cc:106: bool SSLClientSocket::InSessionCache() const { On 2014/06/19 19:38:10, wtc wrote: > > Remove the definition of SSLClientSocket::InSessionCache(). Declare > SSLClientSocket::InSessionCache() as abstract (= 0) in the .h file. > > This means we'll need to define InSessionCache() in all subclasses of > SSLClientSocket. This is a hassle, but is appropriate in this case because the > SSL session cache is very specific to the implementation of SSLClientSocket. Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:88: // concatenated with its session cache shard. On 2014/06/19 19:38:10, wtc wrote: > > Since this method doesn't have a |cache_key| parameter, change "|cache key|" and > "|cache_key|" to just "cache key". > > The "|...|" notation is to simulate a code font, so the text inside the || is a > piece of code, usually a variable or function parameter name. > > "SSLConnectJobs host_and_port name" also needs work. Definitely remove the > reference to SSLConnectJob. I think we can just say: > > The cache key consists of a host_and_port concatenated with a session cache > shard. Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:89: virtual bool InSessionCache() const; On 2014/06/19 19:38:10, wtc wrote: > > Could you declare this virtual method as abstract (= 0)? Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:65: virtual bool InSessionCache() const OVERRIDE; On 2014/06/19 19:38:10, wtc wrote: > > Nit: move this declaration right before GetSSLCertRequestInfo (line 60). This > matches the order in which the methods are defined in the .cc file (a > recommendation of the Style Guide) and the order in which the methods are > declared in ssl_client_socket.h (the base class). Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:101: // so it must outlive the job. On 2014/06/19 19:38:11, wtc wrote: > > Please move this comment to the .h file. Ideally someone should be able to read > just the .h file and understand how to use the class. So API contracts are > usually documented in the .h file. Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:259: next_state_ = STATE_CREATE_SSL_SOCKET; On 2014/06/19 19:38:11, wtc wrote: > > Nit: keep this method and DoSOCKSConnectComplete (lines 278-282) the same. Pick > the form that you find easier to understand. We value consistency highly. Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:316: On 2014/06/19 19:38:10, wtc wrote: > > Nit: reverse lines 315 and 316. Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:340: } On 2014/06/19 19:38:10, wtc wrote: > > Please confirm that you think lines 325-340 should NOT be moved to > DoSSLConnect(). I believe it is correct to leave them here. Yes -- I don't completely understand why, but leaving those lines in DoSSLConnect() led to segfaults. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:351: // If the group is in the cache, continue with the session resumption On 2014/06/19 19:38:10, wtc wrote: > > Nit: say "If the group's session is in the cache" or "If the group has a session > in the cache". Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:361: next_state_ = STATE_CHECK_FOR_RESUME; On 2014/06/19 19:38:11, wtc wrote: > > I think we should change next_state_ to STATE_SSL_CONNECT here, and in > ResumeSSLConnection, simply DCHECK that next_state_ is equal to > STATE_SSL_CONNECT. Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:367: pending_jobs_->push_back(this); On 2014/06/19 19:38:11, wtc wrote: > > I got confused by this before, so a comment might be useful. I didn't understand > why we need to add |this| to pending_jobs_ even though |this| is continuing to > STATE_SSL_CONNECT. The non-obvious reason is that this will cause other jobs to > wait. Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:529: pending_jobs_->erase(it); On 2014/06/19 19:38:10, wtc wrote: > > We should figure out how to erase the first job or clear all jobs of > pending_jobs_, *before* calling ResumeSSLConnection. This may require copying > (or rather, swapping with a temporary PendingJobList variable). > > You can make this change in a follow-up CL. Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:651: SSLConnectJob::PendingJobList* pending_job_list; On 2014/06/19 19:38:10, wtc wrote: > > This code should also be controlled by the enable_job_waiting_ boolean. It would > be better to move that static bool member into the > SSLClientSocketPool::SSLConnectJobFactory class or even the SSLClientSocketPool > class, and expose it to the SSLConnectJob class. > > As a short-term solution, add a getter method for > SSLConnectJob::enable_job_waiting_ and check it here. Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:654: group_name, new SSLConnectJob::PendingJobList())); On 2014/06/19 19:38:11, wtc wrote: > > IMPORTANT: there are two memory leaks of PendingJobLists. We should at least fix > the first leak before committing this CL. > > 1. If the map already has an entry for the group_name, the new PendingJobList we > create here is immediately leaked. > > Can you try to come up with a solution? I think you can check the bool > (it.second) returned by pending_jobs_map_->insert(), and only allocate a new > PendingJobList and store it in it.first->second if the bool (it.second) is true. > > 2. We need to delete all the PendingJobLists in the map when the map is > destroyed. This needs to be done very carefully because some SSLConnectJobs may > still be using the PendingJobLists. Please add a TODO comment about this leak. Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:655: pending_job_list = it.first->second; On 2014/06/19 19:38:11, wtc wrote: > > Nit: Delete the declaration of pending_job_list on line 651 and declare it here: > > SSLConnectJob::PendingJobList* pending_job_list = pending_job_list; Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:22: #include "net/socket/ssl_client_socket_openssl.h" On 2014/06/19 19:38:11, wtc wrote: > > Remove this header. Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:128: // Allows a command line flag to enable SSLConnectJob waiting On 2014/06/19 19:38:11, wtc wrote: > > Nit: the comment should avoid describing a particular user, so don't mention the > command line flag. Just say something like "Enables SSLConnectJob waiting if > |enable| is true". Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:323: scoped_ptr<PendingJobMap> pending_jobs_map_; On 2014/06/19 19:38:11, wtc wrote: > > It saves a memory allocation from the heap to declare this as: > PendingJobMap pending_jobs_map_; > > A scoped_ptr is appropriate if the PendingJobMap is constructed by something > else, or if we need to delete the PendingJobMap before this object is deleted. Done. https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_session_... File net/socket/ssl_session_cache_openssl.cc (right): https://codereview.chromium.org/328903004/diff/200001/net/socket/ssl_session_... net/socket/ssl_session_cache_openssl.cc:480: mutable base::Lock lock_; // Protects access to containers below. On 2014/06/19 19:38:11, wtc wrote: > > In general we should avoid the 'mutable' keyword. It is commonly used on members > that are a lock or mutex, or a cached value that is computed lazily. So this is > fine. > > But if I understand the code correctly, 'mutable' is necessary only if we also > declare the SSLSessionIsInCache method on line 239 as const, right? We should do > that. Done.
https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:88: // cache shard. This second half an implementation detail; doesn't need to be noted in the header (eg: it can change w/o any outside observable effect) Sorry for the confusing/conflicting advice between Wan-Teh and I - your first comment (here) was fine, we just usually omit the comment from the (derived) classes. https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:335: // |connect_start| doesn't include dns times, and it adjusts the time so s/dns/DNS/ https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:369: // will know that there is a job in progress. nit: Pronouns considered harmful ( https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M ) // Add |this| to pending jobs, so ensure that future jobs will // know that there is already a job in progress. https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:525: pending_jobs_->clear(); DANGER: A pending job may cause a modification to pending_jobs_ job->ResumeSSLConnection() job::OnIOComplete(...) job::DoConnect(...) SSLCS::DoConnect(...) SSLCS::DoLoop(...) SSLCS::DoHandshake(...) transport_->Write(...) return err; job::DoConnectComplete(...) job::[...] (notify whatever layer that the job failed) [some higher layer] -> RequestSocket(...) ClientSocketPoolBase::NewConnectJob(...) ... SSLConnectJob::DoCheckForResume pending_jobs_->push_back(this); It's a very convoluted call sequence, but this would be the danger. To handle this, you need to do something like PendingJobsList pending_jobs(it + 1, pending_jobs_->end()); pending_jobs_->clear(); for (PendingJobsList::iterator it = pending_jobs.begin(); it != pending_jobs.end(); ++it) { ... Stuff } https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:531: pending_jobs_->erase(it); Ditto here, for the possibility of mutations when you resume affecting things. Note that this also potentially violates the statement from line 516 that it will be the leading job; if ResumeSSLConnection() causes a failure, you could end up in the ProcessPendingJobs for that one, except it will be this job, and it+1 will be the actual 'leading' job. Luckily, this is easy to solve: it = pending_jobs_->erase(it); // "it" now contains "it+1" - that is, erase() returns the item that followed, or, if there was no item, pending_jobs_->end() https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:108: // so it must outlive the job. I think the "Note:" here is probably best omitted. This is true of all the other pointer-variables passed here (eg: no ownership changes happen; the owner must ensure the object outlives the job). https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:129: // Enable SSLConnectJob waiting if |enable| is true. // Enables or disables SSLConnectJob waiting. // If waiting is enabled, simultaneous attempts to connect // to the same server will be serialized until an SSL session // is cached, allowing the remaining connections to perform // resumption handshakes. That is, provide some description of what "job waiting" is; not too much, not too little :) https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:197: // TODO(mshelley) ensure that these pointers are deleted. Is this TODO still applicable? https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:327: scoped_ptr<PendingJobMap> pending_jobs_map_; Why can't this just be a PendingJobsMap? That is, why a pointer at all?
https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:88: // cache shard. On 2014/06/24 23:40:44, Ryan Sleevi wrote: > This second half an implementation detail; doesn't need to be noted in the > header (eg: it can change w/o any outside observable effect) > > Sorry for the confusing/conflicting advice between Wan-Teh and I - your first > comment (here) was fine, we just usually omit the comment from the (derived) > classes. Done. https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:335: // |connect_start| doesn't include dns times, and it adjusts the time so On 2014/06/24 23:40:44, Ryan Sleevi wrote: > s/dns/DNS/ Done. https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:369: // will know that there is a job in progress. On 2014/06/24 23:40:44, Ryan Sleevi wrote: > nit: Pronouns considered harmful ( > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M > ) > > // Add |this| to pending jobs, so ensure that future jobs will > // know that there is already a job in progress. Done. https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:525: pending_jobs_->clear(); On 2014/06/24 23:40:44, Ryan Sleevi wrote: > DANGER: A pending job may cause a modification to pending_jobs_ > > job->ResumeSSLConnection() > job::OnIOComplete(...) > job::DoConnect(...) > SSLCS::DoConnect(...) > SSLCS::DoLoop(...) > SSLCS::DoHandshake(...) > transport_->Write(...) > return err; > job::DoConnectComplete(...) > job::[...] (notify whatever layer that the job failed) > [some higher layer] -> RequestSocket(...) > ClientSocketPoolBase::NewConnectJob(...) > ... > SSLConnectJob::DoCheckForResume > pending_jobs_->push_back(this); > > It's a very convoluted call sequence, but this would be the danger. > > To handle this, you need to do something like > > PendingJobsList pending_jobs(it + 1, pending_jobs_->end()); > pending_jobs_->clear(); > for (PendingJobsList::iterator it = pending_jobs.begin(); > it != pending_jobs.end(); > ++it) { > ... Stuff > } Done. https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:531: pending_jobs_->erase(it); On 2014/06/24 23:40:44, Ryan Sleevi wrote: > Ditto here, for the possibility of mutations when you resume affecting things. > > Note that this also potentially violates the statement from line 516 that it > will be the leading job; if ResumeSSLConnection() causes a failure, you could > end up in the ProcessPendingJobs for that one, except it will be this job, and > it+1 will be the actual 'leading' job. > > Luckily, this is easy to solve: > it = pending_jobs_->erase(it); > // "it" now contains "it+1" - that is, erase() returns the item that followed, > or, if there was no item, pending_jobs_->end() Done. https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:108: // so it must outlive the job. On 2014/06/24 23:40:44, Ryan Sleevi wrote: > I think the "Note:" here is probably best omitted. > > This is true of all the other pointer-variables passed here (eg: no ownership > changes happen; the owner must ensure the object outlives the job). Done. https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:129: // Enable SSLConnectJob waiting if |enable| is true. On 2014/06/24 23:40:44, Ryan Sleevi wrote: > // Enables or disables SSLConnectJob waiting. > // If waiting is enabled, simultaneous attempts to connect > // to the same server will be serialized until an SSL session > // is cached, allowing the remaining connections to perform > // resumption handshakes. > > That is, provide some description of what "job waiting" is; not too much, not > too little :) Done. https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:197: // TODO(mshelley) ensure that these pointers are deleted. On 2014/06/24 23:40:44, Ryan Sleevi wrote: > Is this TODO still applicable? Yes -- I need to make sure that the pending jobs inside of the pending_jobs_map are delted. Wan-teh suggested making that another CL. https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:327: scoped_ptr<PendingJobMap> pending_jobs_map_; If this is just a PendingJobsMap then I can't insert into the map inside of the const NewConnectJob method (that was my understanding at least). Removing const from the method impacts several classes that are derived from the class, so I figured this was a better solution.
I think this looks good, although I have a few final comments and a request for onnnnne more update :) https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/328903004/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:327: scoped_ptr<PendingJobMap> pending_jobs_map_; On 2014/06/25 17:03:56, mshelley1 wrote: > If this is just a PendingJobsMap then I can't insert into the map inside of the > const NewConnectJob method (that was my understanding at least). Removing const > from the method impacts several classes that are derived from the class, so I > figured this was a better solution. Ah, I missed that. Yeah, it's definitely weird, and strictly speaking, a violation of const-correctness, but one that's subtle and easy to make (e.g.: This class already does a lot with violating const-correctness by virtue of the pools) In an "ideal" world (one few people will ever see in reality), when a method is declared const, it won't mutate any state on the object - OR any state on any of the sub-objects it holds. This is enforced somewhat when you have member variables (as you discovered), but not when the member variable is a pointer (*shrug*). I think this is probably fine for the short-term, but let's add a TODO here to come up with a better solution (or perhaps it may change entirely) https://codereview.chromium.org/328903004/diff/260001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/328903004/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:257: next_state_ = STATE_CREATE_SSL_SOCKET; I'm curious why these were changed? Perhaps I missed a comment go by? https://codereview.chromium.org/328903004/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:357: } style nit: no braces on single-line if when in net/ https://codereview.chromium.org/328903004/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:411: ProcessPendingJobs(ERR_NPN_NEGOTIATION_FAILED); So, this does seem a little 'dangerous', in that you have to catch every place you return from this function to restart pending jobs. If you miss any (eg: someone adds a new early return), then you'd end up in a bad state. One possible solution for this would be to introduce a new terminal state, STATE_PROCESS_PENDING_JOBS And set that as the next_state_ in line 384 Then you can remove these checks (eg: line 410/411, 507/508), and handle them in a method DoProcessPendingJobs(int result) { if (enable_job_waiting_) ProcessPendingJobs(result); return result; } Which will cause next_state_ to be STATE_NONE, which will then break at line 233 and return to the original caller. https://codereview.chromium.org/328903004/diff/260001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/328903004/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:173: static bool enable_job_waiting_; STYLE: This member variable should be moved to line 183 ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... )
https://codereview.chromium.org/328903004/diff/260001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/328903004/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:257: next_state_ = STATE_CREATE_SSL_SOCKET; On 2014/06/25 19:13:21, Ryan Sleevi wrote: > I'm curious why these were changed? Perhaps I missed a comment go by? I think I restructured it originally because back when I was still checking for |enable_job_waiting_| at this point it was simpler this way somehow. https://codereview.chromium.org/328903004/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:357: } On 2014/06/25 19:13:21, Ryan Sleevi wrote: > style nit: no braces on single-line if when in net/ Done. https://codereview.chromium.org/328903004/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:411: ProcessPendingJobs(ERR_NPN_NEGOTIATION_FAILED); On 2014/06/25 19:13:21, Ryan Sleevi wrote: > So, this does seem a little 'dangerous', in that you have to catch every place > you return from this function to restart pending jobs. If you miss any (eg: > someone adds a new early return), then you'd end up in a bad state. > > One possible solution for this would be to introduce a new terminal state, > > STATE_PROCESS_PENDING_JOBS > > And set that as the next_state_ in line 384 > > Then you can remove these checks (eg: line 410/411, 507/508), and handle them in > a method > > DoProcessPendingJobs(int result) { > if (enable_job_waiting_) > ProcessPendingJobs(result); > return result; > } > > Which will cause next_state_ to be STATE_NONE, which will then break at line 233 > and return to the original caller. Done. https://codereview.chromium.org/328903004/diff/260001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/328903004/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:173: static bool enable_job_waiting_; On 2014/06/25 19:13:21, Ryan Sleevi wrote: > STYLE: This member variable should be moved to line 183 ( > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... > ) Done.
lgtm
On 2014/06/26 01:49:35, Ryan Sleevi wrote: > lgtm Actually, lemme say "Not LGTM" since we're still working on testing ;)
Patch set 9 LGTM. Please wait for Ryan's approval. I only reviewed the diffs between patch sets 6 and 9. High-level comments: 1. I understand this CL is missing tests. I am fine with committing this CL first to facilitate future code reviews. 2. Please try building without use_openssl=1 in your GYP_DEFINES. I think you'll need a stub implementation of InSessionCache() in ssl_client_socket_nss.*. I suggest some small changes below. https://codereview.chromium.org/328903004/diff/280001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/328903004/diff/280001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:370: // cache shard. Nit: A comment at this location usually describes what the function (InSessionCache) does. But this comment describes the cache_key local variable. So this comment should be moved into the function at line 373, or to the GetSocketSessionCacheKey function at line 92. https://codereview.chromium.org/328903004/diff/280001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/328903004/diff/280001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:355: // resumption SSL handshake. Move this comment to line 358. https://codereview.chromium.org/328903004/diff/280001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:373: return OK; Nit: it is possible to rewrite lines 361-373 to eliminate the duplicate pending_jobs_->push_back(this) calls: int rv; if (pending_jobs_->empty()) { // If there are no pending jobs, continue the full SSL handshake // because a resumption handshake is not possible. rv = OK; } else { // If there are pending jobs, wait. rv = ERR_IO_PENDING; } pending_jobs_->push_back(this); return rv; The comment I asked you to add at lines 370-371 would be very confusing in the restructured code, so I didn't include it. If the restructured code is not clear to you, you can ignore this suggestion. https://codereview.chromium.org/328903004/diff/280001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:518: void SSLConnectJob::ProcessPendingJobs(int result) { Nit: I would get rid of the ProcessPendingJobs function. Just inline the code in DoProcessPendingJobs. https://codereview.chromium.org/328903004/diff/280001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:528: for (PendingJobList::iterator job = pending_jobs.begin(); Nit: this can be a const_iterator. https://codereview.chromium.org/328903004/diff/280001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:546: next_state_ = STATE_SSL_CONNECT; Delete this line. https://codereview.chromium.org/328903004/diff/280001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:550: bool SSLConnectJob::GetEnableJobWaiting() { Move this to line 178, to follow the definition of EnableJobWaiting. This matches their declaration order in the .h file. https://codereview.chromium.org/328903004/diff/280001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:669: std::pair<PendingJobMap::iterator, bool> iter = Nit: it is a little confusing to have variables named "it" and "iter" in proximity. I suggest renaming this variable, such as "result" or "inserted". https://codereview.chromium.org/328903004/diff/280001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:670: pending_jobs_map_->insert(PendingJobMap::value_type( Nit: it seems that this line is indented with three spaces. Please run git cl format. https://codereview.chromium.org/328903004/diff/280001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/328903004/diff/280001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:107: SSLConnectJob(const std::string& group_name, Please move the comment here: // Note: the SSLConnectJob does not own pending_jobs_list // so it must outlive the job. https://codereview.chromium.org/328903004/diff/280001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:133: static bool GetEnableJobWaiting(); Nit: this function can be named "JobWaitingEnabled" or "JobWaitingIsEnabled". One naming convention for getter and setter methods is GetFoo() and SetFoo(). Since the setter method EnableJobWaiting doesn't follow this convention, it is confusing for the getter method to be named this way. https://codereview.chromium.org/328903004/diff/280001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:329: scoped_ptr<PendingJobMap> pending_jobs_map_; Please declare this member as a non-pointer, if that works: PendingJobMap pending_jobs_map_; This will save a memory allocation from the heap. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
