From 743032056bfd351fe37c01f2e7b0e1b5096ec15d Mon Sep 17 00:00:00 2001 From: Kershaw Chang Date: Thu, 18 Nov 2021 10:22:15 +0000 Subject: [PATCH] Bug 1734609 - Make sure we don't dispatch a transaction more than once, r=necko-reviewers,dragana Differential Revision: https://phabricator.services.mozilla.com/D130250 --- netwerk/protocol/http/ConnectionEntry.cpp | 2 +- netwerk/protocol/http/ConnectionEntry.h | 2 +- netwerk/protocol/http/Http2Session.cpp | 2 + netwerk/protocol/http/Http2Session.h | 1 + netwerk/protocol/http/nsAHttpConnection.h | 2 + netwerk/protocol/http/nsHttpConnectionMgr.cpp | 41 ++++++++++++++++--- netwerk/protocol/http/nsHttpConnectionMgr.h | 5 ++- netwerk/protocol/http/nsHttpTransaction.cpp | 24 ++++++++--- netwerk/protocol/http/nsHttpTransaction.h | 10 ++++- 9 files changed, 74 insertions(+), 15 deletions(-) diff --git a/netwerk/protocol/http/ConnectionEntry.cpp b/netwerk/protocol/http/ConnectionEntry.cpp index 35d700e91f58..4095cc546f7b 100644 --- a/netwerk/protocol/http/ConnectionEntry.cpp +++ b/netwerk/protocol/http/ConnectionEntry.cpp @@ -216,7 +216,7 @@ void ConnectionEntry::InsertTransaction( bool aInsertAsFirstForTheSamePriority /* = false */) { mPendingQ.InsertTransaction(pendingTransInfo, aInsertAsFirstForTheSamePriority); - pendingTransInfo->Transaction()->OnPendingQueueInserted(); + pendingTransInfo->Transaction()->OnPendingQueueInserted(mConnInfo->HashKey()); } nsTArray>* diff --git a/netwerk/protocol/http/ConnectionEntry.h b/netwerk/protocol/http/ConnectionEntry.h index aaec7a5e332c..c324e543ce2e 100644 --- a/netwerk/protocol/http/ConnectionEntry.h +++ b/netwerk/protocol/http/ConnectionEntry.h @@ -90,7 +90,7 @@ class ConnectionEntry { HttpRetParams GetConnectionData(); void LogConnections(); - RefPtr mConnInfo; + const RefPtr mConnInfo; bool AvailableForDispatchNow(); diff --git a/netwerk/protocol/http/Http2Session.cpp b/netwerk/protocol/http/Http2Session.cpp index 45103a77635a..d2678c1b6077 100644 --- a/netwerk/protocol/http/Http2Session.cpp +++ b/netwerk/protocol/http/Http2Session.cpp @@ -4669,5 +4669,7 @@ bool Http2Session::CanAcceptWebsocket() { (mPeerAllowsWebsockets || !mProcessedWaitingWebsockets); } +void Http2Session::SanityCheck() { MOZ_DIAGNOSTIC_ASSERT(mConnection); } + } // namespace net } // namespace mozilla diff --git a/netwerk/protocol/http/Http2Session.h b/netwerk/protocol/http/Http2Session.h index b4996743dd0d..c3f853c3aa38 100644 --- a/netwerk/protocol/http/Http2Session.h +++ b/netwerk/protocol/http/Http2Session.h @@ -230,6 +230,7 @@ class Http2Session final : public ASpdySession, // overload of nsAHttpConnection void TransactionHasDataToWrite(nsAHttpTransaction*) override; void TransactionHasDataToRecv(nsAHttpTransaction*) override; + void SanityCheck() override; // a similar version for Http2Stream void TransactionHasDataToWrite(Http2Stream*); diff --git a/netwerk/protocol/http/nsAHttpConnection.h b/netwerk/protocol/http/nsAHttpConnection.h index f1e47c8baa64..220d5eecbceb 100644 --- a/netwerk/protocol/http/nsAHttpConnection.h +++ b/netwerk/protocol/http/nsAHttpConnection.h @@ -164,6 +164,8 @@ class nsAHttpConnection : public nsISupports { virtual nsresult GetPeerAddr(NetAddr* addr) = 0; virtual bool ResolvedByTRR() = 0; virtual bool GetEchConfigUsed() = 0; + + virtual void SanityCheck() {} }; NS_DEFINE_STATIC_IID_ACCESSOR(nsAHttpConnection, NS_AHTTPCONNECTION_IID) diff --git a/netwerk/protocol/http/nsHttpConnectionMgr.cpp b/netwerk/protocol/http/nsHttpConnectionMgr.cpp index 2816c0b8d20f..0acf649339e5 100644 --- a/netwerk/protocol/http/nsHttpConnectionMgr.cpp +++ b/netwerk/protocol/http/nsHttpConnectionMgr.cpp @@ -297,6 +297,8 @@ nsHttpConnectionMgr::Observe(nsISupports* subject, const char* topic, nsresult nsHttpConnectionMgr::AddTransaction(HttpTransactionShell* trans, int32_t priority) { LOG(("nsHttpConnectionMgr::AddTransaction [trans=%p %d]\n", trans, priority)); + // Make sure a transaction is not in a pending queue. + CheckTransInPendingQueue(trans->AsHttpTransaction()); return PostEvent(&nsHttpConnectionMgr::OnMsgNewTransaction, priority, trans->AsHttpTransaction()); } @@ -326,6 +328,9 @@ nsresult nsHttpConnectionMgr::AddTransactionWithStickyConn( ("nsHttpConnectionMgr::AddTransactionWithStickyConn " "[trans=%p %d transWithStickyConn=%p]\n", trans, priority, transWithStickyConn)); + // Make sure a transaction is not in a pending queue. + CheckTransInPendingQueue(trans->AsHttpTransaction()); + RefPtr data = new NewTransactionData(trans->AsHttpTransaction(), priority, transWithStickyConn->AsHttpTransaction()); @@ -1622,6 +1627,9 @@ nsresult nsHttpConnectionMgr::ProcessNewTransaction(nsHttpTransaction* trans) { return NS_OK; } + // Make sure a transaction is not in a pending queue. + CheckTransInPendingQueue(trans); + trans->SetPendingTime(); RefPtr pushedStreamWrapper = @@ -3419,9 +3427,6 @@ void nsHttpConnectionMgr::ExcludeHttp3(const nsHttpConnectionInfo* ci) { } ent->DontReuseHttp3Conn(); - // Need to cancel the transactions in the pending queue. Otherwise, they'll - // stay in the queue forever. - ent->CancelAllTransactions(NS_ERROR_NET_RESET); } void nsHttpConnectionMgr::MoveToWildCardConnEntry( @@ -3468,14 +3473,19 @@ void nsHttpConnectionMgr::MoveToWildCardConnEntry( ent->MoveConnection(proxyConn, wcEnt); } -bool nsHttpConnectionMgr::RemoveTransFromConnEntry(nsHttpTransaction* aTrans) { +bool nsHttpConnectionMgr::RemoveTransFromConnEntry(nsHttpTransaction* aTrans, + const nsACString& aHashKey) { MOZ_ASSERT(OnSocketThread(), "not on socket thread"); LOG(("nsHttpConnectionMgr::RemoveTransFromConnEntry: trans=%p ci=%s", aTrans, - aTrans->ConnectionInfo()->HashKey().get())); + PromiseFlatCString(aHashKey).get())); + + if (aHashKey.IsEmpty()) { + return false; + } // Step 1: Get the transaction's connection entry. - ConnectionEntry* entry = mCT.GetWeak(aTrans->ConnectionInfo()->HashKey()); + ConnectionEntry* entry = mCT.GetWeak(aHashKey); if (!entry) { return false; } @@ -3543,5 +3553,24 @@ void nsHttpConnectionMgr::DecrementNumIdleConns() { ConditionallyStopPruneDeadConnectionsTimer(); } +void nsHttpConnectionMgr::CheckTransInPendingQueue(nsHttpTransaction* aTrans) { +#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED + // We only do this check on socket thread. When this function is called on + // main thread, the transaction is newly created, so we can skip this check. + if (!OnSocketThread()) { + return; + } + + nsAutoCString hashKey; + aTrans->GetHashKeyOfConnectionEntry(hashKey); + if (hashKey.IsEmpty()) { + return; + } + + bool foundInPendingQ = RemoveTransFromConnEntry(aTrans, hashKey); + MOZ_DIAGNOSTIC_ASSERT(!foundInPendingQ); +#endif +} + } // namespace net } // namespace mozilla diff --git a/netwerk/protocol/http/nsHttpConnectionMgr.h b/netwerk/protocol/http/nsHttpConnectionMgr.h index b03b7409c361..80717fb043fa 100644 --- a/netwerk/protocol/http/nsHttpConnectionMgr.h +++ b/netwerk/protocol/http/nsHttpConnectionMgr.h @@ -73,7 +73,8 @@ class nsHttpConnectionMgr final : public HttpConnectionMgrShell, // Remove a transaction from the pendingQ of it's connection entry. Returns // true if the transaction is removed successfully, otherwise returns false. - bool RemoveTransFromConnEntry(nsHttpTransaction* aTrans); + bool RemoveTransFromConnEntry(nsHttpTransaction* aTrans, + const nsACString& aHashKey); // Directly dispatch the transaction or insert it in to the pendingQ. [[nodiscard]] nsresult ProcessNewTransaction(nsHttpTransaction* aTrans); @@ -449,6 +450,8 @@ class nsHttpConnectionMgr final : public HttpConnectionMgrShell, // Then, it notifies selected transactions' connection of the new active tab // id. void NotifyConnectionOfBrowsingContextIdChange(uint64_t previousId); + + void CheckTransInPendingQueue(nsHttpTransaction* aTrans); }; } // namespace net diff --git a/netwerk/protocol/http/nsHttpTransaction.cpp b/netwerk/protocol/http/nsHttpTransaction.cpp index d99332a39a98..e87c384b9057 100644 --- a/netwerk/protocol/http/nsHttpTransaction.cpp +++ b/netwerk/protocol/http/nsHttpTransaction.cpp @@ -418,9 +418,15 @@ static inline void CreateAndStartTimer(nsCOMPtr& aTimer, nsITimer::TYPE_ONE_SHOT); } -void nsHttpTransaction::OnPendingQueueInserted() { +void nsHttpTransaction::OnPendingQueueInserted( + const nsACString& aConnectionHashKey) { MOZ_ASSERT(OnSocketThread(), "not on socket thread"); + { + MutexAutoLock lock(mLock); + mHashKeyOfConnectionEntry.Assign(aConnectionHashKey); + } + // Don't create mHttp3BackupTimer if HTTPS RR is in play. if (mConnInfo->IsHttp3() && !mOrigConnInfo) { // Backup timer should only be created once. @@ -521,6 +527,9 @@ void nsHttpTransaction::SetConnection(nsAHttpConnection* conn) { mConnection = conn; if (mConnection) { mIsHttp3Used = mConnection->Version() == HttpVersion::v3_0; +#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED + mConnection->SanityCheck(); +#endif } } } @@ -3170,8 +3179,8 @@ nsresult nsHttpTransaction::OnHTTPSRRAvailable( RefPtr newInfo = mConnInfo->CloneAndAdoptHTTPSSVCRecord(svcbRecord); bool needFastFallback = newInfo->IsHttp3(); - bool foundInPendingQ = - gHttpHandler->ConnMgr()->RemoveTransFromConnEntry(this); + bool foundInPendingQ = gHttpHandler->ConnMgr()->RemoveTransFromConnEntry( + this, mHashKeyOfConnectionEntry); // Adopt the new connection info, so this transaction will be added into the // new connection entry. @@ -3378,8 +3387,8 @@ void nsHttpTransaction::HandleFallback( LOG(("nsHttpTransaction %p HandleFallback to connInfo[%s]", this, aFallbackConnInfo->HashKey().get())); - bool foundInPendingQ = - gHttpHandler->ConnMgr()->RemoveTransFromConnEntry(this); + bool foundInPendingQ = gHttpHandler->ConnMgr()->RemoveTransFromConnEntry( + this, mHashKeyOfConnectionEntry); if (!foundInPendingQ) { MOZ_ASSERT(false, "transaction not in entry"); return; @@ -3448,5 +3457,10 @@ void nsHttpTransaction::CollectTelemetryForUploads() { mTimings.responseStart); } +void nsHttpTransaction::GetHashKeyOfConnectionEntry(nsACString& aResult) { + MutexAutoLock lock(mLock); + aResult.Assign(mHashKeyOfConnectionEntry); +} + } // namespace net } // namespace mozilla diff --git a/netwerk/protocol/http/nsHttpTransaction.h b/netwerk/protocol/http/nsHttpTransaction.h index 3b28a19dc262..9ad4670ca144 100644 --- a/netwerk/protocol/http/nsHttpTransaction.h +++ b/netwerk/protocol/http/nsHttpTransaction.h @@ -172,6 +172,8 @@ class nsHttpTransaction final : public nsAHttpTransaction, nsIDNSHTTPSSVCRecord* aHTTPSSVCRecord, nsISVCBRecord* aHighestPriorityRecord) override; + void GetHashKeyOfConnectionEntry(nsACString& aResult); + private: friend class DeleteHttpTransaction; virtual ~nsHttpTransaction(); @@ -501,7 +503,7 @@ class nsHttpTransaction final : public nsAHttpTransaction, ASpdySession* TunnelProvider() { return mTunnelProvider; } nsIInterfaceRequestor* SecurityCallbacks() { return mCallbacks; } // Called when this transaction is inserted in the pending queue. - void OnPendingQueueInserted(); + void OnPendingQueueInserted(const nsACString& aConnectionHashKey); private: RefPtr mTunnelProvider; @@ -553,6 +555,12 @@ class nsHttpTransaction final : public nsAHttpTransaction, bool ShouldRestartOn0RttError(nsresult reason); nsCOMPtr mEarlyHintObserver; + // This hash key is set when a transaction is inserted into the connection + // entry's pending queue. + // See nsHttpConnectionMgr::GetOrCreateConnectionEntry(). A transaction could + // be associated with the connection entry whose hash key is not the same as + // this transaction's. + nsCString mHashKeyOfConnectionEntry; }; } // namespace net