[Merge] [Private Network Access] Avoid notifying about closed socket.
This can happen due to the following race in the network code:
1. Socket is re-used from socket pools, `SocketPosix::IsConnected()`
returns true.
2. Remote side closes the socket.
3. `HttpStream::GetRemoteEndpoint()` fails during
`HttpNetworkTransaction::DoConnectedCallback()` because the socket is
not connected anymore
The race caused flakiness when establishing connections to non-public
servers that do not serve `Connection: close` headers yet support only a
single request per socket. This is the case for some web platform tests.
Tested this manually by running the flaky test 5000 times with no
flakes. Previously it would fail once every couple hundred times fairly
reliably.
(cherry picked from commit 5cf08f1bfdc34ca9b68520d12f7dd61177384dd3)
Fixed: chromium:1322072
Change-Id: I8cdbf52d04cbec3f52a1d3e0a8a7779bd4346605
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3652198
Reviewed-by: David Benjamin <davidben@chromium.org>
Auto-Submit: Titouan Rigoudy <titouan@chromium.org>
Commit-Queue: Titouan Rigoudy <titouan@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1005811}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3692108
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/branch-heads/5060@{#659}
Cr-Branched-From: b83393d0f4038aeaf67f970a024d8101df7348d1-refs/heads/main@{#1002911}
diff --git a/net/http/http_basic_stream.cc b/net/http/http_basic_stream.cc
index d82e153..1dcf2b0 100644
--- a/net/http/http_basic_stream.cc
+++ b/net/http/http_basic_stream.cc
@@ -176,11 +176,11 @@
parser()->GetSSLCertRequestInfo(cert_request_info);
}
-bool HttpBasicStream::GetRemoteEndpoint(IPEndPoint* endpoint) {
+int HttpBasicStream::GetRemoteEndpoint(IPEndPoint* endpoint) {
if (!state_.connection() || !state_.connection()->socket())
- return false;
+ return ERR_SOCKET_NOT_CONNECTED;
- return state_.connection()->socket()->GetPeerAddress(endpoint) == OK;
+ return state_.connection()->socket()->GetPeerAddress(endpoint);
}
void HttpBasicStream::Drain(HttpNetworkSession* session) {
diff --git a/net/http/http_basic_stream.h b/net/http/http_basic_stream.h
index 8c765a8..2d1c561 100644
--- a/net/http/http_basic_stream.h
+++ b/net/http/http_basic_stream.h
@@ -87,7 +87,7 @@
void GetSSLCertRequestInfo(SSLCertRequestInfo* cert_request_info) override;
- bool GetRemoteEndpoint(IPEndPoint* endpoint) override;
+ int GetRemoteEndpoint(IPEndPoint* endpoint) override;
void Drain(HttpNetworkSession* session) override;
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 977b28c..a28530c 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -908,9 +908,18 @@
// HttpStream::GetAcceptChViaAlps() needs the HttpRequestInfo to retrieve
// the ACCEPT_CH frame payload.
stream_->RegisterRequest(request_);
- stream_->GetRemoteEndpoint(&remote_endpoint_);
next_state_ = STATE_CONNECTED_CALLBACK_COMPLETE;
+ int result = stream_->GetRemoteEndpoint(&remote_endpoint_);
+ if (result != OK) {
+ // `GetRemoteEndpoint()` fails when the underlying socket is not connected
+ // anymore, even though the peer's address is known. This can happen when
+ // we picked a socket from socket pools while it was still connected, but
+ // the remote side closes it before we get a chance to send our request.
+ // See if we should retry the request based on the error code we got.
+ return HandleIOError(result);
+ }
+
if (connected_callback_.is_null()) {
return OK;
}
@@ -1581,9 +1590,10 @@
}
// This method determines whether it is safe to resend the request after an
-// IO error. It can only be called in response to request header or body
-// write errors or response header read errors. It should not be used in
-// other cases, such as a Connect error.
+// IO error. It should only be called in response to errors received before
+// final set of response headers have been successfully parsed, that the
+// transaction may need to be retried on.
+// It should not be used in other cases, such as a Connect error.
int HttpNetworkTransaction::HandleIOError(int error) {
// Because the peer may request renegotiation with client authentication at
// any time, check and handle client authentication errors.
@@ -1730,9 +1740,7 @@
// NOTE: we resend a request only if we reused a keep-alive connection.
// This automatically prevents an infinite resend loop because we'll run
// out of the cached keep-alive connections eventually.
- if (connection_is_proven && !has_received_headers)
- return true;
- return false;
+ return connection_is_proven && !has_received_headers;
}
bool HttpNetworkTransaction::HasExceededMaxRetries() const {
diff --git a/net/http/http_response_body_drainer_unittest.cc b/net/http/http_response_body_drainer_unittest.cc
index ed70a0d..a8ffe04 100644
--- a/net/http/http_response_body_drainer_unittest.cc
+++ b/net/http/http_response_body_drainer_unittest.cc
@@ -125,7 +125,9 @@
}
void GetSSLInfo(SSLInfo* ssl_info) override {}
void GetSSLCertRequestInfo(SSLCertRequestInfo* cert_request_info) override {}
- bool GetRemoteEndpoint(IPEndPoint* endpoint) override { return false; }
+ int GetRemoteEndpoint(IPEndPoint* endpoint) override {
+ return ERR_UNEXPECTED;
+ }
// Mocked API
int ReadResponseBody(IOBuffer* buf,
diff --git a/net/http/http_stream.h b/net/http/http_stream.h
index 04d1507f..80712a6 100644
--- a/net/http/http_stream.h
+++ b/net/http/http_stream.h
@@ -171,9 +171,9 @@
virtual void GetSSLCertRequestInfo(SSLCertRequestInfo* cert_request_info) = 0;
// Gets the remote endpoint of the socket that the HTTP stream is using, if
- // any. Returns true and fills in |endpoint| if it is available; returns false
- // and does not modify |endpoint| if it is unavailable.
- virtual bool GetRemoteEndpoint(IPEndPoint* endpoint) = 0;
+ // any. Returns OK and fills in |endpoint| if it is available; returns an
+ // error and does not modify |endpoint| otherwise.
+ virtual int GetRemoteEndpoint(IPEndPoint* endpoint) = 0;
// In the case of an HTTP error or redirect, flush the response body (usually
// a simple error or "this page has moved") so that we can re-use the
diff --git a/net/http/http_stream_factory_unittest.cc b/net/http/http_stream_factory_unittest.cc
index 7145365..6f5f552 100644
--- a/net/http/http_stream_factory_unittest.cc
+++ b/net/http/http_stream_factory_unittest.cc
@@ -163,7 +163,9 @@
}
void GetSSLInfo(SSLInfo* ssl_info) override {}
void GetSSLCertRequestInfo(SSLCertRequestInfo* cert_request_info) override {}
- bool GetRemoteEndpoint(IPEndPoint* endpoint) override { return false; }
+ int GetRemoteEndpoint(IPEndPoint* endpoint) override {
+ return ERR_UNEXPECTED;
+ }
void Drain(HttpNetworkSession* session) override {}
void PopulateNetErrorDetails(NetErrorDetails* details) override { return; }
void SetPriority(RequestPriority priority) override {}
diff --git a/net/quic/quic_chromium_client_session.cc b/net/quic/quic_chromium_client_session.cc
index b54e6e5..2f9d8e5 100644
--- a/net/quic/quic_chromium_client_session.cc
+++ b/net/quic/quic_chromium_client_session.cc
@@ -1409,9 +1409,9 @@
return crypto_stream_.get();
}
-bool QuicChromiumClientSession::GetRemoteEndpoint(IPEndPoint* endpoint) {
+int QuicChromiumClientSession::GetRemoteEndpoint(IPEndPoint* endpoint) {
*endpoint = ToIPEndPoint(peer_address());
- return true;
+ return OK;
}
// TODO(rtenneti): Add unittests for GetSSLInfo which exercise the various ways
diff --git a/net/quic/quic_chromium_client_session.h b/net/quic/quic_chromium_client_session.h
index 806fa2a..58c70a036 100644
--- a/net/quic/quic_chromium_client_session.h
+++ b/net/quic/quic_chromium_client_session.h
@@ -699,7 +699,7 @@
void OnStreamClosed(quic::QuicStreamId stream_id) override;
// MultiplexedSession methods:
- bool GetRemoteEndpoint(IPEndPoint* endpoint) override;
+ int GetRemoteEndpoint(IPEndPoint* endpoint) override;
bool GetSSLInfo(SSLInfo* ssl_info) const override;
base::StringPiece GetAcceptChViaAlps(
const url::SchemeHostPort& scheme_host_port) const override;
diff --git a/net/spdy/multiplexed_http_stream.cc b/net/spdy/multiplexed_http_stream.cc
index 236fb40..b4e47bc 100644
--- a/net/spdy/multiplexed_http_stream.cc
+++ b/net/spdy/multiplexed_http_stream.cc
@@ -19,7 +19,7 @@
MultiplexedHttpStream::~MultiplexedHttpStream() = default;
-bool MultiplexedHttpStream::GetRemoteEndpoint(IPEndPoint* endpoint) {
+int MultiplexedHttpStream::GetRemoteEndpoint(IPEndPoint* endpoint) {
return session_->GetRemoteEndpoint(endpoint);
}
diff --git a/net/spdy/multiplexed_http_stream.h b/net/spdy/multiplexed_http_stream.h
index 27d4ce5c..56e1cd0 100644
--- a/net/spdy/multiplexed_http_stream.h
+++ b/net/spdy/multiplexed_http_stream.h
@@ -21,7 +21,7 @@
std::unique_ptr<MultiplexedSessionHandle> session);
~MultiplexedHttpStream() override;
- bool GetRemoteEndpoint(IPEndPoint* endpoint) override;
+ int GetRemoteEndpoint(IPEndPoint* endpoint) override;
void GetSSLInfo(SSLInfo* ssl_info) override;
void GetSSLCertRequestInfo(SSLCertRequestInfo* cert_request_info) override;
void Drain(HttpNetworkSession* session) override;
diff --git a/net/spdy/multiplexed_session.cc b/net/spdy/multiplexed_session.cc
index 06bd12a..f51ccde 100644
--- a/net/spdy/multiplexed_session.cc
+++ b/net/spdy/multiplexed_session.cc
@@ -14,9 +14,9 @@
MultiplexedSessionHandle::~MultiplexedSessionHandle() = default;
-bool MultiplexedSessionHandle::GetRemoteEndpoint(IPEndPoint* endpoint) {
+int MultiplexedSessionHandle::GetRemoteEndpoint(IPEndPoint* endpoint) {
if (!session_)
- return false;
+ return ERR_SOCKET_NOT_CONNECTED;
return session_->GetRemoteEndpoint(endpoint);
}
diff --git a/net/spdy/multiplexed_session.h b/net/spdy/multiplexed_session.h
index c5110cc..0aaa2669 100644
--- a/net/spdy/multiplexed_session.h
+++ b/net/spdy/multiplexed_session.h
@@ -26,9 +26,9 @@
virtual bool GetSSLInfo(SSLInfo* ssl_info) const = 0;
// Gets the remote endpoint of the socket that the HTTP stream is using, if
- // any. Returns true and fills in |endpoint| if it is available; returns false
- // and does not modify |endpoint| if it is unavailable.
- virtual bool GetRemoteEndpoint(IPEndPoint* endpoint) = 0;
+ // any. Returns OK and fills in |endpoint| if it is available; returns an
+ // error and does not modify |endpoint| otherwise.
+ virtual int GetRemoteEndpoint(IPEndPoint* endpoint) = 0;
// The value corresponding to |scheme_host_port| in the ACCEPT_CH frame
// received during TLS handshake via the ALPS extension, or the empty string
@@ -50,9 +50,9 @@
virtual ~MultiplexedSessionHandle();
// Gets the remote endpoint of the socket that the HTTP stream is using, if
- // any. Returns true and fills in |endpoint| if it is available; returns false
- // and does not modify |endpoint| if it is unavailable.
- bool GetRemoteEndpoint(IPEndPoint* endpoint);
+ // any. Returns OK and fills in |endpoint| if it is available; returns an
+ // error and does not modify |endpoint| otherwise.
+ int GetRemoteEndpoint(IPEndPoint* endpoint);
// Fills SSL info in |ssl_info| and returns true when SSL is in use.
bool GetSSLInfo(SSLInfo* ssl_info) const;
diff --git a/net/spdy/spdy_http_stream.cc b/net/spdy/spdy_http_stream.cc
index 09525d2..f4db5b6 100644
--- a/net/spdy/spdy_http_stream.cc
+++ b/net/spdy/spdy_http_stream.cc
@@ -704,11 +704,11 @@
std::move(response_callback_).Run(rv);
}
-bool SpdyHttpStream::GetRemoteEndpoint(IPEndPoint* endpoint) {
+int SpdyHttpStream::GetRemoteEndpoint(IPEndPoint* endpoint) {
if (!spdy_session_)
- return false;
+ return ERR_SOCKET_NOT_CONNECTED;
- return spdy_session_->GetPeerAddress(endpoint) == OK;
+ return spdy_session_->GetPeerAddress(endpoint);
}
void SpdyHttpStream::PopulateNetErrorDetails(NetErrorDetails* details) {
diff --git a/net/spdy/spdy_http_stream.h b/net/spdy/spdy_http_stream.h
index ddf9e25..7dfc6e8 100644
--- a/net/spdy/spdy_http_stream.h
+++ b/net/spdy/spdy_http_stream.h
@@ -86,7 +86,7 @@
bool GetAlternativeService(
AlternativeService* alternative_service) const override;
bool GetLoadTimingInfo(LoadTimingInfo* load_timing_info) const override;
- bool GetRemoteEndpoint(IPEndPoint* endpoint) override;
+ int GetRemoteEndpoint(IPEndPoint* endpoint) override;
void PopulateNetErrorDetails(NetErrorDetails* details) override;
void SetPriority(RequestPriority priority) override;
const std::set<std::string>& GetDnsAliases() const override;
diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index d0f186b..0733e87 100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -1507,8 +1507,8 @@
return LOAD_STATE_IDLE;
}
-bool SpdySession::GetRemoteEndpoint(IPEndPoint* endpoint) {
- return GetPeerAddress(endpoint) == OK;
+int SpdySession::GetRemoteEndpoint(IPEndPoint* endpoint) {
+ return GetPeerAddress(endpoint);
}
bool SpdySession::GetSSLInfo(SSLInfo* ssl_info) const {
diff --git a/net/spdy/spdy_session.h b/net/spdy/spdy_session.h
index 5dfc306..4566fbe 100644
--- a/net/spdy/spdy_session.h
+++ b/net/spdy/spdy_session.h
@@ -530,7 +530,7 @@
LoadState GetLoadState() const;
// MultiplexedSession methods:
- bool GetRemoteEndpoint(IPEndPoint* endpoint) override;
+ int GetRemoteEndpoint(IPEndPoint* endpoint) override;
bool GetSSLInfo(SSLInfo* ssl_info) const override;
base::StringPiece GetAcceptChViaAlps(
const url::SchemeHostPort& scheme_host_port) const override;
diff --git a/net/websockets/websocket_basic_handshake_stream.cc b/net/websockets/websocket_basic_handshake_stream.cc
index d72fe99..2b20820 100644
--- a/net/websockets/websocket_basic_handshake_stream.cc
+++ b/net/websockets/websocket_basic_handshake_stream.cc
@@ -363,11 +363,11 @@
parser()->GetSSLCertRequestInfo(cert_request_info);
}
-bool WebSocketBasicHandshakeStream::GetRemoteEndpoint(IPEndPoint* endpoint) {
+int WebSocketBasicHandshakeStream::GetRemoteEndpoint(IPEndPoint* endpoint) {
if (!state_.connection() || !state_.connection()->socket())
- return false;
+ return ERR_SOCKET_NOT_CONNECTED;
- return state_.connection()->socket()->GetPeerAddress(endpoint) == OK;
+ return state_.connection()->socket()->GetPeerAddress(endpoint);
}
void WebSocketBasicHandshakeStream::PopulateNetErrorDetails(
diff --git a/net/websockets/websocket_basic_handshake_stream.h b/net/websockets/websocket_basic_handshake_stream.h
index 78b5444..8496184 100644
--- a/net/websockets/websocket_basic_handshake_stream.h
+++ b/net/websockets/websocket_basic_handshake_stream.h
@@ -76,7 +76,7 @@
bool GetLoadTimingInfo(LoadTimingInfo* load_timing_info) const override;
void GetSSLInfo(SSLInfo* ssl_info) override;
void GetSSLCertRequestInfo(SSLCertRequestInfo* cert_request_info) override;
- bool GetRemoteEndpoint(IPEndPoint* endpoint) override;
+ int GetRemoteEndpoint(IPEndPoint* endpoint) override;
void Drain(HttpNetworkSession* session) override;
void SetPriority(RequestPriority priority) override;
void PopulateNetErrorDetails(NetErrorDetails* details) override;
diff --git a/net/websockets/websocket_http2_handshake_stream.cc b/net/websockets/websocket_http2_handshake_stream.cc
index 65d2fc6..c0d9c83 100644
--- a/net/websockets/websocket_http2_handshake_stream.cc
+++ b/net/websockets/websocket_http2_handshake_stream.cc
@@ -219,8 +219,11 @@
NOTREACHED();
}
-bool WebSocketHttp2HandshakeStream::GetRemoteEndpoint(IPEndPoint* endpoint) {
- return session_ && session_->GetRemoteEndpoint(endpoint);
+int WebSocketHttp2HandshakeStream::GetRemoteEndpoint(IPEndPoint* endpoint) {
+ if (!session_)
+ return ERR_SOCKET_NOT_CONNECTED;
+
+ return session_->GetRemoteEndpoint(endpoint);
}
void WebSocketHttp2HandshakeStream::PopulateNetErrorDetails(
diff --git a/net/websockets/websocket_http2_handshake_stream.h b/net/websockets/websocket_http2_handshake_stream.h
index 341c5210..d0a425a 100644
--- a/net/websockets/websocket_http2_handshake_stream.h
+++ b/net/websockets/websocket_http2_handshake_stream.h
@@ -88,7 +88,7 @@
bool GetLoadTimingInfo(LoadTimingInfo* load_timing_info) const override;
void GetSSLInfo(SSLInfo* ssl_info) override;
void GetSSLCertRequestInfo(SSLCertRequestInfo* cert_request_info) override;
- bool GetRemoteEndpoint(IPEndPoint* endpoint) override;
+ int GetRemoteEndpoint(IPEndPoint* endpoint) override;
void Drain(HttpNetworkSession* session) override;
void SetPriority(RequestPriority priority) override;
void PopulateNetErrorDetails(NetErrorDetails* details) override;
