Receiving Connection: Proxy-Bypass induces proxy fallback.

Receiving Connection: Proxy-Bypass induces proxy fallback.

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

[Michael Piatek, 2012-09-26 00:14:22]

First-cut at fallback via header. Thanks for the tip, Eric, interposing in
HttpNetworkTransaction is pretty straightforward.

Although I've tried this manually, I haven't written tests yet -- I'd like to
reach consensus on the approach beforehand since I'm adding some non-const
accessors.

Feedback appreciated. Thanks!

[eroman, 2012-09-26 01:36:20]

https://chromiumcodereview.appspot.com/10987043/diff/1/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://chromiumcodereview.appspot.com/10987043/diff/1/net/http/http_network_...
net/http/http_network_transaction.cc:871: // TODO(piatek): const_cast, or
non-const accessor?
Internalize the mutation of the proxy retry map into a method of ProxyService
(moreover don't mutate proxy_info_).

https://chromiumcodereview.appspot.com/10987043/diff/1/net/http/http_network_...
net/http/http_network_transaction.cc:875: ResetConnectionAndRequestForResend();
I thought your strategy was going to be to have the proxy return a redirect to
the same URL?

With this approach, you need to consider:
  - Protections against entering an infinite resend-loop
  - What to do with POSTs

It seems the assumption is that the proxy list is terminated by a DIRECT option.
If it isn't, then I suspect this will result in continually resending the
request to the same proxy.

[Michael Piatek, 2012-09-26 20:13:53]

https://chromiumcodereview.appspot.com/10987043/diff/1/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://chromiumcodereview.appspot.com/10987043/diff/1/net/http/http_network_...
net/http/http_network_transaction.cc:871: // TODO(piatek): const_cast, or
non-const accessor?
On 2012/09/26 01:36:20, eroman wrote:
> Internalize the mutation of the proxy retry map into a method of ProxyService
> (moreover don't mutate proxy_info_).

Done. (Although, I think proxy_info_'s list needs to be modified to remove the
bad proxy, otherwise this seems to loop forever trying the same proxy, even if
added to the service's ProxyRetryInfoMap.)

https://chromiumcodereview.appspot.com/10987043/diff/1/net/http/http_network_...
net/http/http_network_transaction.cc:875: ResetConnectionAndRequestForResend();
On 2012/09/26 01:36:20, eroman wrote:
> I thought your strategy was going to be to have the proxy return a redirect to
> the same URL?
> 
> With this approach, you need to consider:
>   - Protections against entering an infinite resend-loop
>   - What to do with POSTs
> 
> It seems the assumption is that the proxy list is terminated by a DIRECT
option.
> If it isn't, then I suspect this will result in continually resending the
> request to the same proxy.

If there's no DIRECT option, Fallback() returns false and the response provided
by the proxy is displayed. (This seems preferable to a self-redirect, since the
proxy can provide an error page for browsers that do not support fallback
without inducing a redirect loop.)

Good point RE: POSTs, it seems clear that we shouldn't retry in that case, since
there's no guarantee about the proxy having taken no action. Anything else you
were thinking of along those lines?

[eroman, 2012-09-26 22:01:57]

https://chromiumcodereview.appspot.com/10987043/diff/1/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://chromiumcodereview.appspot.com/10987043/diff/1/net/http/http_network_...
net/http/http_network_transaction.cc:865: // Server-induced fallback is
supported only if this is a PAC configured
Can you additionally link to the document you wrote, so that others reading this
will have some context (since proxy-bypass is not a standard thing).

https://chromiumcodereview.appspot.com/10987043/diff/1/net/http/http_network_...
net/http/http_network_transaction.cc:871: // TODO(piatek): const_cast, or
non-const accessor?
On 2012/09/26 20:13:53, Michael Piatek wrote:
> On 2012/09/26 01:36:20, eroman wrote:
> > Internalize the mutation of the proxy retry map into a method of
ProxyService
> > (moreover don't mutate proxy_info_).
> 
> Done. (Although, I think proxy_info_'s list needs to be modified to remove the
> bad proxy, otherwise this seems to loop forever trying the same proxy, even if
> added to the service's ProxyRetryInfoMap.)

The reason I don't want proxy_info_ to be modified here, is because it
represents the information for the current response. If we modify it here, that
is no longer true for this window of time (until request is restarted), making
it easier to introduce subtle bugs in the future.

My understanding is that when you call ResetConnectionAndRequestForResend() it
will re-create the stream, which will in turn re-fetch a proxy_info_. So I don't
believe the value needs to be removed. Are you seeing something different
happen?
 modified here, then it introduces the possibility for

https://chromiumcodereview.appspot.com/10987043/diff/1/net/http/http_network_...
net/http/http_network_transaction.cc:875: ResetConnectionAndRequestForResend();
On 2012/09/26 20:13:53, Michael Piatek wrote:
> On 2012/09/26 01:36:20, eroman wrote:
> > I thought your strategy was going to be to have the proxy return a redirect
to
> > the same URL?
> > 
> > With this approach, you need to consider:
> >   - Protections against entering an infinite resend-loop
> >   - What to do with POSTs
> > 
> > It seems the assumption is that the proxy list is terminated by a DIRECT
> option.
> > If it isn't, then I suspect this will result in continually resending the
> > request to the same proxy.
> 
> If there's no DIRECT option, Fallback() returns false and the response
provided
> by the proxy is displayed. (This seems preferable to a self-redirect, since
the
> proxy can provide an error page for browsers that do not support fallback
> without inducing a redirect loop.)
> 
> Good point RE: POSTs, it seems clear that we shouldn't retry in that case,
since
> there's no guarantee about the proxy having taken no action. Anything else you
> were thinking of along those lines?

Ah good point, that resend loop won't be possible then. Be sure to cover this in
the unit-tests later :)

https://chromiumcodereview.appspot.com/10987043/diff/5001/net/proxy/proxy_info.h
File net/proxy/proxy_info.h (right):

https://chromiumcodereview.appspot.com/10987043/diff/5001/net/proxy/proxy_inf...
net/proxy/proxy_info.h:115: bool Fallback(ProxyRetryInfoMap& proxy_retry_info,
Our style is to use pointers for mutable state -- please use a pointer here.

[Michael Piatek, 2012-09-27 00:30:24]

https://chromiumcodereview.appspot.com/10987043/diff/1/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://chromiumcodereview.appspot.com/10987043/diff/1/net/http/http_network_...
net/http/http_network_transaction.cc:865: // Server-induced fallback is
supported only if this is a PAC configured
On 2012/09/26 22:01:58, eroman wrote:
> Can you additionally link to the document you wrote, so that others reading
this
> will have some context (since proxy-bypass is not a standard thing).

Added a link to the bug, which has additional context and discussion.

https://chromiumcodereview.appspot.com/10987043/diff/1/net/http/http_network_...
net/http/http_network_transaction.cc:871: // TODO(piatek): const_cast, or
non-const accessor?
On 2012/09/26 22:01:58, eroman wrote:

> The reason I don't want proxy_info_ to be modified here, is because it
> represents the information for the current response. If we modify it here,
that
> is no longer true for this window of time (until request is restarted), making
> it easier to introduce subtle bugs in the future.
> 
> My understanding is that when you call ResetConnectionAndRequestForResend() it
> will re-create the stream, which will in turn re-fetch a proxy_info_. So I
don't
> believe the value needs to be removed. Are you seeing something different
> happen?

Done. (You're right -- I'm not sure what I was doing before, but factoring out a
const helper to only update the bad proxies map seems to work fine. Thanks!)

https://chromiumcodereview.appspot.com/10987043/diff/1/net/http/http_network_...
net/http/http_network_transaction.cc:875: ResetConnectionAndRequestForResend();
On 2012/09/26 22:01:58, eroman wrote:

> Ah good point, that resend loop won't be possible then. Be sure to cover this
in
> the unit-tests later :)

will do

https://chromiumcodereview.appspot.com/10987043/diff/5001/net/proxy/proxy_info.h
File net/proxy/proxy_info.h (right):

https://chromiumcodereview.appspot.com/10987043/diff/5001/net/proxy/proxy_inf...
net/proxy/proxy_info.h:115: bool Fallback(ProxyRetryInfoMap& proxy_retry_info,
On 2012/09/26 22:01:58, eroman wrote:
> Our style is to use pointers for mutable state -- please use a pointer here.

Done.

[eroman, 2012-09-28 23:19:15]

OK, i think this is ready for some unittests now (mostly these will be at the
the http_network_transaction layer).

The changes to http_network_transaction.cc look fine to me. But it has been a
while since I hacked on that file, so I suggest having Chris take a look as
well.

https://chromiumcodereview.appspot.com/10987043/diff/10001/net/proxy/proxy_se...
File net/proxy/proxy_service.cc (right):

https://chromiumcodereview.appspot.com/10987043/diff/10001/net/proxy/proxy_se...
net/proxy/proxy_service.cc:1180: return result.proxy_list_.size() > 1;
According to the documentation in header, I expect this to return true when
proxy_list_.size() >= 1 (which is the return value from
UpdateRetryInfoOnFallback).

Can you explain why this needs to be strictly greater than 1?

https://chromiumcodereview.appspot.com/10987043/diff/10001/net/proxy/proxy_se...
File net/proxy/proxy_service.h (right):

https://chromiumcodereview.appspot.com/10987043/diff/10001/net/proxy/proxy_se...
net/proxy/proxy_service.h:152: bool Fallback(const ProxyInfo& results, const
BoundNetLog& net_log);
I wonder if a name like "MarkProxyAsBad()" wouldn't be more descriptive.
(Fallback seems to imply that |results| will be  mutated).

[Michael Piatek, 2012-10-01 23:23:11]

+tests

https://chromiumcodereview.appspot.com/10987043/diff/10001/net/proxy/proxy_se...
File net/proxy/proxy_service.cc (right):

https://chromiumcodereview.appspot.com/10987043/diff/10001/net/proxy/proxy_se...
net/proxy/proxy_service.cc:1180: return result.proxy_list_.size() > 1;
On 2012/09/28 23:19:15, eroman wrote:
> According to the documentation in header, I expect this to return true when
> proxy_list_.size() >= 1 (which is the return value from
> UpdateRetryInfoOnFallback).
> 
> Can you explain why this needs to be strictly greater than 1?

Done.

https://chromiumcodereview.appspot.com/10987043/diff/10001/net/proxy/proxy_se...
File net/proxy/proxy_service.h (right):

https://chromiumcodereview.appspot.com/10987043/diff/10001/net/proxy/proxy_se...
net/proxy/proxy_service.h:152: bool Fallback(const ProxyInfo& results, const
BoundNetLog& net_log);
On 2012/09/28 23:19:15, eroman wrote:
> I wonder if a name like "MarkProxyAsBad()" wouldn't be more descriptive.
> (Fallback seems to imply that |results| will be  mutated).

Done.

[eroman, 2012-10-08 22:29:25]

Slow to respond, as was OOO last week.

https://chromiumcodereview.appspot.com/10987043/diff/14001/net/http/http_netw...
File net/http/http_network_layer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10987043/diff/14001/net/http/http_netw...
net/http/http_network_layer_unittest.cc:188: EXPECT_EQ("bad:8080",
(*proxy_service_->proxy_retry_info().begin()).first);
Please also verify the headers -- check at least one header value, which is
different in the proxy-bypass response from the second one.

https://chromiumcodereview.appspot.com/10987043/diff/14001/net/proxy/proxy_se...
File net/proxy/proxy_service.cc (right):

https://chromiumcodereview.appspot.com/10987043/diff/14001/net/proxy/proxy_se...
net/proxy/proxy_service.cc:1184: return result.proxy_list_.size() > 1;
I don't believe this is good enough to prevent loops.

Note that the "ProxyRetryInfoMap" is not used to remove proxies from being tried
-- it is only used to "deprioritize" them. In other words, the proxy list will
be re-ordered, and so-called "bad proxies" may still be tried during fallback.

So for instance, if the proxy list was:

"foobar:80; foobar80: foobar80:", then I expect MarkProxyAsBad() would return
true every time, and we would be stuck in a resend loop if the proxy returns
proxy-bypass.

[Michael Piatek, 2012-10-09 21:05:08]

https://chromiumcodereview.appspot.com/10987043/diff/14001/net/http/http_netw...
File net/http/http_network_layer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10987043/diff/14001/net/http/http_netw...
net/http/http_network_layer_unittest.cc:188: EXPECT_EQ("bad:8080",
(*proxy_service_->proxy_retry_info().begin()).first);
On 2012/10/08 22:29:25, eroman wrote:
> Please also verify the headers -- check at least one header value, which is
> different in the proxy-bypass response from the second one.

Done.

https://chromiumcodereview.appspot.com/10987043/diff/14001/net/proxy/proxy_se...
File net/proxy/proxy_service.cc (right):

https://chromiumcodereview.appspot.com/10987043/diff/14001/net/proxy/proxy_se...
net/proxy/proxy_service.cc:1184: return result.proxy_list_.size() > 1;
On 2012/10/08 22:29:25, eroman wrote:
> I don't believe this is good enough to prevent loops.
> 
> Note that the "ProxyRetryInfoMap" is not used to remove proxies from being
tried
> -- it is only used to "deprioritize" them. In other words, the proxy list will
> be re-ordered, and so-called "bad proxies" may still be tried during fallback.
> 
> So for instance, if the proxy list was:
> 
> "foobar:80; foobar80: foobar80:", then I expect MarkProxyAsBad() would return
> true every time, and we would be stuck in a resend loop if the proxy returns
> proxy-bypass.

Done.

[eroman, 2012-10-12 18:30:58]

lgtm after addressing these comments

http://codereview.chromium.org/10987043/diff/23001/net/http/http_network_laye...
File net/http/http_network_layer_unittest.cc (right):

http://codereview.chromium.org/10987043/diff/23001/net/http/http_network_laye...
net/http/http_network_layer_unittest.cc:213: data_writes,
arraysize(data_writes));
nit: indent by 1 more

http://codereview.chromium.org/10987043/diff/23001/net/http/http_network_laye...
net/http/http_network_layer_unittest.cc:215: data_writes,
arraysize(data_writes));
same

http://codereview.chromium.org/10987043/diff/23001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

http://codereview.chromium.org/10987043/diff/23001/net/proxy/proxy_list.cc#ne...
net/proxy/proxy_list.cc:85: return true;
Note |proxies_| can also contain entries for "DIRECT" (as it represents a PAC
fallback list).

So it is possible for this function to return true due to hitting a DIRECT
pseudo-proxy.

This isn't "wrong", but it is subtle enough that I would like you to include
tests for it (both in http_network_transaction_unittest, and in
proxy_list_unittest).

The case I am thinking of, is when connecting through DIRECT also returns a
proxy-bypass. I believe your code will work as-is, since
http_network_transaction is explicitly testing for "was_fetched_via_proxy" so it
shouldn't loop.

http://codereview.chromium.org/10987043/diff/23001/net/proxy/proxy_list_unitt...
File net/proxy/proxy_list_unittest.cc (right):

http://codereview.chromium.org/10987043/diff/23001/net/proxy/proxy_list_unitt...
net/proxy/proxy_list_unittest.cc:110: list.SetFromPacString("PROXY bad1:80;
PROXY bad2:80");
Also consider adding a test with PAC string that contains "DIRECT".

[Michael Piatek, 2012-10-15 17:10:16]

https://chromiumcodereview.appspot.com/10987043/diff/23001/net/http/http_netw...
File net/http/http_network_layer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10987043/diff/23001/net/http/http_netw...
net/http/http_network_layer_unittest.cc:213: data_writes,
arraysize(data_writes));
On 2012/10/12 18:30:58, eroman wrote:
> nit: indent by 1 more

Done.

https://chromiumcodereview.appspot.com/10987043/diff/23001/net/http/http_netw...
net/http/http_network_layer_unittest.cc:215: data_writes,
arraysize(data_writes));
On 2012/10/12 18:30:58, eroman wrote:
> same

Done.

https://chromiumcodereview.appspot.com/10987043/diff/23001/net/proxy/proxy_li...
File net/proxy/proxy_list.cc (right):

https://chromiumcodereview.appspot.com/10987043/diff/23001/net/proxy/proxy_li...
net/proxy/proxy_list.cc:85: return true;
On 2012/10/12 18:30:58, eroman wrote:
> Note |proxies_| can also contain entries for "DIRECT" (as it represents a PAC
> fallback list).
> 
> So it is possible for this function to return true due to hitting a DIRECT
> pseudo-proxy.
> 
> This isn't "wrong", but it is subtle enough that I would like you to include
> tests for it (both in http_network_transaction_unittest, and in
> proxy_list_unittest).
> 
> The case I am thinking of, is when connecting through DIRECT also returns a
> proxy-bypass. I believe your code will work as-is, since
> http_network_transaction is explicitly testing for "was_fetched_via_proxy" so
it
> shouldn't loop.

Done.

https://chromiumcodereview.appspot.com/10987043/diff/23001/net/proxy/proxy_li...
File net/proxy/proxy_list_unittest.cc (right):

https://chromiumcodereview.appspot.com/10987043/diff/23001/net/proxy/proxy_li...
net/proxy/proxy_list_unittest.cc:110: list.SetFromPacString("PROXY bad1:80;
PROXY bad2:80");
On 2012/10/12 18:30:58, eroman wrote:
> Also consider adding a test with PAC string that contains "DIRECT".

Done.

[commit-bot: I haz the power, 2012-10-16 17:33:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piatek@google.com/10987043/46001

[commit-bot: I haz the power, 2012-10-16 19:51:33]

Change committed as 162218