Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3110006:
Add support for speaking SSL to an HTTP Proxy, to HttpProxyClientSocketPool (and friends) (Closed)
|
Created:
10 years, 4 months ago by Ryan Hamilton Modified:
7 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd support for speaking SSL to an HTTP Proxy, to
HttpProxyClientSocketPool (and friends)
More information about an HTTPS Proxy can be found here:
http://dev.chromium.org/spdy/spdy-proxy
This implementation supports both http:// and https:// requests,
as well as support for both Proxy and Server auth.
BUG=29625
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=57333
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 57
Patch Set 5 : '' #
Total comments: 9
Patch Set 6 : '' #
Total comments: 8
Patch Set 7 : '' #
Total comments: 27
Patch Set 8 : '' #Patch Set 9 : '' #
Total comments: 20
Patch Set 10 : '' #Patch Set 11 : '' #Patch Set 12 : '' #Patch Set 13 : '' #
Total comments: 15
Patch Set 14 : '' #
Total comments: 1
Patch Set 15 : '' #Patch Set 16 : '' #Patch Set 17 : '' #Patch Set 18 : '' #Patch Set 19 : '' #Patch Set 20 : '' #Messages
Total messages: 25 (0 generated)
Drive-by with a test comment. http://codereview.chromium.org/3110006/diff/21002/30006 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/3110006/diff/21002/30006#newcode1722 net/http/http_network_transaction_unittest.cc:1722: /* How about rather disabling the test (DISABLED_)? If commented out, it's guaranteed to rot quickly.
Adding Erik Chen.
http://codereview.chromium.org/3110006/diff/21002/30006 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/3110006/diff/21002/30006#newcode1722 net/http/http_network_transaction_unittest.cc:1722: /* On 2010/08/11 16:57:41, Paweł Hajdan Jr. wrote: > How about rather disabling the test (DISABLED_)? If commented out, it's > guaranteed to rot quickly. Great idea! I didn't realize that was possible. I intend to resurrect that test ASAP, but I've made your change in my work area.
I couldn't get through this entire CL before my brain stopped accepting new info from it. http://codereview.chromium.org/3110006/diff/21002/30002 File net/http/http_network_session.cc (right): http://codereview.chromium.org/3110006/diff/21002/30002#newcode96 net/http/http_network_session.cc:96: new SSLClientSocketPool( Maybe HttpNetworkSession::GetSocketPoolForSSLWithProxy should populate the TCP pool part, and then we could call that from here. http://codereview.chromium.org/3110006/diff/21002/30004 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/3110006/diff/21002/30004#newcode753 net/http/http_network_transaction.cc:753: if (using_ssl_ && !authentication_url.SchemeIs("https")) { Does this code need to run in the https proxy case as well? If so, maybe better to handle the http and https case in one block. The differences are only tcp or ssl params? http://codereview.chromium.org/3110006/diff/21002/30004#newcode805 net/http/http_network_transaction.cc:805: scoped_refptr<SSLClientSocketPool> ssl_pool = proxy_info_.is_direct() Should this be is_direct() || is_https() ? When layering an ssl socket on top of another ssl socket, you probably want to use different ssl pools in order to not double count against the total socket limit. http://codereview.chromium.org/3110006/diff/21002/30004#newcode806 net/http/http_network_transaction.cc:806: ? session_->ssl_socket_pool() no reason for the trinary operator here. http://codereview.chromium.org/3110006/diff/21002/30004#newcode843 net/http/http_network_transaction.cc:843: ssl_started = ssl_started || proxy_info_.is_https(); This is incorrect - the counter example is a tcp connect problem when trying for an https proxy. http://codereview.chromium.org/3110006/diff/21002/30004#newcode1102 net/http/http_network_transaction.cc:1102: !using_ssl_ && proxy_info_.is_http(), &request_line, Do you want auth headers in the is_https() && using_ssl_ case? I thought that would be handled with an Http Connect. http://codereview.chromium.org/3110006/diff/21002/30004#newcode1169 net/http/http_network_transaction.cc:1169: SSLClientSocket* ssl_socket = GetSslClientSocket(); I'm not sure this would work in the Https proxy case. http://codereview.chromium.org/3110006/diff/21002/30004#newcode1249 net/http/http_network_transaction.cc:1249: SSLClientSocket* ssl_socket = GetSslClientSocket(); In this case you don't want the ssl socket to the https proxy, you want the ssl socket that's layered on top of it. http://codereview.chromium.org/3110006/diff/21002/30004#newcode1538 net/http/http_network_transaction.cc:1538: SSLClientSocket* ssl_socket = GetSslClientSocket(); You could have a cert error at either level. http://codereview.chromium.org/3110006/diff/21002/30004#newcode1749 net/http/http_network_transaction.cc:1749: return /*proxy_info_.is_https() || */(!using_ssl_ && proxy_info_.is_http()); extra http://codereview.chromium.org/3110006/diff/21002/30007 File net/http/http_proxy_client_socket.cc (right): http://codereview.chromium.org/3110006/diff/21002/30007#newcode79 net/http/http_proxy_client_socket.cc:79: // DCHECK(transport_->socket()->IsConnected()); This should get figured out. As far as I know, the transport socket should always be connected when we connect the HttpProxy socket. http://codereview.chromium.org/3110006/diff/21002/30007#newcode213 net/http/http_proxy_client_socket.cc:213: ClientSocket* HttpProxyClientSocket::socket() const { Note to self: Hmm? http://codereview.chromium.org/3110006/diff/21002/30008 File net/http/http_proxy_client_socket.h (right): http://codereview.chromium.org/3110006/diff/21002/30008#newcode76 net/http/http_proxy_client_socket.h:76: ClientSocket* socket() const; transport_socket() ? http://codereview.chromium.org/3110006/diff/21002/30009 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/21002/30009#newcode160 net/http/http_proxy_client_socket_pool.cc:160: result = OK; // Ignore cert errors for now What's the story for handling this case? Fine to make progress, but IMHO, this isn't production ready. http://codereview.chromium.org/3110006/diff/21002/30009#newcode165 net/http/http_proxy_client_socket_pool.cc:165: // so that a fast TCP connection plus a slow HttpProxy failure doesn't take TCP->SSL http://codereview.chromium.org/3110006/diff/21002/30009#newcode176 net/http/http_proxy_client_socket_pool.cc:176: params_->tcp_params() == NULL an if might be easier to read here. http://codereview.chromium.org/3110006/diff/21002/30009#newcode219 net/http/http_proxy_client_socket_pool.cc:219: return tcp_pool_->ConnectionTimeout() + This also needs to be updated. See the function in SSLClientSocketPool. http://codereview.chromium.org/3110006/diff/21002/30010 File net/http/http_proxy_client_socket_pool.h (right): http://codereview.chromium.org/3110006/diff/21002/30010#newcode16 net/http/http_proxy_client_socket_pool.h:16: #include "net/base/host_resolver.h" You should rebase your CL. http://codereview.chromium.org/3110006/diff/21002/30010#newcode37 net/http/http_proxy_client_socket_pool.h:37: HttpProxySocketParams(const scoped_refptr<TCPSocketParams>& http_proxy, A comment about when each type of socket param is needed would be good. See SSLSocketParams http://codereview.chromium.org/3110006/diff/21002/30010#newcode38 net/http/http_proxy_client_socket_pool.h:38: const scoped_refptr<SSLSocketParams>& https_proxy, Would be better if args and accessors matched. http://codereview.chromium.org/3110006/diff/21002/30010#newcode91 net/http/http_proxy_client_socket_pool.h:91: kStateSslConnect, SSL http://codereview.chromium.org/3110006/diff/21002/30010#newcode129 net/http/http_proxy_client_socket_pool.h:129: scoped_ptr<ClientSocketHandle> socket_handle_; transport_socket_handle_ ? (see SSLClientSocketPool) http://codereview.chromium.org/3110006/diff/21002/30011 File net/http/http_proxy_client_socket_pool_unittest.cc (right): http://codereview.chromium.org/3110006/diff/21002/30011#newcode63 net/http/http_proxy_client_socket_pool_unittest.cc:63: http_proxy_histograms_, NULL, tcp_socket_pool_, ssl_socket_pool_, Do any of these tests actually need the ssl pool? i.e. would NULL work? http://codereview.chromium.org/3110006/diff/21002/30013 File net/socket/socket_test_util.h (right): http://codereview.chromium.org/3110006/diff/21002/30013#newcode743 net/socket/socket_test_util.h:743: // TCPClientSocketPool methods. SSL
http://codereview.chromium.org/3110006/diff/21002/30002 File net/http/http_network_session.cc (right): http://codereview.chromium.org/3110006/diff/21002/30002#newcode96 net/http/http_network_session.cc:96: new SSLClientSocketPool( On 2010/08/11 19:02:05, vandebo wrote: > Maybe HttpNetworkSession::GetSocketPoolForSSLWithProxy should populate the TCP > pool part, and then we could call that from here. Hmm... Currently GetSocketPoolForSSLWithProxy calls GetSocketPoolForHTTPProxy, so we'd have to make sure we don't recurse infinitely. But I'm not sure that I understand the motivation. Do you think it would be more correct to go that route? http://codereview.chromium.org/3110006/diff/21002/30004 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/3110006/diff/21002/30004#newcode753 net/http/http_network_transaction.cc:753: if (using_ssl_ && !authentication_url.SchemeIs("https")) { On 2010/08/11 19:02:05, vandebo wrote: > Does this code need to run in the https proxy case as well? If so, maybe better > to handle the http and https case in one block. The differences are only tcp or > ssl params? Yeah, I think you're right. Done. http://codereview.chromium.org/3110006/diff/21002/30004#newcode805 net/http/http_network_transaction.cc:805: scoped_refptr<SSLClientSocketPool> ssl_pool = proxy_info_.is_direct() On 2010/08/11 19:02:05, vandebo wrote: > Should this be is_direct() || is_https() ? When layering an ssl socket on top > of another ssl socket, you probably want to use different ssl pools in order to > not double count against the total socket limit. Hmmm... Interesting. There are a lot of nested pools here. So in the case of SSL over SSL through the proxy, the HttpProxyClientSocketPool will contain and SSLClientSocketPool. When we call GetSocketPoolForSSLWithProxy, we create a new SSLClientSocketPool which sits on top of the HttpProxyClientSocketPool (and hence on top of the SSLClientSocketPool). So this sounds to me like we already have two different SSLClientSocketPools. We only allocate a socket from the lowest pool when we create a new connection to the proxy. And we only allocate a socket from the higher pool when we make an SSL connection to an endpoint; doing so does not allocate an additional socket from the lower pool. Does that sounds right to you, or am I misunderstanding the problem? http://codereview.chromium.org/3110006/diff/21002/30004#newcode806 net/http/http_network_transaction.cc:806: ? session_->ssl_socket_pool() On 2010/08/11 19:02:05, vandebo wrote: > no reason for the trinary operator here. Heh, I take it from your phrasing that the trinary operator is not particularly popular here? I used it in this case because the code is 3 lines instead of 5 with an if, and because it only references ssl_pool a single time. We used it extensively at my last job and so I've come to love it, but if that's not the style here, I'll unlearn it :> http://codereview.chromium.org/3110006/diff/21002/30004#newcode843 net/http/http_network_transaction.cc:843: ssl_started = ssl_started || proxy_info_.is_https(); On 2010/08/11 19:02:05, vandebo wrote: > This is incorrect - the counter example is a tcp connect problem when trying for > an https proxy. *nod* In thinking about this more, I think that the right course of action is going to be to completely hide the "SSL to the proxy" details from the HttpNetworkTransaction. What do you think about that? http://codereview.chromium.org/3110006/diff/21002/30004#newcode1102 net/http/http_network_transaction.cc:1102: !using_ssl_ && proxy_info_.is_http(), &request_line, On 2010/08/11 19:02:05, vandebo wrote: > Do you want auth headers in the is_https() && using_ssl_ case? I thought that > would be handled with an Http Connect. Good point. Done. http://codereview.chromium.org/3110006/diff/21002/30004#newcode1249 net/http/http_network_transaction.cc:1249: SSLClientSocket* ssl_socket = GetSslClientSocket(); On 2010/08/11 19:02:05, vandebo wrote: > In this case you don't want the ssl socket to the https proxy, you want the ssl > socket that's layered on top of it. Good point. Done. http://codereview.chromium.org/3110006/diff/21002/30007 File net/http/http_proxy_client_socket.cc (right): http://codereview.chromium.org/3110006/diff/21002/30007#newcode79 net/http/http_proxy_client_socket.cc:79: // DCHECK(transport_->socket()->IsConnected()); On 2010/08/11 19:02:05, vandebo wrote: > This should get figured out. As far as I know, the transport socket should > always be connected when we connect the HttpProxy socket. Totally agree. It seems to always be true when I'm running chrome, but no when I ran the unit test which mocked out the network and attempts to use an https proxy. I will track this down, though. http://codereview.chromium.org/3110006/diff/21002/30007#newcode213 net/http/http_proxy_client_socket.cc:213: ClientSocket* HttpProxyClientSocket::socket() const { On 2010/08/11 19:02:05, vandebo wrote: > Note to self: Hmm? So I needed this in order to extract the SSLClientSocket used to talk to the HTTPS proxy. After thinking about this (and what to do with cert errors when talking to the proxy) it makes me wonder if we should completely hide the detail of the SSL communication to the proxy from HttpNetworkTransaction. Does that sound desirable to you? http://codereview.chromium.org/3110006/diff/21002/30008 File net/http/http_proxy_client_socket.h (right): http://codereview.chromium.org/3110006/diff/21002/30008#newcode76 net/http/http_proxy_client_socket.h:76: ClientSocket* socket() const; On 2010/08/11 19:02:05, vandebo wrote: > transport_socket() ? Done. http://codereview.chromium.org/3110006/diff/21002/30009 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/21002/30009#newcode160 net/http/http_proxy_client_socket_pool.cc:160: result = OK; // Ignore cert errors for now On 2010/08/11 19:02:05, vandebo wrote: > What's the story for handling this case? Fine to make progress, but IMHO, this > isn't production ready. Good question. In talking with folks on the security side it sounds like they would like us to not use an HTTPS proxy if it has any sort of SSL issues, ala STS. At the moment, I don't have a valid cert to play with so I need to use something more liberal for now. Do you have a sense for how you would like to see this play out? http://codereview.chromium.org/3110006/diff/21002/30009#newcode165 net/http/http_proxy_client_socket_pool.cc:165: // so that a fast TCP connection plus a slow HttpProxy failure doesn't take On 2010/08/11 19:02:05, vandebo wrote: > TCP->SSL Done. http://codereview.chromium.org/3110006/diff/21002/30009#newcode176 net/http/http_proxy_client_socket_pool.cc:176: params_->tcp_params() == NULL On 2010/08/11 19:02:05, vandebo wrote: > an if might be easier to read here. No love for the ternary operator. :> I'm sure this is my C++ newbie-ness showing up but, I'm not clear how to do this with an if. Since tcp_destination is a reference, I need to initialize it when I declare it. Something like this doesn't work: const HostResolver::RequestInfo& tcp_destination; if (params_->tcp_params() == NULL) tcp_destination = params_->ssl_params()->tcp_params()->destination(); else tcp_destination = params_->tcp_params()->destination(); Am I missing something obvious? In any case, I extracted the logic into a destination() method on HttpProxyClientSocketParams, and that seems to work nicely. Does that work for you? http://codereview.chromium.org/3110006/diff/21002/30009#newcode219 net/http/http_proxy_client_socket_pool.cc:219: return tcp_pool_->ConnectionTimeout() + On 2010/08/11 19:02:05, vandebo wrote: > This also needs to be updated. See the function in SSLClientSocketPool. So SSLClientSocketPool returns a timeout set in the constructor by looking at the timeouts from the respective pools. In our case, I'm guessing we should simply return the ConnectionTimout() from whichever pool is non null? (I'm not sure that I understood the logic in SSLClientSocketPool for, say, the SOCKS case: pool_timeout = socks_pool_->ConnectionTimeout(); if (pool_timeout > max_transport_timeout) max_transport_timeout = pool_timeout; max_transport_timeout seems to be initialized to 0, so I'm guessing pool_timeout can sometimes be negative?) http://codereview.chromium.org/3110006/diff/21002/30010 File net/http/http_proxy_client_socket_pool.h (right): http://codereview.chromium.org/3110006/diff/21002/30010#newcode16 net/http/http_proxy_client_socket_pool.h:16: #include "net/base/host_resolver.h" On 2010/08/11 19:02:05, vandebo wrote: > You should rebase your CL. Done. http://codereview.chromium.org/3110006/diff/21002/30010#newcode37 net/http/http_proxy_client_socket_pool.h:37: HttpProxySocketParams(const scoped_refptr<TCPSocketParams>& http_proxy, On 2010/08/11 19:02:05, vandebo wrote: > A comment about when each type of socket param is needed would be good. See > SSLSocketParams Done. I also added the following check to the constructor: DCHECK((tcp_params == NULL && ssl_params != NULL) || (tcp_params != NULL && ssl_params == NULL)); http://codereview.chromium.org/3110006/diff/21002/30010#newcode38 net/http/http_proxy_client_socket_pool.h:38: const scoped_refptr<SSLSocketParams>& https_proxy, On 2010/08/11 19:02:05, vandebo wrote: > Would be better if args and accessors matched. Happy to do that! The code earlier had "proxy_server" as the arg, and tcp_params as the accessor so I stayed with the convention. I've changed the args to match the accessors. http://codereview.chromium.org/3110006/diff/21002/30010#newcode91 net/http/http_proxy_client_socket_pool.h:91: kStateSslConnect, On 2010/08/11 19:02:05, vandebo wrote: > SSL Hm, really? I sent mail yesterday about SSL vs Ssl, and I thought the conclusion was that the style guide says to treat acronyms as words. This from Tony Gentilcore: The style guide treats acronyms as words. See the Url example here: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Type_N... http://codereview.chromium.org/3110006/diff/21002/30010#newcode129 net/http/http_proxy_client_socket_pool.h:129: scoped_ptr<ClientSocketHandle> socket_handle_; On 2010/08/11 19:02:05, vandebo wrote: > transport_socket_handle_ ? (see SSLClientSocketPool) Done. http://codereview.chromium.org/3110006/diff/21002/30011 File net/http/http_proxy_client_socket_pool_unittest.cc (right): http://codereview.chromium.org/3110006/diff/21002/30011#newcode63 net/http/http_proxy_client_socket_pool_unittest.cc:63: http_proxy_histograms_, NULL, tcp_socket_pool_, ssl_socket_pool_, On 2010/08/11 19:02:05, vandebo wrote: > Do any of these tests actually need the ssl pool? i.e. would NULL work? Wow, good point. I'm not testing the new ssl codepath. Since the semantics of the HttpProxyClientSocketPool are virtually identical if the proxy is ssl or straight tcp, I suspect we should make each of the existing tests try both the ssl and tcp cases? Does that sound like the right approach? http://codereview.chromium.org/3110006/diff/21002/30013 File net/socket/socket_test_util.h (right): http://codereview.chromium.org/3110006/diff/21002/30013#newcode743 net/socket/socket_test_util.h:743: // TCPClientSocketPool methods. On 2010/08/11 19:02:05, vandebo wrote: > SSL Done.
http://codereview.chromium.org/3110006/diff/21002/30004 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/3110006/diff/21002/30004#newcode1102 net/http/http_network_transaction.cc:1102: !using_ssl_ && proxy_info_.is_http(), &request_line, On 2010/08/11 19:02:05, vandebo wrote: > Do you want auth headers in the is_https() && using_ssl_ case? I thought that > would be handled with an Http Connect. I've just uploaded a new version of the CL which makes a few minor changes to support proxy auth (and server auth, for that matter) in the https proxy case. http://codereview.chromium.org/3110006/diff/21002/30004#newcode1169 net/http/http_network_transaction.cc:1169: SSLClientSocket* ssl_socket = GetSslClientSocket(); On 2010/08/11 19:02:05, vandebo wrote: > I'm not sure this would work in the Https proxy case. Agreed. For the moment, I have change GetSslClientSocket to DCHECK(using_ssl_) and to simply return connection_->socket() with the appropriate cast. This is a first attempt at hiding from HttpNetworkTransaction, the details of the SSL to the proxy . http://codereview.chromium.org/3110006/diff/21002/30007 File net/http/http_proxy_client_socket.cc (right): http://codereview.chromium.org/3110006/diff/21002/30007#newcode79 net/http/http_proxy_client_socket.cc:79: // DCHECK(transport_->socket()->IsConnected()); On 2010/08/11 19:02:05, vandebo wrote: > This should get figured out. As far as I know, the transport socket should > always be connected when we connect the HttpProxy socket. So I did some digging. I'm able to get my test to pass if I change the ssl data provider to not be async: - SSLSocketDataProvider ssl(true, OK); + SSLSocketDataProvider ssl(false, OK); I'm not sure that I understand the MockSSLClientSocket implementation correctly, but it looks like there might be a bug in it's Connect method. ConnectCallback* connect_callback = new ConnectCallback( this, callback, data_->connect.result); int rv = transport_->socket()->Connect(connect_callback); if (rv == net::OK) { delete connect_callback; if (data_->connect.async) { RunCallbackAsync(callback, data_->connect.result); return net::ERR_IO_PENDING; } Notice that in the async case, it appears to be running the passed in callback. This callback does not appear to set the connected status of the MockSSLClientSocket. Do you read this the same way?
a few minor comments so far. http://codereview.chromium.org/3110006/diff/49004/50004 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/3110006/diff/49004/50004#newcode1797 net/http/http_network_transaction.cc:1797: return GURL("https://" + I probably should have done something like: const char* scheme = proxy_info_.is_http() ? "http://" : "https://"; and then the GURL line doesn't have to be duplicated. http://codereview.chromium.org/3110006/diff/49004/50004#newcode1829 net/http/http_network_transaction.cc:1829: DCHECK(using_ssl_); using_ssl_ starts to get confusing now - is it the connection which is SSL? Or is it the request which is SSL? Let's at least disambiguate. I suspect there might be places in this code which think that using_ssl_ means that we're fetching a https:// request? http://codereview.chromium.org/3110006/diff/49004/50005 File net/http/http_network_transaction.h (right): http://codereview.chromium.org/3110006/diff/49004/50005#newcode260 net/http/http_network_transaction.h:260: SSLClientSocket* GetSslClientSocket(); callers should be aware that calling this function on a non-ssl socket will likely lead to nasty crashes! http://codereview.chromium.org/3110006/diff/49004/50007 File net/http/http_proxy_client_socket.cc (right): http://codereview.chromium.org/3110006/diff/49004/50007#newcode81 net/http/http_proxy_client_socket.cc:81: // DCHECK(transport_->socket()->IsConnected()); style: indentation
http://codereview.chromium.org/3110006/diff/49004/50004 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/3110006/diff/49004/50004#newcode1797 net/http/http_network_transaction.cc:1797: return GURL("https://" + On 2010/08/13 21:43:04, Mike Belshe wrote: > I probably should have done something like: > const char* scheme = proxy_info_.is_http() ? "http://" : "https://"; > > and then the GURL line doesn't have to be duplicated. Ah! Yes, much better. For some reason I thought that "+" was only defined for strings, not for char *, but your suggestion works perfectly. http://codereview.chromium.org/3110006/diff/49004/50004#newcode1829 net/http/http_network_transaction.cc:1829: DCHECK(using_ssl_); On 2010/08/13 21:43:04, Mike Belshe wrote: > using_ssl_ starts to get confusing now - is it the connection which is SSL? Or > is it the request which is SSL? Let's at least disambiguate. I suspect there > might be places in this code which think that using_ssl_ means that we're > fetching a https:// request? After Steve's feedback, I decided to go with the earlier definition of using_ssl_ which is an https request. (The mere presence of an HTTPS proxy in the request, or the use of spdy, will not cause it to be true.) As a result, this method returns the SSL connection to the origin server. Sound plausible? I've updated the comment in the .h file to reflect this definition... I would be happy to rename this to is_https_request, ala your change if you'd prefer? (Though since you're about to do that, I can also wait) http://codereview.chromium.org/3110006/diff/49004/50005 File net/http/http_network_transaction.h (right): http://codereview.chromium.org/3110006/diff/49004/50005#newcode260 net/http/http_network_transaction.h:260: SSLClientSocket* GetSslClientSocket(); On 2010/08/13 21:43:04, Mike Belshe wrote: > callers should be aware that calling this function on a non-ssl socket will > likely lead to nasty crashes! I've updated the comment. Should I change the DCHECK(using_ssl_) to a CHECK? http://codereview.chromium.org/3110006/diff/49004/50007 File net/http/http_proxy_client_socket.cc (right): http://codereview.chromium.org/3110006/diff/49004/50007#newcode81 net/http/http_proxy_client_socket.cc:81: // DCHECK(transport_->socket()->IsConnected()); On 2010/08/13 21:43:04, Mike Belshe wrote: > style: indentation Done. (Though I hope to un-comment this before I commit)
Sorry for the long delay. http://codereview.chromium.org/3110006/diff/21002/30002 File net/http/http_network_session.cc (right): http://codereview.chromium.org/3110006/diff/21002/30002#newcode96 net/http/http_network_session.cc:96: new SSLClientSocketPool( On 2010/08/12 04:18:39, rch wrote: > On 2010/08/11 19:02:05, vandebo wrote: > > Maybe HttpNetworkSession::GetSocketPoolForSSLWithProxy should populate the TCP > > pool part, and then we could call that from here. > > Hmm... Currently GetSocketPoolForSSLWithProxy calls GetSocketPoolForHTTPProxy, > so we'd have to make sure we don't recurse infinitely. But I'm not sure that I > understand the motivation. Do you think it would be more correct to go that > route? The motivation was just to control the number of pools. But an infinite recursion is of course no good. http://codereview.chromium.org/3110006/diff/21002/30004 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/3110006/diff/21002/30004#newcode806 net/http/http_network_transaction.cc:806: ? session_->ssl_socket_pool() On 2010/08/12 04:18:39, rch wrote: > On 2010/08/11 19:02:05, vandebo wrote: > > no reason for the trinary operator here. > > Heh, I take it from your phrasing that the trinary operator is not particularly > popular here? I used it in this case because the code is 3 lines instead of 5 > with an if, and because it only references ssl_pool a single time. We used it > extensively at my last job and so I've come to love it, but if that's not the > style here, I'll unlearn it :> It's used infrequently, at least in this bit of code. An 'if' conditional is more explicit and easier to read. There are some places where an 'if' isn't practical, so the '?' operator is appropriate. i.e. use it sparingly. http://codereview.chromium.org/3110006/diff/21002/30009 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/21002/30009#newcode160 net/http/http_proxy_client_socket_pool.cc:160: result = OK; // Ignore cert errors for now On 2010/08/12 04:18:39, rch wrote: > On 2010/08/11 19:02:05, vandebo wrote: > > What's the story for handling this case? Fine to make progress, but IMHO, > this > > isn't production ready. > > Good question. In talking with folks on the security side it sounds like they > would like us to not use an HTTPS proxy if it has any sort of SSL issues, ala > STS. At the moment, I don't have a valid cert to play with so I need to use > something more liberal for now. Do you have a sense for how you would like to > see this play out? Rejecting proxy connections with cert errors seems reasonable. I wonder if using the flags to ignore cert errors make it possible change this now and still test the code. http://codereview.chromium.org/3110006/diff/21002/30009#newcode219 net/http/http_proxy_client_socket_pool.cc:219: return tcp_pool_->ConnectionTimeout() + On 2010/08/12 04:18:39, rch wrote: > On 2010/08/11 19:02:05, vandebo wrote: > > This also needs to be updated. See the function in SSLClientSocketPool. > > So SSLClientSocketPool returns a timeout set in the constructor by looking at > the timeouts from the respective pools. In our case, I'm guessing we should > simply return the ConnectionTimout() from whichever pool is non null? (I'm not > sure that I understood the logic in SSLClientSocketPool for, say, the SOCKS > case: > > pool_timeout = socks_pool_->ConnectionTimeout(); > if (pool_timeout > max_transport_timeout) > max_transport_timeout = pool_timeout; > > max_transport_timeout seems to be initialized to 0, so I'm guessing pool_timeout > can sometimes be negative?) This code is in the ConnectJobFactory constructor. It doesn't know what kind of proxy the connect job will be for, so it has to use the maximum timeout. There's a limitation in the lower level pool implementation that makes this the case. http://codereview.chromium.org/3110006/diff/21002/30010 File net/http/http_proxy_client_socket_pool.h (right): http://codereview.chromium.org/3110006/diff/21002/30010#newcode91 net/http/http_proxy_client_socket_pool.h:91: kStateSslConnect, On 2010/08/12 04:18:39, rch wrote: > On 2010/08/11 19:02:05, vandebo wrote: > > SSL > > Hm, really? I sent mail yesterday about SSL vs Ssl, and I thought the > conclusion was that the style guide says to treat acronyms as words. > > This from Tony Gentilcore: > > The style guide treats acronyms as words. See the Url example here: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Type_N... Yes, because of http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Existing_Non-c... The current code uses SSL, so for consistency... http://codereview.chromium.org/3110006/diff/21002/30011 File net/http/http_proxy_client_socket_pool_unittest.cc (right): http://codereview.chromium.org/3110006/diff/21002/30011#newcode63 net/http/http_proxy_client_socket_pool_unittest.cc:63: http_proxy_histograms_, NULL, tcp_socket_pool_, ssl_socket_pool_, On 2010/08/12 04:18:39, rch wrote: > On 2010/08/11 19:02:05, vandebo wrote: > > Do any of these tests actually need the ssl pool? i.e. would NULL work? > > Wow, good point. I'm not testing the new ssl codepath. Since the semantics of > the HttpProxyClientSocketPool are virtually identical if the proxy is ssl or > straight tcp, I suspect we should make each of the existing tests try both the > ssl and tcp cases? Does that sound like the right approach? Seems reasonable. http://codereview.chromium.org/3110006/diff/44001/45003 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/3110006/diff/44001/45003#newcode766 net/http/http_network_transaction.cc:766: if (proxy_info_.is_http()) { You can make this simpler with: scoped ... ssl_params; if (proxy_info_.is_https()) ssl_params = ... http://codereview.chromium.org/3110006/diff/44001/45008 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/44001/45008#newcode35 net/http/http_proxy_client_socket_pool.cc:35: const HostResolver::RequestInfo& HttpProxySocketParams::destination() const { like. http://codereview.chromium.org/3110006/diff/44001/45008#newcode228 net/http/http_proxy_client_socket_pool.cc:228: return ssl_pool_->ConnectionTimeout(); Need to add the HttpProxyConnectJob timeout to both of these. http://codereview.chromium.org/3110006/diff/44001/45009 File net/http/http_proxy_client_socket_pool.h (right): http://codereview.chromium.org/3110006/diff/44001/45009#newcode201 net/http/http_proxy_client_socket_pool.h:201: net_log_(net_log) { } extra space http://codereview.chromium.org/3110006/diff/44001/45013 File net/socket/ssl_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/44001/45013#newcode13 net/socket/ssl_client_socket_pool.cc:13: #include "net/socket/ssl_client_socket.h" Bad merge? http://codereview.chromium.org/3110006/diff/13011/54004 File net/http/http_network_transaction.cc (left): http://codereview.chromium.org/3110006/diff/13011/54004#oldcode1740 net/http/http_network_transaction.cc:1740: No need to remove these formatting lines. http://codereview.chromium.org/3110006/diff/13011/54004 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/3110006/diff/13011/54004#newcode924 net/http/http_network_transaction.cc:924: DCHECK(using_ssl_ || proxy_info_.is_https()); You're not passing cert errors up, so this change isn't necessary. http://codereview.chromium.org/3110006/diff/13011/54004#newcode1363 net/http/http_network_transaction.cc:1363: want_ssl_over_spdy_without_npn_, I know you're just moving this, but since this argument doesn't fit on one line, they should all just start four spaces in from the function call. http://codereview.chromium.org/3110006/diff/13011/54004#newcode1788 net/http/http_network_transaction.cc:1788: { {'s don't go on new lines. http://codereview.chromium.org/3110006/diff/13011/54005 File net/http/http_network_transaction.h (right): http://codereview.chromium.org/3110006/diff/13011/54005#newcode258 net/http/http_network_transaction.h:258: // Helper method for retrieving the SSL socket for the current request. Since this method doesn't do anything extra anymore, I'd rather just return the call sites to how they were. Over time, the trajectory should be to remove casts and leaving them ugly makes that more likely to happen. http://codereview.chromium.org/3110006/diff/13011/54006 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/3110006/diff/13011/54006#newcode1670 net/http/http_network_transaction_unittest.cc:1670: TEST_F(HttpNetworkTransactionTest, HttpsProxyGet) { Should also test the failing cases. http://codereview.chromium.org/3110006/diff/13011/54007 File net/http/http_proxy_client_socket.cc (right): http://codereview.chromium.org/3110006/diff/13011/54007#newcode81 net/http/http_proxy_client_socket.cc:81: //DCHECK(transport_->socket()->IsConnected()); Is this fixed by http://crrev.com/56219 ? http://codereview.chromium.org/3110006/diff/13011/54008 File net/http/http_proxy_client_socket.h (right): http://codereview.chromium.org/3110006/diff/13011/54008#newcode76 net/http/http_proxy_client_socket.h:76: // TODO(rch): can we find a way to hide the details of this? Is this used anymore? http://codereview.chromium.org/3110006/diff/13011/54009 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/13011/54009#newcode86 net/http/http_proxy_client_socket_pool.cc:86: next_state_ = params_->tcp_params() == NULL No need for the trinary operator here. http://codereview.chromium.org/3110006/diff/13011/54009#newcode225 net/http/http_proxy_client_socket_pool.cc:225: if (tcp_pool_) Won't tcp_pool_ always be true here? http://codereview.chromium.org/3110006/diff/13011/54011 File net/http/http_proxy_client_socket_pool_unittest.cc (right): http://codereview.chromium.org/3110006/diff/13011/54011#newcode43 net/http/http_proxy_client_socket_pool_unittest.cc:43: ssl_socket_pool_(new MockSSLClientSocketPool(kMaxSockets, This isn't used (yet?). http://codereview.chromium.org/3110006/diff/13011/54013 File net/socket/socket_test_util.h (right): http://codereview.chromium.org/3110006/diff/13011/54013#newcode713 net/socket/socket_test_util.h:713: class MockSSLClientSocketPool : public SSLClientSocketPool { Not used?
http://codereview.chromium.org/3110006/diff/21002/30011 File net/http/http_proxy_client_socket_pool_unittest.cc (right): http://codereview.chromium.org/3110006/diff/21002/30011#newcode63 net/http/http_proxy_client_socket_pool_unittest.cc:63: http_proxy_histograms_, NULL, tcp_socket_pool_, ssl_socket_pool_, On 2010/08/17 02:01:07, vandebo wrote: > On 2010/08/12 04:18:39, rch wrote: > > On 2010/08/11 19:02:05, vandebo wrote: > > > Do any of these tests actually need the ssl pool? i.e. would NULL work? > > > > Wow, good point. I'm not testing the new ssl codepath. Since the semantics of > > the HttpProxyClientSocketPool are virtually identical if the proxy is ssl or > > straight tcp, I suspect we should make each of the existing tests try both the > > ssl and tcp cases? Does that sound like the right approach? > > Seems reasonable. Done! I was able to make this test parameterized on the proxy type and so test both cases. http://codereview.chromium.org/3110006/diff/44001/45003 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/3110006/diff/44001/45003#newcode766 net/http/http_network_transaction.cc:766: if (proxy_info_.is_http()) { On 2010/08/17 02:01:07, vandebo wrote: > You can make this simpler with: > scoped ... ssl_params; > if (proxy_info_.is_https()) > ssl_params = ... > Done. http://codereview.chromium.org/3110006/diff/44001/45008 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/44001/45008#newcode228 net/http/http_proxy_client_socket_pool.cc:228: return ssl_pool_->ConnectionTimeout(); On 2010/08/17 02:01:07, vandebo wrote: > Need to add the HttpProxyConnectJob timeout to both of these. Done. http://codereview.chromium.org/3110006/diff/44001/45009 File net/http/http_proxy_client_socket_pool.h (right): http://codereview.chromium.org/3110006/diff/44001/45009#newcode201 net/http/http_proxy_client_socket_pool.h:201: net_log_(net_log) { } On 2010/08/17 02:01:07, vandebo wrote: > extra space Done. http://codereview.chromium.org/3110006/diff/44001/45013 File net/socket/ssl_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/44001/45013#newcode13 net/socket/ssl_client_socket_pool.cc:13: #include "net/socket/ssl_client_socket.h" On 2010/08/17 02:01:07, vandebo wrote: > Bad merge? Done. http://codereview.chromium.org/3110006/diff/13011/54004 File net/http/http_network_transaction.cc (left): http://codereview.chromium.org/3110006/diff/13011/54004#oldcode1740 net/http/http_network_transaction.cc:1740: On 2010/08/17 02:01:08, vandebo wrote: > No need to remove these formatting lines. Done. http://codereview.chromium.org/3110006/diff/13011/54004 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/3110006/diff/13011/54004#newcode924 net/http/http_network_transaction.cc:924: DCHECK(using_ssl_ || proxy_info_.is_https()); On 2010/08/17 02:01:08, vandebo wrote: > You're not passing cert errors up, so this change isn't necessary. Done. http://codereview.chromium.org/3110006/diff/13011/54004#newcode1363 net/http/http_network_transaction.cc:1363: want_ssl_over_spdy_without_npn_, On 2010/08/17 02:01:08, vandebo wrote: > I know you're just moving this, but since this argument doesn't fit on one line, > they should all just start four spaces in from the function call. Done. http://codereview.chromium.org/3110006/diff/13011/54004#newcode1788 net/http/http_network_transaction.cc:1788: { On 2010/08/17 02:01:08, vandebo wrote: > {'s don't go on new lines. Even for case blocks? Ok, done. http://codereview.chromium.org/3110006/diff/13011/54005 File net/http/http_network_transaction.h (right): http://codereview.chromium.org/3110006/diff/13011/54005#newcode258 net/http/http_network_transaction.h:258: // Helper method for retrieving the SSL socket for the current request. On 2010/08/17 02:01:08, vandebo wrote: > Since this method doesn't do anything extra anymore, I'd rather just return the > call sites to how they were. Over time, the trajectory should be to remove > casts and leaving them ugly makes that more likely to happen. Done. http://codereview.chromium.org/3110006/diff/13011/54006 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/3110006/diff/13011/54006#newcode1670 net/http/http_network_transaction_unittest.cc:1670: TEST_F(HttpNetworkTransactionTest, HttpsProxyGet) { On 2010/08/17 02:01:08, vandebo wrote: > Should also test the failing cases. Looking at the existing proxy tests, I'll add HTTPS testing to BasicAuthProxyCancelTunnel, HTTPSBadCertificateViaProxy, ReconsiderProxyAfterFailedConnection Does that sound right to you? http://codereview.chromium.org/3110006/diff/13011/54007 File net/http/http_proxy_client_socket.cc (right): http://codereview.chromium.org/3110006/diff/13011/54007#newcode81 net/http/http_proxy_client_socket.cc:81: //DCHECK(transport_->socket()->IsConnected()); On 2010/08/17 02:01:08, vandebo wrote: > Is this fixed by http://crrev.com/56219 ? Yes. Woo hoo :> http://codereview.chromium.org/3110006/diff/13011/54008 File net/http/http_proxy_client_socket.h (right): http://codereview.chromium.org/3110006/diff/13011/54008#newcode76 net/http/http_proxy_client_socket.h:76: // TODO(rch): can we find a way to hide the details of this? On 2010/08/17 02:01:08, vandebo wrote: > Is this used anymore? Done. http://codereview.chromium.org/3110006/diff/13011/54009 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/13011/54009#newcode86 net/http/http_proxy_client_socket_pool.cc:86: next_state_ = params_->tcp_params() == NULL On 2010/08/17 02:01:08, vandebo wrote: > No need for the trinary operator here. Done. http://codereview.chromium.org/3110006/diff/13011/54009#newcode225 net/http/http_proxy_client_socket_pool.cc:225: if (tcp_pool_) On 2010/08/17 02:01:08, vandebo wrote: > Won't tcp_pool_ always be true here? Ugh. You're totally right. So if I read this correctly, the Factory is required to have a single ConnectionTimeout, but in the HttpProxy case, there are two different code paths based on the args passed in to the actual job. I'm not sure how to resolve this. It appears that the semantics are mismatch here. How would you like to see me resolve this? http://codereview.chromium.org/3110006/diff/13011/54011 File net/http/http_proxy_client_socket_pool_unittest.cc (right): http://codereview.chromium.org/3110006/diff/13011/54011#newcode43 net/http/http_proxy_client_socket_pool_unittest.cc:43: ssl_socket_pool_(new MockSSLClientSocketPool(kMaxSockets, On 2010/08/17 02:01:08, vandebo wrote: > This isn't used (yet?). After parameterizing this test to use both HTTP and HTTPS proxies, this is now used. (But yeah, you're right, it was not being used.) http://codereview.chromium.org/3110006/diff/13011/54013 File net/socket/socket_test_util.h (right): http://codereview.chromium.org/3110006/diff/13011/54013#newcode713 net/socket/socket_test_util.h:713: class MockSSLClientSocketPool : public SSLClientSocketPool { On 2010/08/17 02:01:08, vandebo wrote: > Not used? Used now that HttpProxyClientSocketTest actually tests the HTTPS proxy case.
http://codereview.chromium.org/3110006/diff/21002/30011 File net/http/http_proxy_client_socket_pool_unittest.cc (right): http://codereview.chromium.org/3110006/diff/21002/30011#newcode63 net/http/http_proxy_client_socket_pool_unittest.cc:63: http_proxy_histograms_, NULL, tcp_socket_pool_, ssl_socket_pool_, On 2010/08/17 02:01:07, vandebo wrote: > On 2010/08/12 04:18:39, rch wrote: > > On 2010/08/11 19:02:05, vandebo wrote: > > > Do any of these tests actually need the ssl pool? i.e. would NULL work? > > > > Wow, good point. I'm not testing the new ssl codepath. Since the semantics of > > the HttpProxyClientSocketPool are virtually identical if the proxy is ssl or > > straight tcp, I suspect we should make each of the existing tests try both the > > ssl and tcp cases? Does that sound like the right approach? > > Seems reasonable. Done! I was able to make this test parameterized on the proxy type and so test both cases. http://codereview.chromium.org/3110006/diff/44001/45003 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/3110006/diff/44001/45003#newcode766 net/http/http_network_transaction.cc:766: if (proxy_info_.is_http()) { On 2010/08/17 02:01:07, vandebo wrote: > You can make this simpler with: > scoped ... ssl_params; > if (proxy_info_.is_https()) > ssl_params = ... > Done. http://codereview.chromium.org/3110006/diff/44001/45008 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/44001/45008#newcode228 net/http/http_proxy_client_socket_pool.cc:228: return ssl_pool_->ConnectionTimeout(); On 2010/08/17 02:01:07, vandebo wrote: > Need to add the HttpProxyConnectJob timeout to both of these. Done. http://codereview.chromium.org/3110006/diff/44001/45009 File net/http/http_proxy_client_socket_pool.h (right): http://codereview.chromium.org/3110006/diff/44001/45009#newcode201 net/http/http_proxy_client_socket_pool.h:201: net_log_(net_log) { } On 2010/08/17 02:01:07, vandebo wrote: > extra space Done. http://codereview.chromium.org/3110006/diff/44001/45013 File net/socket/ssl_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/44001/45013#newcode13 net/socket/ssl_client_socket_pool.cc:13: #include "net/socket/ssl_client_socket.h" On 2010/08/17 02:01:07, vandebo wrote: > Bad merge? Done. http://codereview.chromium.org/3110006/diff/13011/54004 File net/http/http_network_transaction.cc (left): http://codereview.chromium.org/3110006/diff/13011/54004#oldcode1740 net/http/http_network_transaction.cc:1740: On 2010/08/17 02:01:08, vandebo wrote: > No need to remove these formatting lines. Done. http://codereview.chromium.org/3110006/diff/13011/54004 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/3110006/diff/13011/54004#newcode924 net/http/http_network_transaction.cc:924: DCHECK(using_ssl_ || proxy_info_.is_https()); On 2010/08/17 02:01:08, vandebo wrote: > You're not passing cert errors up, so this change isn't necessary. Done. http://codereview.chromium.org/3110006/diff/13011/54004#newcode1363 net/http/http_network_transaction.cc:1363: want_ssl_over_spdy_without_npn_, On 2010/08/17 02:01:08, vandebo wrote: > I know you're just moving this, but since this argument doesn't fit on one line, > they should all just start four spaces in from the function call. Done. http://codereview.chromium.org/3110006/diff/13011/54004#newcode1788 net/http/http_network_transaction.cc:1788: { On 2010/08/17 02:01:08, vandebo wrote: > {'s don't go on new lines. Even for case blocks? Ok, done. http://codereview.chromium.org/3110006/diff/13011/54005 File net/http/http_network_transaction.h (right): http://codereview.chromium.org/3110006/diff/13011/54005#newcode258 net/http/http_network_transaction.h:258: // Helper method for retrieving the SSL socket for the current request. On 2010/08/17 02:01:08, vandebo wrote: > Since this method doesn't do anything extra anymore, I'd rather just return the > call sites to how they were. Over time, the trajectory should be to remove > casts and leaving them ugly makes that more likely to happen. Done. http://codereview.chromium.org/3110006/diff/13011/54006 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/3110006/diff/13011/54006#newcode1670 net/http/http_network_transaction_unittest.cc:1670: TEST_F(HttpNetworkTransactionTest, HttpsProxyGet) { On 2010/08/17 02:01:08, vandebo wrote: > Should also test the failing cases. Looking at the existing proxy tests, I'll add HTTPS testing to BasicAuthProxyCancelTunnel, HTTPSBadCertificateViaProxy, ReconsiderProxyAfterFailedConnection Does that sound right to you? http://codereview.chromium.org/3110006/diff/13011/54007 File net/http/http_proxy_client_socket.cc (right): http://codereview.chromium.org/3110006/diff/13011/54007#newcode81 net/http/http_proxy_client_socket.cc:81: //DCHECK(transport_->socket()->IsConnected()); On 2010/08/17 02:01:08, vandebo wrote: > Is this fixed by http://crrev.com/56219 ? Yes. Woo hoo :> http://codereview.chromium.org/3110006/diff/13011/54008 File net/http/http_proxy_client_socket.h (right): http://codereview.chromium.org/3110006/diff/13011/54008#newcode76 net/http/http_proxy_client_socket.h:76: // TODO(rch): can we find a way to hide the details of this? On 2010/08/17 02:01:08, vandebo wrote: > Is this used anymore? Done. http://codereview.chromium.org/3110006/diff/13011/54009 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/13011/54009#newcode86 net/http/http_proxy_client_socket_pool.cc:86: next_state_ = params_->tcp_params() == NULL On 2010/08/17 02:01:08, vandebo wrote: > No need for the trinary operator here. Done. http://codereview.chromium.org/3110006/diff/13011/54009#newcode225 net/http/http_proxy_client_socket_pool.cc:225: if (tcp_pool_) On 2010/08/17 02:01:08, vandebo wrote: > Won't tcp_pool_ always be true here? Ugh. You're totally right. So if I read this correctly, the Factory is required to have a single ConnectionTimeout, but in the HttpProxy case, there are two different code paths based on the args passed in to the actual job. I'm not sure how to resolve this. It appears that the semantics are mismatch here. How would you like to see me resolve this? http://codereview.chromium.org/3110006/diff/13011/54011 File net/http/http_proxy_client_socket_pool_unittest.cc (right): http://codereview.chromium.org/3110006/diff/13011/54011#newcode43 net/http/http_proxy_client_socket_pool_unittest.cc:43: ssl_socket_pool_(new MockSSLClientSocketPool(kMaxSockets, On 2010/08/17 02:01:08, vandebo wrote: > This isn't used (yet?). After parameterizing this test to use both HTTP and HTTPS proxies, this is now used. (But yeah, you're right, it was not being used.) http://codereview.chromium.org/3110006/diff/13011/54013 File net/socket/socket_test_util.h (right): http://codereview.chromium.org/3110006/diff/13011/54013#newcode713 net/socket/socket_test_util.h:713: class MockSSLClientSocketPool : public SSLClientSocketPool { On 2010/08/17 02:01:08, vandebo wrote: > Not used? Used now that HttpProxyClientSocketTest actually tests the HTTPS proxy case.
I'm basically happy with this change now; there's just a few small things and a rebase. http://codereview.chromium.org/3110006/diff/13011/54009 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/13011/54009#newcode225 net/http/http_proxy_client_socket_pool.cc:225: if (tcp_pool_) On 2010/08/18 14:46:29, rch wrote: > On 2010/08/17 02:01:08, vandebo wrote: > > Won't tcp_pool_ always be true here? > > Ugh. You're totally right. So if I read this correctly, the Factory is > required to have a single ConnectionTimeout, but in the HttpProxy case, there > are two different code paths based on the args passed in to the actual job. I'm > not sure how to resolve this. It appears that the semantics are mismatch here. > How would you like to see me resolve this? This is to set a timeout - hopefully it won't be hit. The conservative option (what is done in the SSL pool) is to take the max timeout. At least in the SSL case, a given pool may not have all the subordinate pools, we take the max timeout of all passed pools. http://codereview.chromium.org/3110006/diff/21018/14006 File net/http/http_network_session.h (right): http://codereview.chromium.org/3110006/diff/21018/14006#newcode155 net/http/http_network_session.h:155: tcp_for_https_proxy_pool_histograms_; When you land this change, you'll need to update the histogram description page. http://codereview.chromium.org/3110006/diff/21018/14007 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/3110006/diff/21018/14007#newcode685 net/http/http_network_transaction.cc:685: ProxyServer::SCHEME_DIRECT | mbelshe landed his stream factory, so this change needs to rebased :-( http://codereview.chromium.org/3110006/diff/21018/14007#newcode1790 net/http/http_network_transaction.cc:1790: const char* scheme = proxy_info_.is_http() ? "http://" : "https://"; This seems right, but might actually confuse the proxy. Unless you know this is the right thing, you should ask cbentzel about it. http://codereview.chromium.org/3110006/diff/21018/14009 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/3110006/diff/21018/14009#newcode1772 net/http/http_network_transaction_unittest.cc:1772: StaticSocketDataProvider data1(data_reads1, arraysize(data_reads1), Do you need two of everything here? I think this will happen over one connection, and thus you only need one data/ssl provider. http://codereview.chromium.org/3110006/diff/21018/14011 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/21018/14011#newcode170 net/http/http_proxy_client_socket_pool.cc:170: if (IsCertificateError(result)) If it will work, I'd really rather have tests set the ignore cert error flag and fail safe here. As soon as this goes into the tree, random people might start using it. http://codereview.chromium.org/3110006/diff/21018/14012 File net/http/http_proxy_client_socket_pool.h (right): http://codereview.chromium.org/3110006/diff/21018/14012#newcode119 net/http/http_proxy_client_socket_pool.h:119: int DoSslConnect(); SSL http://codereview.chromium.org/3110006/diff/21018/14013 File net/http/http_proxy_client_socket_pool_unittest.cc (right): http://codereview.chromium.org/3110006/diff/21018/14013#newcode30 net/http/http_proxy_client_socket_pool_unittest.cc:30: const char* kHostUrl = "http://host"; These constants seem overkill. http://codereview.chromium.org/3110006/diff/21018/14013#newcode79 net/http/http_proxy_client_socket_pool_unittest.cc:79: return scoped_refptr<TCPSocketParams>(NULL); I think the NULL is unnecessary.
I'll plan on looking this CL a bit more thoroughly after you merge with the HttpStreamFactory change. http://codereview.chromium.org/3110006/diff/21018/14007 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/3110006/diff/21018/14007#newcode1790 net/http/http_network_transaction.cc:1790: const char* scheme = proxy_info_.is_http() ? "http://" : "https://"; On 2010/08/19 18:40:56, vandebo wrote: > This seems right, but might actually confuse the proxy. Unless you know this is > the right thing, you should ask cbentzel about it. https: is the right thing here. Basically, the auth controllers will use this URL to lookup cached username/password values in the cache (amongst other things). It's also probably a corner case since it should only be used when doing an HTTP request through an HTTPS proxy which wants to use HTTP authentication. Nit: is there a better way to set the scheme? Also, you may want to explicitly check for is_http/is_https here on proxy_info_ rather than doing the ternary.
Ok, time to rebase for the StreamFactory. Whee! http://codereview.chromium.org/3110006/diff/13011/54009 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/13011/54009#newcode225 net/http/http_proxy_client_socket_pool.cc:225: if (tcp_pool_) On 2010/08/19 18:40:56, vandebo wrote: > On 2010/08/18 14:46:29, rch wrote: > > On 2010/08/17 02:01:08, vandebo wrote: > > > Won't tcp_pool_ always be true here? > > > > Ugh. You're totally right. So if I read this correctly, the Factory is > > required to have a single ConnectionTimeout, but in the HttpProxy case, there > > are two different code paths based on the args passed in to the actual job. > I'm > > not sure how to resolve this. It appears that the semantics are mismatch > here. > > How would you like to see me resolve this? > > This is to set a timeout - hopefully it won't be hit. The conservative option > (what is done in the SSL pool) is to take the max timeout. At least in the SSL > case, a given pool may not have all the subordinate pools, we take the max > timeout of all passed pools. > > Done. I decided to move the logic to the constructor (ala the SSL pool) and then moved the constructor to the .cc file. However, I'm not sure I wrapped it correctly. Does it look right to you? http://codereview.chromium.org/3110006/diff/21018/14006 File net/http/http_network_session.h (right): http://codereview.chromium.org/3110006/diff/21018/14006#newcode155 net/http/http_network_session.h:155: tcp_for_https_proxy_pool_histograms_; On 2010/08/19 18:40:56, vandebo wrote: > When you land this change, you'll need to update the histogram description page. Sounds great. Where/How do I do that? http://codereview.chromium.org/3110006/diff/21018/14007 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/3110006/diff/21018/14007#newcode685 net/http/http_network_transaction.cc:685: ProxyServer::SCHEME_DIRECT | On 2010/08/19 18:40:56, vandebo wrote: > mbelshe landed his stream factory, so this change needs to rebased :-( Yeah, I knew that was coming. Not thrilled about it, but I'd rather do the merge than force him to :> http://codereview.chromium.org/3110006/diff/21018/14007#newcode1790 net/http/http_network_transaction.cc:1790: const char* scheme = proxy_info_.is_http() ? "http://" : "https://"; On 2010/08/19 18:40:56, vandebo wrote: > This seems right, but might actually confuse the proxy. Unless you know this is > the right thing, you should ask cbentzel about it. I'm not sure that I follow (so I'll ask cbentzel :>). I thought this URL was only used inside of chrome as a key for the HttpAuthController to pick the right credentials. I didn't think it ever went over the wire. http://codereview.chromium.org/3110006/diff/21018/14009 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/3110006/diff/21018/14009#newcode1772 net/http/http_network_transaction_unittest.cc:1772: StaticSocketDataProvider data1(data_reads1, arraysize(data_reads1), On 2010/08/19 18:40:56, vandebo wrote: > Do you need two of everything here? I think this will happen over one > connection, and thus you only need one data/ssl provider. Hm. I confess, I'm not particularly clear about how the various data providers work. If I only have a single array, I end up with the following error: [3809:3809:0819/151230:1995563169293:FATAL:./net/socket/socket_test_util.h(357)] Check failed: next_index_ < data_providers_.size() (1 vs. 1) From what you said, I should interpret this error as telling me that the first connection has been shut down, and a new connection is being attempted? http://codereview.chromium.org/3110006/diff/21018/14011 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/21018/14011#newcode170 net/http/http_proxy_client_socket_pool.cc:170: if (IsCertificateError(result)) On 2010/08/19 18:40:56, vandebo wrote: > If it will work, I'd really rather have tests set the ignore cert error flag and > fail safe here. As soon as this goes into the tree, random people might start > using it. Sounds reasonable. It's not actually unit tests which trigger this problem, rather when I'm playing around with my self-signed certs. I'll remove this hack. http://codereview.chromium.org/3110006/diff/21018/14012 File net/http/http_proxy_client_socket_pool.h (right): http://codereview.chromium.org/3110006/diff/21018/14012#newcode119 net/http/http_proxy_client_socket_pool.h:119: int DoSslConnect(); On 2010/08/19 18:40:56, vandebo wrote: > SSL Done. http://codereview.chromium.org/3110006/diff/21018/14013 File net/http/http_proxy_client_socket_pool_unittest.cc (right): http://codereview.chromium.org/3110006/diff/21018/14013#newcode30 net/http/http_proxy_client_socket_pool_unittest.cc:30: const char* kHostUrl = "http://host"; On 2010/08/19 18:40:56, vandebo wrote: > These constants seem overkill. Done. http://codereview.chromium.org/3110006/diff/21018/14013#newcode79 net/http/http_proxy_client_socket_pool_unittest.cc:79: return scoped_refptr<TCPSocketParams>(NULL); On 2010/08/19 18:40:56, vandebo wrote: > I think the NULL is unnecessary. Done.
http://codereview.chromium.org/3110006/diff/13011/54009 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/13011/54009#newcode225 net/http/http_proxy_client_socket_pool.cc:225: if (tcp_pool_) On 2010/08/19 22:30:25, rch wrote: > On 2010/08/19 18:40:56, vandebo wrote: > > On 2010/08/18 14:46:29, rch wrote: > > > On 2010/08/17 02:01:08, vandebo wrote: > > > > Won't tcp_pool_ always be true here? > > > > > > Ugh. You're totally right. So if I read this correctly, the Factory is > > > required to have a single ConnectionTimeout, but in the HttpProxy case, > there > > > are two different code paths based on the args passed in to the actual job. > > I'm > > > not sure how to resolve this. It appears that the semantics are mismatch > > here. > > > How would you like to see me resolve this? > > > > This is to set a timeout - hopefully it won't be hit. The conservative option > > (what is done in the SSL pool) is to take the max timeout. At least in the > SSL > > case, a given pool may not have all the subordinate pools, we take the max > > timeout of all passed pools. > > > > > > Done. I decided to move the logic to the constructor (ala the SSL pool) and > then moved the constructor to the .cc file. However, I'm not sure I wrapped it > correctly. Does it look right to you? That's a tricky corner case. Seems reasonable to me, but willchan could tell you for sure. http://codereview.chromium.org/3110006/diff/21018/14006 File net/http/http_network_session.h (right): http://codereview.chromium.org/3110006/diff/21018/14006#newcode155 net/http/http_network_session.h:155: tcp_for_https_proxy_pool_histograms_; On 2010/08/19 22:30:25, rch wrote: > On 2010/08/19 18:40:56, vandebo wrote: > > When you land this change, you'll need to update the histogram description > page. > > Sounds great. Where/How do I do that? Search internally for Chrome Histograms. http://codereview.chromium.org/3110006/diff/21018/14009 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/3110006/diff/21018/14009#newcode1772 net/http/http_network_transaction_unittest.cc:1772: StaticSocketDataProvider data1(data_reads1, arraysize(data_reads1), On 2010/08/19 22:30:25, rch wrote: > On 2010/08/19 18:40:56, vandebo wrote: > > Do you need two of everything here? I think this will happen over one > > connection, and thus you only need one data/ssl provider. > > Hm. I confess, I'm not particularly clear about how the various data providers > work. If I only have a single array, I end up with the following error: > > [3809:3809:0819/151230:1995563169293:FATAL:./net/socket/socket_test_util.h(357)] > Check failed: next_index_ < data_providers_.size() (1 vs. 1) > > From what you said, I should interpret this error as telling me that the first > connection has been shut down, and a new connection is being attempted? That's a reasonable interpretation. There are some bugs that trigger a second connection even though it's not used. So you should look at it a bit more carefully and see when this is coming up. According to your understanding, it should all happen over one connection, right?
On Aug 19, 2010 2:40:19 PM PDT, cbentzel <cbentzel@chromium.org> wrote: > I'll plan on looking this CL a bit more thoroughly after you merge with > the HttpStreamFactory change. Great, thanks. I've uploaded a new version of the CL after merging in the HttpStreamFactory change. I still have a bit of work to do on some HttpNetworkTransaction unit tests, but I doubt the CL will change significantly. >> http://codereview.chromium.org/3110006/diff/21018/14007 >> File net/http/http_network_transaction.cc (right): >> http://codereview.chromium.org/3110006/diff/21018/14007#newcode1790 >> net/http/http_network_transaction.cc:1790: const char* scheme = >> proxy_info_.is_http() ? "http://" : "https://"; >> On 2010/08/19 18:40:56, vandebo wrote: >> This seems right, but might actually confuse the proxy. Unless you >> know this is >> the right thing, you should ask cbentzel about it. > https: is the right thing here. Basically, the auth controllers will use > this URL to lookup cached username/password values in the cache (amongst > other things). It's also probably a corner case since it should only be > used when doing an HTTP request through an HTTPS proxy which wants to > use HTTP authentication. > Nit: is there a better way to set the scheme? Also, you may want to > explicitly check for is_http/is_https here on proxy_info_ rather than > doing the ternary. Are you concerned about the prospect of proxy_info being neither http nor https? If so, I guess the old semantics were to use a scheme of http:// in that case. How 'bout in invert the logic so we only use "https://" if we're https, and then "http://" otherwise? Or would you rather have some sort of DCHECK that we're one of the two? Not sure what you mean by a "better way to set the scheme"... Are you thinking we might want a method like proxy_info.SchemeToString() which would return "http://" or "https://" (or "socks://, I suppose?)
http://codereview.chromium.org/3110006/diff/21018/14009 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/3110006/diff/21018/14009#newcode1772 net/http/http_network_transaction_unittest.cc:1772: StaticSocketDataProvider data1(data_reads1, arraysize(data_reads1), On 2010/08/19 22:43:24, vandebo wrote: > On 2010/08/19 22:30:25, rch wrote: > > On 2010/08/19 18:40:56, vandebo wrote: > > > Do you need two of everything here? I think this will happen over one > > > connection, and thus you only need one data/ssl provider. > > > > Hm. I confess, I'm not particularly clear about how the various data > providers > > work. If I only have a single array, I end up with the following error: > > > > > [3809:3809:0819/151230:1995563169293:FATAL:./net/socket/socket_test_util.h(357)] > > Check failed: next_index_ < data_providers_.size() (1 vs. 1) > > > > From what you said, I should interpret this error as telling me that the first > > connection has been shut down, and a new connection is being attempted? > > That's a reasonable interpretation. There are some bugs that trigger a second > connection even though it's not used. So you should look at it a bit more > carefully and see when this is coming up. According to your understanding, it > should all happen over one connection, right? Yes, I would have expected a single connection here. I did some debugging and realized that the basic problem was that the MockRead (i.e. the server response) did not have a Content-Length: header. Doh! I mostly copied this test from somewhere else, but I must have picked a bad example. Anyway, I set a content length, and it now works with a single data provider. Thanks for encouraging me to look into this.
Just a couple more small things. http://codereview.chromium.org/3110006/diff/60004/34008 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/3110006/diff/60004/34008#newcode3696 net/http/http_network_transaction_unittest.cc:3696: SessionDependencies session_deps(CreateFixedProxyService("https://myproxy:70")); 80 columns http://codereview.chromium.org/3110006/diff/60004/34008#newcode3752 net/http/http_network_transaction_unittest.cc:3752: SessionDependencies session_deps(CreateFixedProxyService("https://myproxy:70")); 80 columns http://codereview.chromium.org/3110006/diff/60004/34008#newcode3759 net/http/http_network_transaction_unittest.cc:3759: MockWrite proxy_writes[] = { These variable names could be improved if I understand how this test works. http://codereview.chromium.org/3110006/diff/60004/34008#newcode3807 net/http/http_network_transaction_unittest.cc:3807: for (int i = 0; i < 2; i++) { Why do you have this loop (and the next line)? It just tests the same thing twice... http://codereview.chromium.org/3110006/diff/60004/34009 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/60004/34009#newcode178 net/http/http_proxy_client_socket_pool.cc:178: // TODO(rch): need to figure out the long term solution here This still bothers me - what's the plan? http://codereview.chromium.org/3110006/diff/60004/34009#newcode237 net/http/http_proxy_client_socket_pool.cc:237: max_pool_timeout = max(max_pool_timeout, ssl_pool_->ConnectionTimeout()); I would just use std::max here and remove the using statement. http://codereview.chromium.org/3110006/diff/60004/34010 File net/http/http_proxy_client_socket_pool.h (right): http://codereview.chromium.org/3110006/diff/60004/34010#newcode137 net/http/http_proxy_client_socket_pool.h:137: scoped_ptr<ClientSocket> transport_socket_handle_; This isn't a handle...
http://codereview.chromium.org/3110006/diff/60004/34008 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/3110006/diff/60004/34008#newcode3696 net/http/http_network_transaction_unittest.cc:3696: SessionDependencies session_deps(CreateFixedProxyService("https://myproxy:70")); On 2010/08/23 23:11:55, vandebo wrote: > 80 columns Done. http://codereview.chromium.org/3110006/diff/60004/34008#newcode3752 net/http/http_network_transaction_unittest.cc:3752: SessionDependencies session_deps(CreateFixedProxyService("https://myproxy:70")); On 2010/08/23 23:11:55, vandebo wrote: > 80 columns Done. http://codereview.chromium.org/3110006/diff/60004/34008#newcode3759 net/http/http_network_transaction_unittest.cc:3759: MockWrite proxy_writes[] = { On 2010/08/23 23:11:55, vandebo wrote: > These variable names could be improved if I understand how this test works. Done. http://codereview.chromium.org/3110006/diff/60004/34008#newcode3807 net/http/http_network_transaction_unittest.cc:3807: for (int i = 0; i < 2; i++) { On 2010/08/23 23:11:55, vandebo wrote: > Why do you have this loop (and the next line)? It just tests the same thing > twice... Done. This tests is a copy of HTTPSBadCertificateViaProxy, but with the correct setup to make use of an HTTPS proxy instead of an HTTP proxy. Other than that, the test is the same. I assumed that the existing test was correct and since the semantics of an HTTP vs HTTPS proxy should be identical in this case. That being said, I've happily removed the for loop. http://codereview.chromium.org/3110006/diff/60004/34009 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/60004/34009#newcode178 net/http/http_proxy_client_socket_pool.cc:178: // TODO(rch): need to figure out the long term solution here On 2010/08/23 23:11:55, vandebo wrote: > This still bothers me - what's the plan? Done. Sorry, sorry. I meant to remove that. Down the road, we'll want to surface to the user the ability to add the proxy's cert to their cache of trusted certs, but we don't need that immediately. http://codereview.chromium.org/3110006/diff/60004/34009#newcode237 net/http/http_proxy_client_socket_pool.cc:237: max_pool_timeout = max(max_pool_timeout, ssl_pool_->ConnectionTimeout()); On 2010/08/23 23:11:55, vandebo wrote: > I would just use std::max here and remove the using statement. Done. http://codereview.chromium.org/3110006/diff/60004/34010 File net/http/http_proxy_client_socket_pool.h (right): http://codereview.chromium.org/3110006/diff/60004/34010#newcode137 net/http/http_proxy_client_socket_pool.h:137: scoped_ptr<ClientSocket> transport_socket_handle_; On 2010/08/23 23:11:55, vandebo wrote: > This isn't a handle... Done. *facepalm* I misread the variable you wanted me to rename. Sorry about that.
Where are the try job results? http://codereview.chromium.org/3110006/diff/60004/34008 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/3110006/diff/60004/34008#newcode3807 net/http/http_network_transaction_unittest.cc:3807: for (int i = 0; i < 2; i++) { On 2010/08/23 23:48:46, rch wrote: > On 2010/08/23 23:11:55, vandebo wrote: > > Why do you have this loop (and the next line)? It just tests the same thing > > twice... > > Done. This tests is a copy of HTTPSBadCertificateViaProxy, but with the correct > setup to make use of an HTTPS proxy instead of an HTTP proxy. Other than that, > the test is the same. I assumed that the existing test was correct and since > the semantics of an HTTP vs HTTPS proxy should be identical in this case. That > being said, I've happily removed the for loop. Hmm, it seems that the loop in HTTPSBadCertificateViaProxy is also pointless. I've emailed the original author to try to get to the bottom of it. http://codereview.chromium.org/3110006/diff/89002/23018 File net/http/http_proxy_client_socket_pool.cc (right): http://codereview.chromium.org/3110006/diff/89002/23018#newcode175 net/http/http_proxy_client_socket_pool.cc:175: if (result != OK) Because certificate errors are recoverable errors, you need to disconnect the socket so that it doesn't get returned to the pool as an idle socket.
Argh! I replied last night, but somehow I sent the reply only to myself. *sigh* On 2010/08/24 01:55:02, vandebo wrote: > Where are the try job results? Oh, I guess they get reset with each version of the patch? That makes sense, now that I think about it. I last ran them against Patch #9. All were green except for linux, which appeared to be totally unrelated. I've fired off another batch. > http://codereview.chromium.org/3110006/diff/60004/34008 > File net/http/http_network_transaction_unittest.cc (right): > > http://codereview.chromium.org/3110006/diff/60004/34008#newcode3807 > net/http/http_network_transaction_unittest.cc:3807: for (int i = 0; i < 2; i++) > { > On 2010/08/23 23:48:46, rch wrote: > > On 2010/08/23 23:11:55, vandebo wrote: > > > Why do you have this loop (and the next line)? It just tests the same thing > > > twice... > > > > Done. This tests is a copy of HTTPSBadCertificateViaProxy, but with the > correct > > setup to make use of an HTTPS proxy instead of an HTTP proxy. Other than > that, > > the test is the same. I assumed that the existing test was correct and since > > the semantics of an HTTP vs HTTPS proxy should be identical in this case. > That > > being said, I've happily removed the for loop. > > Hmm, it seems that the loop in HTTPSBadCertificateViaProxy is also pointless. > I've emailed the original author to try to get to the bottom of it. Sounds great. > http://codereview.chromium.org/3110006/diff/89002/23018 > File net/http/http_proxy_client_socket_pool.cc (right): > > http://codereview.chromium.org/3110006/diff/89002/23018#newcode175 > net/http/http_proxy_client_socket_pool.cc:175: if (result != OK) > Because certificate errors are recoverable errors, you need to disconnect the > socket so that it doesn't get returned to the pool as an idle socket. Hm.. Something like this? if (IsCertificateError(result)) { transport_socket_->Disconnect(); }
On 2010/08/24 23:01:13, rch wrote: > On 2010/08/24 01:55:02, vandebo wrote: > > http://codereview.chromium.org/3110006/diff/89002/23018 > > File net/http/http_proxy_client_socket_pool.cc (right): > > > > http://codereview.chromium.org/3110006/diff/89002/23018#newcode175 > > net/http/http_proxy_client_socket_pool.cc:175: if (result != OK) > > Because certificate errors are recoverable errors, you need to disconnect the > > socket so that it doesn't get returned to the pool as an idle socket. > > Hm.. Something like this? > > if (IsCertificateError(result)) { > transport_socket_->Disconnect(); > } Actually I'd do: if (result < 0) { if(transport_socket_->socket()) transport_socket_->Disconnet(); return result; }
On 2010/08/24 23:07:36, vandebo wrote: > On 2010/08/24 23:01:13, rch wrote: > > On 2010/08/24 01:55:02, vandebo wrote: > > > http://codereview.chromium.org/3110006/diff/89002/23018 > > > File net/http/http_proxy_client_socket_pool.cc (right): > > > > > > http://codereview.chromium.org/3110006/diff/89002/23018#newcode175 > > > net/http/http_proxy_client_socket_pool.cc:175: if (result != OK) > > > Because certificate errors are recoverable errors, you need to disconnect > the > > > socket so that it doesn't get returned to the pool as an idle socket. > > > > Hm.. Something like this? > > > > if (IsCertificateError(result)) { > > transport_socket_->Disconnect(); > > } > > Actually I'd do: > if (result < 0) { > if(transport_socket_->socket()) > transport_socket_->Disconnet(); > return result; > } Done. (As discussed)
LGTM! |
