net: also do TLS 1.1 -> 1.0 fallback on ERR_CONNECTION_ABORTED.

net: also do TLS 1.1 -> 1.0 fallback on ERR_CONNECTION_ABORTED.

We currently perform TLS 1.1 -> 1.0 fallback for ERR_CONNECTION_RESET
to workaround some buggy servers. This change causes ERR_CONNECTION_ABORTED
to be treated like ERR_CONNECTION_RESET because, with non-blocking I/O,
it appears that we get this error rather than ERR_CONNECTION_RESET.
See r165170 and the bug.

See https://code.google.com/p/chromium/issues/detail?id=178672#c17

BUG=178672, 179037
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186218

[agl, 2013-03-04 15:53:48]

I would be happy to have a reason not to land this :(

[wtc, 2013-03-04 20:55:49]

LGTM.

Have we confirmed that Chrome gets ERR_CONNECTION_RESET
on non-Windows platforms in the same environment?

Looking at the net-internals log in
https://code.google.com/p/chromium/issues/detail?id=178672#c7,
I did not recognize this pattern. So this incompatibility
either is new or was hidden by our broad fallback on
ERR_CONNECTION_RESET before. (I only remember seeing
ERR_CONNECTION_RESET very early in the handshake,
without reading so many bytes of handshake messages
from the server.) So it may be a good idea to understand
this better.

https://codereview.chromium.org/12390059/diff/2001/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/12390059/diff/2001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:1918: prerr == PR_CONNECT_ABORTED_ERROR) &&

Perhaps only allow PR_CONNECT_ABORTED_ERROR for OS_WIN?
It would require some ugly #ifdef. Not sure if that's worth
it.

[agl, 2013-03-04 21:01:13]

> So it may be a good idea to understand this better.

I've not been able to reproduce (even on Windows), but the net-internals logs
clearly show it happening somehow. I don't know what else can be done unless we
can get a reproduction somewhere.

https://codereview.chromium.org/12390059/diff/2001/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/12390059/diff/2001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:1918: prerr == PR_CONNECT_ABORTED_ERROR) &&
On 2013/03/04 20:55:49, wtc wrote:
> Perhaps only allow PR_CONNECT_ABORTED_ERROR for OS_WIN?
> It would require some ugly #ifdef. Not sure if that's worth
> it.

I would tend not to have different behaviours on different platforms unless it's
clearly warranted, and I'm not sure that it is here. Maybe we have a subtle
issue on all platforms that's only just showing through on some Windows boxes.

[wtc, 2013-03-04 23:51:27]

Do you know whether the ERR_CONNECTION_ABORTED comes from
the destination server or something in the middle?

https://codereview.chromium.org/12390059/diff/2001/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/12390059/diff/2001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:1916: // RESET on Windows. See
crbug.com/178672.

I found one change to tcp_client_socket_win.cc in Chrome 25
that could explain the ERR_CONNECTION_RESET to
ERR_CONNECTION_ABORTED change. rsleevi changed how
SSLClientSocket reads from the TCPClientSocket, but his change
(r174136) is cross-platform and is in Chrome 26 only
(Chrome 25 branched at revision 173683). So I think
pmeenan's change to tcp_client_socket_win.cc (r165170)
is the most likely cause for the error code change on
Windows.

[wtc, 2013-03-05 18:27:10]

Patch set 2 LGTM. Please go ahead and commit this.

I answered my own question. In my debugging of bug 178672,
the TCP reset comes from the destination server.

https://codereview.chromium.org/12390059/diff/2001/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/12390059/diff/2001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:1916: // RESET on Windows. See
crbug.com/178672.

We can replace "sometimes" with something like "if non-blocking
reads instead of async/overlapped reads are used"

[agl, 2013-03-05 18:48:20]

Have updated comment in the code and the commit message.

https://codereview.chromium.org/12390059/diff/2001/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/12390059/diff/2001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:1916: // RESET on Windows. See
crbug.com/178672.
On 2013/03/05 18:27:10, wtc wrote:
> 
> We can replace "sometimes" with something like "if non-blocking
> reads instead of async/overlapped reads are used"

Done.

[commit-bot: I haz the power, 2013-03-05 18:51:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/12390059/15001

[commit-bot: I haz the power, 2013-03-05 19:01:08]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[agl, 2013-03-05 19:07:15]

Committed patchset #3 manually as r186218 (presubmit successful).