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

Allow a non-200 (or non-407) response for a CONNECT request from an HTTPS proxy
to be consumed by chrome.  Among other things, this will allow the proxy to
inform the user that the hostname could not be resolved or similar conditions.

This adds a new OnHttpsProxyTunnelConnectionResponse method to
StreamRequest::Delegate which is invoked when an HTTPS proxy returns a non-200,
non-407 response.  The method is called with an HttpResponseInfor argument to
access the request headers, and an HttpStream argument to access the response body.

BUG=none
TEST=HttpNetworkTransactionTest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69513

[Ryan Hamilton, 2010-12-02 23:08:00]

I believe I have this CL complete.  Steve, I bounced an earlier version of it
past you before Thanksgiving, and it's wrapped up now.  Can you review it?  (It
affects a number of files, so let me know if you'd like me to walk you through
it)

mbelshe: FYI

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

Feel free to ignore any of the name suggestions.

http://codereview.chromium.org/4935001/diff/192001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (left):

http://codereview.chromium.org/4935001/diff/192001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/resource_dispatcher_host.cc:1575: // If there is no
SSL info attached to this request, we must either be a non
Why did this code need to go?

http://codereview.chromium.org/4935001/diff/192001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/base/net_error_list.h#...
net/base/net_error_list.h:220: NET_ERROR(HTTPS_PROXY_TUNNEL_CONNECTION_RESPONSE,
-140)
The comment should be a bit more descriptive.  Do you like HTTPS_PROXY_ERROR, or
HTTPS_PROXY_CONNECTION_MESSAGE better?

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
File net/http/connect_response_http_stream.cc (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.cc:1: // Copyright (c) 2006-2009 The
Chromium Authors. All rights reserved.
2010

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.cc:1: // Copyright (c) 2006-2009 The
Chromium Authors. All rights reserved.
Since this is so close to HttpBasicStream, would it be better to just add a
constructor to HttpBasicStream? I guess probably not, but I wonder if you
considered it.

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.cc:56: return
parser_->Close(not_reusable);
Should this be ->Close(true); ?  Or maybe a class bool to indicate if the stream
is definitely not reusable.

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
File net/http/connect_response_http_stream.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
Why do you need a ConnectResponseHttpStream?  Why not just use an HttpStream?

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
2010

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.h:24: //class HttpResponseInfo;
remove

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.h:30: class ConnectResponseHttpStream :
public HttpStream {
ResponseOnlyHttpStream ?

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.h:32:
ConnectResponseHttpStream(HttpStreamParser* parser);
explicit

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.h:78: HttpResponseInfo* response_info_;
This doesn't seem to be used.

http://codereview.chromium.org/4935001/diff/192001/net/http/http_network_tran...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/http_network_tran...
net/http/http_network_transaction.cc:294: // also don't worry about this for an
HTTPS Proxy, because an the
because the

http://codereview.chromium.org/4935001/diff/192001/net/http/http_network_tran...
net/http/http_network_transaction.cc:552: next_state_ = STATE_NONE;
A comment here might be helpful:
// Return OK and let the caller read the proxy's error page

http://codereview.chromium.org/4935001/diff/192001/net/http/http_network_tran...
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/http_network_tran...
net/http/http_network_transaction.h:68: virtual void
OnHttpsProxyTunnelConnectionResponse(
Maybe OnHttpsProxyMessage ?

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket.cc:15: #include
"net/http/connect_response_http_stream.h"
Is this header needed?

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket.cc:151: if (transport_->socket())
When is this false?

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket.cc:412:
LogBlockedTunnelResponse(response_.headers->response_code());
Shouldn't this Log call be in the else clause?

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
File net/http/http_proxy_client_socket.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket.h:50: bool is_https_proxy);
Can you look at proxy_server instead of passing is_https_proxy?

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket.h:64: virtual HttpStream*
GetConnectResponseStream() {
This should probably go in the .cpp file (and take the header it needs with it).

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.cc:259:
handle->set_ssl_error_response_info(error_response_info_);
Do you need to set this in this case?

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.cc:336: error_response_info_ =
*transport_socket_->GetConnectResponseInfo();
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?

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.cc:340:
set_socket(transport_socket_.release());
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?

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.h:165: scoped_ptr<HttpStream>
pending_https_proxy_response_stream_;
would https_response_stream_ or proxy_connect_response_stream_ be just as
descriptive?

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_tunnel...
File net/http/http_proxy_tunnel_client_socket.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_tunnel...
net/http/http_proxy_tunnel_client_socket.h:16: class HttpProxyTunnelClientSocket
: public ClientSocket {
Should this be something like ProxyClientSocket?  It's the common ancestor of
HttpProxyClientSocket and SpdyProxyClientSocket (and the descendant of
ClientSocket).

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_tunnel...
net/http/http_proxy_tunnel_client_socket.h:25: // Transfers ownersip of a newly
created HttpStread to the caller
HttpStream

http://codereview.chromium.org/4935001/diff/192001/net/http/http_stream_reque...
File net/http/http_stream_request.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/http_stream_reque...
net/http/http_stream_request.h:112: void
OnHttpsProxyTunnelConnectionResponseCallback(
OnHttpsProxyConnectResponseCallback ?

http://codereview.chromium.org/4935001/diff/192001/net/http/stream_factory.h
File net/http/stream_factory.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/stream_factory.h#...
net/http/stream_factory.h:76: // Ownership of |response_info| is retained by the
StreamRequest.
I don't think you need to note the ownership of a const class ref

http://codereview.chromium.org/4935001/diff/192001/net/http/stream_factory.h#...
net/http/stream_factory.h:78: virtual void OnHttpsProxyTunnelConnectionResponse(
Connection -> Connect ?

http://codereview.chromium.org/4935001/diff/192001/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_pool.cc (right):

http://codereview.chromium.org/4935001/diff/192001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_pool.cc:267: transport_socket_handle_->
Indeed, you get the pending https proxy response stream from the transport
socket (recoverable error) and not the additional error state.  Since you can't
do the same thing here (the socket would be returned to the wrong pool), maybe
you should always use the additional error state method and not treat the http
proxy client socket non200/407 as a recoverable error.

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.cc:44: buffered_read_callback_pending_(false),
Should some of these settings come from the passed spdy_stream?

i.e. looks like SpdyHttpStream::ReadResponseBody might use spdy_session_

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.h:37: SpdyHttpStream(SpdyStream* spdy_stream);
explicit

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_proxy_client...
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_proxy_client...
net/spdy/spdy_proxy_client_socket.cc:108: return IsConnected() &&
spdy_stream_.get() && !spdy_stream_->is_idle();
Why did this change?

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_proxy_client...
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_proxy_client...
net/spdy/spdy_proxy_client_socket.h:72: SpdyStream* stream = spdy_stream_;
Should this implementation go in the .cpp ?

[Mike Belshe, 2010-12-07 17:17:23]

+cc WillChan:  you might want to take a look?

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

Steve,

Thanks for the review!  I believe I have addressed all of your (non-naming)
comments.  Can you take a look?  Like you, I'm not thrilled with some of the
names and I think your suggestions are closer.  However, I didn't want to
confuse the renaming with the changes in functionality.  After you're cool with
the functionality of the patch, I'll upload a new version with the renames. 
(and I'll send another mail to discussion the new names).

Cheers,

Ryan

http://codereview.chromium.org/4935001/diff/192001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (left):

http://codereview.chromium.org/4935001/diff/192001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/resource_dispatcher_host.cc:1575: // If there is no
SSL info attached to this request, we must either be a non
On 2010/12/04 00:30:37, vandebo wrote:
> Why did this code need to go?

Because *any* response from a HTTPS URL (when going through an HTTPS Proxy) is
now a potentially valid response.  (That's what this CL is all about).  If there
were a way to check for the type of proxy used in this request, then we could
add a more restrictive check, but since that information is not available, we
have no choice...

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
File net/http/connect_response_http_stream.cc (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.cc:1: // Copyright (c) 2006-2009 The
Chromium Authors. All rights reserved.
On 2010/12/04 00:30:37, vandebo wrote:
> 2010

Done.

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.cc:1: // Copyright (c) 2006-2009 The
Chromium Authors. All rights reserved.
On 2010/12/04 00:30:37, vandebo wrote:
> Since this is so close to HttpBasicStream, would it be better to just add a
> constructor to HttpBasicStream? I guess probably not, but I wonder if you
> considered it.

You know, I initially considered this approach, and then decided that I didn't
like the idea of constructing an HttpBasicStream in a way that didn't permit
InitializeStream / SendRequest to be called.  However, I think you're right.  I
think it's cleaner to go the HttpBasicStream route.  Done.

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.cc:56: return
parser_->Close(not_reusable);
On 2010/12/04 00:30:37, vandebo wrote:
> Should this be ->Close(true); ?  Or maybe a class bool to indicate if the
stream
> is definitely not reusable.

Done.

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
File net/http/connect_response_http_stream.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
On 2010/12/04 00:30:37, vandebo wrote:
> Why do you need a ConnectResponseHttpStream?  Why not just use an HttpStream?

Well HttpStream is abstract/virtual, right?  Did you mean HttpBasicStream?

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
On 2010/12/04 00:30:37, vandebo wrote:
> 2010

Done.

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.h:24: //class HttpResponseInfo;
On 2010/12/04 00:30:37, vandebo wrote:
> remove

Done.

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.h:32:
ConnectResponseHttpStream(HttpStreamParser* parser);
On 2010/12/04 00:30:37, vandebo wrote:
> explicit

Done.  (Then I changed the signature, and removed explicit :|)

http://codereview.chromium.org/4935001/diff/192001/net/http/connect_response_...
net/http/connect_response_http_stream.h:78: HttpResponseInfo* response_info_;
On 2010/12/04 00:30:37, vandebo wrote:
> This doesn't seem to be used.

Done.

http://codereview.chromium.org/4935001/diff/192001/net/http/http_network_tran...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/http_network_tran...
net/http/http_network_transaction.cc:294: // also don't worry about this for an
HTTPS Proxy, because an the
On 2010/12/04 00:30:37, vandebo wrote:
> because the

Done.

http://codereview.chromium.org/4935001/diff/192001/net/http/http_network_tran...
net/http/http_network_transaction.cc:552: next_state_ = STATE_NONE;
On 2010/12/04 00:30:37, vandebo wrote:
> A comment here might be helpful:
> // Return OK and let the caller read the proxy's error page

Done.

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket.cc:15: #include
"net/http/connect_response_http_stream.h"
On 2010/12/04 00:30:37, vandebo wrote:
> Is this header needed?

Done.

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket.cc:151: if (transport_->socket())
On 2010/12/04 00:30:37, vandebo wrote:
> When is this false?

So I think this was somewhat of a bug which I describe in my response to your
"recoverable error" comment.  However, I had to change this code to check for
transport_ to be non-null, since it will be null after
GetConnectResponseStream() is called.  Nice catch.

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket.cc:412:
LogBlockedTunnelResponse(response_.headers->response_code());
On 2010/12/04 00:30:37, vandebo wrote:
> Shouldn't this Log call be in the else clause?

Done.

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
File net/http/http_proxy_client_socket.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket.h:50: bool is_https_proxy);
On 2010/12/04 00:30:37, vandebo wrote:
> Can you look at proxy_server instead of passing is_https_proxy?

Alas not.  proxy_server is merely a HostPortPair, but it does not tell us if we
are talking SSL to it.

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket.h:64: virtual HttpStream*
GetConnectResponseStream() {
On 2010/12/04 00:30:37, vandebo wrote:
> This should probably go in the .cpp file (and take the header it needs with
it).

Done.

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.cc:259:
handle->set_ssl_error_response_info(error_response_info_);
On 2010/12/04 00:30:37, vandebo wrote:
> Do you need to set this in this case?

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

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.cc:336: error_response_info_ =
*transport_socket_->GetConnectResponseInfo();
On 2010/12/04 00:30:37, vandebo wrote:
> 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?

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?

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.cc:340:
set_socket(transport_socket_.release());
On 2010/12/04 00:30:37, vandebo wrote:
> 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?

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!

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_tunnel...
File net/http/http_proxy_tunnel_client_socket.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_tunnel...
net/http/http_proxy_tunnel_client_socket.h:25: // Transfers ownersip of a newly
created HttpStread to the caller
On 2010/12/04 00:30:37, vandebo wrote:
> HttpStream

Done.

http://codereview.chromium.org/4935001/diff/192001/net/http/stream_factory.h
File net/http/stream_factory.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/stream_factory.h#...
net/http/stream_factory.h:76: // Ownership of |response_info| is retained by the
StreamRequest.
On 2010/12/04 00:30:37, vandebo wrote:
> I don't think you need to note the ownership of a const class ref

Oh, duh!

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.cc:44: buffered_read_callback_pending_(false),
On 2010/12/04 00:30:37, vandebo wrote:
> Should some of these settings come from the passed spdy_stream?
> 
> i.e. looks like SpdyHttpStream::ReadResponseBody might use spdy_session_

I don't *think* any of the fields need to come from the spdy_stream.  In
ReadResponseBody, there was a call to spdy_session_->flow_control(), but
flow_control() is a static method so I changed it (in an earlier CL) to call
SpdySession::flow_control(). As far as I was able to tell (and test)
SpdyHttpStream works find (in this mode) when spdy_session_ is null.

None of the other fields look to me like they could be initialized from the
spdy_stream.

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.h:37: SpdyHttpStream(SpdyStream* spdy_stream);
On 2010/12/04 00:30:37, vandebo wrote:
> explicit

Done.

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_proxy_client...
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_proxy_client...
net/spdy/spdy_proxy_client_socket.cc:108: return IsConnected() &&
spdy_stream_.get() && !spdy_stream_->is_idle();
On 2010/12/04 00:30:37, vandebo wrote:
> Why did this change?

Bug, actually.  Removed.

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_proxy_client...
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/4935001/diff/192001/net/spdy/spdy_proxy_client...
net/spdy/spdy_proxy_client_socket.h:72: SpdyStream* stream = spdy_stream_;
On 2010/12/04 00:30:37, vandebo wrote:
> Should this implementation go in the .cpp ?

Done.

[Ryan Hamilton, 2010-12-09 21:39:22]

On 2010/12/04 00:30:37, vandebo wrote:
> Feel free to ignore any of the name suggestions.

No way!  I like your thinking :> You proposed the following renames:

1) HttpProxyTunnelClientSocket => ProxyClientSocket

2) HTTPS_PROXY_TUNNEL_CONNECTION_RESPONSE => 
  HTTPS_PROXY_ERROR
  HTTPS_PROXY_CONNECTION_MESSAGE

3) OnHttpsProxyTunnelConnectionResponse => 
  OnHttpsProxyMessage
  OnHttpsProxyTunnelConnectResponse

4) OnHttpsProxyTunnelConnectionResponseCallback =
  OnHttpsProxyConnectResponseCallback

5) pending_https_proxy_response_stream_ =>
  https_response_stream_
  proxy_connect_response_stream_

Here are my thoughts:
1) ProxyClientSocket sounds great!
2-4)  I'd like to make sure we use the same string (with various prefixes,
suffixes, and capitalization) for all three identifiers.  In my code I chose
"HTTPS Proxy Tunnel Connection Response", but I agree with you that it is way
too long.  I like your suggestion of "HTTPS Proxy Connect Response".  But I
worry that "CONNECT" might confuse someone into thinking it's a socket connect .
 How about "HTTPS Proxy Tunnel Response", which would yield the following:

HTTPS_PROXY_TUNNEL_RESPONSE
OnHttpsProxyTunnelResponse
OnHttpsProxyTunnelResponseCallback

5) I'm happy to drop pending, but I'm not sure about dropping the HTTPS prefix
since this doesn't happen in the non-https case.  How bout:

https_proxy_tunnel_response_stream_

(Which is consistent with the previous renames)  Or, if you'd prefer to drop
"HTTPS":

proxy_tunnel_response_stream_

Cheers,

Ryan

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

A couple quick comments, I'll look through the code changes next.

On 2010/12/09 21:39:22, Ryan Hamilton wrote:
> Here are my thoughts:
> 1) ProxyClientSocket sounds great!
> 2-4)  I'd like to make sure we use the same string (with various prefixes,
> suffixes, and capitalization) for all three identifiers.  In my code I chose

Agreed, this is part of the motivation for suggesting new names.

> "HTTPS Proxy Tunnel Connection Response", but I agree with you that it is way

Plus, some where Connect and some where Connection.

> too long.  I like your suggestion of "HTTPS Proxy Connect Response".  But I
> worry that "CONNECT" might confuse someone into thinking it's a socket connect
.
>  How about "HTTPS Proxy Tunnel Response", which would yield the following:
> 
> HTTPS_PROXY_TUNNEL_RESPONSE
> OnHttpsProxyTunnelResponse
> OnHttpsProxyTunnelResponseCallback

Sounds good.

> 5) I'm happy to drop pending, but I'm not sure about dropping the HTTPS prefix
> since this doesn't happen in the non-https case.  How bout:
> 
> https_proxy_tunnel_response_stream_

Seems good.  But I still hopeful you can make this
https_proxy_tunnel_response_socket_/connection_

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.cc:336: error_response_info_ =
*transport_socket_->GetConnectResponseInfo();
On 2010/12/09 21:19:35, Ryan Hamilton wrote:
> On 2010/12/04 00:30:37, vandebo wrote:
> > 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?
> 
> 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?

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

http://codereview.chromium.org/4935001/diff/251001/net/http/http_proxy_client...
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/4935001/diff/251001/net/http/http_proxy_client...
net/http/http_proxy_client_socket.cc:421: } else {
You don't need the else part because of the return.

[Ryan Hamilton, 2010-12-13 19:41:11]

Ok, I've changed this CL to traffic in Sockets instead of Streams from the pool
up to the StreamRequest.  Sounds like we agree on the renaming, but I'm holding
off on that (trivial but will make the deltas ugly) until we're happy with the
functionality...

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/4935001/diff/192001/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.cc:336: error_response_info_ =
*transport_socket_->GetConnectResponseInfo();
On 2010/12/11 02:47:49, vandebo wrote:
> On 2010/12/09 21:19:35, Ryan Hamilton wrote:
> > On 2010/12/04 00:30:37, vandebo wrote:
> > > 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?
> > 
> > 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?
> 
> 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

Done.

http://codereview.chromium.org/4935001/diff/251001/net/http/http_proxy_client...
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/4935001/diff/251001/net/http/http_proxy_client...
net/http/http_proxy_client_socket.cc:421: } else {
On 2010/12/11 02:47:49, vandebo wrote:
> You don't need the else part because of the return.

Done.

[vandebo (ex-Chrome), 2010-12-14 00:30:22]

I'm sorry to say that I found another hiccup; as is, this doesn't handle
connection limits.  I think that can be solved by making
ERR_HTTPS_PROXY_TUNNEL_CONNECTION_RESPONSE a recoverable error at the
HttpProxyClientSocket level and then pushing the SSLConnectJob's transport
socket up to the StreamRequest as additional error state.  Ownership of that
ClientSocketHandle is questionable though, I don't remember where
ClientSocketHandle ownership landed with respect to HttpStreamFactory.

Also, I think it's fine to do those class renames when ever you get a chance, no
need to hold off.

http://codereview.chromium.org/4935001/diff/234003/net/base/net_error_list.h
File net/base/net_error_list.h (right):

http://codereview.chromium.org/4935001/diff/234003/net/base/net_error_list.h#...
net/base/net_error_list.h:218: // A request to create and SSL tunnel connection
through the HTTPS proxy
and->an

http://codereview.chromium.org/4935001/diff/234003/net/http/http_basic_stream.h
File net/http/http_basic_stream.h (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_basic_stream...
net/http/http_basic_stream.h:34: HttpBasicStream(HttpStreamParser* parser,
ClientSocketHandle* connection);
Maybe comments describing the different use cases for these constructors?

http://codereview.chromium.org/4935001/diff/234003/net/http/http_network_tran...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_network_tran...
net/http/http_network_transaction.cc:287: !stream_.get()) {
This seems to make the conditional a bit more fragile, is there something better
to conditional on?  Maybe an explicit bool ok_read_tunnel_body_ ?

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
net/http/http_proxy_client_socket.cc:419: return
ERR_HTTPS_PROXY_TUNNEL_CONNECTION_RESPONSE;
Should you call response_.headers.RemoveHeader("KeepAlive") here to ensure that
the socket is not reused?

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.cc:335: error_response_info_ =
*transport_socket_->GetConnectResponseInfo();
Urg - it just occurred to me this connection will not be accounted for in the
socket limits.  In order to do that, you'd need to make this a recoverable error
and then in the SSlClientSocketPool, push it into a ClientSocketHandle with the
appropriate pool so that when it was released the right pool would get it back.

At least in the non-spdy case, that means you can't create the stream until this
point.  I don't have a good enough model of the spdy pool to think about it in
this context.

Would "trafficing" in ClientSocketHandles from this point up make this workable?

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool_unittest.cc:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
I'd like to see a unit test (here or at the HttpNetworkTransaction if necessary)
that ensures that when a http_proxy_client_socket returns
ERR_HTTPS_PROXY_TUNNEL_CONNECTION_RESPONSE the connection is not reusable.

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_tunnel...
File net/http/http_proxy_tunnel_client_socket.h (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_tunnel...
net/http/http_proxy_tunnel_client_socket.h:27: virtual HttpStream*
GetConnectResponseStream() = 0;
Maybe call this Create/MakeConnectResponseStream.  Logically, it doesn't exist
until you ask for it.

[willchan no longer on Chromium, 2010-12-14 00:34:42]

This is very interesting!  I think I'm going to defer to vandebo for almost
everything.  I maybe should take a look at the SpdyHttpStream thing you're doing
where you initialize it with a SpdyStream.  It probably makes sense but I just
want to make sure.  Don't wait on me, I'll look when I get a chance.  I've sent
you some style nits that I saw while skimming this change.

http://codereview.chromium.org/4935001/diff/234003/net/http/http_basic_stream.h
File net/http/http_basic_stream.h (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_basic_stream...
net/http/http_basic_stream.h:34: HttpBasicStream(HttpStreamParser* parser,
ClientSocketHandle* connection);
Overloading is against the style guide.  It's not immediately clear what the
difference is between the two constructors.  I think you should merge them and
document them and use DCHECKs to make sure the values make sense.

http://codereview.chromium.org/4935001/diff/234003/net/http/http_network_tran...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_network_tran...
net/http/http_network_transaction_unittest.cc:4472: SessionDependencies
session_deps(ProxyService::CreateFixed("https://proxy:70"));
80 columns :)

http://codereview.chromium.org/4935001/diff/234003/net/http/http_network_tran...
net/http/http_network_transaction_unittest.cc:4521: SessionDependencies
session_deps(ProxyService::CreateFixed("https://proxy:70"));
80 columns :)

http://codereview.chromium.org/4935001/diff/234003/net/http/http_network_tran...
net/http/http_network_transaction_unittest.cc:4581: SessionDependencies
session_deps(ProxyService::CreateFixed("https://proxy:70"));
80 columns :)

http://codereview.chromium.org/4935001/diff/234003/net/http/http_network_tran...
net/http/http_network_transaction_unittest.cc:4634: SessionDependencies
session_deps(ProxyService::CreateFixed("https://proxy:70"));
80 columns :)

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
File net/http/http_proxy_client_socket.h (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
net/http/http_proxy_client_socket.h:16: #include
"net/http/http_proxy_tunnel_client_socket.h"
Sort alphabetically?

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
net/http/http_proxy_client_socket.h:35: class HttpProxyClientSocket : public
HttpProxyTunnelClientSocket {
This naming looks weird.  I didn't read through all the comments between you and
vandebo, but I think I remember some discussion on fixing naming in the future. 
Please do so :)

http://codereview.chromium.org/4935001/diff/234003/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/4935001/diff/234003/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.h:36: // Constructs a SpdyHttpStream that wraps an
existing SpdyStream.
Same style comment on overloading being disallowed.  In this case, I think you
might actually want to use a separate member function to set the stream.  It
would be a parallel of HttpStream::InitializeStream().  Maybe
InitializeWithExistingStream().

[Ryan Hamilton, 2010-12-14 18:01:01]

On 2010/12/14 00:30:22, vandebo wrote:
> I'm sorry to say that I found another hiccup; as is, this doesn't handle
> connection limits.  I think that can be solved by making
> ERR_HTTPS_PROXY_TUNNEL_CONNECTION_RESPONSE a recoverable error at the
> HttpProxyClientSocket level and then pushing the SSLConnectJob's transport
> socket up to the StreamRequest as additional error state.  Ownership of that
> ClientSocketHandle is questionable though, I don't remember where
> ClientSocketHandle ownership landed with respect to HttpStreamFactory.

I'll get to work on your changes, but I don't think I understand the "why"
here...  Why does this approach not honor the connection limits?  (I'm sure
you're right, I just don't know how they work)

[vandebo (ex-Chrome), 2010-12-14 18:09:23]

On 2010/12/14 18:01:01, Ryan Hamilton wrote:
> On 2010/12/14 00:30:22, vandebo wrote:
> > I'm sorry to say that I found another hiccup; as is, this doesn't handle
> > connection limits.  I think that can be solved by making
> > ERR_HTTPS_PROXY_TUNNEL_CONNECTION_RESPONSE a recoverable error at the
> > HttpProxyClientSocket level and then pushing the SSLConnectJob's transport
> > socket up to the StreamRequest as additional error state.  Ownership of that
> > ClientSocketHandle is questionable though, I don't remember where
> > ClientSocketHandle ownership landed with respect to HttpStreamFactory.
> 
> I'll get to work on your changes, but I don't think I understand the "why"
> here...  Why does this approach not honor the connection limits?  (I'm sure
> you're right, I just don't know how they work)

Connection limit tracking is done in the pool by how many sockets are handed
out.  By making the HttpProxyClientSocketPool hand out the socket (via a
"recoverable error"), you enforce the connection limit for that pool and
implicitly any pools layered on top of it.  The rest of the work is to ensure
that the socket gets released back to the correct pool.

[Ryan Hamilton, 2010-12-15 20:14:17]

I've addressed all of Will's comments, and most of Steve's.  The only two
outstanding issues are:
1) Socket reuse.  Steve, can you confirm that I understand your request
correctly?
2) Socket limit issues.  Again, Steve, can you confirm that I'm on the right
track and actually grok the issue..?

http://codereview.chromium.org/4935001/diff/234003/net/base/net_error_list.h
File net/base/net_error_list.h (right):

http://codereview.chromium.org/4935001/diff/234003/net/base/net_error_list.h#...
net/base/net_error_list.h:218: // A request to create and SSL tunnel connection
through the HTTPS proxy
On 2010/12/14 00:30:23, vandebo wrote:
> and->an

Done.

http://codereview.chromium.org/4935001/diff/234003/net/http/http_basic_stream.h
File net/http/http_basic_stream.h (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_basic_stream...
net/http/http_basic_stream.h:34: HttpBasicStream(HttpStreamParser* parser,
ClientSocketHandle* connection);
On 2010/12/14 00:34:42, willchan wrote:
> Overloading is against the style guide.  It's not immediately clear what the
> difference is between the two constructors.  I think you should merge them and
> document them and use DCHECKs to make sure the values make sense.

Done.

http://codereview.chromium.org/4935001/diff/234003/net/http/http_basic_stream...
net/http/http_basic_stream.h:34: HttpBasicStream(HttpStreamParser* parser,
ClientSocketHandle* connection);
On 2010/12/14 00:30:23, vandebo wrote:
> Maybe comments describing the different use cases for these constructors?

Sounds like a good idea, though willchan pointed out that overloading is
verboten, so I've reworked this.

http://codereview.chromium.org/4935001/diff/234003/net/http/http_network_tran...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_network_tran...
net/http/http_network_transaction.cc:287: !stream_.get()) {
On 2010/12/14 00:30:23, vandebo wrote:
> This seems to make the conditional a bit more fragile, is there something
better
> to conditional on?  Maybe an explicit bool ok_read_tunnel_body_ ?

Looks like a cleaner alternative is to simply reset stream_request in
OnHttpsProxyTunnelResponse(), which I've done.  (This leaves the conditional
unchanged.)

http://codereview.chromium.org/4935001/diff/234003/net/http/http_network_tran...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_network_tran...
net/http/http_network_transaction_unittest.cc:4472: SessionDependencies
session_deps(ProxyService::CreateFixed("https://proxy:70"));
On 2010/12/14 00:34:42, willchan wrote:
> 80 columns :)

Done.  I wonder why the pre-commit check didn't warn me about this...

http://codereview.chromium.org/4935001/diff/234003/net/http/http_network_tran...
net/http/http_network_transaction_unittest.cc:4521: SessionDependencies
session_deps(ProxyService::CreateFixed("https://proxy:70"));
On 2010/12/14 00:34:42, willchan wrote:
> 80 columns :)

Done.

http://codereview.chromium.org/4935001/diff/234003/net/http/http_network_tran...
net/http/http_network_transaction_unittest.cc:4581: SessionDependencies
session_deps(ProxyService::CreateFixed("https://proxy:70"));
On 2010/12/14 00:34:42, willchan wrote:
> 80 columns :)

Done.

http://codereview.chromium.org/4935001/diff/234003/net/http/http_network_tran...
net/http/http_network_transaction_unittest.cc:4634: SessionDependencies
session_deps(ProxyService::CreateFixed("https://proxy:70"));
On 2010/12/14 00:34:42, willchan wrote:
> 80 columns :)

Done.

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
net/http/http_proxy_client_socket.cc:419: return
ERR_HTTPS_PROXY_TUNNEL_CONNECTION_RESPONSE;
On 2010/12/14 00:30:23, vandebo wrote:
> Should you call response_.headers.RemoveHeader("KeepAlive") here to ensure
that
> the socket is not reused?

I'm not sure I follow.  Are you concerned about the SSL & TCP sockets (to the
proxy) being reused, or the Http|SpdyProxyClientSockets?  In the case of the
former, we send a valid HTTP request (CONNECT) to the proxy, and received a
valid (though non 200) HTTP response.  As such, it seems perfectly fine to reuse
the connection to the proxy.  Do you agree?

In the case of the Spdy|HttpProxyClientSocket, you're right that we don't want
them to be reused.  Of course, since the have released their transport sockets,
the're definitely not reusable :>  Does that sound right?

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
File net/http/http_proxy_client_socket.h (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
net/http/http_proxy_client_socket.h:16: #include
"net/http/http_proxy_tunnel_client_socket.h"
On 2010/12/14 00:34:42, willchan wrote:
> Sort alphabetically?

Done.

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
net/http/http_proxy_client_socket.h:35: class HttpProxyClientSocket : public
HttpProxyTunnelClientSocket {
On 2010/12/14 00:34:42, willchan wrote:
> This naming looks weird.  I didn't read through all the comments between you
and
> vandebo, but I think I remember some discussion on fixing naming in the
future. 
> Please do so :)

You're right, I missed one.  Thanks!

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.cc:335: error_response_info_ =
*transport_socket_->GetConnectResponseInfo();
On 2010/12/14 00:30:23, vandebo wrote:
> Urg - it just occurred to me this connection will not be accounted for in the
> socket limits.  In order to do that, you'd need to make this a recoverable
error
> and then in the SSlClientSocketPool, push it into a ClientSocketHandle with
the
> appropriate pool so that when it was released the right pool would get it
back.
> 
> At least in the non-spdy case, that means you can't create the stream until
this
> point.  I don't have a good enough model of the spdy pool to think about it in
> this context.
> 
> Would "trafficing" in ClientSocketHandles from this point up make this
workable?

I'm still not sure that I understand the technical details here.  I think that
perhaps you're saying that if a socket is created as part of
ClientSocketHandle.Init(), it either needs to be destroyed in the process, or
must be saved via the Handle's set_socket method?  Is that right?  If a socket
(ssl, or proxy) is created on top of an existing socket, I guess the idea is
that it is created on top of an existing SocketHandle (not just Socket) so that
the Handle can manage the interaction with the pool.

Does thats sound right?  If so, I think you might be right... Trafficing in
Handles might be the way to go.  I'll take a look...

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool_unittest.cc:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2010/12/14 00:30:23, vandebo wrote:
> I'd like to see a unit test (here or at the HttpNetworkTransaction if
necessary)
> that ensures that when a http_proxy_client_socket returns
> ERR_HTTPS_PROXY_TUNNEL_CONNECTION_RESPONSE the connection is not reusable.

Just to confirm.  It's ok to reuse the TCP/SSL socket, but not the ProxySocket,
right?

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_tunnel...
File net/http/http_proxy_tunnel_client_socket.h (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_tunnel...
net/http/http_proxy_tunnel_client_socket.h:27: virtual HttpStream*
GetConnectResponseStream() = 0;
On 2010/12/14 00:30:23, vandebo wrote:
> Maybe call this Create/MakeConnectResponseStream.  Logically, it doesn't exist
> until you ask for it.

Done.

http://codereview.chromium.org/4935001/diff/234003/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/4935001/diff/234003/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.h:36: // Constructs a SpdyHttpStream that wraps an
existing SpdyStream.
On 2010/12/14 00:34:42, willchan wrote:
> Same style comment on overloading being disallowed.  In this case, I think you
> might actually want to use a separate member function to set the stream.  It
> would be a parallel of HttpStream::InitializeStream().  Maybe
> InitializeWithExistingStream().

Done.

[Ryan Hamilton, 2010-12-15 22:26:35]

(Argh!  I just composed a long reply, but got an invalid XSRF token which ate my
comments.   *sigh*)

On 2010/12/14 00:30:22, vandebo wrote:
> I'm sorry to say that I found another hiccup; as is, this doesn't handle
> connection limits.  I think that can be solved by making
> ERR_HTTPS_PROXY_TUNNEL_CONNECTION_RESPONSE a recoverable error at the
> HttpProxyClientSocket level and then pushing the SSLConnectJob's transport
> socket up to the StreamRequest as additional error state.  Ownership of that
> ClientSocketHandle is questionable though, I don't remember where
> ClientSocketHandle ownership landed with respect to HttpStreamFactory.
> ...
> Urg - it just occurred to me this connection will not be accounted for in the
> socket limits.  In order to do that, you'd need to make this a recoverable
error
> and then in the SSlClientSocketPool, push it into a ClientSocketHandle with
the
> appropriate pool so that when it was released the right pool would get it
back.
> 
> At least in the non-spdy case, that means you can't create the stream until
this
> point.  I don't have a good enough model of the spdy pool to think about it in
> this context.
> 
> Would "trafficing" in ClientSocketHandles from this point up make this
workable?

Here is what I think I understand about this situation.  When we receive the
response to the CONNECT request, there are 3 sockets involved:
1) TCP to proxy
2) SSL to proxy (possibly wrapped up in a SpdySession)
3) Spdy|HttpProxyClientSocket to origin
Sockets 1 and 2 are fully connected, are in the form of ClientSocketHandles, and
my be reused for subsequent connections.  Socket #3 is not connected, is not
wrapped in a Handle, and my not be used for subsequent connections.

In order for HttpNetworkTransaction to read the response body, it will
eventually need an HttpStream on top of Sockets 1 and 2.  Said stream may be
created by calling the CreateResponseStream method of socket 3.

From all of this, it seems to me that the right behavior is to destroy socket #3
inside the HttpProxyConnectJob (after calling CreateResponseStream) and passing
up the HttpStream as the error state.  While it seems "odd" for a socket pool to
actually create a stream, it is not any different than the socket creating
HttpResponseHeaders (which it already traffics in).  If we try to pass up socket
#3, I think it's just going to be ugly.

[vandebo (ex-Chrome), 2010-12-15 22:52:37]

On 2010/12/15 22:26:35, Ryan Hamilton wrote:
> (Argh!  I just composed a long reply, but got an invalid XSRF token which ate
my
> comments.   *sigh*)
> 
> On 2010/12/14 00:30:22, vandebo wrote:
> > I'm sorry to say that I found another hiccup; as is, this doesn't handle
> > connection limits.  I think that can be solved by making
> > ERR_HTTPS_PROXY_TUNNEL_CONNECTION_RESPONSE a recoverable error at the
> > HttpProxyClientSocket level and then pushing the SSLConnectJob's transport
> > socket up to the StreamRequest as additional error state.  Ownership of that
> > ClientSocketHandle is questionable though, I don't remember where
> > ClientSocketHandle ownership landed with respect to HttpStreamFactory.
> > ...
> > Urg - it just occurred to me this connection will not be accounted for in
the
> > socket limits.  In order to do that, you'd need to make this a recoverable
> error
> > and then in the SSlClientSocketPool, push it into a ClientSocketHandle with
> the
> > appropriate pool so that when it was released the right pool would get it
> back.
> > 
> > At least in the non-spdy case, that means you can't create the stream until
> this
> > point.  I don't have a good enough model of the spdy pool to think about it
in
> > this context.
> > 
> > Would "trafficing" in ClientSocketHandles from this point up make this
> workable?
> 
> Here is what I think I understand about this situation.  When we receive the
> response to the CONNECT request, there are 3 sockets involved:
> 1) TCP to proxy
> 2) SSL to proxy (possibly wrapped up in a SpdySession)
> 3) Spdy|HttpProxyClientSocket to origin
> Sockets 1 and 2 are fully connected, are in the form of ClientSocketHandles,
and
> my be reused for subsequent connections.  Socket #3 is not connected, is not
> wrapped in a Handle, and my not be used for subsequent connections.
> 
> In order for HttpNetworkTransaction to read the response body, it will
> eventually need an HttpStream on top of Sockets 1 and 2.  Said stream may be
> created by calling the CreateResponseStream method of socket 3.
> 
> From all of this, it seems to me that the right behavior is to destroy socket
#3
> inside the HttpProxyConnectJob (after calling CreateResponseStream) and
passing
> up the HttpStream as the error state.  While it seems "odd" for a socket pool
to
> actually create a stream, it is not any different than the socket creating
> HttpResponseHeaders (which it already traffics in).  If we try to pass up
socket
> #3, I think it's just going to be ugly.

I don't see how passing up socket 3 is ugly and doing so will preserve, as much
as possible, the abstraction of streams on top of socket handles on top of pools
on top of sockets.

The intended stack is:
A) TCP to proxy
B) SSL to proxy
C) ProxyClientSocket connecting proxy to origin
D) SSL to origin
But at each level there's a pool wrapping the socket by way of a
ClientSocketHandle.  When the connect fails, I think what should make it a
recoverable error at level C and pass that ClientSocketHandle up in additional
error state at level D.

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
net/http/http_proxy_client_socket.cc:419: return
ERR_HTTPS_PROXY_TUNNEL_CONNECTION_RESPONSE;
On 2010/12/15 20:14:17, Ryan Hamilton wrote:
> On 2010/12/14 00:30:23, vandebo wrote:
> > Should you call response_.headers.RemoveHeader("KeepAlive") here to ensure
> that
> > the socket is not reused?
> 
> I'm not sure I follow.  Are you concerned about the SSL & TCP sockets (to the
> proxy) being reused, or the Http|SpdyProxyClientSockets?  In the case of the
> former, we send a valid HTTP request (CONNECT) to the proxy, and received a
> valid (though non 200) HTTP response.  As such, it seems perfectly fine to
reuse
> the connection to the proxy.  Do you agree?
> 
> In the case of the Spdy|HttpProxyClientSocket, you're right that we don't want
> them to be reused.  Of course, since the have released their transport
sockets,
> the're definitely not reusable :>  Does that sound right?

You're right about this.  I wasn't thinking that this was a recoverable
condition, but of course it is.  The proxy connection can be reused for a
different CONNECT request.  The SSL socket is never set up, so there's no way
for it to get reused.

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.cc:335: error_response_info_ =
*transport_socket_->GetConnectResponseInfo();
On 2010/12/15 20:14:17, Ryan Hamilton wrote:
> On 2010/12/14 00:30:23, vandebo wrote:
> > Urg - it just occurred to me this connection will not be accounted for in
the
> > socket limits.  In order to do that, you'd need to make this a recoverable
> error
> > and then in the SSlClientSocketPool, push it into a ClientSocketHandle with
> the
> > appropriate pool so that when it was released the right pool would get it
> back.
> > 
> > At least in the non-spdy case, that means you can't create the stream until
> this
> > point.  I don't have a good enough model of the spdy pool to think about it
in
> > this context.
> > 
> > Would "trafficing" in ClientSocketHandles from this point up make this
> workable?
> 
> I'm still not sure that I understand the technical details here.  I think that
> perhaps you're saying that if a socket is created as part of
> ClientSocketHandle.Init(), it either needs to be destroyed in the process, or
> must be saved via the Handle's set_socket method?  Is that right?  If a socket
> (ssl, or proxy) is created on top of an existing socket, I guess the idea is
> that it is created on top of an existing SocketHandle (not just Socket) so
that
> the Handle can manage the interaction with the pool.
> 
> Does thats sound right?  If so, I think you might be right... Trafficing in
> Handles might be the way to go.  I'll take a look...

I think you've got this right.  A ClientSocketHandle has a reference to a
particular pool and a slot for a socket.  Init tries to fill that slot.  Sockets
that layer will internally have a ClientSocketHandle (usually called
transport_socket_) to manage that underlying pool allocation.  If a pool were to
return (by any means) a ClientSocket without putting it into a
ClientSocketHandle, that socket wouldn't be counted against the connection limit
and there would be no link to the pool that created it.

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/4935001/diff/234003/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool_unittest.cc:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2010/12/15 20:14:17, Ryan Hamilton wrote:
> On 2010/12/14 00:30:23, vandebo wrote:
> > I'd like to see a unit test (here or at the HttpNetworkTransaction if
> necessary)
> > that ensures that when a http_proxy_client_socket returns
> > ERR_HTTPS_PROXY_TUNNEL_CONNECTION_RESPONSE the connection is not reusable.
> 
> Just to confirm.  It's ok to reuse the TCP/SSL socket, but not the
ProxySocket,
> right?

Yea, I had a think-o.

http://codereview.chromium.org/4935001/diff/309002/net/http/http_proxy_client...
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/4935001/diff/309002/net/http/http_proxy_client...
net/http/http_proxy_client_socket.cc:53: LOG(INFO) << "Creating an
HttpProxyClientSocket: " << this;
dd

http://codereview.chromium.org/4935001/diff/309002/net/http/http_proxy_client...
net/http/http_proxy_client_socket.cc:63: LOG(INFO) << "Destroying an
HttpProxyClientSocket: " << this;
dd

[Ryan Hamilton, 2010-12-15 23:29:21]

On Wed, Dec 15, 2010 at 2:52 PM, <vandebo@chromium.org> wrote:
>
>
> I don't see how passing up socket 3 is ugly and doing so will preserve, as
> much
> as possible, the abstraction of streams on top of socket handles on top of
> pools
> on top of sockets.
>

I don't believe there the abstraction of socket handles on top of pools on
top of sockets is well defined in the case of errors.  For example, socket
pools already know how to pass up HttpResponseInfo which has nothing to do
with sockets or pools, be everything to do with the realities of what
happens when you try to create an a CONNECT tunnel.

Further, since in the event of a non-200 (not 407) response to a CONNECT
method, the HttpProxyClientSocket has, effectively, failed to connect and
can not be reused.  As such, I think that such a socket should probably
*not* count against the HttpProxyClientSocket limit.

The intended stack is:
> A) TCP to proxy
> B) SSL to proxy
> C) ProxyClientSocket connecting proxy to origin
> D) SSL to origin
>

*nod*


> But at each level there's a pool wrapping the socket by way of a
> ClientSocketHandle.


Sure, if connect for a given socket succeeds, then Init() will succeed and
the socket will be wrapped up in a Handle.  From looking at the code it
seems, at best atypical, for a socket that failed to connect (in a
non-recoverable way) to be wrapped up in a Handle.  That's what we're
proposing here.  I just feels ugly to wrap a completely failed socket into a
Handle...

However, since you feel strongly that this is the way to go, I'll make it
so.


> When the connect fails, I think what should make it a
> recoverable error at level C and pass that ClientSocketHandle up in
> additional
> error state at level D.


So, I'll call set_socket() in HttpProxyConnectJob for this condition.  In
the SslClientSocketPool I will detect the ERR_HTTPS_PROXY_RESPONSE, and
essentially treat it the same way as ERR_PROXY_AUTH_REQUESTED.

http://codereview.chromium.org/4935001/diff/309002/net/http/http_proxy_client...
>
> File net/http/http_proxy_client_socket.cc (right):
>
>
>
http://codereview.chromium.org/4935001/diff/309002/net/http/http_proxy_client...
> net/http/http_proxy_client_socket.cc:53: LOG(INFO) << "Creating an
> HttpProxyClientSocket: " << this;
> dd
>

Doh!  I'll remove.


>
>
http://codereview.chromium.org/4935001/diff/309002/net/http/http_proxy_client...
> net/http/http_proxy_client_socket.cc:63: LOG(INFO) << "Destroying an
> HttpProxyClientSocket: " << this;
> dd


Doh!  I'll remove.

[Ryan Hamilton, 2010-12-16 00:21:03]

On 2010/12/15 22:52:37, vandebo wrote:
> I don't see how passing up socket 3 is ugly and doing so will preserve, as
much
> as possible, the abstraction of streams on top of socket handles on top of
pools
> on top of sockets.
> 
> The intended stack is:
> A) TCP to proxy
> B) SSL to proxy
> C) ProxyClientSocket connecting proxy to origin
> D) SSL to origin
> But at each level there's a pool wrapping the socket by way of a
> ClientSocketHandle.  When the connect fails, I think what should make it a
> recoverable error at level C and pass that ClientSocketHandle up in additional
> error state at level D.

Once more unto the breach... I believe I have this working as you suggested.

[vandebo (ex-Chrome), 2010-12-16 02:24:31]

Ehh, I liked it better before... J/K  Thank you for making this change.  It ends
up unifying with the proxy auth case to a large extent and requires less code,
though I didn't realize how much a priori.

LGTM with some nits at your discretion.

On 2010/12/15 23:29:21, Ryan Hamilton wrote:

Maybe replying to your comments will make you feel better about the structure
I've forced on you...

> On Wed, Dec 15, 2010 at 2:52 PM, <mailto:vandebo@chromium.org> wrote:
> >
> >
> > I don't see how passing up socket 3 is ugly and doing so will preserve, as
> > much
> > as possible, the abstraction of streams on top of socket handles on top of
> > pools
> > on top of sockets.
> >
> 
> I don't believe there the abstraction of socket handles on top of pools on
> top of sockets is well defined in the case of errors.  For example, socket
> pools already know how to pass up HttpResponseInfo which has nothing to do
> with sockets or pools, be everything to do with the realities of what
> happens when you try to create an a CONNECT tunnel.

I admit that HttpProxyClientSocket is a strange beast, but I think all the
exceptions to the layering I was describing are due to it.  This approach
minimizes the violations of that layering.
 
> Further, since in the event of a non-200 (not 407) response to a CONNECT
> method, the HttpProxyClientSocket has, effectively, failed to connect and
> can not be reused.  As such, I think that such a socket should probably
> *not* count against the HttpProxyClientSocket limit.

A "recoverable error" has a lot to do with the internal structure of the socket
pool.  We can only hand out a socket from a pool by setting the socket.  Until
recently, if you got a socket, that meant you were good to go.  Recoverable
error is a name I gave to the situation where we have to report an error, but
also want to hand out a socket.  The name doesn't apply as well to this
situation as the initial two (ssl cert error, proxy auth error), but the result
and semantics are the same.

Logically it should be the HttpProxyClientSocket that reports the "recoverable
error" because that is where the information for the error lives.  It would be
strange to ask the transport SSL socket for information about the CONNECT
transaction that failed.

http://codereview.chromium.org/4935001/diff/357003/net/http/http_network_tran...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/4935001/diff/357003/net/http/http_network_tran...
net/http/http_network_transaction_unittest.cc:4582:
TEST_F(HttpNetworkTransactionTest, ErrorResponseTofHttpsConnectViaHttpsProxy) {
Can you check, in this test and the next, that the SSL cert is invalid?  We
don't want to show a lock icon for a page provided by a proxy.

http://codereview.chromium.org/4935001/diff/357003/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/4935001/diff/357003/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.cc:328: result ==
ERR_HTTPS_PROXY_TUNNEL_RESPONSE) {
nit: can you update the comment in client_socket_pool.h about possible
recoverable errors.

http://codereview.chromium.org/4935001/diff/357003/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool.h (right):

http://codereview.chromium.org/4935001/diff/357003/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.h:161: scoped_ptr<ProxyClientSocket>
transport_socket_;
nit: I don't think the changes in this file are required anymore.

http://codereview.chromium.org/4935001/diff/357003/net/http/http_stream_reque...
File net/http/http_stream_request.cc (right):

http://codereview.chromium.org/4935001/diff/357003/net/http/http_stream_reque...
net/http/http_stream_request.cc:260:
static_cast<HttpProxyClientSocket*>(connection_->socket());
note, no change necessary: Should we now cast this to ProxyClientSocket? Would
that help spdy proxy auth work?

http://codereview.chromium.org/4935001/diff/357003/net/http/proxy_client_sock...
File net/http/proxy_client_socket.h (right):

http://codereview.chromium.org/4935001/diff/357003/net/http/proxy_client_sock...
net/http/proxy_client_socket.h:18: ProxyClientSocket() {}
nit: Not needed.

[Ryan Hamilton, 2010-12-16 05:23:49]

On 2010/12/16 02:24:31, vandebo wrote:
> Ehh, I liked it better before...

*hah*  I almost passwd out when I read that :>

>  J/K  Thank you for making this change.  It ends
> up unifying with the proxy auth case to a large extent and requires less code,
> though I didn't realize how much a priori.

Yeah, that really did turn out to be nice.  Good call...

> A "recoverable error" has a lot to do with the internal structure of the
socket
> pool.  We can only hand out a socket from a pool by setting the socket.  Until
> recently, if you got a socket, that meant you were good to go.  Recoverable
> error is a name I gave to the situation where we have to report an error, but
> also want to hand out a socket.  The name doesn't apply as well to this
> situation as the initial two (ssl cert error, proxy auth error), but the
result
> and semantics are the same.

Ah, I understand now.  I should have read the comments in client_socket_pool.h
which helps to clear this up.  I think I agree with you about this.

> Logically it should be the HttpProxyClientSocket that reports the "recoverable
> error" because that is where the information for the error lives.  It would be
> strange to ask the transport SSL socket for information about the CONNECT
> transaction that failed.

*nod*

http://codereview.chromium.org/4935001/diff/357003/net/http/http_network_tran...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/4935001/diff/357003/net/http/http_network_tran...
net/http/http_network_transaction_unittest.cc:4582:
TEST_F(HttpNetworkTransactionTest, ErrorResponseTofHttpsConnectViaHttpsProxy) {
On 2010/12/16 02:24:31, vandebo wrote:
> Can you check, in this test and the next, that the SSL cert is invalid?  We
> don't want to show a lock icon for a page provided by a proxy.

Done.

http://codereview.chromium.org/4935001/diff/357003/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/4935001/diff/357003/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.cc:328: result ==
ERR_HTTPS_PROXY_TUNNEL_RESPONSE) {
On 2010/12/16 02:24:31, vandebo wrote:
> nit: can you update the comment in client_socket_pool.h about possible
> recoverable errors.

Done.

http://codereview.chromium.org/4935001/diff/357003/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool.h (right):

http://codereview.chromium.org/4935001/diff/357003/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.h:161: scoped_ptr<ProxyClientSocket>
transport_socket_;
On 2010/12/16 02:24:31, vandebo wrote:
> nit: I don't think the changes in this file are required anymore.

Agreed.  I noticed that to.  On the other hand, I kinda prefer this change since
it's a bit more specific about what the client socket pool traffics in.  If you
don't mind, I think I'd like to keep these changes...

http://codereview.chromium.org/4935001/diff/357003/net/http/http_stream_reque...
File net/http/http_stream_request.cc (right):

http://codereview.chromium.org/4935001/diff/357003/net/http/http_stream_reque...
net/http/http_stream_request.cc:260:
static_cast<HttpProxyClientSocket*>(connection_->socket());
On 2010/12/16 02:24:31, vandebo wrote:
> note, no change necessary: Should we now cast this to ProxyClientSocket? Would
> that help spdy proxy auth work?

Yeah, I thought about that and think it's a good change that we should make. 
However, we need to widen the ProxyClientSocket interface to expose the
auth_controller() method called a few lines later.  I'll do that in a second CL.

http://codereview.chromium.org/4935001/diff/357003/net/http/proxy_client_sock...
File net/http/proxy_client_socket.h (right):

http://codereview.chromium.org/4935001/diff/357003/net/http/proxy_client_sock...
net/http/proxy_client_socket.h:18: ProxyClientSocket() {}
On 2010/12/16 02:24:31, vandebo wrote:
> nit: Not needed.

Hm.  I just tried to remove it, but got this message:

net/spdy/spdy_proxy_client_socket.cc:48: error: no matching function for call to
‘net::ProxyClientSocket::ProxyClientSocket()’

Is this an artifact of:

DISALLOW_COPY_AND_ASSIGN(ProxyClientSocket);

[vandebo (ex-Chrome), 2010-12-16 17:59:23]

LGTM

On 2010/12/16 05:23:49, Ryan Hamilton wrote:
> Yeah, that really did turn out to be nice.  Good call...

Hurray!

http://codereview.chromium.org/4935001/diff/335002/net/http/http_network_tran...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/4935001/diff/335002/net/http/http_network_tran...
net/http/http_network_transaction_unittest.cc:4628:
EXPECT_FALSE(response->ssl_info.is_valid());
Just to double check, this is true in some other unit tests?

http://codereview.chromium.org/4935001/diff/335002/net/http/http_proxy_client...
File net/http/http_proxy_client_socket_pool.h (right):

http://codereview.chromium.org/4935001/diff/335002/net/http/http_proxy_client...
net/http/http_proxy_client_socket_pool.h:161: scoped_ptr<ProxyClientSocket>
transport_socket_;
Looking at this more closely, it's misnamed.  I wouldn't mind if you renamed it
to proxy_socket_.

http://codereview.chromium.org/4935001/diff/335002/net/spdy/spdy_proxy_client...
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/4935001/diff/335002/net/spdy/spdy_proxy_client...
net/spdy/spdy_proxy_client_socket.h:62: // HttpProxyTunnelClientSocket methods:
nit: ProxyClientSocket

[Ryan Hamilton, 2010-12-17 03:33:04]

On 2010/12/16 17:59:23, vandebo wrote:
> LGTM

Awesome.

http://codereview.chromium.org/4935001/diff/335002/net/http/http_network_tran...
> File net/http/http_network_transaction_unittest.cc (right):
> 
>
http://codereview.chromium.org/4935001/diff/335002/net/http/http_network_tran...
> net/http/http_network_transaction_unittest.cc:4628:
> EXPECT_FALSE(response->ssl_info.is_valid());
> Just to double check, this is true in some other unit tests?

Doh!  Alas not.  Looks like out test infrastructure doesn't set this in the
successful case.  Sounds like that would be a good improvement.  I'll leave the
code in the test to  express the intent at least (and if we ever fix the
framework, even better).

http://codereview.chromium.org/4935001/diff/335002/net/http/http_proxy_client...
> File net/http/http_proxy_client_socket_pool.h (right):
> 
>
http://codereview.chromium.org/4935001/diff/335002/net/http/http_proxy_client...
> net/http/http_proxy_client_socket_pool.h:161: scoped_ptr<ProxyClientSocket>
> transport_socket_;
> Looking at this more closely, it's misnamed.  I wouldn't mind if you renamed
it
> to proxy_socket_.

Sounds good, though I think I'll do this in a subsequent CL, along with adding
auth_controller() to ProxyClientSocket, and changing HttpStreamRequest to cast
the proxy_socket accordingly.

http://codereview.chromium.org/4935001/diff/335002/net/spdy/spdy_proxy_client...
> File net/spdy/spdy_proxy_client_socket.h (right):
> 
>
http://codereview.chromium.org/4935001/diff/335002/net/spdy/spdy_proxy_client...
> net/spdy/spdy_proxy_client_socket.h:62: // HttpProxyTunnelClientSocket
methods:
> nit: ProxyClientSocket

Done.

[vandebo (ex-Chrome), 2010-12-17 17:36:22]

On 2010/12/17 03:33:04, Ryan Hamilton wrote:
> On 2010/12/16 17:59:23, vandebo wrote:
http://codereview.chromium.org/4935001/diff/335002/net/http/http_network_tran...
> > net/http/http_network_transaction_unittest.cc:4628:
> > EXPECT_FALSE(response->ssl_info.is_valid());
> > Just to double check, this is true in some other unit tests?
> 
> Doh!  Alas not.  Looks like out test infrastructure doesn't set this in the
> successful case.  Sounds like that would be a good improvement.  I'll leave
the
> code in the test to  express the intent at least (and if we ever fix the
> framework, even better).

Actually, it could be misleading to see this code and think it does the check we
want.  Maybe change this to a TODO in your cleanup CL.