From 44e53b3ed9fc938267b492cae30e97896dbf3bb9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= <marten.nordheim@qt.io>
Date: Wed, 4 Oct 2023 17:54:33 +0200
Subject: [PATCH] http2: When a reply is removed from the queue, only remove
 one

We were using the .remove(Key) API on the map instead of
erase(iterator), so we were removing any reply of the same priority that
had not yet been popped from the queues.

Rewrote to drop loop and only work with iterators.
This issue was there since SPDY days, so not picking all the way back to
5.15, where HTTP2 anyway is not enabled by default.

As a drive-by, drop the #ifndef QT_NO_SSL, which was also there from
SPDY times, which was TLS-only.

Pick-to: 6.5 6.2
Fixes: QTBUG-116167
Change-Id: Id7e1eb311e009b86054c1fe3d049c760d711a18a
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
(cherry picked from commit 13c4e11c49d16d6a83a142215d28e1ec660e1bba)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
---
 src/network/access/qhttpnetworkconnection.cpp | 21 ++++----
 tests/auto/network/access/http2/tst_http2.cpp | 49 +++++++++++++++++++
 2 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/src/network/access/qhttpnetworkconnection.cpp b/src/network/access/qhttpnetworkconnection.cpp
index 46baaa37456..f183a6b7191 100644
--- a/src/network/access/qhttpnetworkconnection.cpp
+++ b/src/network/access/qhttpnetworkconnection.cpp
@@ -988,19 +988,18 @@ void QHttpNetworkConnectionPrivate::removeReply(QHttpNetworkReply *reply)
                return;
             }
         }
-#ifndef QT_NO_SSL
         // is the reply inside the H2 pipeline of this channel already?
-        QMultiMap<int, HttpMessagePair>::iterator it = channels[i].h2RequestsToSend.begin();
-        QMultiMap<int, HttpMessagePair>::iterator end = channels[i].h2RequestsToSend.end();
-        for (; it != end; ++it) {
-            if (it.value().second == reply) {
-                channels[i].h2RequestsToSend.remove(it.key());
-
-                QMetaObject::invokeMethod(q, "_q_startNextRequest", Qt::QueuedConnection);
-                return;
-            }
+        const auto foundReply = [reply](const HttpMessagePair &pair) {
+            return pair.second == reply;
+        };
+        auto &seq = channels[i].h2RequestsToSend;
+        const auto end = seq.cend();
+        auto it = std::find_if(seq.cbegin(), end, foundReply);
+        if (it != end) {
+            seq.erase(it);
+            QMetaObject::invokeMethod(q, "_q_startNextRequest", Qt::QueuedConnection);
+            return;
         }
-#endif
     }
     // remove from the high priority queue
     if (!highPriorityQueue.isEmpty()) {
diff --git a/tests/auto/network/access/http2/tst_http2.cpp b/tests/auto/network/access/http2/tst_http2.cpp
index bcb94ac439d..971568ea144 100644
--- a/tests/auto/network/access/http2/tst_http2.cpp
+++ b/tests/auto/network/access/http2/tst_http2.cpp
@@ -101,6 +101,8 @@ private slots:
 
     void trailingHEADERS();
 
+    void duplicateRequestsWithAborts();
+
 protected slots:
     // Slots to listen to our in-process server:
     void serverStarted(quint16 port);
@@ -1318,6 +1320,53 @@ void tst_Http2::trailingHEADERS()
     QTRY_VERIFY(serverGotSettingsACK);
 }
 
+void tst_Http2::duplicateRequestsWithAborts()
+{
+    clearHTTP2State();
+    serverPort = 0;
+
+    ServerPtr targetServer(newServer(defaultServerSettings, defaultConnectionType()));
+
+    QMetaObject::invokeMethod(targetServer.data(), "startServer", Qt::QueuedConnection);
+    runEventLoop();
+
+    QVERIFY(serverPort != 0);
+
+    constexpr int ExpectedSuccessfulRequests = 1;
+    nRequests = ExpectedSuccessfulRequests;
+
+    const auto url = requestUrl(defaultConnectionType());
+    QNetworkRequest request(url);
+    // H2C might be used on macOS where SecureTransport doesn't support server-side ALPN
+    request.setAttribute(QNetworkRequest::Http2CleartextAllowedAttribute, true);
+
+    qint32 finishedCount = 0;
+    auto connectToSlots = [this, &finishedCount](QNetworkReply *reply){
+        const auto onFinished = [&finishedCount, reply, this]() {
+            ++finishedCount;
+            if (reply->error() == QNetworkReply::NoError)
+                replyFinished();
+        };
+        connect(reply, &QNetworkReply::finished, reply, onFinished);
+    };
+
+    std::vector<QNetworkReply *> replies;
+    for (qint32 i = 0; i < 3; ++i) {
+        auto &reply = replies.emplace_back(manager->get(request));
+        connectToSlots(reply);
+        if (i < 2) // Delete and abort all-but-one:
+            reply->deleteLater();
+        // Since we're using self-signed certificates, ignore SSL errors:
+        reply->ignoreSslErrors();
+    }
+
+    runEventLoop();
+    STOP_ON_FAILURE
+
+    QCOMPARE(nRequests, 0);
+    QCOMPARE(finishedCount, ExpectedSuccessfulRequests);
+}
+
 void tst_Http2::serverStarted(quint16 port)
 {
     serverPort = port;