Stabilize tst_qwebenginedownloads

The waitForRequest/waitForSignal function used by these tests is broken: it
assumes that calling QEventLoop::exit() will immediately exit from the event
loop. In actuality this is not immediate and the event loop may continue to
execute more signals handlers even after exit() has been called. This means that
in e.g. waitForRequest the 'result' variable may be assigned to twice.

Additionally there is a race condition in downloadTwoLinks, where we sometimes
skip the first download. This is not a bug but simply one pending navigation
being aborted in favor of another.

Changes

  - Delete waitForRequest. Define one HTTP request handler per test, using state
    variables to communicate with the main body of the test. Ignore unknown
    requests (including favicon requests). Same for downloadRequested signals.

  - Expand downloadTwoLinks and fix expectations.

  - Add logging to HTTPServer.

  - Unblacklist.

Task-number: QTBUG-66888
Change-Id: I718cac6c4b32a8cc68400fa8ee5b853686c77fcb
Reviewed-by: Peter Varga <pvarga@inf.u-szeged.hu>

5 files changed, 176 insertions, 150 deletions

diff --git a/tests/auto/shared/httpreqrep.cpp b/tests/auto/shared/httpreqrep.cpp
index eb2db6890..15a86631c 100644
--- a/tests/auto/shared/httpreqrep.cpp
+++ b/tests/auto/shared/httpreqrep.cpp
@@ -31,11 +31,14 @@ HttpReqRep::HttpReqRep(QTcpSocket *socket, QObject *parent)
     : QObject(parent), m_socket(socket)
 {
     m_socket->setParent(this);
-    connect(m_socket, &QIODevice::readyRead, this, &HttpReqRep::handleReadyRead);
+    connect(m_socket, &QTcpSocket::readyRead, this, &HttpReqRep::handleReadyRead);
+    connect(m_socket, &QTcpSocket::disconnected, this, &HttpReqRep::handleDisconnected);
 }
 
 void HttpReqRep::sendResponse()
 {
+    if (m_state != State::REQUEST_RECEIVED)
+        return;
     m_socket->write("HTTP/1.1 ");
     m_socket->write(QByteArray::number(m_responseStatusCode));
     m_socket->write(" OK?\r\n");
@@ -45,9 +48,21 @@ void HttpReqRep::sendResponse()
         m_socket->write(kv.second);
         m_socket->write("\r\n");
     }
+    m_socket->write("Connection: close\r\n");
     m_socket->write("\r\n");
     m_socket->write(m_responseBody);
-    m_socket->close();
+    m_state = State::DISCONNECTING;
+    m_socket->disconnectFromHost();
+    Q_EMIT responseSent();
+}
+
+void HttpReqRep::close()
+{
+    if (m_state != State::REQUEST_RECEIVED)
+        return;
+    m_state = State::DISCONNECTING;
+    m_socket->disconnectFromHost();
+    Q_EMIT error(QStringLiteral("missing response"));
 }
 
 QByteArray HttpReqRep::requestHeader(const QByteArray &key) const
@@ -60,32 +75,60 @@ QByteArray HttpReqRep::requestHeader(const QByteArray &key) const
 
 void HttpReqRep::handleReadyRead()
 {
-    const auto requestLine = m_socket->readLine();
-    const auto requestLineParts = requestLine.split(' ');
-    if (requestLineParts.size() != 3 || !requestLineParts[2].toUpper().startsWith("HTTP/")) {
-        qWarning("HttpReqRep: invalid request line");
-        Q_EMIT readFinished(false);
-        return;
-    }
-
-    decltype(m_requestHeaders) headers;
-    for (;;) {
-        const auto headerLine = m_socket->readLine();
-        if (headerLine == QByteArrayLiteral("\r\n"))
+    while (m_socket->canReadLine()) {
+        switch (m_state) {
+        case State::RECEIVING_REQUEST: {
+            const auto requestLine = m_socket->readLine();
+            const auto requestLineParts = requestLine.split(' ');
+            if (requestLineParts.size() != 3 || !requestLineParts[2].toUpper().startsWith("HTTP/")) {
+                m_state = State::DISCONNECTING;
+                m_socket->disconnectFromHost();
+                Q_EMIT error(QStringLiteral("invalid request line"));
+                return;
+            }
+            m_requestMethod = requestLineParts[0];
+            m_requestPath = requestLineParts[1];
+            m_state = State::RECEIVING_HEADERS;
+            break;
+        }
+        case State::RECEIVING_HEADERS: {
+            const auto headerLine = m_socket->readLine();
+            if (headerLine == QByteArrayLiteral("\r\n")) {
+                m_state = State::REQUEST_RECEIVED;
+                Q_EMIT requestReceived();
+                return;
+            }
+            int colonIndex = headerLine.indexOf(':');
+            if (colonIndex < 0) {
+                m_state = State::DISCONNECTING;
+                m_socket->disconnectFromHost();
+                Q_EMIT error(QStringLiteral("invalid header line"));
+                return;
+            }
+            auto headerKey = headerLine.left(colonIndex).trimmed().toLower();
+            auto headerValue = headerLine.mid(colonIndex + 1).trimmed().toLower();
+            m_requestHeaders.emplace(headerKey, headerValue);
             break;
-        int colonIndex = headerLine.indexOf(':');
-        if (colonIndex < 0) {
-            qWarning("HttpReqRep: invalid header line");
-            Q_EMIT readFinished(false);
+        }
+        default:
             return;
         }
-        auto headerKey = headerLine.left(colonIndex).trimmed().toLower();
-        auto headerValue = headerLine.mid(colonIndex + 1).trimmed().toLower();
-        headers.emplace(headerKey, headerValue);
     }
+}
 
-    m_requestMethod = requestLineParts[0];
-    m_requestPath = requestLineParts[1];
-    m_requestHeaders = headers;
-    Q_EMIT readFinished(true);
+void HttpReqRep::handleDisconnected()
+{
+    switch (m_state) {
+    case State::RECEIVING_REQUEST:
+    case State::RECEIVING_HEADERS:
+    case State::REQUEST_RECEIVED:
+        Q_EMIT error(QStringLiteral("unexpected disconnect"));
+        break;
+    case State::DISCONNECTING:
+        break;
+    case State::DISCONNECTED:
+        Q_UNREACHABLE();
+    }
+    m_state = State::DISCONNECTED;
+    Q_EMIT closed();
 }
diff --git a/tests/auto/shared/httpreqrep.h b/tests/auto/shared/httpreqrep.h
index 4e9f10dff..bee8119eb 100644
--- a/tests/auto/shared/httpreqrep.h
+++ b/tests/auto/shared/httpreqrep.h
@@ -30,6 +30,7 @@
 
 #include <QTcpSocket>
 
+#include <map>
 #include <utility>
 
 // Represents an HTTP request-response exchange.
@@ -37,11 +38,20 @@ class HttpReqRep : public QObject
 {
     Q_OBJECT
 public:
-    HttpReqRep(QTcpSocket *socket, QObject *parent = nullptr);
+    explicit HttpReqRep(QTcpSocket *socket, QObject *parent = nullptr);
+
     void sendResponse();
+    void close();
+
+    // Request parameters (only valid after requestReceived())
+
     QByteArray requestMethod() const { return m_requestMethod; }
     QByteArray requestPath() const { return m_requestPath; }
     QByteArray requestHeader(const QByteArray &key) const;
+
+    // Response parameters (can be set until sendResponse()/close()).
+
+    int responseStatus() const { return m_responseStatusCode; }
     void setResponseStatus(int statusCode)
     {
         m_responseStatusCode = statusCode;
@@ -50,6 +60,7 @@ public:
     {
         m_responseHeaders[key.toLower()] = std::move(value);
     }
+    QByteArray responseBody() const { return m_responseBody; }
     void setResponseBody(QByteArray content)
     {
         m_responseHeaders["content-length"] = QByteArray::number(content.size());
@@ -57,13 +68,34 @@ public:
     }
 
 Q_SIGNALS:
-    void readFinished(bool ok);
+    // Emitted when the request has been correctly parsed.
+    void requestReceived();
+    // Emitted on first call to sendResponse().
+    void responseSent();
+    // Emitted when something goes wrong.
+    void error(const QString &error);
+    // Emitted during or some time after sendResponse() or close().
+    void closed();
 
 private Q_SLOTS:
     void handleReadyRead();
+    void handleDisconnected();
 
 private:
+    enum class State {
+        // Waiting for first line of request.
+        RECEIVING_REQUEST,      // Next: RECEIVING_HEADERS or DISCONNECTING.
+        // Waiting for header lines.
+        RECEIVING_HEADERS,      // Next: REQUEST_RECEIVED or DISCONNECTING.
+        // Request parsing succeeded, waiting for sendResponse() or close().
+        REQUEST_RECEIVED,       // Next: DISCONNECTING.
+        // Waiting for network.
+        DISCONNECTING,          // Next: DISCONNECTED.
+        // Connection is dead.
+        DISCONNECTED,           // Next: -
+    };
     QTcpSocket *m_socket = nullptr;
+    State m_state = State::RECEIVING_REQUEST;
     QByteArray m_requestMethod;
     QByteArray m_requestPath;
     std::map<QByteArray, QByteArray> m_requestHeaders;
diff --git a/tests/auto/shared/httpserver.cpp b/tests/auto/shared/httpserver.cpp
index 6012379f2..8d14c18ff 100644
--- a/tests/auto/shared/httpserver.cpp
+++ b/tests/auto/shared/httpserver.cpp
@@ -27,44 +27,59 @@
 ****************************************************************************/
 #include "httpserver.h"
 
-#include "waitforsignal.h"
+#include <QLoggingCategory>
+
+Q_LOGGING_CATEGORY(gHttpServerLog, "HttpServer")
 
 HttpServer::HttpServer(QObject *parent) : QObject(parent)
 {
     connect(&m_tcpServer, &QTcpServer::newConnection, this, &HttpServer::handleNewConnection);
-    if (!m_tcpServer.listen())
-        qWarning("HttpServer: listen() failed");
-    m_url = QStringLiteral("http://127.0.0.1:") + QString::number(m_tcpServer.serverPort());
 }
 
-QUrl HttpServer::url(const QString &path) const
+bool HttpServer::start()
 {
-    auto copy = m_url;
-    copy.setPath(path);
-    return copy;
+    m_error = false;
+
+    if (!m_tcpServer.listen()) {
+        qCWarning(gHttpServerLog).noquote() << m_tcpServer.errorString();
+        return false;
+    }
+
+    m_url.setScheme(QStringLiteral("http"));
+    m_url.setHost(QStringLiteral("127.0.0.1"));
+    m_url.setPort(m_tcpServer.serverPort());
+
+    return true;
 }
 
-void HttpServer::handleNewConnection()
+bool HttpServer::stop()
 {
-    auto reqRep = new HttpReqRep(m_tcpServer.nextPendingConnection(), this);
-    connect(reqRep, &HttpReqRep::readFinished, this, &HttpServer::handleReadFinished);
+    m_tcpServer.close();
+    return !m_error;
 }
 
-void HttpServer::handleReadFinished(bool ok)
+QUrl HttpServer::url(const QString &path) const
 {
-    auto reqRep = qobject_cast<HttpReqRep *>(sender());
-    if (ok)
-        Q_EMIT newRequest(reqRep);
-    else
-        reqRep->deleteLater();
+    auto copy = m_url;
+    copy.setPath(path);
+    return copy;
 }
 
-std::unique_ptr<HttpReqRep> waitForRequest(HttpServer *server)
+void HttpServer::handleNewConnection()
 {
-    std::unique_ptr<HttpReqRep> result;
-    waitForSignal(server, &HttpServer::newRequest, [&](HttpReqRep *rr) {
-        rr->setParent(nullptr);
-        result.reset(rr);
+    auto rr = new HttpReqRep(m_tcpServer.nextPendingConnection(), this);
+    connect(rr, &HttpReqRep::requestReceived, [this, rr]() {
+        Q_EMIT newRequest(rr);
+        rr->close();
+    });
+    connect(rr, &HttpReqRep::responseSent, [this, rr]() {
+        qCInfo(gHttpServerLog).noquote() << rr->requestMethod() << rr->requestPath()
+                                         << rr->responseStatus() << rr->responseBody().size();
+    });
+    connect(rr, &HttpReqRep::error, [this, rr](const QString &error) {
+        qCWarning(gHttpServerLog).noquote() << rr->requestMethod() << rr->requestPath()
+                                            << error;
+        m_error = true;
     });
-    return result;
+    connect(rr, &HttpReqRep::closed, rr, &QObject::deleteLater);
 }
diff --git a/tests/auto/shared/httpserver.h b/tests/auto/shared/httpserver.h
index e45743b7b..ddbab433c 100644
--- a/tests/auto/shared/httpserver.h
+++ b/tests/auto/shared/httpserver.h
@@ -33,29 +33,55 @@
 #include <QTcpServer>
 #include <QUrl>
 
-#include <memory>
-
 // Listens on a TCP socket and creates HttpReqReps for each connection.
+//
+// Usage:
+//
+//     HttpServer server;
+//     connect(&server, &HttpServer::newRequest, [](HttpReqRep *rr) {
+//         if (rr->requestPath() == "/myPage.html") {
+//             rr->setResponseBody("<html><body>Hello, World!</body></html>");
+//             rr->sendResponse();
+//         }
+//     });
+//     QVERIFY(server.start());
+//     /* do stuff */
+//     QVERIFY(server.stop());
+//
+// HttpServer owns the HttpReqRep objects. The signal handler should not store
+// references to HttpReqRep objects.
+//
+// Only if a handler calls sendResponse() will a response be actually sent. This
+// means that multiple handlers can be connected to the signal, with different
+// handlers responsible for different paths.
 class HttpServer : public QObject
 {
     Q_OBJECT
+public:
+    explicit HttpServer(QObject *parent = nullptr);
 
-    QTcpServer m_tcpServer;
-    QUrl m_url;
+    // Must be called to start listening.
+    //
+    // Returns true if a TCP port has been successfully bound.
+    Q_REQUIRED_RESULT bool start();
 
-public:
-    HttpServer(QObject *parent = nullptr);
+    // Stops listening and performs final error checks.
+    Q_REQUIRED_RESULT bool stop();
+
+    // Full URL for given relative path
     QUrl url(const QString &path = QStringLiteral("/")) const;
 
 Q_SIGNALS:
+    // Emitted after a HTTP request has been successfully parsed.
     void newRequest(HttpReqRep *reqRep);
 
 private Q_SLOTS:
     void handleNewConnection();
-    void handleReadFinished(bool ok);
-};
 
-// Wait for an HTTP request to be received.
-std::unique_ptr<HttpReqRep> waitForRequest(HttpServer *server);
+private:
+    QTcpServer m_tcpServer;
+    QUrl m_url;
+    bool m_error = false;
+};
 
 #endif // !HTTPSERVER_H
diff --git a/tests/auto/shared/waitforsignal.h b/tests/auto/shared/waitforsignal.h
deleted file mode 100644
index 9b2daf7af..000000000
--- a/tests/auto/shared/waitforsignal.h
+++ /dev/null
@@ -1,90 +0,0 @@
-/****************************************************************************
-**
-** Copyright (C) 2017 The Qt Company Ltd.
-** Contact: https://www.qt.io/licensing/
-**
-** This file is part of the QtWebEngine module of the Qt Toolkit.
-**
-** $QT_BEGIN_LICENSE:GPL-EXCEPT$
-** Commercial License Usage
-** Licensees holding valid commercial Qt licenses may use this file in
-** accordance with the commercial license agreement provided with the
-** Software or, alternatively, in accordance with the terms contained in
-** a written agreement between you and The Qt Company. For licensing terms
-** and conditions see https://www.qt.io/terms-conditions. For further
-** information use the contact form at https://www.qt.io/contact-us.
-**
-** GNU General Public License Usage
-** Alternatively, this file may be used under the terms of the GNU
-** General Public License version 3 as published by the Free Software
-** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT
-** included in the packaging of this file. Please review the following
-** information to ensure the GNU General Public License requirements will
-** be met: https://www.gnu.org/licenses/gpl-3.0.html.
-**
-** $QT_END_LICENSE$
-**
-****************************************************************************/
-#ifndef WAITFORSIGNAL_H
-#define WAITFORSIGNAL_H
-
-#include <QObject>
-#include <QTestEventLoop>
-
-#include <utility>
-
-// Implementation details of waitForSignal.
-namespace {
-    // Wraps a functor to set a flag and exit from event loop if called.
-    template <class SignalHandler>
-    struct waitForSignal_SignalHandlerWrapper {
-        waitForSignal_SignalHandlerWrapper(SignalHandler &&sh)
-            : signalHandler(std::forward<SignalHandler>(sh)) {}
-
-        template <class... Args>
-        void operator()(Args && ... args) {
-            signalHandler(std::forward<Args>(args)...);
-            hasBeenCalled = true;
-            loop.exitLoop();
-        }
-
-        SignalHandler &&signalHandler;
-        QTestEventLoop loop;
-        bool hasBeenCalled = false;
-    };
-
-    // No-op functor.
-    struct waitForSignal_DefaultSignalHandler {
-        template <class... Args>
-        void operator()(Args && ...) {}
-    };
-} // namespace
-
-// Wait for a signal to be emitted.
-//
-// The given functor is called with the signal arguments allowing the arguments
-// to be modified before returning from the signal handler (unlike QSignalSpy).
-template <class Sender, class Signal, class SignalHandler>
-bool waitForSignal(Sender &&sender, Signal &&signal, SignalHandler &&signalHandler, int timeout = 15000)
-{
-    waitForSignal_SignalHandlerWrapper<SignalHandler> signalHandlerWrapper(
-        std::forward<SignalHandler>(signalHandler));
-    auto connection = QObject::connect(
-        std::forward<Sender>(sender),
-        std::forward<Signal>(signal),
-        std::ref(signalHandlerWrapper));
-    signalHandlerWrapper.loop.enterLoopMSecs(timeout);
-    QObject::disconnect(connection);
-    return signalHandlerWrapper.hasBeenCalled;
-}
-
-template <class Sender, class Signal>
-bool waitForSignal(Sender &&sender, Signal &&signal, int timeout = 15000)
-{
-    return waitForSignal(std::forward<Sender>(sender),
-                         std::forward<Signal>(signal),
-                         waitForSignal_DefaultSignalHandler(),
-                         timeout);
-}
-
-#endif // !WAITFORSIGNAL_H