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

@@ -216,7 +216,7 @@ void ConnectionEntry::InsertTransaction(
     bool aInsertAsFirstForTheSamePriority /* = false */) {
   mPendingQ.InsertTransaction(pendingTransInfo,
                               aInsertAsFirstForTheSamePriority);
-  pendingTransInfo->Transaction()->OnPendingQueueInserted();
+  pendingTransInfo->Transaction()->OnPendingQueueInserted(mConnInfo->HashKey());
 }
 
 nsTArray<RefPtr<PendingTransactionInfo>>*

netwerk/protocol/http/ConnectionEntry.h

@@ -90,7 +90,7 @@ class ConnectionEntry {
   HttpRetParams GetConnectionData();
   void LogConnections();
 
-  RefPtr<nsHttpConnectionInfo> mConnInfo;
+  const RefPtr<nsHttpConnectionInfo> mConnInfo;
 
   bool AvailableForDispatchNow();
 

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

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*);

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)

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<NewTransactionData> 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<Http2PushedStreamWrapper> 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

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

netwerk/protocol/http/nsHttpTransaction.cpp

@@ -418,9 +418,15 @@ static inline void CreateAndStartTimer(nsCOMPtr<nsITimer>& 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<nsHttpConnectionInfo> 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

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<ASpdySession> mTunnelProvider;
@@ -553,6 +555,12 @@ class nsHttpTransaction final : public nsAHttpTransaction,
   bool ShouldRestartOn0RttError(nsresult reason);
 
   nsCOMPtr<nsIEarlyHintObserver> 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