QNAM: More zerocopy changes

The zerocopy download buffer is now QSharedPointer<char>
instead of the QSharedPointer to QVarLengthArray<char>.
This will be a bit leaner to handle by QML and QtWebKit
and does not tie us to a QVLA that much.
Also fix some bugs related to signal emissions and the
return value of bytesAvailable(). Now the behaviour should be
the same if a zerocopy buffer is used or not.

Reviewed-by: Peter Hartmann
Reviewed-by: Thiago Macieira

5 files changed, 209 insertions, 29 deletions

diff --git a/src/network/access/qhttpnetworkconnectionchannel.cpp b/src/network/access/qhttpnetworkconnectionchannel.cpp
index e39f9ed..617602d 100644
--- a/src/network/access/qhttpnetworkconnectionchannel.cpp
+++ b/src/network/access/qhttpnetworkconnectionchannel.cpp
@@ -427,7 +427,8 @@ void QHttpNetworkConnectionChannel::_q_receiveReply()
                replyPrivate->totalProgress += haveRead;
 
                // the user will get notified of it via progress signal
-               emit reply->dataReadProgress(replyPrivate->totalProgress, replyPrivate->bodyLength);
+               if (haveRead > 0)
+                   emit reply->dataReadProgress(replyPrivate->totalProgress, replyPrivate->bodyLength);
            } else if (!replyPrivate->isChunked() && !replyPrivate->autoDecompress
                  && replyPrivate->bodyLength > 0) {
                  // bulk files like images should fulfill these properties and
diff --git a/src/network/access/qnetworkreplyimpl.cpp b/src/network/access/qnetworkreplyimpl.cpp
index 3a629cf..2dbd21c 100644
--- a/src/network/access/qnetworkreplyimpl.cpp
+++ b/src/network/access/qnetworkreplyimpl.cpp
@@ -49,16 +49,12 @@
 #include "QtNetwork/qnetworksession.h"
 #include "qnetworkaccesshttpbackend_p.h"
 #include "qnetworkaccessmanager_p.h"
-#include <QVarLengthArray>
 
 #include <QtCore/QCoreApplication>
 
 QT_BEGIN_NAMESPACE
-typedef QSharedPointer<QVarLengthArray<char, 0> > QVarLengthArraySharedPointer;
-QT_END_NAMESPACE
-Q_DECLARE_METATYPE(QVarLengthArraySharedPointer)
 
-QT_BEGIN_NAMESPACE
+Q_DECLARE_METATYPE(QSharedPointer<char>)
 
 inline QNetworkReplyImplPrivate::QNetworkReplyImplPrivate()
     : backend(0), outgoingData(0), outgoingDataBuffer(0),
@@ -69,7 +65,9 @@ inline QNetworkReplyImplPrivate::QNetworkReplyImplPrivate()
       httpStatusCode(0),
       state(Idle)
       , downloadBuffer(0)
-      , downloadBufferPosition(0)
+      , downloadBufferReadPosition(0)
+      , downloadBufferCurrentSize(0)
+      , downloadBufferMaximumSize(0)
 {
 }
 
@@ -603,6 +601,11 @@ void QNetworkReplyImplPrivate::appendDownstreamData(const QByteArray &data)
     qFatal("QNetworkReplyImplPrivate::appendDownstreamData not implemented");
 }
 
+static void downloadBufferDeleter(char *ptr)
+{
+    delete[] ptr;
+}
+
 char* QNetworkReplyImplPrivate::getDownloadBuffer(qint64 size)
 {
     Q_Q(QNetworkReplyImpl);
@@ -611,12 +614,12 @@ char* QNetworkReplyImplPrivate::getDownloadBuffer(qint64 size)
     if (!downloadBuffer) {
         QVariant bufferAllocationPolicy = request.attribute(QNetworkRequest::MaximumDownloadBufferSizeAttribute);
         if (bufferAllocationPolicy.isValid() && bufferAllocationPolicy.toLongLong() >= size) {
-            downloadBufferArray = QSharedPointer<QVarLengthArray<char, 0> >(new QVarLengthArray<char, 0>());
-            downloadBufferArray->reserve(size);
-
-            downloadBuffer = downloadBufferArray->data();
+            downloadBufferCurrentSize = 0;
+            downloadBufferMaximumSize = size;
+            downloadBuffer = new char[downloadBufferMaximumSize]; // throws if allocation fails
+            downloadBufferPointer = QSharedPointer<char>(downloadBuffer, downloadBufferDeleter);
 
-            q->setAttribute(QNetworkRequest::DownloadBufferAttribute, qVariantFromValue<QSharedPointer<QVarLengthArray<char, 0> > > (downloadBufferArray));
+            q->setAttribute(QNetworkRequest::DownloadBufferAttribute, qVariantFromValue<QSharedPointer<char> > (downloadBufferPointer));
         }
     }
 
@@ -644,12 +647,12 @@ void QNetworkReplyImplPrivate::appendDownstreamDataDownloadBuffer(qint64 bytesRe
     bytesDownloaded = bytesReceived;
     lastBytesDownloaded = bytesReceived;
 
-    // Update the array so our user (e.g. QtWebKit) knows the real size
-    if (bytesReceived > 0)
-        downloadBufferArray->resize(bytesReceived);
+    downloadBufferCurrentSize = bytesReceived;
 
     emit q->downloadProgress(bytesDownloaded, bytesTotal);
-    emit q->readyRead();
+    // Only emit readyRead when actual data is there
+    if (bytesDownloaded > 0)
+        emit q->readyRead();
 }
 
 void QNetworkReplyImplPrivate::finished()
@@ -846,7 +849,7 @@ qint64 QNetworkReplyImpl::bytesAvailable() const
     // Special case for the "zero copy" download buffer
     Q_D(const QNetworkReplyImpl);
     if (d->downloadBuffer) {
-        qint64 maxAvail = d->downloadBufferArray->size() - d->downloadBufferPosition;
+        qint64 maxAvail = d->downloadBufferCurrentSize - d->downloadBufferReadPosition;
         return QNetworkReply::bytesAvailable() + maxAvail;
     }
 
@@ -907,12 +910,12 @@ qint64 QNetworkReplyImpl::readData(char *data, qint64 maxlen)
 
     // Special case code if we have the "zero copy" download buffer
     if (d->downloadBuffer) {
-        qint64 maxAvail = qMin<qint64>(d->downloadBufferArray->size() - d->downloadBufferPosition, maxlen);
+        qint64 maxAvail = qMin<qint64>(d->downloadBufferCurrentSize - d->downloadBufferReadPosition, maxlen);
         if (maxAvail == 0)
             return d->state == QNetworkReplyImplPrivate::Finished ? -1 : 0;
         // FIXME what about "Aborted" state?
-        qMemCopy(data, d->downloadBuffer + d->downloadBufferPosition, maxAvail);
-        d->downloadBufferPosition += maxAvail;
+        qMemCopy(data, d->downloadBuffer + d->downloadBufferReadPosition, maxAvail);
+        d->downloadBufferReadPosition += maxAvail;
         return maxAvail;
     }
 
diff --git a/src/network/access/qnetworkreplyimpl_p.h b/src/network/access/qnetworkreplyimpl_p.h
index 2cb3082..e944601 100644
--- a/src/network/access/qnetworkreplyimpl_p.h
+++ b/src/network/access/qnetworkreplyimpl_p.h
@@ -62,7 +62,7 @@
 #include "QtCore/qbuffer.h"
 #include "private/qringbuffer_p.h"
 #include "private/qbytedata_p.h"
-#include <QVarLengthArray>
+#include <QSharedPointer>
 
 QT_BEGIN_NAMESPACE
 
@@ -206,9 +206,12 @@ public:
     State state;
 
     // only used when the "zero copy" style is used. Else readBuffer is used.
-    QSharedPointer< QVarLengthArray<char, 0> > downloadBufferArray;
+    // Please note that the whole "zero copy" download buffer API is private right now. Do not use it.
+    qint64 downloadBufferReadPosition;
+    qint64 downloadBufferCurrentSize;
+    qint64 downloadBufferMaximumSize;
+    QSharedPointer<char> downloadBufferPointer;
     char* downloadBuffer;
-    qint64 downloadBufferPosition;
 
     Q_DECLARE_PUBLIC(QNetworkReplyImpl)
 };
diff --git a/tests/auto/qnetworkreply/tst_qnetworkreply.cpp b/tests/auto/qnetworkreply/tst_qnetworkreply.cpp
index 2ad4060..f7f0519 100644
--- a/tests/auto/qnetworkreply/tst_qnetworkreply.cpp
+++ b/tests/auto/qnetworkreply/tst_qnetworkreply.cpp
@@ -75,8 +75,7 @@
 
 #include "../network-settings.h"
 
-typedef QSharedPointer<QVarLengthArray<char, 0> > QVarLengthArraySharedPointer;
-Q_DECLARE_METATYPE(QVarLengthArraySharedPointer)
+Q_DECLARE_METATYPE(QSharedPointer<char>)
 Q_DECLARE_METATYPE(QNetworkReply*)
 Q_DECLARE_METATYPE(QAuthenticator*)
 Q_DECLARE_METATYPE(QNetworkProxy)
@@ -292,6 +291,8 @@ private Q_SLOTS:
 
     void getFromHttpIntoBuffer_data();
     void getFromHttpIntoBuffer();
+    void getFromHttpIntoBuffer2_data();
+    void getFromHttpIntoBuffer2();
 
     void ioGetFromHttpWithoutContentLength();
 
@@ -4297,6 +4298,7 @@ void tst_QNetworkReply::getFromHttpIntoBuffer_data()
     QTest::newRow("rfc-internal") << QUrl("http://" + QtNetworkSettings::serverName() + "/qtest/rfc3252.txt");
 }
 
+// Please note that the whole "zero copy" download buffer API is private right now. Do not use it.
 void tst_QNetworkReply::getFromHttpIntoBuffer()
 {
     QFETCH(QUrl, url);
@@ -4319,9 +4321,11 @@ void tst_QNetworkReply::getFromHttpIntoBuffer()
 
     // Compare the memory buffer
     QVariant downloadBufferAttribute = reply->attribute(QNetworkRequest::DownloadBufferAttribute);
+    QVERIFY(downloadBufferAttribute.isValid());
+    QSharedPointer<char> sharedPointer = downloadBufferAttribute.value<QSharedPointer<char> >();
     bool memoryComparison =
             (0 == memcmp(static_cast<void*>(reference.readAll().data()),
-                         downloadBufferAttribute.value<QSharedPointer<QVarLengthArray<char, 0> > >()->constData(), reference.size()));
+                         sharedPointer.data(), reference.size()));
     QVERIFY(memoryComparison);
 
     // Make sure the normal reading works
@@ -4335,6 +4339,174 @@ void tst_QNetworkReply::getFromHttpIntoBuffer()
     QVERIFY(reply->atEnd());
 }
 
+// FIXME we really need to consolidate all those server implementations
+class GetFromHttpIntoBuffer2Server : QObject {
+    Q_OBJECT;
+    qint64 dataSize;
+    qint64 dataSent;
+    QTcpServer server;
+    QTcpSocket *client;
+    bool serverSendsContentLength;
+    bool chunkedEncoding;
+
+public:
+    GetFromHttpIntoBuffer2Server (qint64 ds, bool sscl, bool ce) : dataSize(ds), dataSent(0),
+    client(0), serverSendsContentLength(sscl), chunkedEncoding(ce) {
+        server.listen();
+        connect(&server, SIGNAL(newConnection()), this, SLOT(newConnectionSlot()));
+    }
+
+    int serverPort() {
+        return server.serverPort();
+    }
+
+public slots:
+
+    void newConnectionSlot() {
+        client = server.nextPendingConnection();
+        client->setParent(this);
+        connect(client, SIGNAL(readyRead()), this, SLOT(readyReadSlot()));
+        connect(client, SIGNAL(bytesWritten(qint64)), this, SLOT(bytesWrittenSlot(qint64)));
+    }
+
+    void readyReadSlot() {
+        client->readAll();
+        client->write("HTTP/1.0 200 OK\n");
+        if (serverSendsContentLength)
+            client->write(QString("Content-Length: " + QString::number(dataSize) + "\n").toAscii());
+        if (chunkedEncoding)
+            client->write(QString("Transfer-Encoding: chunked\n").toAscii());
+        client->write("Connection: close\n\n");
+    }
+
+    void bytesWrittenSlot(qint64 amount) {
+        Q_UNUSED(amount);
+        if (dataSent == dataSize && client) {
+            // close eventually
+
+            // chunked encoding: we have to send a last "empty" chunk
+            if (chunkedEncoding)
+                client->write(QString("0\r\n\r\n").toAscii());
+
+            client->disconnectFromHost();
+            server.close();
+            client = 0;
+            return;
+        }
+
+        // send data
+        if (client && client->bytesToWrite() < 100*1024 && dataSent < dataSize) {
+            qint64 amount = qMin(qint64(16*1024), dataSize - dataSent);
+            QByteArray data(amount, '@');
+
+            if (chunkedEncoding) {
+                client->write(QString(QString("%1").arg(amount,0,16).toUpper() + "\r\n").toAscii());
+                client->write(data.constData(), amount);
+                client->write(QString("\r\n").toAscii());
+            } else {
+                client->write(data.constData(), amount);
+            }
+
+            dataSent += amount;
+        }
+    }
+};
+
+class GetFromHttpIntoBuffer2Client : QObject {
+    Q_OBJECT
+private:
+    bool useDownloadBuffer;
+    QNetworkReply *reply;
+    qint64 uploadSize;
+    QList<qint64> bytesAvailableList;
+public:
+    GetFromHttpIntoBuffer2Client (QNetworkReply *reply, bool useDownloadBuffer, qint64 uploadSize)
+        : useDownloadBuffer(useDownloadBuffer), reply(reply), uploadSize(uploadSize)
+    {
+        connect(reply, SIGNAL(metaDataChanged()), this, SLOT(metaDataChangedSlot()));
+        connect(reply, SIGNAL(readyRead()), this, SLOT(readyReadSlot()));
+        connect(reply, SIGNAL(finished()), this, SLOT(finishedSlot()));
+    }
+
+    public slots:
+    void metaDataChangedSlot() {
+        if (useDownloadBuffer) {
+            QSharedPointer<char> sharedPointer = qvariant_cast<QSharedPointer<char> >(reply->attribute(QNetworkRequest::DownloadBufferAttribute));
+            QVERIFY(!sharedPointer.isNull()); // It will be 0 if it failed
+        }
+
+        // metaDataChanged needs to come before everything else
+        QVERIFY(bytesAvailableList.isEmpty());
+    }
+
+    void readyReadSlot() {
+        QVERIFY(!reply->isFinished());
+
+        qint64 bytesAvailable = reply->bytesAvailable();
+
+        // bytesAvailable must never be 0
+        QVERIFY(bytesAvailable != 0);
+
+        if (bytesAvailableList.length() < 5) {
+            // We assume that the first few times the bytes available must be less than the complete size, e.g.
+            // the bytesAvailable() function works correctly in case of a downloadBuffer.
+            QVERIFY(bytesAvailable < uploadSize);
+        }
+        if (!bytesAvailableList.isEmpty()) {
+            // Also check that the same bytesAvailable is not coming twice in a row
+            QVERIFY(bytesAvailableList.last() != bytesAvailable);
+        }
+
+        bytesAvailableList.append(bytesAvailable);
+        // Add bytesAvailable to a list an parse
+    }
+
+    void finishedSlot() {
+        // We should have already received all readyRead
+        QVERIFY(bytesAvailableList.last() == uploadSize);
+    }
+};
+
+void tst_QNetworkReply::getFromHttpIntoBuffer2_data()
+{
+    QTest::addColumn<bool>("useDownloadBuffer");
+
+    QTest::newRow("use-download-buffer") << true;
+    QTest::newRow("do-not-use-download-buffer") << false;
+}
+
+// This test checks mostly that signal emissions are in correct order
+// Please note that the whole "zero copy" download buffer API is private right now. Do not use it.
+void tst_QNetworkReply::getFromHttpIntoBuffer2()
+{
+    QFETCH(bool, useDownloadBuffer);
+
+    // On my Linux Desktop the results are already visible with 128 kB, however we use this to have good results.
+#if defined(Q_OS_SYMBIAN) || defined(Q_WS_WINCE_WM)
+    // Show some mercy to non-desktop platform/s
+    enum {UploadSize = 4*1024*1024}; // 4 MB
+#else
+    enum {UploadSize = 32*1024*1024}; // 32 MB
+#endif
+
+    GetFromHttpIntoBuffer2Server server(UploadSize, true, false);
+
+    QNetworkRequest request(QUrl("http://127.0.0.1:" + QString::number(server.serverPort()) + "/?bare=1"));
+    if (useDownloadBuffer)
+        request.setAttribute(QNetworkRequest::MaximumDownloadBufferSizeAttribute, 1024*1024*128); // 128 MB is max allowed
+
+    QNetworkAccessManager manager;
+    QNetworkReplyPtr reply = manager.get(request);
+
+    GetFromHttpIntoBuffer2Client client(reply, useDownloadBuffer, UploadSize);
+
+    connect(reply, SIGNAL(finished()), &QTestEventLoop::instance(), SLOT(exitLoop()), Qt::QueuedConnection);
+    QTestEventLoop::instance().enterLoop(40);
+    QCOMPARE(reply->error(), QNetworkReply::NoError);
+    QVERIFY(!QTestEventLoop::instance().timeout());
+}
+
+
 // Is handled somewhere else too, introduced this special test to have it more accessible
 void tst_QNetworkReply::ioGetFromHttpWithoutContentLength()
 {
diff --git a/tests/benchmarks/network/access/qnetworkreply/tst_qnetworkreply.cpp b/tests/benchmarks/network/access/qnetworkreply/tst_qnetworkreply.cpp
index 0098d8e..f75d681 100644
--- a/tests/benchmarks/network/access/qnetworkreply/tst_qnetworkreply.cpp
+++ b/tests/benchmarks/network/access/qnetworkreply/tst_qnetworkreply.cpp
@@ -51,8 +51,7 @@
 #include "../../../../auto/network-settings.h"
 
 
-typedef QSharedPointer<QVarLengthArray<char, 0> > QVarLengthArraySharedPointer;
-Q_DECLARE_METATYPE(QVarLengthArraySharedPointer)
+Q_DECLARE_METATYPE(QSharedPointer<char>)
 
 class TimedSender: public QThread
 {
@@ -661,6 +660,7 @@ private:
     bool useDownloadBuffer;
     QNetworkReply *reply;
     qint64 uploadSize;
+    QList<qint64> bytesAvailableList;
 public:
     HttpDownloadPerformanceClientDownloadBuffer (QNetworkReply *reply, bool useDownloadBuffer, qint64 uploadSize)
         : useDownloadBuffer(useDownloadBuffer), reply(reply), uploadSize(uploadSize)
@@ -672,7 +672,7 @@ public:
     void finishedSlot() {
         if (useDownloadBuffer) {
             QVariant downloadBufferAttribute = reply->attribute(QNetworkRequest::DownloadBufferAttribute);
-            QSharedPointer<QVarLengthArray<char, 0> > data = downloadBufferAttribute.value<QSharedPointer<QVarLengthArray<char, 0> > >();
+            QSharedPointer<char> data = downloadBufferAttribute.value<QSharedPointer<char> >();
         } else {
             // We did not have a download buffer but we still need to benchmark having the data, e.g. reading it all.
             char* replyData = (char*) qMalloc(uploadSize);
@@ -690,6 +690,7 @@ void tst_qnetworkreply::httpDownloadPerformanceDownloadBuffer_data()
     QTest::newRow("do-not-use-download-buffer") << false;
 }
 
+// Please note that the whole "zero copy" download buffer API is private right now. Do not use it.
 void tst_qnetworkreply::httpDownloadPerformanceDownloadBuffer()
 {
     QFETCH(bool, useDownloadBuffer);