Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
384873002:
This CL changes the lifespan of SSLConnectJobMessengers so that they are created only when needed, (Closed)
DescriptionThis CL changes the lifespan of SSlConnectJobMessengers so that they are created only when needed,
and deleted as soon as they are no longer necessary.
This CL adds methods to SSLClientSocketPool that are passed to the SSLConnectJob and SSLConnectJobMessenger
as callbacks. These allow the SSLConnectJob to tell the SSLClientSocketPool to
create a messenger for the job when appropriate, and the SSLConnectJobMessenger to tell the SSLCLientSocketPool to remove a messenger when appropriate. An SSLConnectJob will
now only create an SSLConnectJobMessenger if its connection's session is not already
in the session cache. The messenger will then ask to be removed when there are
no remaining pending or connecting sockets in the messenger.
BUG=
Patch Set 1 : #Patch Set 2 : Removed FormatSessionCacheKey function. #
Total comments: 13
Patch Set 3 : Brought this CL up to date with changes in https://codereview.chromium.org/353713005/ #Patch Set 4 : Gave the SSLClientSocketPool ownership of messengers, and altered scope of adding/deleting messenge… #Patch Set 5 : Fixed bug in which this feature was no longer disabled without my flag. #Patch Set 6 : Rebase, fixed issue where messenger field wasn't set to NULL after deletion #
Total comments: 39
Patch Set 7 : Implemented simpler method of nullifying messengers at the correct time. #
Total comments: 25
Patch Set 8 : Added documentation and removed the cache_key argument from ConnectionCompleteCallback #
Total comments: 33
Patch Set 9 : Renamed methods and member vars. #
Total comments: 24
Patch Set 10 : Removed CanProceed method & reordered OnJobComplete to avoid potential issues. #
Total comments: 2
Patch Set 11 : Re-added CanProceed method #
Total comments: 2
Patch Set 12 : Fixed nits #
Total comments: 2
Patch Set 13 : Fixed copiler issue in SSLClientSocketNSS #
Messages
Total messages: 30 (1 generated)
Quick question: Does this depend on any CLs in progress for review, and/or is affected by any CLs in progress for review? If so, could you list them, just to make sure I keep ordering straight :)
On 2014/07/10 20:49:32, Ryan Sleevi wrote: > Quick question: Does this depend on any CLs in progress for review, and/or is > affected by any CLs in progress for review? > > If so, could you list them, just to make sure I keep ordering straight :) Yep, this CL is upstream from both https://codereview.chromium.org/353713005/ & https://codereview.chromium.org/364943002/, though it really is only directly affected by #353713005. The ordering is https://codereview.chromium.org/353713005/ --> https://codereview.chromium.org/364943002/ --> https://codereview.chromium.org/384873002/.
OK, my biggest concern here is I think the lifetime responsibilities are a bit too coupled between the different layers. That is, we ideally want a single object creating and deleting messengers (and deciding when to do so), and that seems like it should be the Pool. From a conceptual standpoint, your callbacks offer some degree of abstraction, but it's still conceptually "Socket saying to Delete this Messenger", which is wrong. A better way to think about this problem is unpacking the statement "When should a messenger be deleted", and then think about how the Socket can inform (anyone) interested in that state, and the Pool can decide to delete the Messenger. https://codereview.chromium.org/384873002/diff/40001/net/socket/socket_test_u... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/384873002/diff/40001/net/socket/socket_test_u... net/socket/socket_test_util.cc:785: return ""; return std::string() https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:2886: return ""; std::string() - but it does seem very weird, since we can and do format the session key for NSS sockets. https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... net/socket/ssl_client_socket_pool.h:146: bool IsNeeded(); What does "in use" mean? By who? Better to expand on this (e.g. do you mean whether or not callbacks are pending? If we're still monitoring sockets? Something else?) https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... net/socket/ssl_client_socket_pool.h:162: typedef base::Callback<SSLConnectJobMessenger*(std::string)> What is the ownership semantics of the Messenger passed back? Does the caller own it? Or is it a pointer to an existing object? What's the lifetime of that object. https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... net/socket/ssl_client_socket_pool.h:230: void DeleteMessenger(std::string cache_key); This seems like a layering violation. The SSLConnectJob shouldn't be responsible for the lifetime management of the Messenger. It might help to write out a bit about what the expected lifetimes are of the objects (just in this CL as a description/reply to this), so then we can make sure the code matches those lifetimes. I, perhaps naievely, assumed the Pool managed the Messenger. In which case, Sockets should talk to the Pool, but whether or not a Messenger is deleted or not is opaque to the Socket. https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... net/socket/ssl_client_socket_pool.h:261: DeleteMessengerCallback delete_messenger_callback_; What are these callbacks? Connect() can be called multiple times, but general principal is not to re-use a callback, so how does that affect how this works?
Here I've responded to all of Ryan's CL comments, and made some slight changes to bring this CL up to date with my larger CL. I think this change definitely requires a larger redesign though to fix the coupling issues that Ryan mentioned -- working on that now. https://codereview.chromium.org/384873002/diff/40001/net/socket/socket_test_u... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/384873002/diff/40001/net/socket/socket_test_u... net/socket/socket_test_util.cc:785: return ""; On 2014/07/25 21:40:07, Ryan Sleevi wrote: > return std::string() Done. https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:2886: return ""; On 2014/07/25 21:40:07, Ryan Sleevi wrote: > std::string() - but it does seem very weird, since we can and do format the > session key for NSS sockets. This method was changed from a static method to a method of SSLClientSocketOpenSSL somewhere in https://codereview.chromium.org/353713005/ because there wasn't really any reason for it to be static. I can't actually find any place in SSLClientSocketNSS where a cache key is created...but it doesn't seem to have an equivalent static method that this could replace. https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... net/socket/ssl_client_socket_pool.h:146: bool IsNeeded(); On 2014/07/25 21:40:07, Ryan Sleevi wrote: > What does "in use" mean? By who? Better to expand on this (e.g. do you mean > whether or not callbacks are pending? If we're still monitoring sockets? > Something else?) Done. https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... net/socket/ssl_client_socket_pool.h:162: typedef base::Callback<SSLConnectJobMessenger*(std::string)> On 2014/07/25 21:40:07, Ryan Sleevi wrote: > What is the ownership semantics of the Messenger passed back? Does the caller > own it? Or is it a pointer to an existing object? What's the lifetime of that > object. Done. https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... net/socket/ssl_client_socket_pool.h:230: void DeleteMessenger(std::string cache_key); On 2014/07/25 21:40:07, Ryan Sleevi wrote: > This seems like a layering violation. The SSLConnectJob shouldn't be responsible > for the lifetime management of the Messenger. > > It might help to write out a bit about what the expected lifetimes are of the > objects (just in this CL as a description/reply to this), so then we can make > sure the code matches those lifetimes. I, perhaps naievely, assumed the Pool > managed the Messenger. In which case, Sockets should talk to the Pool, but > whether or not a Messenger is deleted or not is opaque to the Socket. Right now, the SSLConnectJobFactory manages the lifetime of messengers. Basically, a messenger should be given to an SSL Connect Job whenever that connection is not in the session cache. This is because whenever a session isn't in the cache, the job must be either a leading job, or a future pending job (so it needs a messenger). Thus, messengers are created whenever a job isn't in the cache, and a messenger doesn't already exist for that job's cache key. Messengers are deleted whenever a connection finishes and there are no pending jobs left in the messenger. Since this is determined by the SSLConnectJobs, I put the connect job factory in charge of creating/owning messengers because then giving SSLConnectJobs callbacks to control creating/deleting messengers would be relatively straightforward. I can easily change this so that the pool manages this though, that does seem more logical to me. https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... net/socket/ssl_client_socket_pool.h:261: DeleteMessengerCallback delete_messenger_callback_; On 2014/07/25 21:40:07, Ryan Sleevi wrote: > What are these callbacks? > > Connect() can be called multiple times, but general principal is not to re-use a > callback, so how does that affect how this works? These are callbacks to methods of the SSLConnectJobFactory, which actually controls creating/deleting messengers. The SSLConnectJob methods GetMessenger & DeleteMessenger just run these callbacks.
https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/384873002/diff/40001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:2886: return ""; On 2014/07/28 21:05:32, mshelley wrote: > On 2014/07/25 21:40:07, Ryan Sleevi wrote: > > std::string() - but it does seem very weird, since we can and do format the > > session key for NSS sockets. > > This method was changed from a static method to a method of > SSLClientSocketOpenSSL somewhere in https://codereview.chromium.org/353713005/ > because there wasn't really any reason for it to be static. I can't actually > find any place in SSLClientSocketNSS where a cache key is created...but it > doesn't seem to have an equivalent static method that this could replace. Surprisingly hidden, https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli...
This patch attempts to correct the coupling issues that were mentioned on patch set 2.
https://codereview.chromium.org/384873002/diff/160001/net/socket/client_socke... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/client_socke... net/socket/client_socket_pool_base.cc:1274: SSLConnectJob* job = static_cast<SSLConnectJob*>(*it); This is definitely a layering violation (It's a circular dependency - the base pool should not know about its children) https://codereview.chromium.org/384873002/diff/160001/net/socket/client_socke... File net/socket/client_socket_pool_base.h (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/client_socke... net/socket/client_socket_pool_base.h:295: const std::string& cache_key) const; This feels like a layering violation. ClientSocketPoolBase applies to all client sockets, but Messengers are strictly an SSL-layer concept (e.g. they do not exist for SOCKS proxies or TCP connections) https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:657: } This should be / is a separate CL, right? https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:444: base::Bind(&net::SSLConnectJob::ResumeSSLConnection, no need for net:: here https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:122: std::string group_name); Do you really need |group_name| to be exposed at all? It seems unused in the implementation. If so, you can just smuggle it with base::Bind currying. And if that made no sense, throw something on my calendar :) https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:151: bool IsNeeded(); Let's rename this then, to better match that it does bool HasPendingConnections ? https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:171: const ConnectionCompleteCallback connection_complete_callback_; Why const? https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:181: UncachedSessionCallback; I guess I'm still unclear what this means, or how to use it properly, just from reading the header. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:195: UncachedSessionCallback uncached_session_callback, This still feels a bit circular. Perhaps we should chat in person?
Review comments on patch set 6: This CL is more complicated than I thought, so I suspect the design may not be optimal. I also saw some changes that look like merge/rebase errors. I haven't internalized the code to be able to suggest fixes for the design, so most of my comments are on minutiae. https://codereview.chromium.org/384873002/diff/160001/net/socket/client_socke... File net/socket/client_socket_pool_base.h (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/client_socke... net/socket/client_socket_pool_base.h:206: ConnectJob::Delegate* delegate) = 0; Why can't this method be const? I guess it's because the method may need to modify the MessengerMap? But you moved the MessengerMap from the SSLConnectJobFactory class to the SSLClientSocketPool class. So it seems that this method can still be const. https://codereview.chromium.org/384873002/diff/160001/net/socket/socket_test_... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/socket_test_... net/socket/socket_test_util.cc:768: NOTREACHED(); Nit: use NOTIMPLEMENTED() instead, to match the other two methods below? https://codereview.chromium.org/384873002/diff/160001/net/socket/socket_test_... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/socket_test_... net/socket/socket_test_util.h:1007: scoped_ptr<ClientSocketHandle> transport_; Nit: I suggest you declare the |host_port_pair_| member after this line, to match the arguments to the constructor. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:86: virtual std::string GetSessionCacheKey() const = 0; Please document this method. Can you explain why you used to believe you could delete this method? (See your TODO comment on line 89 in the original code.) Note: I suspect this might be a merge error. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:89: // for the given cache key. This seems to be a merge error. The original comment is better. Please undo this change. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:2838: NOTREACHED(); Nit: perhaps NOTIMPLEMENTED(), to match what you're using in SetHandshakeCompletionCallback (line 2849)? https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (left): https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:1082: This is probably a merge error. I seem to remember you added this blank line in the main CL. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:378: // Return a string. I remember I asked you to revise this comment before. This shows this is very likely a merge error. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:657: } On 2014/08/12 00:27:42, Ryan Sleevi wrote: > This should be / is a separate CL, right? This is a merge error. The current code is on lines 660-664. Please undo this change. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:687: } Please undo this change. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:118: ConnectionCompleteCallback; Please document what the two string inputs to ConnectionCompleteCallback are. Also, they probably should be const std::string references. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:122: std::string group_name); If |group_name| is needed, it should be a const reference.
https://codereview.chromium.org/384873002/diff/160001/net/socket/client_socke... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/client_socke... net/socket/client_socket_pool_base.cc:1274: SSLConnectJob* job = static_cast<SSLConnectJob*>(*it); On 2014/08/12 00:27:42, Ryan Sleevi wrote: > This is definitely a layering violation > > (It's a circular dependency - the base pool should not know about its children) Done. https://codereview.chromium.org/384873002/diff/160001/net/socket/client_socke... File net/socket/client_socket_pool_base.h (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/client_socke... net/socket/client_socket_pool_base.h:206: ConnectJob::Delegate* delegate) = 0; On 2014/08/12 14:51:00, wtc wrote: > > Why can't this method be const? I guess it's because the method may need to > modify the MessengerMap? But you moved the MessengerMap from the > SSLConnectJobFactory class to the SSLClientSocketPool class. So it seems that > this method can still be const. Done. https://codereview.chromium.org/384873002/diff/160001/net/socket/client_socke... net/socket/client_socket_pool_base.h:295: const std::string& cache_key) const; On 2014/08/12 00:27:42, Ryan Sleevi wrote: > This feels like a layering violation. ClientSocketPoolBase applies to all client > sockets, but Messengers are strictly an SSL-layer concept (e.g. they do not > exist for SOCKS proxies or TCP connections) Done. https://codereview.chromium.org/384873002/diff/160001/net/socket/socket_test_... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/socket_test_... net/socket/socket_test_util.cc:768: NOTREACHED(); On 2014/08/12 14:51:00, wtc wrote: > > Nit: use NOTIMPLEMENTED() instead, to match the other two methods below? Done. https://codereview.chromium.org/384873002/diff/160001/net/socket/socket_test_... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/socket_test_... net/socket/socket_test_util.h:1007: scoped_ptr<ClientSocketHandle> transport_; On 2014/08/12 14:51:00, wtc wrote: > > Nit: I suggest you declare the |host_port_pair_| member after this line, to > match the arguments to the constructor. Done. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:86: virtual std::string GetSessionCacheKey() const = 0; On 2014/08/12 14:51:00, wtc wrote: > > Please document this method. > > Can you explain why you used to believe you could delete this method? (See your > TODO comment on line 89 in the original code.) > > Note: I suspect this might be a merge error. My comment below meant that I could delete the static CreateSessionCacheKey method because I no longer need to have a cache key prior to the creation of a socket, because now I don't create messengers until after I've created a socket. Thus, I replaced the static method with GetSessionCacheKey. Someone suggested doing this on an earlier CL I believe. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:89: // for the given cache key. On 2014/08/12 14:51:00, wtc wrote: > > This seems to be a merge error. The original comment is better. Please undo this > change. Done. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:2838: NOTREACHED(); On 2014/08/12 14:51:00, wtc wrote: > > Nit: perhaps NOTIMPLEMENTED(), to match what you're using in > SetHandshakeCompletionCallback (line 2849)? Done. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:378: // Return a string. On 2014/08/12 14:51:01, wtc wrote: > > I remember I asked you to revise this comment before. This shows this is very > likely a merge error. Done. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:657: } On 2014/08/12 14:51:00, wtc wrote: > > On 2014/08/12 00:27:42, Ryan Sleevi wrote: > > This should be / is a separate CL, right? > > This is a merge error. The current code is on lines 660-664. > > Please undo this change. > Done. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:687: } On 2014/08/12 14:51:01, wtc wrote: > > Please undo this change. Done. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:444: base::Bind(&net::SSLConnectJob::ResumeSSLConnection, On 2014/08/12 00:27:42, Ryan Sleevi wrote: > no need for net:: here Done. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:118: ConnectionCompleteCallback; On 2014/08/12 14:51:01, wtc wrote: > > Please document what the two string inputs to ConnectionCompleteCallback are. > > Also, they probably should be const std::string references. Done. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:122: std::string group_name); On 2014/08/12 00:27:43, Ryan Sleevi wrote: > Do you really need |group_name| to be exposed at all? It seems unused in the > implementation. If so, you can just smuggle it with base::Bind currying. > > And if that made no sense, throw something on my calendar :) Done. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:151: bool IsNeeded(); On 2014/08/12 00:27:43, Ryan Sleevi wrote: > Let's rename this then, to better match that it does > > bool HasPendingConnections > > ? Realized I don't actually need this anymore, because we always resume all pending connections whenever the leader completes. Thus we know that the messenger is not needed as soon as the leader calls RunAllCallbacks in OnSSLHandshakeCompleted. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:171: const ConnectionCompleteCallback connection_complete_callback_; On 2014/08/12 00:27:42, Ryan Sleevi wrote: > Why const? Done. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:181: UncachedSessionCallback; On 2014/08/12 00:27:43, Ryan Sleevi wrote: > I guess I'm still unclear what this means, or how to use it properly, just from > reading the header. Done. https://codereview.chromium.org/384873002/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:195: UncachedSessionCallback uncached_session_callback, On 2014/08/12 00:27:43, Ryan Sleevi wrote: > This still feels a bit circular. Perhaps we should chat in person? Done.
Much cleaner - and MUCH closer! https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:153: ssl_socket = connecting_sockets_.front(); Are these changes unrelated? Under what scenario can this happen? It may be worth adding a comment to a doc to explain this condition. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:211: weak_factory_(this), see comment about Weak Factory's always needing to be last. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:418: RunGetMessengerForUncachedSessionCallback( This is only called in one place (here). I'd just inline the callback call. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:856: base::Unretained(this))))); To hide the need to *pass* the cache key to the SSLConnectJobMessenger new SSLConnectJobMessenger(base::Bind(&SSLClientSocketPool::DeleteSSLConnectJobMessenger, base::Unretained(this), cache_key)) Then on line 159, connection_complete_callback_.Run(); There's no need to look up the cache key from the SSL socket, and it helps split up the coupling a bit (since it's the *Pool*, not the *Messenger*, that decides how some of these bits are mapped. And, to a degree, the *socket* when requesting from the *pool*. But not the *messenger*) https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:865: delete it->second; CHECK_NE(it, messenger_map_.end()) ? Otherwise, a possible double free here. Better to CHECK and force a quick crash and a double-free security bug. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:119: typedef base::Callback<void(const std::string&)> ConnectionCompleteCallback; Usually we annotate callbacks like typedef base::Callback<void(const std::string& /* session_cache_key */>) ConnectionCompletionCallback Then something like // |session_cache_key| is the identifier of the SSL session that will // be handled by this messenger. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:122: const ConnectionCompleteCallback& connection_complete_callback); A bit of documentation is needed here to explain why/how someone would set the connection_complete_callback, and what it means For example, why is it necessary to expose (to the higher layer) the SSL session key? Couldn't we manage it some other way? Since we need to communicate to the creator when a session is completed, they could use base::Bind() to smuggle whatever information they need through us. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:163: std::string group_name_; No longer needed? https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:165: base::WeakPtrFactory<SSLConnectJobMessenger> weak_factory_; WeakPtrFactory's should always be last. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:175: // owned by the SSLClientSocketPool. // Callback to allow the SSLConnectJob to obtain an SSLConnectJobMessenger to coordinate connecting. // The SSLConnectJob will supply a unique identifer (ex: the SSL session ID), with the expectation that the same Messenger will be returned for // all such ConnectJobs. // Note: It will only be called for situations where the SSL session cache // does not already have a candidate session to resume. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:192: GetMessengerForUncachedSessionCallback uncached_session_callback, const-ref the callback. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:271: bool enable_ssl_connect_job_waiting_; 1) Document 2) Is the bool really needed? https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:360: void DeleteSSLConnectJobMessenger(const std::string& cache_key); what's cache_key? group_name? session id? Something else? Whatever it is, ensure this header file consistently calls it the same thing
https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:153: ssl_socket = connecting_sockets_.front(); On 2014/08/13 01:02:15, Ryan Sleevi wrote: > Are these changes unrelated? Under what scenario can this happen? It may be > worth adding a comment to a doc to explain this condition. Done. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:211: weak_factory_(this), On 2014/08/13 01:02:12, Ryan Sleevi wrote: > see comment about Weak Factory's always needing to be last. Done. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:418: RunGetMessengerForUncachedSessionCallback( On 2014/08/13 01:02:11, Ryan Sleevi wrote: > This is only called in one place (here). I'd just inline the callback call. Done. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:856: base::Unretained(this))))); On 2014/08/13 01:02:11, Ryan Sleevi wrote: > To hide the need to *pass* the cache key to the SSLConnectJobMessenger > > new > SSLConnectJobMessenger(base::Bind(&SSLClientSocketPool::DeleteSSLConnectJobMessenger, > base::Unretained(this), cache_key)) > > Then on line 159, > connection_complete_callback_.Run(); > > There's no need to look up the cache key from the SSL socket, and it helps split > up the coupling a bit (since it's the *Pool*, not the *Messenger*, that decides > how some of these bits are mapped. And, to a degree, the *socket* when > requesting from the *pool*. But not the *messenger*) Done. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:865: delete it->second; On 2014/08/13 01:02:15, Ryan Sleevi wrote: > CHECK_NE(it, messenger_map_.end()) ? > > Otherwise, a possible double free here. Better to CHECK and force a quick crash > and a double-free security bug. Done. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:119: typedef base::Callback<void(const std::string&)> ConnectionCompleteCallback; On 2014/08/13 01:02:15, Ryan Sleevi wrote: > Usually we annotate callbacks like > typedef base::Callback<void(const std::string& /* session_cache_key */>) > ConnectionCompletionCallback > > Then something like > // |session_cache_key| is the identifier of the SSL session that will > // be handled by this messenger. > > Done. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:163: std::string group_name_; On 2014/08/13 01:02:15, Ryan Sleevi wrote: > No longer needed? Done. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:165: base::WeakPtrFactory<SSLConnectJobMessenger> weak_factory_; On 2014/08/13 01:02:15, Ryan Sleevi wrote: > WeakPtrFactory's should always be last. Done. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:175: // owned by the SSLClientSocketPool. On 2014/08/13 01:02:15, Ryan Sleevi wrote: > // Callback to allow the SSLConnectJob to obtain an SSLConnectJobMessenger to > coordinate connecting. > // The SSLConnectJob will supply a unique identifer (ex: the SSL session ID), > with the expectation that the same Messenger will be returned for > // all such ConnectJobs. > // Note: It will only be called for situations where the SSL session cache > // does not already have a candidate session to resume. Done. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:192: GetMessengerForUncachedSessionCallback uncached_session_callback, On 2014/08/13 01:02:15, Ryan Sleevi wrote: > const-ref the callback. Done. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:271: bool enable_ssl_connect_job_waiting_; On 2014/08/13 01:02:15, Ryan Sleevi wrote: > 1) Document > 2) Is the bool really needed? It can be avoided -- previously I thought I needed it so that I only created/retrieved a messenger for the job if the flag was enabled. I can just return NULL if the flag is enabled instead though, which means only the pool needs to know about the flag. https://codereview.chromium.org/384873002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:360: void DeleteSSLConnectJobMessenger(const std::string& cache_key); On 2014/08/13 01:02:15, Ryan Sleevi wrote: > what's cache_key? group_name? session id? Something else? > > Whatever it is, ensure this header file consistently calls it the same thing Done.
Review comments on patch set 8: I'm very happy that you found a solution whose complexity meets my expectations. I think the use of callbacks is excessive -- please see the two comments I marked with "DESIGN". The use of callbacks makes your code more general than necessary, and makes it harder to understand. Some of the type or function names also make the code harder to understand because they are inaccurate or a little too abstract. I suggested some alternative names. Thanks. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:86: // Compute a unique key string for the SSL session cache. Nit: Compute => Computes https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:2837: std::string SSLClientSocketNSS::GetSessionCacheKey() const { IMPORTANT: add a declaration of this method (with "OVERRIDE") to ssl_client_socket_nss.h. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:2839: return std::string(); Note: you can actually implement this function -- just use the same code as SSLClientSocketOpenSSL. Just wanted to the reason you didn't implement this method is that it is unneeded. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:377: // Compute a unique string for the SSL session cache. Nit: delete this comment -- just rely on the comment in ssl_client_socket.h (the base class) for this method. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:155: connection_complete_callback_.Run(); IMPORTANT: you need to save connection_complete_callback_ in a local variable before you call RunAllCallbacks, and then invoke the Run() method on the local variable. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:692: delete it->second; There should be a function in base/stl_util.h that you can use. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:122: const ConnectionCompleteCallback& connection_complete_callback); DESIGN: instead of an abstract callback function, I would pass an SSLClientSocketPool* pointer and the cache key, and have the SSLConnectJobMessenger call socket_pool->DeleteSSLConnectJobMessenger(cache_key) directly. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:176: // will be returned for all such ConnectJobs. Please reformat this comment block. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:181: GetMessengerForUncachedSessionCallback; Nit: I would shorten this type to "GetMessengerCallback". https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:196: const GetMessengerForUncachedSessionCallback& uncached_session_callback, Between the "get messenger" and the "uncached session", I think "get messenger" is more important. So I suggest naming this parameter "get_messenger_callback". Similarly, I think the uncached_session_callback_ member should be renamed "get_messenger_callback_". https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:196: const GetMessengerForUncachedSessionCallback& uncached_session_callback, DESIGN: I would just pass a SSLClientSocketPool* pointer, and have the SSLConnectJob call socket_pool->AddSSLConnectJobMessenger(ssl_socket_->GetSessionCacheKey()) directly. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:266: SSLConnectJob::GetMessengerForUncachedSessionCallback You should be able to omit "SSLConnectJob::". https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:354: // Creates an SSLConnectJobMessenger for the given ssl session |cache_key| This comment is not accurate because the method may simply look up an existing SSLConnectJobMessenger from |messenger_map_|. So, the "AddSSLConnectJobMessenger" function name is also not accurate because "Add" is not always done. I suggest using "Get" or "Lookup" instead. (We've also used "GetOrCreate" in this situation if you don't mind the verbosity.) https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:380: uncached_session_callback, Rename this "get_messenger_callback". https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:402: uncached_session_callback_; Rename this "get_messenger_callback".
https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:117: typedef base::Callback<void()> ConnectionCompleteCallback; We have a typedef for this already - base::Closure https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:122: const ConnectionCompleteCallback& connection_complete_callback); On 2014/08/14 00:46:46, wtc wrote: > > DESIGN: instead of an abstract callback function, I would pass an > SSLClientSocketPool* pointer and the cache key, and have the > SSLConnectJobMessenger call socket_pool->DeleteSSLConnectJobMessenger(cache_key) > directly. I recommended against this, to avoid introducing additional circular dependencies for what is a header-visible class. This may be unnecessary complexity for now, but the concern was about messengers with different connection strategies, and wanting to decouple them as much as possible from the appropriate bits. Since ConnectJob depends on Messenger, and Pool depends on ConnectJob, I wanted to avoid a circular dependency where Messenger depends on Pool (thus making Pool depend on ConnectJob and ConnectJob depend, indirectly, on Pool) https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:196: const GetMessengerForUncachedSessionCallback& uncached_session_callback, On 2014/08/14 00:46:46, wtc wrote: > > DESIGN: I would just pass a SSLClientSocketPool* pointer, and have the > SSLConnectJob call > socket_pool->AddSSLConnectJobMessenger(ssl_socket_->GetSessionCacheKey()) > directly. Same comment re: coupling. To date, there's nothing that requires the SSLConnectJob depend on a Pool, and I was hoping to avoid that. For example, one can supply a callback that returns NULL to not implement this feature, or other (non-SocketPools) can use messengers to coordinate session establishment outside of the use of a pool.
https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:86: // Compute a unique key string for the SSL session cache. On 2014/08/14 00:46:45, wtc wrote: > > Nit: Compute => Computes Done. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:2837: std::string SSLClientSocketNSS::GetSessionCacheKey() const { On 2014/08/14 00:46:45, wtc wrote: > > IMPORTANT: add a declaration of this method (with "OVERRIDE") to > ssl_client_socket_nss.h. Done. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:2839: return std::string(); On 2014/08/14 00:46:45, wtc wrote: > > Note: you can actually implement this function -- just use the same code as > SSLClientSocketOpenSSL. > > Just wanted to the reason you didn't implement this method is that it is > unneeded. OK -- does the session cache used by NSS use the same session cache key as the one used by OpenSSL? I didn't think that it did, and thus thought that implementing this method would be misleading. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:377: // Compute a unique string for the SSL session cache. On 2014/08/14 00:46:45, wtc wrote: > > Nit: delete this comment -- just rely on the comment in ssl_client_socket.h (the > base class) for this method. Done. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:155: connection_complete_callback_.Run(); On 2014/08/14 00:46:45, wtc wrote: > > IMPORTANT: you need to save connection_complete_callback_ in a local variable > before you call RunAllCallbacks, and then invoke the Run() method on the local > variable. Done. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:692: delete it->second; On 2014/08/14 00:46:45, wtc wrote: > > There should be a function in base/stl_util.h that you can use. Done. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:117: typedef base::Callback<void()> ConnectionCompleteCallback; On 2014/08/14 01:20:20, Ryan Sleevi wrote: > We have a typedef for this already - base::Closure Done. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:122: const ConnectionCompleteCallback& connection_complete_callback); On 2014/08/14 00:46:46, wtc wrote: > > DESIGN: instead of an abstract callback function, I would pass an > SSLClientSocketPool* pointer and the cache key, and have the > SSLConnectJobMessenger call socket_pool->DeleteSSLConnectJobMessenger(cache_key) > directly. So is the benefit of passing in the pointer directly just that it's more straightforward? I've left it as it is because I'm not sure if that concern outweighs Ryan's concern about circular dependencies or not. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:176: // will be returned for all such ConnectJobs. On 2014/08/14 00:46:46, wtc wrote: > > Please reformat this comment block. Done. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:181: GetMessengerForUncachedSessionCallback; On 2014/08/14 00:46:46, wtc wrote: > > Nit: I would shorten this type to "GetMessengerCallback". Done. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:196: const GetMessengerForUncachedSessionCallback& uncached_session_callback, On 2014/08/14 00:46:46, wtc wrote: > > Between the "get messenger" and the "uncached session", I think "get messenger" > is more important. So I suggest naming this parameter "get_messenger_callback". > Similarly, I think the uncached_session_callback_ member should be renamed > "get_messenger_callback_". Done. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:266: SSLConnectJob::GetMessengerForUncachedSessionCallback On 2014/08/14 00:46:46, wtc wrote: > > You should be able to omit "SSLConnectJob::". Done. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:354: // Creates an SSLConnectJobMessenger for the given ssl session |cache_key| On 2014/08/14 00:46:45, wtc wrote: > > This comment is not accurate because the method may simply look up an existing > SSLConnectJobMessenger from |messenger_map_|. So, the > "AddSSLConnectJobMessenger" function name is also not accurate because "Add" is > not always done. I suggest using "Get" or "Lookup" instead. (We've also used > "GetOrCreate" in this situation if you don't mind the verbosity.) Done. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:380: uncached_session_callback, On 2014/08/14 00:46:46, wtc wrote: > > Rename this "get_messenger_callback". Done. https://codereview.chromium.org/384873002/diff/240001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:402: uncached_session_callback_; On 2014/08/14 00:46:45, wtc wrote: > > Rename this "get_messenger_callback". Done.
Patch set 9 LGTM. 1. Note the comment marked with "IMPORTANT". That could be a bug. 2. Please remember to review the CL's description to make sure it reflects the final code. Thanks. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:3260: } This is where the cache key for SSLClientSocketNSS is computed. You are right that it is not exactly the same as the cache key for SSLClientSocketOpenSSL (they differ when ssl_session_cache_shard_ is empty). So we can't just copy the SSLClientSocketOpenSSL implementation to this file. In any case, it is good to leave SSLClientSocketNSS::GetSessionCacheKey() unimplemented. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:130: bool SSLConnectJobMessenger::CanProceed(SSLClientSocket* ssl_socket) { 1. Remove the unused |ssl_socket| parameter. 2. Nit: the CanProceed() method is now so simple that it probably should be removed. I remember both mmenke and I suggested this before. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:156: connection_complete_callback.Run(); IMPORTANT: I think we should run connection_complete_callback before calling RunAllCallbacks. The reason is that RunAllCallbacks() could potentially cause new connections that use the current cache key. So we need connection_complete_callback to remove the current messenger from the messenger map, to prevent the current messenger from being used for the new connections. Does this make sense? https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:413: } Nit: omit the curly braces. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:421: if (ssl_socket_->InSessionCache() || !messenger_) Remove the redundant ssl_socket_->InSessionCache() test. We just tested it in the previous state (line 411). https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:428: messenger_ = NULL; I suspect you set messenger_ to NULL because we will crash otherwise? https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:434: weak_factory_.GetWeakPtr())); In this case, the right time to set messenger_ to NULL is actually quite subtle. I believe your code is correct. I wonder if we should document it. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:673: base::Unretained(this)), Please confirm that we don't need to use a weak pointer here. (I didn't check this.) https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:840: base::Unretained(this), Please confirm that we don't need a weak pointer here. (I didn't check this.) https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:850: CHECK(it != messenger_map_.end()); You can also assert CHECK_EQ(this, it->second). https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:119: // that a connection monitored by the SSLConnectJobMessenger has completed. I think it is better to name this callback something like "messenger_finished_callback", because this callback is run when we are finished with the messenger, and the callback is expected to remove the messenger from the messenger map and delete the messenger. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:375: SSLConnectJob::GetMessengerCallback get_messenger_callback, Should this parameter be a const reference?
https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:130: bool SSLConnectJobMessenger::CanProceed(SSLClientSocket* ssl_socket) { On 2014/08/14 23:26:26, wtc wrote: > > 1. Remove the unused |ssl_socket| parameter. > > 2. Nit: the CanProceed() method is now so simple that it probably should be > removed. I remember both mmenke and I suggested this before. Done. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:156: connection_complete_callback.Run(); On 2014/08/14 23:26:26, wtc wrote: > > IMPORTANT: I think we should run connection_complete_callback before calling > RunAllCallbacks. > > The reason is that RunAllCallbacks() could potentially cause new connections > that use the current cache key. So we need connection_complete_callback to > remove the current messenger from the messenger map, to prevent the current > messenger from being used for the new connections. > > Does this make sense? Done. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:413: } On 2014/08/14 23:26:26, wtc wrote: > > Nit: omit the curly braces. Done. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:421: if (ssl_socket_->InSessionCache() || !messenger_) On 2014/08/14 23:26:26, wtc wrote: > > Remove the redundant ssl_socket_->InSessionCache() test. We just tested it in > the previous state (line 411). Done. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:428: messenger_ = NULL; On 2014/08/14 23:26:26, wtc wrote: > > I suspect you set messenger_ to NULL because we will crash otherwise? Yep -- I check whether the messenger_ is valid before calling RemovePendingSocket in the SSLConenctJob destructor. If I don't set it to NULL here, then I end up calling that method on a deleted messenger_. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:434: weak_factory_.GetWeakPtr())); On 2014/08/14 23:26:26, wtc wrote: > > In this case, the right time to set messenger_ to NULL is actually quite subtle. > I believe your code is correct. I wonder if we should document it. Probably, I'll add documentation. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:673: base::Unretained(this)), On 2014/08/14 23:26:26, wtc wrote: > > Please confirm that we don't need to use a weak pointer here. (I didn't check > this.) We do not -- the pool owns the SSLConnectJobs, therefore it will always be valid when this callback is run. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:840: base::Unretained(this), On 2014/08/14 23:26:26, wtc wrote: > > Please confirm that we don't need a weak pointer here. (I didn't check this.) We do not, the pool also owns the SSLConnectJobMessengers, therefore it will always be valid when this callback is run. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:850: CHECK(it != messenger_map_.end()); On 2014/08/14 23:26:26, wtc wrote: > > You can also assert CHECK_EQ(this, it->second). I'm not sure if I understand how that would work -- wouldn't that be comparing a SSLClientSocketPool* to a SSLConnectJobMessenger*? https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:119: // that a connection monitored by the SSLConnectJobMessenger has completed. On 2014/08/14 23:26:26, wtc wrote: > > I think it is better to name this callback something like > "messenger_finished_callback", because this callback is run when we are finished > with the messenger, and the callback is expected to remove the messenger from > the messenger map and delete the messenger. Done. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:375: SSLConnectJob::GetMessengerCallback get_messenger_callback, On 2014/08/14 23:26:26, wtc wrote: > > Should this parameter be a const reference? Done.
Patch set 10 LGTM. Please add back the CanProceed() method and remove the connecting_sockets() getter method. Thanks. https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/384873002/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:850: CHECK(it != messenger_map_.end()); On 2014/08/15 16:53:06, mshelley wrote: > > I'm not sure if I understand how that would work -- wouldn't that be comparing a > SSLClientSocketPool* to a SSLConnectJobMessenger*? Ah, you're right. I thought |this| was a pointer to the SSLConnectJobMessenger object we're about to delete. I'm not sure if we should pass that pointer to this method for extra validation, so let's leave this unchanged. https://codereview.chromium.org/384873002/diff/280001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/384873002/diff/280001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:119: // that a connection monitored by the SSLConnectJobMessenger has completed. Nit: this comment should ideally avoid explicit reference to the client socket pool. Would be better to only describe "the messenger is finished" part. https://codereview.chromium.org/384873002/diff/280001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:144: std::vector<SSLClientSocket*> connecting_sockets() { IMPORTANT: I didn't realize that the removal of CanProceed() requires us to give access to the private connecting_sockets_ member. It's bad to add this getter method. Let's resurrect the CanProceed() method. We probably should keep its |ssl_socket| parameter for generality even though it is currently unused. If you decide to delete the |ssl_socket| parameter, please update the comment.
LGTM % comment nits https://codereview.chromium.org/384873002/diff/300001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/384873002/diff/300001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:131: // If there are no connecting sockets, allow the connetion to proceed. typo: connection https://codereview.chromium.org/384873002/diff/300001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/384873002/diff/300001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:128: // Returns ture if |ssl_socket|'s Connect() method should be called. typo: true
The CQ bit was checked by mshelley@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mshelley@chromium.org/384873002/320001
Review comments on patch set 12: there is a compilation error in SSLClientSocketNSS. https://codereview.chromium.org/384873002/diff/320001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.h (right): https://codereview.chromium.org/384873002/diff/320001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.h:71: virtual std::string CreateSessionCacheKey() const OVERRIDE; BUG: this method should be named GetSessionCacheKey(). https://codereview.chromium.org/384873002/diff/320001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/384873002/diff/320001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.h:119: // SSLConnectJobMessenger has completed. This comment is now an incomplete sentence. Also I think it must point out that this callback is invoked when we no longer need the messenger. I suggest something like: // |messenger_finished_callback| is run when we are finished with the SSLConnectJobMessenger. or // |messenger_finished_callback| is run when a connection monitored by the SSLConnectJobMessenger has completed and we are finished with the SSLConnectJobMessenger.
The CQ bit was checked by mshelley@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mshelley@chromium.org/384873002/340001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
We have a compilation error in
chrome/browser/extensions/api/socket/tls_socket_unittest.cc because that file
defines a subclass of SSLClientSocket that needs to define the new
GetSessionCacheKey() method:
../../chrome/browser/extensions/api/socket/tls_socket_unittest.cc:119:50: error:
allocating an object of abstract class type 'extensions::MockSSLClientSocket'
scoped_ptr<MockSSLClientSocket> ssl_sock(new MockSSLClientSocket);
^
../../net/socket/ssl_client_socket.h:87:23: note: unimplemented pure virtual
method 'GetSessionCacheKey' in 'MockSSLClientSocket'
virtual std::string GetSessionCacheKey() const = 0;
I've cloned this CL in https://codereview.chromium.org/476313004/ and will
continue there.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
