Add specific error codes for when SOCKS connect fails....

Add specific error codes for when SOCKS connect fails.

This also fixes HttpNetworkTransaction to fallback to the next proxy after a failure is encountered using a SOCKS proxy server.

Note that I decided against using finer granularity error codes, since it just adds a higher maintenance cost to ReconsiderProxyAfterError().
Power users can get better information on the cause of the failure by looking at the LoadLog anyway.

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

[wtc, 2010-02-03 19:29:28]

LGTM.  Thank you for removing three instances of ERR_FAILED!

http://codereview.chromium.org/567030/diff/1/7
File net/base/net_error_list.h (right):

http://codereview.chromium.org/567030/diff/1/7#newcode128
net/base/net_error_list.h:128: // Failed establishing a connection to SOCKS
proxy server for a target host.
Nit: add "the" or "a" before "SOCKS proxy server".

The comments for error codes are very important, so I'm extra
picky about their quality.

http://codereview.chromium.org/567030/diff/1/7#newcode129
net/base/net_error_list.h:129: NET_ERROR(SOCKS_CONNECT_FAILED, -120)
Nit: For consistency with existing error codes, I suggest
using "CONNECTION" instead of "CONNECT" in these two new
error codes.

http://codereview.chromium.org/567030/diff/1/7#newcode132
net/base/net_error_list.h:132: // because it is unreachable.
Nit: "it" => "the host"

http://codereview.chromium.org/567030/diff/1/2
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/567030/diff/1/2#newcode1591
net/http/http_network_transaction.cc:1591: // Remap the SOCKS-specific "host not
found" error code to a more
Is the error named "host not found" or "host unreachable"?
These are two different errors.

http://codereview.chromium.org/567030/diff/1/4
File net/socket/socks5_client_socket.cc (left):

http://codereview.chromium.org/567030/diff/1/4#oldcode284
net/socket/socks5_client_socket.cc:284: return ERR_INVALID_RESPONSE;
In hindsight, it's wrong to use ERR_INVALID_RESPONSE here
because ERR_INVALID_RESPONSE specifically means an invalid
*HTTP* response.

http://codereview.chromium.org/567030/diff/1/4
File net/socket/socks5_client_socket.cc (right):

http://codereview.chromium.org/567030/diff/1/4#newcode271
net/socket/socks5_client_socket.cc:271: load_log_, "Connection unexpected closed
while reading greeting.");
Nit: unexpected => unexpectedly

Should this log message mention "SOCKS"?  Or will the context
make it clear the log message is talking about SOCKS?

These two comments also apply to the log message at line
382 below.

http://codereview.chromium.org/567030/diff/1/6
File net/socket/socks_client_socket.cc (right):

http://codereview.chromium.org/567030/diff/1/6#newcode329
net/socket/socks_client_socket.cc:329: return ERR_SOCKS_CONNECT_FAILED;
ERR_UNEXPECTED seems appropriate here, because this is a
programming error.

[eroman, 2010-02-03 22:43:37]

Thanks for making this change better!

http://codereview.chromium.org/567030/diff/1/7
File net/base/net_error_list.h (right):

http://codereview.chromium.org/567030/diff/1/7#newcode128
net/base/net_error_list.h:128: // Failed establishing a connection to SOCKS
proxy server for a target host.
On 2010/02/03 19:29:28, wtc wrote:
> Nit: add "the" or "a" before "SOCKS proxy server".

Done.

> The comments for error codes are very important, so I'm extra
> picky about their quality.

Agreed, and I really value the improvements you make!

http://codereview.chromium.org/567030/diff/1/7#newcode129
net/base/net_error_list.h:129: NET_ERROR(SOCKS_CONNECT_FAILED, -120)
On 2010/02/03 19:29:28, wtc wrote:
> Nit: For consistency with existing error codes, I suggest
> using "CONNECTION" instead of "CONNECT" in these two new
> error codes.

I think CONNECT is a little more concise in this case. CONNECT refers to the
"connect" method of the SOCKS server (which binds a socket to remotehost), and
is where these failures occur. (A failure TCP connecting to the SOCKS server for
example would not result in ERR_SOCKS_CONNECT_FAILED for example, whereas a
failure doing the connect method would).

http://codereview.chromium.org/567030/diff/1/7#newcode132
net/base/net_error_list.h:132: // because it is unreachable.
On 2010/02/03 19:29:28, wtc wrote:
> Nit: "it" => "the host"

Done.

http://codereview.chromium.org/567030/diff/1/2
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/567030/diff/1/2#newcode1591
net/http/http_network_transaction.cc:1591: // Remap the SOCKS-specific "host not
found" error code to a more
On 2010/02/03 19:29:28, wtc wrote:
> Is the error named "host not found" or "host unreachable"?
> These are two different errors.

The specific error is named "host unreachable", but it could be caused by either
situation.

A SOCKS5 proxy server will do both the host resolution, and TCP connect
proxy-side. Moreover it uses a single error code for either failure. So we can't
be sure whether the "host unrechable" means the hostname couldn't be resolved,
or there was a failure in establishing TCP connection to it.

I have updated the comment to better reflect this, and explain why we remap the
error in the first place.

http://codereview.chromium.org/567030/diff/1/4
File net/socket/socks5_client_socket.cc (right):

http://codereview.chromium.org/567030/diff/1/4#newcode271
net/socket/socks5_client_socket.cc:271: load_log_, "Connection unexpected closed
while reading greeting.");
On 2010/02/03 19:29:28, wtc wrote:
> Nit: unexpected => unexpectedly
> 
> Should this log message mention "SOCKS"?  Or will the context
> make it clear the log message is talking about SOCKS?
> 
> These two comments also apply to the log message at line
> 382 below.

Since the display of the LoadLog is hierarchical, this message will show up in
the context of the SOCKS_CONNECT phase and should not be confusing.

http://codereview.chromium.org/567030/diff/1/6
File net/socket/socks_client_socket.cc (right):

http://codereview.chromium.org/567030/diff/1/6#newcode329
net/socket/socks_client_socket.cc:329: return ERR_SOCKS_CONNECT_FAILED;
On 2010/02/03 19:29:28, wtc wrote:
> ERR_UNEXPECTED seems appropriate here, because this is a
> programming error.

Done.

[wtc, 2010-02-04 02:51:48]

The reason I prefer CONNECTION is that we already have
error codes
  ERR_CONNECTION_FAILED
  ERR_TUNNEL_CONNECTION_FAILED
so it seems natural to also have
  ERR_SOCKS_CONNECTION_FAILED

In hindsignt, I would use CONNECT for a connection attempt
and CONNECTION for an established connection.

[eroman, 2010-02-05 00:56:48]

Ok, Done.