Allow a non-200 (or non-407) response for a CONNECT request from an HTTPS pro...

net/http/http_proxy_client_socket_pool.cc

Index: net/http/http_proxy_client_socket_pool.cc
===================================================================
--- net/http/http_proxy_client_socket_pool.cc	(revision 68026)
+++ net/http/http_proxy_client_socket_pool.cc	(working copy)
@@ -11,14 +11,17 @@
 #include "googleurl/src/gurl.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
+#include "net/http/connect_response_http_stream.h"
 #include "net/http/http_network_session.h"
 #include "net/http/http_proxy_client_socket.h"
+#include "net/http/http_response_headers.h"
 #include "net/socket/client_socket_factory.h"
 #include "net/socket/client_socket_handle.h"
 #include "net/socket/client_socket_pool_base.h"
 #include "net/socket/ssl_client_socket.h"
 #include "net/socket/ssl_client_socket_pool.h"
 #include "net/socket/tcp_client_socket_pool.h"
+#include "net/spdy/spdy_http_stream.h"
 #include "net/spdy/spdy_proxy_client_socket.h"
 #include "net/spdy/spdy_session.h"
 #include "net/spdy/spdy_session_pool.h"
@@ -252,6 +255,10 @@
   if (error_response_info_.cert_request_info) {
     handle->set_ssl_error_response_info(error_response_info_);
     handle->set_is_ssl_error(true);
+  } else if (error_response_info_.headers) {
+    handle->set_ssl_error_response_info(error_response_info_);

[vandebo (ex-Chrome), 2010/12/04 00:30:37]

Do you need to set this in this case?

[Ryan Hamilton, 2010/12/09 21:19:35]

Yes, definitely.  This is how the response headers are passed up.

+    handle->set_pending_https_proxy_response_stream(
+        pending_https_proxy_response_stream_.release());
   }
 }
 
@@ -317,13 +324,21 @@
                                 params_->http_auth_cache(),
                                 params_->http_auth_handler_factory(),
                                 params_->tunnel(),
-                                using_spdy_));
+                                using_spdy_,
+                                params_->ssl_params() != NULL));
   return transport_socket_->Connect(&callback_);
 }
 
 int HttpProxyConnectJob::DoHttpProxyConnectComplete(int result) {
-  if (result == OK || result == ERR_PROXY_AUTH_REQUESTED)
+  if (result == OK || result == ERR_PROXY_AUTH_REQUESTED) {
       set_socket(transport_socket_.release());
+  } else if (result == ERR_HTTPS_PROXY_TUNNEL_CONNECTION_RESPONSE) {
+    error_response_info_ = *transport_socket_->GetConnectResponseInfo();

[vandebo (ex-Chrome), 2010/12/04 00:30:37]

Now that you've created the HttpProxyTunnelClientSocket with the
GetConnectResponseStream() method, can you pass that up and actually create the
stream at the StreamRequest level, or do ownership issues get in the way?

[Ryan Hamilton, 2010/12/09 21:19:35]

Hm.  You're recommending that we pass up the socket instead of the stream,
right?  I'm not sure that I understand the motivation.  I think you're right
that ownership issues might get in the way, but even if they did not, what would
that buy us?

[vandebo (ex-Chrome), 2010/12/11 02:47:49]

Please look at the ownership issues.  Sounds like maybe you already have the
info from the next comment?  By just passing up the socket and creating the
stream at the StreamRequest level, you maintain the abstraction of streams
sitting above the socket pools

[Ryan Hamilton, 2010/12/13 19:41:12]

Done.

+    pending_https_proxy_response_stream_.reset(
+        transport_socket_->GetConnectResponseStream());
+    DCHECK(!error_response_info_.cert_request_info.get());
+    set_socket(transport_socket_.release());

[vandebo (ex-Chrome), 2010/12/04 00:30:37]

Since you are making this a "recoverable error" (by setting a socket and
returning an error code), do you even need to push the stream up as additional
error state?  Maybe at the next level of encapsulation, but this one?

[Ryan Hamilton, 2010/12/09 21:19:35]

Hm.  It took me a while to understand the implications of your "recoverable
error" comment.  I think I grok it now.  I think this was a bug, not a feature
on my part.  I was calling set_socket() so that the HttpStreamParser's client
socket handle did not get nuked when the HttpProxyClientSocket was destroyed. 
But this approach was flawed.  After thinking through this, I've changed
HttpProxyClientSocket so that when it constructs the ResponseStream, it hands
off both the HttpStreamParser and the ClientSocketHandle to the ResponseStream. 
(On the other hand, I had to change HttpProxyClientSocket's transport_ to be
non-const).  This seems much better.  Thanks for catching this!

+  }
 
   return result;
 }