From c940bbdf067d853b6f5639a1ae950a1f240cda3f Mon Sep 17 00:00:00 2001 From: Honza Bambas Date: Fri, 16 Mar 2018 09:06:00 -0400 Subject: [PATCH] Bug 1430659 - Network requests in "blocked" state for long time (1+ minute) when ipv6 hosts are no longer available. r=valentin --- modules/libpref/init/all.js | 7 ++ netwerk/base/nsISocketTransport.idl | 13 ++++ netwerk/base/nsSocketTransport2.cpp | 51 ++++++++---- netwerk/base/nsSocketTransport2.h | 3 + netwerk/protocol/http/TunnelUtils.cpp | 1 + netwerk/protocol/http/nsHttpConnectionMgr.cpp | 78 +++++++++++++++---- netwerk/protocol/http/nsHttpConnectionMgr.h | 2 + netwerk/protocol/http/nsHttpHandler.cpp | 7 ++ netwerk/protocol/http/nsHttpHandler.h | 2 + 9 files changed, 133 insertions(+), 31 deletions(-) diff --git a/modules/libpref/init/all.js b/modules/libpref/init/all.js index c315c5a9515d..6a706827e391 100644 --- a/modules/libpref/init/all.js +++ b/modules/libpref/init/all.js @@ -1794,6 +1794,13 @@ pref("network.http.connection-timeout", 90); // seconds. pref("network.http.tls-handshake-timeout", 30); +// The number of seconds after which we time out a connection of a retry (fallback) +// socket when a certain IP family is already preferred. This shorter connection +// timeout allows us to find out more quickly that e.g. an IPv6 host is no longer +// available and let us try an IPv4 address, if provided for the host name. +// Set to '0' to use the default connection timeout. +pref("network.http.fallback-connection-timeout", 5); + // The number of seconds to allow active connections to prove that they have // traffic before considered stalled, after a network change has been detected // and signalled. diff --git a/netwerk/base/nsISocketTransport.idl b/netwerk/base/nsISocketTransport.idl index 5a355c966313..d75b84c8aece 100644 --- a/netwerk/base/nsISocketTransport.idl +++ b/netwerk/base/nsISocketTransport.idl @@ -253,6 +253,12 @@ interface nsISocketTransport : nsITransport */ const unsigned long REFRESH_CACHE = (1 << 9); + /** + * If this flag is set then it means that if connecting the preferred ip + * family has failed, retry with the oppsite one once more. + */ + const unsigned long RETRY_WITH_DIFFERENT_IP_FAMILY = (1 << 10); + /** * An opaque flags for non-standard behavior of the TLS system. * It is unlikely this will need to be set outside of telemetry studies @@ -289,4 +295,11 @@ interface nsISocketTransport : nsITransport [noscript] void setFastOpenCallback(in TCPFastOpenPtr aFastOpen); readonly attribute nsresult firstRetryError; + + /** + * If true, this socket transport has found out the prefered family + * according it's connection flags could not be used to establish + * connections any more. Hence, the preference should be reset. + */ + readonly attribute boolean resetIPFamilyPreference; }; diff --git a/netwerk/base/nsSocketTransport2.cpp b/netwerk/base/nsSocketTransport2.cpp index df01e62f55d3..6d18baf8091a 100644 --- a/netwerk/base/nsSocketTransport2.cpp +++ b/netwerk/base/nsSocketTransport2.cpp @@ -775,6 +775,7 @@ nsSocketTransport::nsSocketTransport() , mProxyTransparentResolvesHost(false) , mHttpsProxy(false) , mConnectionFlags(0) + , mResetFamilyPreference(false) , mTlsFlags(0) , mReuseAddrPort(false) , mState(STATE_CLOSED) @@ -1703,8 +1704,10 @@ nsSocketTransport::RecoverFromError() #endif // can only recover from errors in these states - if (mState != STATE_RESOLVING && mState != STATE_CONNECTING) + if (mState != STATE_RESOLVING && mState != STATE_CONNECTING) { + SOCKET_LOG((" not in a recoverable state")); return false; + } nsresult rv; @@ -1729,15 +1732,19 @@ nsSocketTransport::RecoverFromError() mCondition != NS_ERROR_NET_TIMEOUT && mCondition != NS_ERROR_UNKNOWN_HOST && mCondition != NS_ERROR_UNKNOWN_PROXY_HOST && - !(mFDFastOpenInProgress && (mCondition == NS_ERROR_FAILURE))) + !(mFDFastOpenInProgress && (mCondition == NS_ERROR_FAILURE))) { + SOCKET_LOG((" not a recoverable error %" PRIx32, static_cast(mCondition))); return false; + } #else if (mCondition != NS_ERROR_CONNECTION_REFUSED && mCondition != NS_ERROR_PROXY_CONNECTION_REFUSED && mCondition != NS_ERROR_NET_TIMEOUT && mCondition != NS_ERROR_UNKNOWN_HOST && - mCondition != NS_ERROR_UNKNOWN_PROXY_HOST) + mCondition != NS_ERROR_UNKNOWN_PROXY_HOST) { + SOCKET_LOG((" not a recoverable error %" PRIx32, static_cast(mCondition))); return false; + } #endif bool tryAgain = false; @@ -1779,12 +1786,15 @@ nsSocketTransport::RecoverFromError() } } - if (mConnectionFlags & (DISABLE_IPV6 | DISABLE_IPV4) && + if (mConnectionFlags & RETRY_WITH_DIFFERENT_IP_FAMILY && mCondition == NS_ERROR_UNKNOWN_HOST && mState == STATE_RESOLVING && !mProxyTransparentResolvesHost) { - SOCKET_LOG((" trying lookup again with both ipv4/ipv6 enabled\n")); - mConnectionFlags &= ~(DISABLE_IPV6 | DISABLE_IPV4); + SOCKET_LOG((" trying lookup again with opposite ip family\n")); + mConnectionFlags ^= (DISABLE_IPV6 | DISABLE_IPV4); + mConnectionFlags &= ~RETRY_WITH_DIFFERENT_IP_FAMILY; + // This will tell the consuming half-open to reset preference on the connection entry + mResetFamilyPreference = true; tryAgain = true; } @@ -1794,17 +1804,15 @@ nsSocketTransport::RecoverFromError() if (NS_SUCCEEDED(rv)) { SOCKET_LOG((" trying again with next ip address\n")); tryAgain = true; - } - else if (mConnectionFlags & (DISABLE_IPV6 | DISABLE_IPV4)) { + } else if (mConnectionFlags & RETRY_WITH_DIFFERENT_IP_FAMILY) { + SOCKET_LOG((" failed to connect, trying with opposite ip family\n")); // Drop state to closed. This will trigger new round of DNS // resolving bellow. - // XXX Could be optimized to only switch the flags to save - // duplicate connection attempts. - SOCKET_LOG((" failed to connect all ipv4-only or ipv6-only " - "hosts, trying lookup/connect again with both " - "ipv4/ipv6\n")); mState = STATE_CLOSED; - mConnectionFlags &= ~(DISABLE_IPV6 | DISABLE_IPV4); + mConnectionFlags ^= (DISABLE_IPV6 | DISABLE_IPV4); + mConnectionFlags &= ~RETRY_WITH_DIFFERENT_IP_FAMILY; + // This will tell the consuming half-open to reset preference on the connection entry + mResetFamilyPreference = true; tryAgain = true; } else if (!(mConnectionFlags & DISABLE_TRR)) { bool trrEnabled; @@ -2327,6 +2335,8 @@ nsSocketTransport::OnSocketDetached(PRFileDesc *fd) MOZ_ASSERT(OnSocketThread(), "not on socket thread"); + mAttached = false; + // if we didn't initiate this detach, then be sure to pass an error // condition up to our consumers. (e.g., STS is shutting down.) if (NS_SUCCEEDED(mCondition)) { @@ -2551,6 +2561,9 @@ nsSocketTransport::OpenOutputStream(uint32_t flags, NS_IMETHODIMP nsSocketTransport::Close(nsresult reason) { + SOCKET_LOG(("nsSocketTransport::Close %p reason=%" PRIx32, + this, static_cast(reason))); + if (NS_SUCCEEDED(reason)) reason = NS_BASE_STREAM_CLOSED; @@ -3014,8 +3027,11 @@ nsSocketTransport::GetConnectionFlags(uint32_t *value) NS_IMETHODIMP nsSocketTransport::SetConnectionFlags(uint32_t value) { + SOCKET_LOG(("nsSocketTransport::SetConnectionFlags %p flags=%u", this, value)); + mConnectionFlags = value; mIsPrivate = value & nsISocketTransport::NO_PERMANENT_STORAGE; + return NS_OK; } @@ -3558,5 +3574,12 @@ nsSocketTransport::GetFirstRetryError(nsresult *aFirstRetryError) return NS_OK; } +NS_IMETHODIMP +nsSocketTransport::GetResetIPFamilyPreference(bool *aReset) +{ + *aReset = mResetFamilyPreference; + return NS_OK; +} + } // namespace net } // namespace mozilla diff --git a/netwerk/base/nsSocketTransport2.h b/netwerk/base/nsSocketTransport2.h index fc5d8a99cc17..e264a7f7ceb9 100644 --- a/netwerk/base/nsSocketTransport2.h +++ b/netwerk/base/nsSocketTransport2.h @@ -301,6 +301,9 @@ private: bool mProxyTransparentResolvesHost; bool mHttpsProxy; uint32_t mConnectionFlags; + // When we fail to connect using a prefered IP family, we tell the consumer to reset + // the IP family preference on the connection entry. + bool mResetFamilyPreference; uint32_t mTlsFlags; bool mReuseAddrPort; diff --git a/netwerk/protocol/http/TunnelUtils.cpp b/netwerk/protocol/http/TunnelUtils.cpp index bef00fb11f0d..b7a41cdf0b62 100644 --- a/netwerk/protocol/http/TunnelUtils.cpp +++ b/netwerk/protocol/http/TunnelUtils.cpp @@ -1562,6 +1562,7 @@ FWD_TS_PTR(GetTlsFlags, uint32_t); FWD_TS(SetTlsFlags, uint32_t); FWD_TS_PTR(GetRecvBufferSize, uint32_t); FWD_TS(SetRecvBufferSize, uint32_t); +FWD_TS_PTR(GetResetIPFamilyPreference, bool); nsresult SocketTransportShim::GetOriginAttributes(mozilla::OriginAttributes* aOriginAttributes) diff --git a/netwerk/protocol/http/nsHttpConnectionMgr.cpp b/netwerk/protocol/http/nsHttpConnectionMgr.cpp index 88ae15593105..4df10037d93e 100644 --- a/netwerk/protocol/http/nsHttpConnectionMgr.cpp +++ b/netwerk/protocol/http/nsHttpConnectionMgr.cpp @@ -3989,7 +3989,6 @@ nsHttpConnectionMgr::nsHalfOpenSocket::~nsHalfOpenSocket() { MOZ_ASSERT(!mStreamOut); MOZ_ASSERT(!mBackupStreamOut); - MOZ_ASSERT(!mSynTimer); LOG(("Destroying nsHalfOpenSocket [this=%p]\n", this)); if (mEnt) @@ -4071,16 +4070,29 @@ nsHalfOpenSocket::SetupStreams(nsISocketTransport **transport, tmpFlags |= nsISocketTransport::BE_CONSERVATIVE; } - // For backup connections, we disable IPv6. That's because some users have - // broken IPv6 connectivity (leading to very long timeouts), and disabling - // IPv6 on the backup connection gives them a much better user experience - // with dual-stack hosts, though they still pay the 250ms delay for each new - // connection. This strategy is also known as "happy eyeballs". - if (mEnt->mPreferIPv6) { - tmpFlags |= nsISocketTransport::DISABLE_IPV4; - } - else if (mEnt->mPreferIPv4 || - (isBackup && gHttpHandler->FastFallbackToIPv4())) { + if (mEnt->PreferenceKnown()) { + if (mEnt->mPreferIPv6) { + tmpFlags |= nsISocketTransport::DISABLE_IPV4; + } else if (mEnt->mPreferIPv4) { + tmpFlags |= nsISocketTransport::DISABLE_IPV6; + } + + // In case the host is no longer accessible via the preferred IP family, + // try the opposite one and potentially restate the preference. + tmpFlags |= nsISocketTransport::RETRY_WITH_DIFFERENT_IP_FAMILY; + + // From the same reason, let the backup socket fail faster to try the other family. + uint16_t fallbackTimeout = isBackup ? gHttpHandler->GetFallbackSynTimeout() : 0; + if (fallbackTimeout) { + socketTransport->SetTimeout(nsISocketTransport::TIMEOUT_CONNECT, + fallbackTimeout); + } + } else if (isBackup) { + // For backup connections, we disable IPv6. That's because some users have + // broken IPv6 connectivity (leading to very long timeouts), and disabling + // IPv6 on the backup connection gives them a much better user experience + // with dual-stack hosts, though they still pay the 250ms delay for each new + // connection. This strategy is also known as "happy eyeballs". tmpFlags |= nsISocketTransport::DISABLE_IPV6; } @@ -4155,6 +4167,7 @@ nsHttpConnectionMgr::nsHalfOpenSocket::SetupPrimaryStreams() getter_AddRefs(mStreamIn), getter_AddRefs(mStreamOut), false); + LOG(("nsHalfOpenSocket::SetupPrimaryStream [this=%p ent=%s rv=%" PRIx32 "]", this, mEnt->mConnInfo->Origin(), static_cast(rv))); if (NS_FAILED(rv)) { @@ -4180,6 +4193,7 @@ nsHttpConnectionMgr::nsHalfOpenSocket::SetupBackupStreams() getter_AddRefs(mBackupStreamIn), getter_AddRefs(mBackupStreamOut), true); + LOG(("nsHalfOpenSocket::SetupBackupStream [this=%p ent=%s rv=%" PRIx32 "]", this, mEnt->mConnInfo->Origin(), static_cast(rv))); if (NS_FAILED(rv)) { @@ -4230,7 +4244,9 @@ nsHttpConnectionMgr::nsHalfOpenSocket::CancelBackupTimer() LOG(("nsHalfOpenSocket::CancelBackupTimer()")); mSynTimer->Cancel(); - mSynTimer = nullptr; + + // Keeping the reference to the timer to remember we have already + // performed the backup connection. } void @@ -4318,7 +4334,9 @@ nsHttpConnectionMgr::nsHalfOpenSocket::Notify(nsITimer *timer) DebugOnly rv = SetupBackupStreams(); MOZ_ASSERT(NS_SUCCEEDED(rv)); - mSynTimer = nullptr; + // Keeping the reference to the timer to remember we have already + // performed the backup connection. + return NS_OK; } @@ -4819,6 +4837,12 @@ nsHalfOpenSocket::SetupConn(nsIAsyncOutputStream *out, PR_MillisecondsToInterval( static_cast(rtt.ToMilliseconds()))); + bool resetPreference = false; + mSocketTransport->GetResetIPFamilyPreference(&resetPreference); + if (resetPreference) { + mEnt->ResetIPFamilyPreference(); + } + if (!aFastOpen && NS_SUCCEEDED(mSocketTransport->GetPeerAddr(&peeraddr))) { mEnt->RecordIPFamilyPreference(peeraddr.raw.family); @@ -4841,8 +4865,15 @@ nsHalfOpenSocket::SetupConn(nsIAsyncOutputStream *out, PR_MillisecondsToInterval( static_cast(rtt.ToMilliseconds()))); - if (NS_SUCCEEDED(mBackupTransport->GetPeerAddr(&peeraddr))) + bool resetPreference = false; + mBackupTransport->GetResetIPFamilyPreference(&resetPreference); + if (resetPreference) { + mEnt->ResetIPFamilyPreference(); + } + + if (NS_SUCCEEDED(mBackupTransport->GetPeerAddr(&peeraddr))) { mEnt->RecordIPFamilyPreference(peeraddr.raw.family); + } // The nsHttpConnection object now owns these streams and sockets mBackupStreamOut = nullptr; @@ -5365,21 +5396,34 @@ void nsHttpConnectionMgr:: nsConnectionEntry::RecordIPFamilyPreference(uint16_t family) { - if (family == PR_AF_INET && !mPreferIPv6) - mPreferIPv4 = true; + LOG(("nsConnectionEntry::RecordIPFamilyPreference %p, af=%u", this, family)); - if (family == PR_AF_INET6 && !mPreferIPv4) + if (family == PR_AF_INET && !mPreferIPv6) { + mPreferIPv4 = true; + } + + if (family == PR_AF_INET6 && !mPreferIPv4) { mPreferIPv6 = true; + } + + LOG((" %p prefer ipv4=%d, ipv6=%d", this, (bool)mPreferIPv4, (bool)mPreferIPv6)); } void nsHttpConnectionMgr:: nsConnectionEntry::ResetIPFamilyPreference() { + LOG(("nsConnectionEntry::ResetIPFamilyPreference %p", this)); + mPreferIPv4 = false; mPreferIPv6 = false; } +bool net::nsHttpConnectionMgr::nsConnectionEntry::PreferenceKnown() const +{ + return (bool)mPreferIPv4 || (bool)mPreferIPv6; +} + size_t nsHttpConnectionMgr::nsConnectionEntry::PendingQLength() const { diff --git a/netwerk/protocol/http/nsHttpConnectionMgr.h b/netwerk/protocol/http/nsHttpConnectionMgr.h index 14f9e22dad24..8ffdd79715b3 100644 --- a/netwerk/protocol/http/nsHttpConnectionMgr.h +++ b/netwerk/protocol/http/nsHttpConnectionMgr.h @@ -331,6 +331,8 @@ private: void RecordIPFamilyPreference(uint16_t family); // Resets all flags to their default values void ResetIPFamilyPreference(); + // True iff there is currently an established IP family preference + bool PreferenceKnown() const; // Return the count of pending transactions for all window ids. size_t PendingQLength() const; diff --git a/netwerk/protocol/http/nsHttpHandler.cpp b/netwerk/protocol/http/nsHttpHandler.cpp index b369bc2c03a6..2e9850f76a51 100644 --- a/netwerk/protocol/http/nsHttpHandler.cpp +++ b/netwerk/protocol/http/nsHttpHandler.cpp @@ -207,6 +207,7 @@ nsHttpHandler::nsHttpHandler() , mMaxRequestAttempts(6) , mMaxRequestDelay(10) , mIdleSynTimeout(250) + , mFallbackSynTimeout(5) , mH2MandatorySuiteEnabled(false) , mMaxUrgentExcessiveConns(3) , mMaxConnections(24) @@ -1377,6 +1378,12 @@ nsHttpHandler::PrefsChanged(nsIPrefBranch *prefs, const char *pref) mFastFallbackToIPv4 = cVar; } + if (PREF_CHANGED(HTTP_PREF("fallback-connection-timeout"))) { + rv = prefs->GetIntPref(HTTP_PREF("fallback-connection-timeout"), &val); + if (NS_SUCCEEDED(rv)) + mFallbackSynTimeout = (uint16_t) clamped(val, 0, 10 * 60); + } + if (PREF_CHANGED(HTTP_PREF("version"))) { nsAutoCString httpVersion; prefs->GetCharPref(HTTP_PREF("version"), httpVersion); diff --git a/netwerk/protocol/http/nsHttpHandler.h b/netwerk/protocol/http/nsHttpHandler.h index 852caeab6061..3f74dd8f5e6e 100644 --- a/netwerk/protocol/http/nsHttpHandler.h +++ b/netwerk/protocol/http/nsHttpHandler.h @@ -101,6 +101,7 @@ public: uint32_t PhishyUserPassLength() { return mPhishyUserPassLength; } uint8_t GetQoSBits() { return mQoSBits; } uint16_t GetIdleSynTimeout() { return mIdleSynTimeout; } + uint16_t GetFallbackSynTimeout() { return mFallbackSynTimeout; } bool FastFallbackToIPv4() { return mFastFallbackToIPv4; } uint32_t MaxSocketCount(); bool EnforceAssocReq() { return mEnforceAssocReq; } @@ -484,6 +485,7 @@ private: uint16_t mMaxRequestAttempts; uint16_t mMaxRequestDelay; uint16_t mIdleSynTimeout; + uint16_t mFallbackSynTimeout; // seconds bool mH2MandatorySuiteEnabled; uint16_t mMaxUrgentExcessiveConns;