Bug 2051415 - Abandon the stale Happy Eyeballs attempt when an upgrade falls back to HTTP/1, r=necko-reviewers,jesup

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

netwerk/protocol/http/ConnectionAttempt.h

@@ -60,6 +60,10 @@ class ConnectionAttempt : public nsSupportsWeakReference {
   void SetAllow1918(bool val) { mAllow1918 = val; }
   bool HasConnected() { return mHasConnected; }
 
+  // Drop our reference to the real transaction without acting on it, so a
+  // following Abandon()'s teardown won't re-queue or close it (bug 2051415).
+  void ForgetRealTransaction() { mTransaction = nullptr; }
+
  protected:
   virtual ~ConnectionAttempt() = default;
 

netwerk/protocol/http/nsHttpConnectionMgr.cpp

@@ -1101,6 +1101,17 @@ bool nsHttpConnectionMgr::DispatchPendingQ(
              "TryDispatchTransaction returning hard error %" PRIx32 "\n",
              static_cast<uint32_t>(rv)));
         if (rv == NS_ERROR_HTTP2_FALLBACK_TO_HTTP1) {
+          // The transaction is about to restart under a re-keyed entry. Abandon
+          // the in-flight attempt it still owns here, or that attempt completes
+          // later and re-queues the restarted transaction (bug 2051415).
+          nsWeakPtr weak =
+              pendingTransInfo->ForgetConnectionAttemptAndActiveConn();
+          if (RefPtr<ConnectionAttempt> attempt = do_QueryReferent(weak)) {
+            // Detach first so the abandoned attempt's teardown won't touch the
+            // transaction we Close()/Restart() below.
+            attempt->ForgetRealTransaction();
+            ent->RemoveConnectionAttempt(attempt, /* abandon = */ true);
+          }
           pendingTransInfo->Transaction()->Close(
               NS_ERROR_HTTP2_FALLBACK_TO_HTTP1);
         }

netwerk/test/gtest/TestHappyEyeballsConnectionAttempt.cpp

@@ -141,7 +141,7 @@ class FakeConnectionEstablisher final : public ConnectionEstablisher {
     mCallback = std::move(aCallback);
     return true;
   }
-  void Close(nsresult) override {}
+  void Close(nsresult) override { ++mCloseCount; }
   void ResetSpeculativeFlags() override {}
   bool IsUDP() const override { return mIsUDP; }
 
@@ -151,6 +151,8 @@ class FakeConnectionEstablisher final : public ConnectionEstablisher {
   }
   void FireError(nsresult aError) { mCallback(Err(aError)); }
 
+  uint32_t mCloseCount = 0;
+
  private:
   ~FakeConnectionEstablisher() = default;
   void Finish(nsresult) override {}
@@ -538,6 +540,78 @@ TEST(HappyEyeballsConnectionAttempt, ProcessTCPConnSkipsRequeueWhenConnected)
   });
 }
 
+// Bug 2051415: on H2->H1 fallback the connection manager abandons the stale
+// in-flight attempt. Verifies the property the fix relies on: Abandon() closes
+// the establisher, so a late success can't fire and re-queue the transaction.
+TEST(HappyEyeballsConnectionAttempt, AbandonClosesInFlightEstablisher)
+{
+  EnsureHttpHandler();
+  RunOnSocketThread([]() {
+    RefPtr<nsHttpConnectionInfo> ci =
+        new nsHttpConnectionInfo("127.0.0.1"_ns, 443, ""_ns, ""_ns, nullptr,
+                                 OriginAttributes(), /*endToEndSSL*/ true);
+    RefPtr<nsHttpTransaction> realTrans = new nsHttpTransaction();
+    // Stand-in for the WebSocket upgrade.
+    realTrans->SetIsForWebTransport(true);
+    nsTHashSet<ConnectionEntry*> pendingQSet;
+
+    RefPtr<ConnectionEntry> entry = new ConnectionEntry(ci, pendingQSet);
+    RefPtr<FakeConnectionEstablisherFactory> factory =
+        new FakeConnectionEstablisherFactory();
+    RefPtr<RecordingConnMgrDelegate> delegate = new RecordingConnMgrDelegate();
+    RefPtr<HappyEyeballsConnectionAttempt> hca =
+        new HappyEyeballsConnectionAttempt(ci, realTrans, /*caps*/ 0,
+                                           /*speculative*/ false,
+                                           /*urgentStart*/ false);
+    hca->SetConnectionEstablisherFactoryForTesting(factory);
+    hca->SetConnMgrDelegateForTesting(delegate);
+    hca->Init(entry);
+    ASSERT_GE(factory->mTCP.Length(), 1u);
+    EXPECT_FALSE(hca->IsTerminal());
+    EXPECT_EQ(factory->mTCP[0]->mCloseCount, 0u)
+        << "the establisher is still in flight before abandonment";
+
+    // The connection manager abandons the stale attempt on H2->H1 fallback.
+    hca->Abandon();
+
+    EXPECT_TRUE(hca->IsTerminal())
+        << "an abandoned attempt is terminal and cannot reach ProcessTCPConn";
+    EXPECT_EQ(factory->mTCP[0]->mCloseCount, 1u)
+        << "abandoning closes the in-flight establisher, so a late success "
+           "can't fire and re-queue the restarted transaction (bug 2051415)";
+    EXPECT_FALSE(realTrans->Closed())
+        << "abandoning the attempt must not close the restarted transaction";
+  });
+}
+
+// Bug 2051415, 0-RTT corner. The manager detaches the transaction
+// (ForgetRealTransaction) before abandoning, so EnterDone's "0-RTT started, no
+// winner" path can't re-queue it (cf. ZeroRttNoWinnerFallback, which does).
+TEST(HappyEyeballsConnectionAttempt,
+     ForgetRealTransactionPreventsAbandonRequeue)
+{
+  EnsureHttpHandler();
+  RunOnSocketThread([]() {
+    TestHarness h(/*caps*/ 0, /*realTransaction*/ true);
+    h.Init();
+    ASSERT_GE(h.mFactory->mTCP.Length(), 1u);
+
+    // A racer entered 0-RTT but never won -- the case EnterDone re-queues.
+    h.mHE->ZeroRttHandleForTesting()->SetAnyStartedForTesting();
+
+    // Manager takes over the transaction, detaching it before abandoning.
+    h.mHE->ForgetRealTransaction();
+    h.mHE->Abandon();
+
+    EXPECT_TRUE(h.mHE->IsTerminal());
+    EXPECT_EQ(h.mDelegate->Count("AddTransaction"), 0)
+        << "a detached, abandoned attempt must not re-queue the transaction "
+           "the connection manager is restarting (bug 2051415)";
+    EXPECT_EQ(h.mFactory->mTCP[0]->mCloseCount, 1u)
+        << "abandoning still closes the in-flight establisher";
+  });
+}
+
 // 0-RTT winner-offset handling: several racers each advance the real
 // transaction's request stream to a different Request0RttStreamOffset (the
 // bytes that racer sent as early data). When one wins via Finish0RTT(accept),