Bug 1715029 - Keep the transaction alive when we failed to process it, r=necko-reviewers,dragana

Differential Revision: https://phabricator.services.mozilla.com/D119171

netwerk/protocol/http/nsHttpConnectionMgr.cpp

@@ -3457,12 +3457,11 @@ void nsHttpConnectionMgr::MoveToWildCardConnEntry(
   ent->MoveConnection(proxyConn, wcEnt);
 }
 
-bool nsHttpConnectionMgr::MoveTransToNewConnEntry(
-    nsHttpTransaction* aTrans, nsHttpConnectionInfo* aNewCI) {
+bool nsHttpConnectionMgr::RemoveTransFromConnEntry(nsHttpTransaction* aTrans) {
   MOZ_ASSERT(OnSocketThread(), "not on socket thread");
 
-  LOG(("nsHttpConnectionMgr::MoveTransToNewConnEntry: trans=%p aNewCI=%s",
-       aTrans, aNewCI->HashKey().get()));
+  LOG(("nsHttpConnectionMgr::RemoveTransFromConnEntry: trans=%p ci=%s", aTrans,
+       aTrans->ConnectionInfo()->HashKey().get()));
 
   // Step 1: Get the transaction's connection entry.
   ConnectionEntry* entry = mCT.GetWeak(aTrans->ConnectionInfo()->HashKey());
@@ -3471,14 +3470,7 @@ bool nsHttpConnectionMgr::MoveTransToNewConnEntry(
   }
 
   // Step 2: Try to find the undispatched transaction.
-  if (!entry->RemoveTransFromPendingQ(aTrans)) {
-    return false;
-  }
-
-  // Step 3: Add the transaction.
-  aTrans->UpdateConnectionInfo(aNewCI);
-  Unused << ProcessNewTransaction(aTrans);
-  return true;
+  return entry->RemoveTransFromPendingQ(aTrans);
 }
 
 void nsHttpConnectionMgr::IncreaseNumDnsAndConnectSockets() {

netwerk/protocol/http/nsHttpConnectionMgr.h

@@ -69,11 +69,12 @@ class nsHttpConnectionMgr final : public HttpConnectionMgrShell,
                                nsHttpConnectionInfo* wildcardCI,
                                HttpConnectionBase* conn);
 
-  // Move a transaction from the pendingQ of it's connection entry to another
-  // one. Returns true if the transaction is moved successfully, otherwise
-  // returns false.
-  bool MoveTransToNewConnEntry(nsHttpTransaction* aTrans,
-                               nsHttpConnectionInfo* aNewCI);
+  // 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);
+
+  // Directly dispatch the transaction or insert it in to the pendingQ.
+  [[nodiscard]] nsresult ProcessNewTransaction(nsHttpTransaction* aTrans);
 
   // This is used to force an idle connection to be closed and removed from
   // the idle connection list. It is called when the idle connection detects
@@ -260,7 +261,6 @@ class nsHttpConnectionMgr final : public HttpConnectionMgrShell,
                                                      uint32_t,
                                                      HttpConnectionBase*,
                                                      int32_t);
-  [[nodiscard]] nsresult ProcessNewTransaction(nsHttpTransaction*);
   [[nodiscard]] nsresult EnsureSocketThreadTarget();
   void ReportProxyTelemetry(ConnectionEntry* ent);
   void StartedConnect();

netwerk/protocol/http/nsHttpTransaction.cpp

@@ -3026,6 +3026,8 @@ nsresult nsHttpTransaction::OnHTTPSRRAvailable(
     return NS_OK;
   }
 
+  RefPtr<nsHttpTransaction> deleteProtector(this);
+
   uint32_t receivedStage = HTTPSSVC_NO_USABLE_RECORD;
   // Make sure we set the correct value to |mHTTPSSVCReceivedStage|, since we
   // also use this value to indicate whether HTTPS RR is used or not.
@@ -3094,13 +3096,22 @@ nsresult nsHttpTransaction::OnHTTPSRRAvailable(
   RefPtr<nsHttpConnectionInfo> newInfo =
       mConnInfo->CloneAndAdoptHTTPSSVCRecord(svcbRecord);
   bool needFastFallback = newInfo->IsHttp3();
-  if (!gHttpHandler->ConnMgr()->MoveTransToNewConnEntry(this, newInfo)) {
-    // MoveTransToNewConnEntry() returning fail means this transaction is
-    // not in the connection entry's pending queue. This could happen if
-    // OnLookupComplete() is called before this transaction is added in the
-    // queue. We still need to update the connection info, so this transaction
-    // can be added to the right connection entry.
-    UpdateConnectionInfo(newInfo);
+  bool foundInPendingQ =
+      gHttpHandler->ConnMgr()->RemoveTransFromConnEntry(this);
+
+  // Adopt the new connection info, so this transaction will be added into the
+  // new connection entry.
+  UpdateConnectionInfo(newInfo);
+
+  // If this transaction is sucessfully removed from a connection entry, we call
+  // ProcessNewTransaction to process it immediately.
+  // If not, this means that nsHttpTransaction::OnHTTPSRRAvailable happens
+  // before ProcessNewTransaction and this transaction will be processed later.
+  if (foundInPendingQ) {
+    if (NS_FAILED(gHttpHandler->ConnMgr()->ProcessNewTransaction(this))) {
+      LOG(("Failed to process this transaction."));
+      return NS_ERROR_FAILURE;
+    }
   }
 
   // In case we already have mHttp3BackupTimer, cancel it.
@@ -3294,7 +3305,7 @@ void nsHttpTransaction::HandleFallback(
        aFallbackConnInfo->HashKey().get()));
 
   bool foundInPendingQ =
-      gHttpHandler->ConnMgr()->MoveTransToNewConnEntry(this, aFallbackConnInfo);
+      gHttpHandler->ConnMgr()->RemoveTransFromConnEntry(this);
   if (!foundInPendingQ) {
     MOZ_ASSERT(false, "transaction not in entry");
     return;
@@ -3305,6 +3316,9 @@ void nsHttpTransaction::HandleFallback(
   if (seekable) {
     seekable->Seek(nsISeekableStream::NS_SEEK_SET, 0);
   }
+
+  UpdateConnectionInfo(aFallbackConnInfo);
+  Unused << gHttpHandler->ConnMgr()->ProcessNewTransaction(this);
 }
 
 NS_IMETHODIMP