Remove the implicit fallback to DIRECT when proxies fail. This better matches...

Remove the implicit fallback to DIRECT when proxies fail. This better matches other browsers, and simplifies the code.

To better understand what this means, here are some examples how the behaviors will differ for the user:

(1) You start chrome with --proxy-server="foobar:80".
The server "foobar:80" is refusing connections.

  Before: Would fallback to direct after failing to connect through foobar:80.
  Now: Will error-out with connection refused after failing to connect through foobar:80.

(2) You start chrome with --proxy-pac-url="file:///foobar.pac".
The server "foobar:80" is unreachable, and foobar.pac reads:
  function FindProxyForURL(url, host) {
    return "PROXY foobar:80";
  }

  Before: Would fallback to direct after failing to connect through foobar:80.
  Now: Will error-out with connection refused after failing to connect through foobar:80.

(3) You start chrome with --proxy-pac-url="file:///foobar.pac".
The server "foobar:80" is unreachable, and foobar.pac reads:
  function FindProxyForURL(url, host) {
    return "PROXY foobar:80; DIRECT";
  }

  *No change, since the fallback to DIRECT is explicit in the PAC script*

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

[wtc, 2009-12-19 05:14:27]

http://codereview.chromium.org/502068/diff/2007/2016
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/502068/diff/2007/2016#newcode224
net/proxy/proxy_service_unittest.cc:224: EXPECT_EQ(ERR_FAILED, rv);
Can ReconsiderProxyAfterError return a better error code
than the default ERR_FAILED?

Does the connection error page display this error?

[eroman, 2009-12-19 05:59:15]

http://codereview.chromium.org/502068/diff/2007/2016
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/502068/diff/2007/2016#newcode224
net/proxy/proxy_service_unittest.cc:224: EXPECT_EQ(ERR_FAILED, rv);
On 2009/12/19 05:14:27, wtc wrote:
> Can ReconsiderProxyAfterError return a better error code
> than the default ERR_FAILED?
> 
> Does the connection error page display this error?

No, that is not the error that gets displayed on the page (it is just a code to
indicate to the transaction that there are no more proxies left to fallback to).

Please see the TODO in ProxyService::ReconsiderProxyAfterError():

+ // Return synchronous failure if there is nothing left to fall-back to.
+ // TODO(eroman): This is a yucky API, clean it up.
+ return did_fallback ? OK : ERR_FAILED;

[eroman, 2010-01-04 18:35:16]

ping

[darin (slow to review), 2010-01-05 00:22:06]

LGTM, except for:

http://codereview.chromium.org/502068/diff/2007/2009
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/502068/diff/2007/2009#newcode558
net/http/http_network_transaction.cc:558: return ERR_FAILED;
probably worth it to create the new error code now.  it can be frustrating to
track down bug reports for ERR_FAILED.

http://codereview.chromium.org/502068/diff/2007/2018
File net/proxy/proxy_list.cc (right):

http://codereview.chromium.org/502068/diff/2007/2018#newcode106
net/proxy/proxy_list.cc:106: // TODO(eroman): It would be good if instead of
removing failed proxies
great idea!

[eroman, 2010-01-05 01:55:33]

http://codereview.chromium.org/502068/diff/2007/2009
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/502068/diff/2007/2009#newcode558
net/http/http_network_transaction.cc:558: return ERR_FAILED;
On 2010/01/05 00:22:06, darin wrote:
> probably worth it to create the new error code now.  it can be frustrating to
> track down bug reports for ERR_FAILED.

Done. ERR_EMPTY_PROXY_LIST

[wtc, 2010-01-07 20:12:26]

Eric,

Sorry I didn't review this CL in time.  I did a quick and
incomplete review.  Here are some comments.

http://codereview.chromium.org/502068/diff/9006/4036
File net/base/net_error_list.h (right):

http://codereview.chromium.org/502068/diff/9006/4036#newcode291
net/base/net_error_list.h:291: // There are no supported proxies in the provided
list.
Nit: "list" => "proxy list"

Should this error be named "NO_SUPPORTED_PROXIES" or
"NO_WORKING_PROXIES"?  "EMPTY_PROXY_LIST" implies that
the user specifies an empty proxy list.

http://codereview.chromium.org/502068/diff/9006/4026
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/502068/diff/9006/4026#newcode579
net/http/http_network_transaction.cc:579: 
Nit: remove this blank line.

http://codereview.chromium.org/502068/diff/9006/4027
File net/proxy/proxy_info.h (right):

http://codereview.chromium.org/502068/diff/9006/4027#newcode47
net/proxy/proxy_info.h:47: // We don't implicitly fallback to DIRECT unless it
was added to the list.
Why don't you change ProxyList::Get() to do the right thing?
Is it because that's too much work (I think it requires
changing ProxyList::Get() to return a pointer so that it
can report failure)?

http://codereview.chromium.org/502068/diff/9006/4027#newcode84
net/proxy/proxy_info.h:84: // The ordered list of proxy servers (including
DIRECT attempts) remaining to
Nit: the word "attempts" is confusing.  You can just remove it.

http://codereview.chromium.org/502068/diff/9006/4028
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/502068/diff/9006/4028#newcode458
net/proxy/proxy_service.cc:458: return did_fallback ? OK : ERR_FAILED;
I wonder if this ERR_FAILED error code may leak through
the call chain and end up in an error page.

[wtc, 2010-01-07 20:25:37]

http://codereview.chromium.org/502068/diff/9006/4026
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/502068/diff/9006/4026#newcode1594
net/http/http_network_transaction.cc:1594: // If ReconsiderProxyAfterError()
failed synchronously, it means
Nit: ReconsiderProxyAfterError() =>
session_->proxy_service()->ReconsiderProxyAfterError() or
ProxyService::ReconsiderProxyAfterError() because the current
function is also named ReconsiderProxyAfterError().

http://codereview.chromium.org/502068/diff/9006/4026#newcode1596
net/http/http_network_transaction.cc:1596: // with the last connection error we
got.
Nit: "last" => "original".

There is another reason we must report the original
connection error: session_->proxy_service()->ReconsiderProxyAfterError()
returns ERR_FAILED on failure.  If we return ERR_FAILED,
it would be bad (loss of error information).

With your comment (and perhaps my comment above), this isn't
that confusing.  So I don't know what you meant by
"confusing contract".

[eroman, 2010-01-07 21:08:19]

http://codereview.chromium.org/502068/diff/9006/4027
File net/proxy/proxy_info.h (right):

http://codereview.chromium.org/502068/diff/9006/4027#newcode47
net/proxy/proxy_info.h:47: // We don't implicitly fallback to DIRECT unless it
was added to the list.
On 2010/01/07 20:12:27, wtc wrote:
> Why don't you change ProxyList::Get() to do the right thing?
> Is it because that's too much work (I think it requires
> changing ProxyList::Get() to return a pointer so that it
> can report failure)?

I don't understand what you mean by this.

http://codereview.chromium.org/502068/diff/9006/4028
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/502068/diff/9006/4028#newcode458
net/proxy/proxy_service.cc:458: return did_fallback ? OK : ERR_FAILED;
On 2010/01/07 20:12:27, wtc wrote:
> I wonder if this ERR_FAILED error code may leak through
> the call chain and end up in an error page.

This shouldn't leak since HttpNetworkTransaction checks for synchronous failure.
But as the TODO says, the API of the method should be changed so this is an
explicit result of the call.

[wtc, 2010-01-07 21:22:08]

http://codereview.chromium.org/502068/diff/9006/4027
File net/proxy/proxy_info.h (right):

http://codereview.chromium.org/502068/diff/9006/4027#newcode47
net/proxy/proxy_info.h:47: // We don't implicitly fallback to DIRECT unless it
was added to the list.
On 2010/01/07 21:08:19, eroman wrote:
>
> I don't understand what you mean by this.

Right now, you're asking the caller to call ProxyList::Get()
only if ProxyList is not empty.  Instead, I think we can
change ProxyList::Get() to return a pointer:
  const ProxyServer* Get() const;

Then, ProxyList::Get() can return NULL if ProxyList is
empty.  This way the caller can call ProxyList::Get()
without having to check if ProxyList is empty first.
This seems like a better fix to me, although I think
it's more intrusive.

http://codereview.chromium.org/502068/diff/9006/4028
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/502068/diff/9006/4028#newcode458
net/proxy/proxy_service.cc:458: return did_fallback ? OK : ERR_FAILED;
On 2010/01/07 21:08:19, eroman wrote:
>
> This shouldn't leak since HttpNetworkTransaction checks for synchronous
failure.
> But as the TODO says, the API of the method should be changed so this is an
> explicit result of the call.

I see.  Now I think an async failure (with ERR_FAILED
result code) will leak into HttpNetworkTransaction,
because HttpNetworkTransaction does not save the original
connection error code in a member variable.  We should
file a bug about this.

[wtc, 2010-01-07 21:53:56]

http://codereview.chromium.org/502068/diff/9006/4028
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/502068/diff/9006/4028#newcode458
net/proxy/proxy_service.cc:458: return did_fallback ? OK : ERR_FAILED;
On 2010/01/07 21:22:09, wtc wrote:
>
> Now I think an async failure (with ERR_FAILED
> result code) will leak into HttpNetworkTransaction,
> because HttpNetworkTransaction does not save the original
> connection error code in a member variable.  We should
> file a bug about this.

eroman went though the async failure case with me, and
showed me that HttpNetworkTransaction::DoProxyResolveComplete
will fall back to DIRECT rather than failing with the
async failure result code of ProxyResolver.