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.6 6.5 6.2
Fixes: QTBUG-116167
Change-Id: Id7e1eb311e009b86054c1fe3d049c760d711a18a
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>

2 files changed, 59 insertions, 11 deletions

diff --git a/src/network/access/qhttpnetworkconnection.cpp b/src/network/access/qhttpnetworkconnection.cpp
index 74034ebcd3..8a7a6989f4 100644
--- a/src/network/access/qhttpnetworkconnection.cpp
+++ b/src/network/access/qhttpnetworkconnection.cpp
@@ -990,19 +990,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 72db98e911..887f9b5a19 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;