QNetworkReply: Fix missing final emission of readyRead

If we receive compressed data we will decompress it as it comes in,
in some cases the final byte we receive doesn't actually contribute
to the final output. If this byte is handled by itself then, when
combined with QNetworkReply's signal compression, we ended up not
emitting the readyRead signal for any of the data we received while
compressing the signals.

So, instead of relying on checking difference in bytes downloaded
for each batch of data received we keep track of how much data
we had received the last time we emitted readyRead.

Fixes: QTBUG-106354
Change-Id: I4777be29b46bf0de968667e9de9d975c735ac6e6
Reviewed-by: Konrad Kujawa <konrad.kujawa@qt.io>
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
(cherry picked from commit 5387b88aa96d2096a3423190d1f1e5bdd2c52052)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

3 files changed, 55 insertions, 4 deletions

diff --git a/src/network/access/qnetworkreplyhttpimpl.cpp b/src/network/access/qnetworkreplyhttpimpl.cpp
index 08ef1866bd..e609653aa4 100644
--- a/src/network/access/qnetworkreplyhttpimpl.cpp
+++ b/src/network/access/qnetworkreplyhttpimpl.cpp
@@ -1035,9 +1035,6 @@ void QNetworkReplyHttpImplPrivate::replyDownloadData(QByteArray d)
 
     // cache this, we need it later and it's invalidated when dealing with compressed data
     auto dataSize = d.size();
-    // Grab this to compare later (only relevant for compressed data) in case none of the data
-    // will be propagated to the user
-    const qint64 previousBytesDownloaded = bytesDownloaded;
 
     if (cacheEnabled && isCachingAllowed() && !cacheSaveDevice)
         initCacheSaveDevice();
@@ -1121,11 +1118,12 @@ void QNetworkReplyHttpImplPrivate::replyDownloadData(QByteArray d)
 
     // This can occur when downloading compressed data as some of the data may be the content
     // encoding's header. Don't emit anything for this.
-    if (previousBytesDownloaded == bytesDownloaded) {
+    if (lastReadyReadEmittedSize == bytesDownloaded) {
         if (readBufferMaxSize)
             emit q->readBufferFreed(dataSize);
         return;
     }
+    lastReadyReadEmittedSize = bytesDownloaded;
 
     QVariant totalSize = cookedHeaders.value(QNetworkRequest::ContentLengthHeader);
 
diff --git a/src/network/access/qnetworkreplyhttpimpl_p.h b/src/network/access/qnetworkreplyhttpimpl_p.h
index 55953ae878..11897d1420 100644
--- a/src/network/access/qnetworkreplyhttpimpl_p.h
+++ b/src/network/access/qnetworkreplyhttpimpl_p.h
@@ -200,6 +200,10 @@ public:
 
     qint64 bytesDownloaded;
     qint64 bytesBuffered;
+    // We use this to keep track of whether or not we need to emit readyRead
+    // when we deal with signal compression (delaying emission) + decompressing
+    // data (potentially receiving bytes that don't end up in the final output):
+    qint64 lastReadyReadEmittedSize = 0;
 
     QTimer *transferTimeout;
 
diff --git a/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp b/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp
index f97fe88fdd..b4fbee8594 100644
--- a/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp
+++ b/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp
@@ -511,6 +511,7 @@ private Q_SLOTS:
     void downloadProgressWithContentEncoding();
     void contentEncodingError_data();
     void contentEncodingError();
+    void compressedReadyRead();
 
     // NOTE: This test must be last!
     void parentingRepliesToTheApp();
@@ -9880,6 +9881,54 @@ void tst_QNetworkReply::contentEncodingError()
     QTEST(reply->error(), "expectedError");
 }
 
+// When this test is failing it will appear flaky because it relies on the
+// timing of delivery from one socket to another in the OS.
+// + we have to send all the data at once, so the readyRead emissions are
+// compressed into a single emission, so we cannot artificially time it with
+// waits and sleeps.
+void tst_QNetworkReply::compressedReadyRead()
+{
+    // There were historically an issue where a mix of signal compression and
+    // data decompression made it so we accidentally didn't emit the final
+    // readyRead signal before emitting finished(). Test this here to make sure
+    // it happens:
+    const QByteArray gzipPayload =
+            QByteArray::fromBase64("H4sIAAAAAAAAA8tIzcnJVyjPL8pJAQCFEUoNCwAAAA==");
+    const QByteArray expected = "hello world";
+
+    QString header("HTTP/1.0 200 OK\r\nContent-Encoding: gzip\r\nContent-Length: %1\r\n\r\n");
+    header = header.arg(gzipPayload.size());
+    MiniHttpServer server(header.toLatin1()); // only send header automatically
+    server.doClose = false; // don't close and delete client socket right away
+
+    QNetworkRequest request(
+            QUrl(QLatin1String("http://localhost:%1").arg(QString::number(server.serverPort()))));
+    QNetworkReplyPtr reply(manager.get(request));
+
+    QObject::connect(reply.get(), &QNetworkReply::metaDataChanged, reply.get(),
+                     [&server, &gzipPayload]() {
+                         // Client received headers, now send data:
+                         // We do this awkward write,flush,write dance to try to
+                         // make sure the data does not all arrive at the same
+                         // time. By design we send the final "=" byte by itself
+                         qsizetype boundary = gzipPayload.size() - 1;
+                         server.client->write(gzipPayload.sliced(0, boundary));
+                         server.client->flush();
+                         // Let the server take care of deleting the client once
+                         // the rest of the data is written:
+                         server.doClose = true;
+                         server.client->write(gzipPayload.sliced(boundary));
+                     });
+
+    QByteArray received;
+    QObject::connect(reply.get(), &QNetworkReply::readyRead, reply.get(),
+                     [reply = reply.get(), &received]() {
+                         received += reply->readAll();
+                     });
+    QTRY_VERIFY(reply->isFinished());
+    QCOMPARE(received, expected);
+}
+
 // NOTE: This test must be last testcase in tst_qnetworkreply!
 void tst_QNetworkReply::parentingRepliesToTheApp()
 {