Allow chrome to handle 407 auth challenges to CONNECT requests

Allow chrome to handle 407 auth challenges to CONNECT requests
through HTTPS Proxies. This also changes the mechanism used 
to restart HttpProxyClientSocket requests with auth. Previously
the transport socket would be Disconnected, and then re-Connected
(which was not implemented for SSLClientSockets). However, the 
approach was problematic in the face of, for example, ipv6. The
new approach is to close the HttpProxyClientSocket, and request
a new socket from the pool.

Initially was http://codereview.chromium.org/8502024
which turned out to have problems with NTLM auth.

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

[Ryan Hamilton, 2012-01-12 19:37:03]

At long last, I think I've got this to work.  *whew*  Please take a look...

[Ryan Hamilton, 2012-01-17 16:33:22]

Grr.  I sent this out for review last week, but manage to omit a reviewer.  That
explains why you hadn't gotten back to me yet :|

[vandebo (ex-Chrome), 2012-01-17 19:22:50]

On 2012/01/17 16:33:22, Ryan Hamilton wrote:
> Grr.  I sent this out for review last week, but manage to omit a reviewer. 
That
> explains why you hadn't gotten back to me yet :|

I'll certainly give you a review, but do you need a net owner as well?

[Ryan Hamilton, 2012-01-17 19:25:35]

On 2012/01/17 19:22:50, vandebo wrote:
> On 2012/01/17 16:33:22, Ryan Hamilton wrote:
> > Grr.  I sent this out for review last week, but manage to omit a reviewer. 
> That
> > explains why you hadn't gotten back to me yet :|
> 
> I'll certainly give you a review, but do you need a net owner as well?

Hm.  Possibly.  I'm a net OWNER, which I *think* is technically sufficient; is
that right?  Should I add cbentzel as well?

[vandebo (ex-Chrome), 2012-01-17 19:26:23]

On 2012/01/17 19:25:35, Ryan Hamilton wrote:
> On 2012/01/17 19:22:50, vandebo wrote:
> > On 2012/01/17 16:33:22, Ryan Hamilton wrote:
> > > Grr.  I sent this out for review last week, but manage to omit a reviewer.

> > That
> > > explains why you hadn't gotten back to me yet :|
> > 
> > I'll certainly give you a review, but do you need a net owner as well?
> 
> Hm.  Possibly.  I'm a net OWNER, which I *think* is technically sufficient; is
> that right?  Should I add cbentzel as well?

Wouldn't hurt.

[Ryan Hamilton, 2012-01-17 20:08:50]

Adding cbentzel

[cbentzel, 2012-01-17 20:19:57]

I won't get a chance to review this until tomorrow morning. Is that OK?

On Tue, Jan 17, 2012 at 3:08 PM, <rch@chromium.org> wrote:

> Adding cbentzel
>
>
http://codereview.chromium.**org/9148011/<http://codereview.chromium.org/9148...
>

[cbentzel, 2012-01-18 12:14:22]

Adding asanka as well.

http://codereview.chromium.org/9148011/diff/28005/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/9148011/diff/28005/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:189: // TODO(rch): figure
out what to do here
Do you want to figure this out prior to landing this CL?

[rch (use chromium not google), 2012-01-18 17:11:56]

Good point.  Yes, this should be resolved before landing.  I'll grab
someone from jingle to take a look.

On Wed, Jan 18, 2012 at 4:14 AM, <cbentzel@chromium.org> wrote:

> Adding asanka as well.
>
>
> http://codereview.chromium.**org/9148011/diff/28005/jingle/**
>
notifier/base/proxy_resolving_**client_socket.cc<http://codereview.chromium.org/9148011/diff/28005/jingle/notifier/base/proxy_resolving_client_socket.cc>
> File jingle/notifier/base/proxy_**resolving_client_socket.cc (right):
>
> http://codereview.chromium.**org/9148011/diff/28005/jingle/**
>
notifier/base/proxy_resolving_**client_socket.cc#newcode189<http://codereview.chromium.org/9148011/diff/28005/jingle/notifier/base/proxy_resolving_client_socket.cc#newcode189>
> jingle/notifier/base/proxy_**resolving_client_socket.cc:**189: //
> TODO(rch):
> figure out what to do here
> Do you want to figure this out prior to landing this CL?
>
>
http://codereview.chromium.**org/9148011/<http://codereview.chromium.org/9148...
>

[Ryan Hamilton, 2012-01-19 17:21:31]

vandebo, cbentzel: ping

[vandebo (ex-Chrome), 2012-01-19 17:28:46]

On 2012/01/19 17:21:31, Ryan Hamilton wrote:
> vandebo, cbentzel: ping

I'm in the middle of my review, but it sounded like the jingle issue was
blocking anyway.

[rch (use chromium not google), 2012-01-19 17:32:46]

On Thu, Jan 19, 2012 at 9:28 AM, <vandebo@chromium.org> wrote:

> On 2012/01/19 17:21:31, Ryan Hamilton wrote:
>
>> vandebo, cbentzel: ping
>>
>
> I'm in the middle of my review, but it sounded like the jingle issue was
> blocking anyway.
>
>
http://codereview.chromium.**org/9148011/<http://codereview.chromium.org/9148...
>

Cool, thanks.  The jingle issues does need to get resolved before this can
land, and I've contacted a jingle OWNER to work through this with them.
 However, I suspect that it'll largely be orthogonal to the rest of the
review.

[cbentzel, 2012-01-19 17:53:41]

OK. Will proceed then.

On Thu, Jan 19, 2012 at 12:32 PM, Ryan Hamilton <rch@google.com> wrote:

> On Thu, Jan 19, 2012 at 9:28 AM, <vandebo@chromium.org> wrote:
>
>> On 2012/01/19 17:21:31, Ryan Hamilton wrote:
>>
>>> vandebo, cbentzel: ping
>>>
>>
>> I'm in the middle of my review, but it sounded like the jingle issue was
>> blocking anyway.
>>
>>
http://codereview.chromium.**org/9148011/<http://codereview.chromium.org/9148...
>>
>
> Cool, thanks.  The jingle issues does need to get resolved before this can
> land, and I've contacted a jingle OWNER to work through this with them.
>  However, I suspect that it'll largely be orthogonal to the rest of the
> review.
>

[vandebo (ex-Chrome), 2012-01-19 20:12:49]

I haven't looked at the updates to testing code yet, but figured you'd like
these comments sooner.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket.cc (left):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket.cc:263: next_state_ = STATE_TCP_RESTART;
You should also tear out the TCP_RESTART states.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket.cc:259: next_state_ = STATE_NONE;
Seems like in this case we want to make the function return
ERR_CONNECTION_CLOSED or a new error like ERR_NO_KEEP_ALIVE_ON_AUTH_RESTART.  At
this point the connection isn't ready to go and the caller should be directly
informed of that.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.cc (left):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:90:
protocol_negotiated_(SSLClientSocket::kProtoUnknown) {
Why are you removing the initialization of this variable?

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:303: void
HttpProxyConnectJob::HandleProxyAuthChallenge() {
I think you were asking if you could get rid of this method.  My sources think
it's possible, ping ajwong for the exact syntax.  (At your discretion.)

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:315: return ERR_PROXY_AUTH_REQUESTED;
Should we set the socket to the transport socket here?  I'm not sure.  I guess
the reason to do it would be so that the user sees the 407 error page, is that
what we want?

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:325:
set_socket(transport_socket_.release());
I guess maybe we do want to set the socket above, since we do here.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:331: return ConnectInternal();
I think you're already inside a DoLoop here (You got here from callback_ called
from params_->auth_needed_callback(), which did DoRestartWithAuth, and then this
method.  I think you just want to set the state you want and return.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:339: // Handle a proxy auth challenge
by asynchronously invoking the callback.
Wouldn't hurt to explain why it's being done asynchronously.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.h (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.h:47: TunnelAuthNeededCB;
Chrome style says not to abbreviate, also needed makes this hard to read.  Would
TunnelAuthCallback be ok?

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:185: const AuthCredentials&
credentials) {
The credentials are thrown away?  It looks like they were never used; should we
remove the argument?

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:195: }
Null out tunnel_auth_handled_callback_ when you're done with it?

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:354: tunnel_auth_handled_callback_ =
callback;
DCHECK that tunnel_auth_handled_callback_ == NULL ?

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.h (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.h:51: void
ReallyRestartTunnelWithProxyAuth();
nit: These are usually named DoFoo

http://codereview.chromium.org/9148011/diff/28005/net/socket/client_socket_po...
File net/socket/client_socket_pool_manager.h (right):

http://codereview.chromium.org/9148011/diff/28005/net/socket/client_socket_po...
net/socket/client_socket_pool_manager.h:93: TunnelAuthNeededCB
auth_needed_callback);
I think the completion callback is usually last.

http://codereview.chromium.org/9148011/diff/28005/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/9148011/diff/28005/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket.cc:67: return OK;
Same thing here - why return OK when the proxy pool needs to spin up a new
"connection" ?

http://codereview.chromium.org/9148011/diff/28005/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket.cc:81: // TODO(rch): handle 407 proxy auth
requested correctly, perhaps
Is this TODO now handled?

[akalin, 2012-01-19 21:07:37]

http://codereview.chromium.org/9148011/diff/28005/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/9148011/diff/28005/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:189: // TODO(rch): figure
out what to do here
so I tried to trace through what 'regular' sockets do and as far as I can tell
it ends up calling HttpNetworkTransaction::OnNeedsProxyAuth.  This client socket
is used for XMPP connections (used by sync and cloud print) and so there's no
HTTP transaction at all (I assume that HttpNetworkTransaction refers to the
'real' HTTP connection and not possibly the HTTP proxy).

So in Chrome, whenever proxy auth is needed for a web page it pops up a window
asking for credentials, right?  We've never supported that for sync
(http://crbug.com/19581), and I think doing so would require thinking through
the UI flow (since users may be confused by sync popping up an auth dialog when
there's no visible web page).

So, short answer, I think we can 'fail' here, or do whatever it used to do
before this change.

[Ryan Hamilton, 2012-01-19 21:17:25]

On 2012/01/19 21:07:37, akalin wrote:
>
http://codereview.chromium.org/9148011/diff/28005/jingle/notifier/base/proxy_...
> File jingle/notifier/base/proxy_resolving_client_socket.cc (right):
> 
>
http://codereview.chromium.org/9148011/diff/28005/jingle/notifier/base/proxy_...
> jingle/notifier/base/proxy_resolving_client_socket.cc:189: // TODO(rch):
figure
> out what to do here
> so I tried to trace through what 'regular' sockets do and as far as I can tell
> it ends up calling HttpNetworkTransaction::OnNeedsProxyAuth.  This client
socket
> is used for XMPP connections (used by sync and cloud print) and so there's no
> HTTP transaction at all (I assume that HttpNetworkTransaction refers to the
> 'real' HTTP connection and not possibly the HTTP proxy).
> 
> So in Chrome, whenever proxy auth is needed for a web page it pops up a window
> asking for credentials, right?  We've never supported that for sync
> (http://crbug.com/19581), and I think doing so would require thinking through
> the UI flow (since users may be confused by sync popping up an auth dialog
when
> there's no visible web page).
> 
> So, short answer, I think we can 'fail' here, or do whatever it used to do
> before this change.

Great.  That's what the code I added to jingle does.  I've removed the TODO and
added a comment that explains the behavior.

[akalin, 2012-01-19 21:20:39]

http://codereview.chromium.org/9148011/diff/39001/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/9148011/diff/39001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:177:
base::Unretained(this)));
is it really safe to use Unretained here?  If so it's not obvious; would it hurt
to use weak ptrs here (and the other places)?  Is there an easy way to see that
Unretained is safe here?

[Ryan Hamilton, 2012-01-19 21:42:09]

On 2012/01/19 21:20:39, akalin wrote:
>
http://codereview.chromium.org/9148011/diff/39001/jingle/notifier/base/proxy_...
> File jingle/notifier/base/proxy_resolving_client_socket.cc (right):
> 
>
http://codereview.chromium.org/9148011/diff/39001/jingle/notifier/base/proxy_...
> jingle/notifier/base/proxy_resolving_client_socket.cc:177:
> base::Unretained(this)));
> is it really safe to use Unretained here?  If so it's not obvious; would it
hurt
> to use weak ptrs here (and the other places)?  Is there an easy way to see
that
> Unretained is safe here?

Done.  Switched to using a weak pointer.

[akalin, 2012-01-19 22:12:48]

http://codereview.chromium.org/9148011/diff/37006/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/9148011/diff/37006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:186: void
ProxyResolvingClientSocket::OnNeedsProxyTunnelAuthCallback(
actually, since this function doesn't do much, it can be a free function in an
anonymous namespace in this .cc file.  Then you can revert the change to the .h
file, and you wouldn't need the weak ptr at all.

http://codereview.chromium.org/9148011/diff/37006/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9148011/diff/37006/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:741: base::Unretained(this)));
weakptr?

http://codereview.chromium.org/9148011/diff/37006/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:749: base::Unretained(this)));
weakptr?

[Ryan Hamilton, 2012-01-19 23:11:18]

http://codereview.chromium.org/9148011/diff/28005/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/9148011/diff/28005/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:189: // TODO(rch): figure
out what to do here
On 2012/01/18 12:14:22, cbentzel wrote:
> Do you want to figure this out prior to landing this CL?

Done.

http://codereview.chromium.org/9148011/diff/28005/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:189: // TODO(rch): figure
out what to do here
On 2012/01/19 21:07:37, akalin wrote:
> so I tried to trace through what 'regular' sockets do and as far as I can tell
> it ends up calling HttpNetworkTransaction::OnNeedsProxyAuth.  This client
socket
> is used for XMPP connections (used by sync and cloud print) and so there's no
> HTTP transaction at all (I assume that HttpNetworkTransaction refers to the
> 'real' HTTP connection and not possibly the HTTP proxy).
> 
> So in Chrome, whenever proxy auth is needed for a web page it pops up a window
> asking for credentials, right?  We've never supported that for sync
> (http://crbug.com/19581), and I think doing so would require thinking through
> the UI flow (since users may be confused by sync popping up an auth dialog
when
> there's no visible web page).
> 
> So, short answer, I think we can 'fail' here, or do whatever it used to do
> before this change.

Done.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket.cc (left):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket.cc:263: next_state_ = STATE_TCP_RESTART;
On 2012/01/19 20:12:49, vandebo wrote:
> You should also tear out the TCP_RESTART states.

Done.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket.cc:259: next_state_ = STATE_NONE;
On 2012/01/19 20:12:49, vandebo wrote:
> Seems like in this case we want to make the function return
> ERR_CONNECTION_CLOSED or a new error like ERR_NO_KEEP_ALIVE_ON_AUTH_RESTART. 
At
> this point the connection isn't ready to go and the caller should be directly
> informed of that.

Done.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.cc (left):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:90:
protocol_negotiated_(SSLClientSocket::kProtoUnknown) {
On 2012/01/19 20:12:49, vandebo wrote:
> Why are you removing the initialization of this variable?

OUCH!  Thanks for catching this mistake

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:303: void
HttpProxyConnectJob::HandleProxyAuthChallenge() {
On 2012/01/19 20:12:49, vandebo wrote:
> I think you were asking if you could get rid of this method.  My sources think
> it's possible, ping ajwong for the exact syntax.  (At your discretion.)

Will follow up with ajwong offline and possibly implement in a subsequent CL.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:315: return ERR_PROXY_AUTH_REQUESTED;
On 2012/01/19 20:12:49, vandebo wrote:
> Should we set the socket to the transport socket here?  I'm not sure.  I guess
> the reason to do it would be so that the user sees the 407 error page, is that
> what we want?

No, we don't need to do it here, because we do it in
DoRestartWithAuthComplete().  (In the case where DoRestartWithAuth returns
ERR_PROXY_AUTH_REQUESTED, we run DoRestartWithAuthComplete which sets the socket
for us.)

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:325:
set_socket(transport_socket_.release());
On 2012/01/19 20:12:49, vandebo wrote:
> I guess maybe we do want to set the socket above, since we do here.

(To amplify the previous comment, this code here handles the earlier case so
we're just fine.  In fact, adding a set_socket() above means that when this code
executes we end up set_socket() with null since transport_socket has already
been released.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:331: return ConnectInternal();
On 2012/01/19 20:12:49, vandebo wrote:
> I think you're already inside a DoLoop here (You got here from callback_
called
> from params_->auth_needed_callback(), which did DoRestartWithAuth, and then
this
> method.  I think you just want to set the state you want and return.

Done.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:339: // Handle a proxy auth challenge
by asynchronously invoking the callback.
On 2012/01/19 20:12:49, vandebo wrote:
> Wouldn't hurt to explain why it's being done asynchronously.

Done.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.h (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.h:47: TunnelAuthNeededCB;
On 2012/01/19 20:12:49, vandebo wrote:
> Chrome style says not to abbreviate, also needed makes this hard to read. 
Would
> TunnelAuthCallback be ok?

Done.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:185: const AuthCredentials&
credentials) {
On 2012/01/19 20:12:49, vandebo wrote:
> The credentials are thrown away?  It looks like they were never used; should
we
> remove the argument?

Done.  Looks like it could also be removed from HttpStreamFactoryImpl::Request
(and on up the stack).  This sounds like a good idea, but how bout in a
subsequent CL.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:195: }
On 2012/01/19 20:12:49, vandebo wrote:
> Null out tunnel_auth_handled_callback_ when you're done with it?

tunnel_auth_handled_callback_ is not a pointer, it's a CompletionCallback. 
Should I make it a pointer instead?

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:354: tunnel_auth_handled_callback_ =
callback;
On 2012/01/19 20:12:49, vandebo wrote:
> DCHECK that tunnel_auth_handled_callback_ == NULL ?

tunnel_auth_handled_callback_ is not a pointer, it's a CompletionCallback. 
Should I make it a pointer instead?

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.h (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.h:51: void
ReallyRestartTunnelWithProxyAuth();
On 2012/01/19 20:12:49, vandebo wrote:
> nit: These are usually named DoFoo

Done.  Though I thought DoFoo was typically reserved for the methods called from
DoLoop()?  (I also moved this to private)

http://codereview.chromium.org/9148011/diff/28005/net/socket/client_socket_po...
File net/socket/client_socket_pool_manager.h (right):

http://codereview.chromium.org/9148011/diff/28005/net/socket/client_socket_po...
net/socket/client_socket_pool_manager.h:93: TunnelAuthNeededCB
auth_needed_callback);
On 2012/01/19 20:12:49, vandebo wrote:
> I think the completion callback is usually last.

Done.

http://codereview.chromium.org/9148011/diff/28005/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/9148011/diff/28005/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket.cc:67: return OK;
On 2012/01/19 20:12:49, vandebo wrote:
> Same thing here - why return OK when the proxy pool needs to spin up a new
> "connection" ?

Done.

http://codereview.chromium.org/9148011/diff/28005/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket.cc:81: // TODO(rch): handle 407 proxy auth
requested correctly, perhaps
On 2012/01/19 20:12:49, vandebo wrote:
> Is this TODO now handled?

Done. Yes, I've removed the todo.

http://codereview.chromium.org/9148011/diff/37006/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/9148011/diff/37006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:186: void
ProxyResolvingClientSocket::OnNeedsProxyTunnelAuthCallback(
On 2012/01/19 22:12:48, akalin wrote:
> actually, since this function doesn't do much, it can be a free function in an
> anonymous namespace in this .cc file.  Then you can revert the change to the
.h
> file, and you wouldn't need the weak ptr at all.

Done.  (Love it!)

http://codereview.chromium.org/9148011/diff/37006/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9148011/diff/37006/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:741: base::Unretained(this)));
On 2012/01/19 22:12:48, akalin wrote:
> weakptr?

Done.

http://codereview.chromium.org/9148011/diff/37006/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:749: base::Unretained(this)));
On 2012/01/19 22:12:48, akalin wrote:
> weakptr?

Done.

[akalin, 2012-01-20 01:45:56]

sync lgtm after nits

http://codereview.chromium.org/9148011/diff/38007/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.cc (left):

http://codereview.chromium.org/9148011/diff/38007/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:183: void
ProxyResolvingClientSocket::ProcessConnectDone(int status) {
Remove extra space

http://codereview.chromium.org/9148011/diff/38007/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/9148011/diff/38007/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:28: // Since have not way
to respond, simply invoke the callback and the
Have not way -> we have no way

[cbentzel, 2012-01-20 02:09:45]

This approach looks good, as does the test coverage. Will try to do a
nit-picking pass now that I understand and agree with the approach you are
taking.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.h (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.h:39: // from an HTTP proxy when
attempting to establish a CONNECT tunnel to
Is this also in the HTTPS proxy case as well? I don't really know how we are
doing HTTPS tunneling through an HTTPS proxy.

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:186: MessageLoop::current()->PostTask(
Is this needed due to reentrancy concerns with tunnel_auth_handled_callback_?

http://codereview.chromium.org/9148011/diff/38007/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/9148011/diff/38007/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:3454:
TEST_F(HttpNetworkTransactionTest, NTLMAuthFull) {
Should the name/description of this test include anything about establishing a
tunnel through an NTLM authentication proxy?

http://codereview.chromium.org/9148011/diff/38007/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/9148011/diff/38007/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:33: std::string GetProxyUrl(const
scoped_refptr<HttpProxySocketParams>& params) {
NOTE TO SELF: Make sure this is ok.

http://codereview.chromium.org/9148011/diff/38007/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9148011/diff/38007/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:187:
base::Bind(&HttpStreamFactoryImpl::Job::DoRestartTunnelWithProxyAuth,
Is this to prevent potential reentrancy if OnNeedsProxyAuth completes
synchronously?

http://codereview.chromium.org/9148011/diff/38007/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:353: tunnel_auth_handled_callback_ =
callback;
Should this DCHECK that callback is non-null?

http://codereview.chromium.org/9148011/diff/38007/net/http/proxy_client_socket.h
File net/http/proxy_client_socket.h (right):

http://codereview.chromium.org/9148011/diff/38007/net/http/proxy_client_socke...
net/http/proxy_client_socket.h:32: virtual const
scoped_refptr<HttpAuthController>& auth_controller() = 0;
Nit: Should be named GetAuthController. auth_controller implies that it's a
simple getter, which I'd also think of as non-virtual.

http://codereview.chromium.org/9148011/diff/38007/net/socket/client_socket_po...
File net/socket/client_socket_pool_manager.h (right):

http://codereview.chromium.org/9148011/diff/38007/net/socket/client_socket_po...
net/socket/client_socket_pool_manager.h:30: class HttpProxyClientSocketPool;
This forward declaration is no longer needed. However, is there a way to remove
the need for the inclusion of http_proxy_client_socket_pool.h in the first
place? Is this for the callback typedef?

[Ryan Hamilton, 2012-01-20 03:51:02]

akalin: thanks for the lgtm!  I've fixed your nits.

http://codereview.chromium.org/9148011/diff/38007/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.cc (left):

http://codereview.chromium.org/9148011/diff/38007/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:183: void
ProxyResolvingClientSocket::ProcessConnectDone(int status) {
On 2012/01/20 01:45:56, akalin wrote:
> Remove extra space

Done.

http://codereview.chromium.org/9148011/diff/38007/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/9148011/diff/38007/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:28: // Since have not way
to respond, simply invoke the callback and the
On 2012/01/20 01:45:56, akalin wrote:
> Have not way -> we have no way

Done.

[Ryan Hamilton, 2012-01-20 04:20:03]

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.h (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.h:39: // from an HTTP proxy when
attempting to establish a CONNECT tunnel to
On 2012/01/20 02:09:46, cbentzel wrote:
> Is this also in the HTTPS proxy case as well? I don't really know how we are
> doing HTTPS tunneling through an HTTPS proxy.

Yes, good point.  I've updated the comment to include HTTPS.  (Tunneling through
an HTTPS proxy is just like tunneling through and HTTP proxy, except that we get
an SSLClientSocket to the proxy instead of a TCPClientSocket to the proxy.  Once
we have this socket, we wrap it with an HttpProxyClientSocket and issue a
CONNECT request)

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:186: MessageLoop::current()->PostTask(
On 2012/01/20 02:09:46, cbentzel wrote:
> Is this needed due to reentrancy concerns with tunnel_auth_handled_callback_? 

Yes... Specifically, we want to invoke the callback (which might, say, delete
the caller) while the caller is still waiting for this function to return.

http://codereview.chromium.org/9148011/diff/38007/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/9148011/diff/38007/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:3454:
TEST_F(HttpNetworkTransactionTest, NTLMAuthFull) {
On 2012/01/20 02:09:46, cbentzel wrote:
> Should the name/description of this test include anything about establishing a
> tunnel through an NTLM authentication proxy?

Done.

http://codereview.chromium.org/9148011/diff/38007/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/9148011/diff/38007/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:33: std::string GetProxyUrl(const
scoped_refptr<HttpProxySocketParams>& params) {
On 2012/01/20 02:09:46, cbentzel wrote:
> NOTE TO SELF: Make sure this is ok.

The self there is you, not me, right?  (I think it's ok :>)

http://codereview.chromium.org/9148011/diff/38007/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9148011/diff/38007/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:187:
base::Bind(&HttpStreamFactoryImpl::Job::DoRestartTunnelWithProxyAuth,
On 2012/01/20 02:09:46, cbentzel wrote:
> Is this to prevent potential reentrancy if OnNeedsProxyAuth completes
> synchronously? 

Yes... Specifically, we want to invoke the callback (which might, say, delete
the caller) while the caller is still waiting for this function to return.

http://codereview.chromium.org/9148011/diff/38007/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:353: tunnel_auth_handled_callback_ =
callback;
On 2012/01/20 02:09:46, cbentzel wrote:
> Should this DCHECK that callback is non-null?

Steve had the same suggestion, but it's an object, not a pointer.

http://codereview.chromium.org/9148011/diff/38007/net/http/proxy_client_socket.h
File net/http/proxy_client_socket.h (right):

http://codereview.chromium.org/9148011/diff/38007/net/http/proxy_client_socke...
net/http/proxy_client_socket.h:32: virtual const
scoped_refptr<HttpAuthController>& auth_controller() = 0;
On 2012/01/20 02:09:46, cbentzel wrote:
> Nit: Should be named GetAuthController. auth_controller implies that it's a
> simple getter, which I'd also think of as non-virtual.

Done.

http://codereview.chromium.org/9148011/diff/38007/net/socket/client_socket_po...
File net/socket/client_socket_pool_manager.h (right):

http://codereview.chromium.org/9148011/diff/38007/net/socket/client_socket_po...
net/socket/client_socket_pool_manager.h:30: class HttpProxyClientSocketPool;
On 2012/01/20 02:09:46, cbentzel wrote:
> This forward declaration is no longer needed. However, is there a way to
remove
> the need for the inclusion of http_proxy_client_socket_pool.h in the first
> place? Is this for the callback typedef?

Removed the forward decl.  yes, this is for the typedef.  I could move the
typedef into a different file, or I could simply inline it here?

[cbentzel, 2012-01-20 11:15:46]

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:186: MessageLoop::current()->PostTask(
On 2012/01/20 04:20:04, Ryan Hamilton wrote:
> On 2012/01/20 02:09:46, cbentzel wrote:
> > Is this needed due to reentrancy concerns with
tunnel_auth_handled_callback_? 
> 
> Yes... Specifically, we want to invoke the callback (which might, say, delete
> the caller) while the caller is still waiting for this function to return.

Could you add a comment about this? It was non-obvious to me why it was needed
at first glance.

http://codereview.chromium.org/9148011/diff/38007/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/9148011/diff/38007/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:33: std::string GetProxyUrl(const
scoped_refptr<HttpProxySocketParams>& params) {
On 2012/01/20 04:20:04, Ryan Hamilton wrote:
> On 2012/01/20 02:09:46, cbentzel wrote:
> > NOTE TO SELF: Make sure this is ok.
> 
> The self there is you, not me, right?  (I think it's ok :>)

Yeah, it was a note to me while doing the review. I forgot to remove it prior to
sending the review out.

http://codereview.chromium.org/9148011/diff/38007/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9148011/diff/38007/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:353: tunnel_auth_handled_callback_ =
callback;
On 2012/01/20 04:20:04, Ryan Hamilton wrote:
> On 2012/01/20 02:09:46, cbentzel wrote:
> > Should this DCHECK that callback is non-null?
> 
> Steve had the same suggestion, but it's an object, not a pointer. 

I meant DCHECK(!callback.is_null());

I try to use the lowercase null to indicate uninitialized callback, rather than
uninitialized pointer.

[Ryan Hamilton, 2012-01-20 17:20:13]

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:186: MessageLoop::current()->PostTask(
On 2012/01/20 11:15:46, cbentzel wrote:
> On 2012/01/20 04:20:04, Ryan Hamilton wrote:
> > On 2012/01/20 02:09:46, cbentzel wrote:
> > > Is this needed due to reentrancy concerns with
> tunnel_auth_handled_callback_? 
> > 
> > Yes... Specifically, we want to invoke the callback (which might, say,
delete
> > the caller) while the caller is still waiting for this function to return.
> 
> Could you add a comment about this? It was non-obvious to me why it was needed
> at first glance.
> 

Done.

http://codereview.chromium.org/9148011/diff/38007/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9148011/diff/38007/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:353: tunnel_auth_handled_callback_ =
callback;
On 2012/01/20 11:15:46, cbentzel wrote:
> On 2012/01/20 04:20:04, Ryan Hamilton wrote:
> > On 2012/01/20 02:09:46, cbentzel wrote:
> > > Should this DCHECK that callback is non-null?
> > 
> > Steve had the same suggestion, but it's an object, not a pointer. 
> 
> I meant DCHECK(!callback.is_null());
> 
> I try to use the lowercase null to indicate uninitialized callback, rather
than
> uninitialized pointer.
> 

Ah!  I see.  I added a check that callback is not null, and that
tunnel_auth_handled_callback_ is null.  Cool?

[vandebo (ex-Chrome), 2012-01-20 20:49:28]

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.h (right):

http://codereview.chromium.org/9148011/diff/28005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.h:51: void
ReallyRestartTunnelWithProxyAuth();
On 2012/01/19 23:11:19, Ryan Hamilton wrote:
> On 2012/01/19 20:12:49, vandebo wrote:
> > nit: These are usually named DoFoo
> 
> Done.  Though I thought DoFoo was typically reserved for the methods called
from
> DoLoop()?  (I also moved this to private)

In the network code, the DoLoop methods all start with Do, but more generally in
Chrome, there are On and Do methods when a level of indirection is needed

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket.cc (left):

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket.cc:264: transport_->socket()->Disconnect();
I think we still want to disconnect the socket, otherwise it could go back to
the pool and get reused. I don't remember if there's another check for keep
alive down stream, but it doesn't hurt to disconnect it here.

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket.cc:73: if (rv != OK || next_state_ ==
STATE_NONE)
This change isn't needed, rv will now be ERR_NO_KEEP_ALIVE_ON_AUTH_RESTART for
this case.

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket.cc:269: return next_state_ == STATE_NONE ?
ERR_NO_KEEP_ALIVE_ON_AUTH_RESTART : OK;
nit: a tristate isn't necessary.  int ret = OK at the top, reset in the
conditional, and return ret

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:323: if (result ==
ERR_NO_KEEP_ALIVE_ON_AUTH_RESTART) {
nit: put this inside the result != OK block?

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool_unittest.cc:199: // Don't add any auth,
just run the callback
Do you expect this to ever be run?  If not, a NOT_REACHED would be more
appropriate.

http://codereview.chromium.org/9148011/diff/49002/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/9148011/diff/49002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_pool_unittest.cc:153: // Don't add any auth, just
execute the callback.
Do you expect this to ever be run?  If not, a NOT_REACHED would be more
appropriate.

http://codereview.chromium.org/9148011/diff/49002/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket_unittest.cc (right):

http://codereview.chromium.org/9148011/diff/49002/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket_unittest.cc:443:
TEST_F(SpdyProxyClientSocketTest, ConnectWithAuthRequested) {
Is this test now redundant to ConenctWithAuthRestart?  It looks like a simple
subset.

http://codereview.chromium.org/9148011/diff/49002/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket_unittest.cc:518: 
Can you add a test that requests non-basic, non-digest auth to ensure that we
still get ERR_TUNNEL_CONNECTION_FAILED?

[Ryan Hamilton, 2012-01-20 21:51:34]

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket.cc (left):

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket.cc:264: transport_->socket()->Disconnect();
On 2012/01/20 20:49:29, vandebo wrote:
> I think we still want to disconnect the socket, otherwise it could go back to
> the pool and get reused. I don't remember if there's another check for keep
> alive down stream, but it doesn't hurt to disconnect it here.

Done.

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket.cc:73: if (rv != OK || next_state_ ==
STATE_NONE)
On 2012/01/20 20:49:29, vandebo wrote:
> This change isn't needed, rv will now be ERR_NO_KEEP_ALIVE_ON_AUTH_RESTART for
> this case.

Done.

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket.cc:73: if (rv != OK || next_state_ ==
STATE_NONE)
On 2012/01/20 20:49:29, vandebo wrote:
> This change isn't needed, rv will now be ERR_NO_KEEP_ALIVE_ON_AUTH_RESTART for
> this case.

Done.

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket.cc:269: return next_state_ == STATE_NONE ?
ERR_NO_KEEP_ALIVE_ON_AUTH_RESTART : OK;
On 2012/01/20 20:49:29, vandebo wrote:
> nit: a tristate isn't necessary.  int ret = OK at the top, reset in the
> conditional, and return ret 

Done.

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:323: if (result ==
ERR_NO_KEEP_ALIVE_ON_AUTH_RESTART) {
On 2012/01/20 20:49:29, vandebo wrote:
> nit: put this inside the result != OK block?

Done.

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/9148011/diff/49002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool_unittest.cc:199: // Don't add any auth,
just run the callback
On 2012/01/20 20:49:29, vandebo wrote:
> Do you expect this to ever be run?  If not, a NOT_REACHED would be more
> appropriate. 

Yes, it is actually used in the NeedAuth test.

http://codereview.chromium.org/9148011/diff/49002/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/9148011/diff/49002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_pool_unittest.cc:153: // Don't add any auth, just
execute the callback.
On 2012/01/20 20:49:29, vandebo wrote:
> Do you expect this to ever be run?  If not, a NOT_REACHED would be more
> appropriate. 

Yes, it is used in the NeedProxyAuth test.

http://codereview.chromium.org/9148011/diff/49002/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket_unittest.cc (right):

http://codereview.chromium.org/9148011/diff/49002/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket_unittest.cc:443:
TEST_F(SpdyProxyClientSocketTest, ConnectWithAuthRequested) {
On 2012/01/20 20:49:29, vandebo wrote:
> Is this test now redundant to ConenctWithAuthRestart?  It looks like a simple
> subset.

Done.

http://codereview.chromium.org/9148011/diff/49002/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket_unittest.cc:518: 
On 2012/01/20 20:49:29, vandebo wrote:
> Can you add a test that requests non-basic, non-digest auth to ensure that we
> still get ERR_TUNNEL_CONNECTION_FAILED?

Done.  Though now that I think about it, I wonder if it should return
ERR_PROXY_AUTH_UNSUPPORTED instead:


// The proxy requested authentication (for tunnel establishment) with an
// unsupported method.
NET_ERROR(PROXY_AUTH_UNSUPPORTED, -115)

[vandebo (ex-Chrome), 2012-01-20 21:58:20]

LGTM

http://codereview.chromium.org/9148011/diff/49002/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket_unittest.cc (right):

http://codereview.chromium.org/9148011/diff/49002/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket_unittest.cc:518: 
On 2012/01/20 21:51:35, Ryan Hamilton wrote:
> On 2012/01/20 20:49:29, vandebo wrote:
> > Can you add a test that requests non-basic, non-digest auth to ensure that
we
> > still get ERR_TUNNEL_CONNECTION_FAILED?
> 
> Done.  Though now that I think about it, I wonder if it should return
> ERR_PROXY_AUTH_UNSUPPORTED instead:
> 
> 
> // The proxy requested authentication (for tunnel establishment) with an
> // unsupported method.
> NET_ERROR(PROXY_AUTH_UNSUPPORTED, -115)

SGTM

http://codereview.chromium.org/9148011/diff/45008/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/9148011/diff/45008/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:325: next_state_ =
params_->transport_params() ?
You really love that tristate, huh?

http://codereview.chromium.org/9148011/diff/45008/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9148011/diff/45008/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:196: LOG(INFO) << __FUNCTION__;
extra

http://codereview.chromium.org/9148011/diff/45008/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:358: LOG(INFO) << __FUNCTION__;
extra

[Ryan Hamilton, 2012-01-23 18:37:27]

http://codereview.chromium.org/9148011/diff/49002/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket_unittest.cc (right):

http://codereview.chromium.org/9148011/diff/49002/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket_unittest.cc:518: 
On 2012/01/20 21:58:20, vandebo wrote:
> On 2012/01/20 21:51:35, Ryan Hamilton wrote:
> > On 2012/01/20 20:49:29, vandebo wrote:
> > > Can you add a test that requests non-basic, non-digest auth to ensure that
> we
> > > still get ERR_TUNNEL_CONNECTION_FAILED?
> > 
> > Done.  Though now that I think about it, I wonder if it should return
> > ERR_PROXY_AUTH_UNSUPPORTED instead:
> > 
> > 
> > // The proxy requested authentication (for tunnel establishment) with an
> > // unsupported method.
> > NET_ERROR(PROXY_AUTH_UNSUPPORTED, -115)
> 
> SGTM

Done.

http://codereview.chromium.org/9148011/diff/45008/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9148011/diff/45008/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:196: LOG(INFO) << __FUNCTION__;
On 2012/01/20 21:58:20, vandebo wrote:
> extra

Done.

http://codereview.chromium.org/9148011/diff/45008/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:358: LOG(INFO) << __FUNCTION__;
On 2012/01/20 21:58:20, vandebo wrote:
> extra

Done.

[commit-bot: I haz the power, 2012-01-24 21:00:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/9148011/63003

[commit-bot: I haz the power, 2012-01-24 21:33:24]

Try job failure for 9148011-63003 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-01-24 21:54:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/9148011/63009

[commit-bot: I haz the power, 2012-01-25 00:11:24]

Change committed as 118950