HTTP/2 - fix handling of GOAWAY frame

- Fix the case when we erroneously handled stream ID == 0 in a GOAWAY frame as
an invalid stream ID.
- _q_receivedReply: convert do{}while() loop into to while(){} to prevent
it from handling any frames after GOAWAY frame received and all active frame
finished.
- sendRequest - if we received GOAWAY, also clear spdyRequests in the connection
channel, otherwise it keeps re-trying to send requests!
- Http network connection channel never resets a protocolHandler in _q_encrypted/
_q_connected, which is BAD for HTTP/2, since HTTP/2 has unique per-connection
compression context and must be reset - now we recreate the protocol handler in
_q_encrypted or _q_connected (https/http).
- Update autotest.

Task-number: QTBUG-57600
Change-Id: Ib864ce52287bab23334ff43a83ba4b0b7cb52c60
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>

5 files changed, 139 insertions, 24 deletions

diff --git a/src/network/access/qhttp2protocolhandler.cpp b/src/network/access/qhttp2protocolhandler.cpp
index 3fa0c18dc0..f7383d1d3a 100644
--- a/src/network/access/qhttp2protocolhandler.cpp
+++ b/src/network/access/qhttp2protocolhandler.cpp
@@ -208,7 +208,7 @@ void QHttp2ProtocolHandler::_q_receiveReply()
     Q_ASSERT(m_socket);
     Q_ASSERT(m_channel);
 
-    do {
+    while (!goingAway || activeStreams.size()) {
         const auto result = frameReader.read(*m_socket);
         switch (result) {
         case FrameStatus::incompleteFrame:
@@ -264,13 +264,19 @@ void QHttp2ProtocolHandler::_q_receiveReply()
             // 5.1 - ignore unknown frames.
             break;
         }
-    } while (!goingAway || activeStreams.size());
+    }
 }
 
 bool QHttp2ProtocolHandler::sendRequest()
 {
-    if (goingAway)
+    if (goingAway) {
+        // Stop further calls to this method: we have received GOAWAY
+        // so we cannot create new streams.
+        m_channel->emitFinishedWithError(QNetworkReply::ProtocolUnknownError,
+                                         "GOAWAY received, cannot start a request");
+        m_channel->spdyRequestsToSend.clear();
         return false;
+    }
 
     if (!prefaceSent && !sendClientPreface())
         return false;
@@ -756,14 +762,10 @@ void QHttp2ProtocolHandler::handleGOAWAY()
         // "The last stream identifier can be set to 0 if no
         // streams were processed."
         lastStreamID = 1;
-    }
-
-    if (!(lastStreamID & 0x1)) {
+    } else if (!(lastStreamID & 0x1)) {
         // 5.1.1 - we (client) use only odd numbers as stream identifiers.
         return connectionError(PROTOCOL_ERROR, "GOAWAY with invalid last stream ID");
-    }
-
-    if (lastStreamID >= nextID) {
+    } else if (lastStreamID >= nextID) {
         // "A server that is attempting to gracefully shut down a connection SHOULD
         // send an initial GOAWAY frame with the last stream identifier set to 2^31-1
         // and a NO_ERROR code."
@@ -776,6 +778,13 @@ void QHttp2ProtocolHandler::handleGOAWAY()
 
     goingAway = true;
 
+    // For the requests (and streams) we did not start yet, we have to report an
+    // error.
+    m_channel->emitFinishedWithError(QNetworkReply::ProtocolUnknownError,
+                                     "GOAWAY received, cannot start a request");
+    // Also, prevent further calls to sendRequest:
+    m_channel->spdyRequestsToSend.clear();
+
     QNetworkReply::NetworkError error = QNetworkReply::NoError;
     QString message;
     qt_error(errorCode, error, message);
diff --git a/src/network/access/qhttpnetworkconnectionchannel.cpp b/src/network/access/qhttpnetworkconnectionchannel.cpp
index 205490b830..646ede0022 100644
--- a/src/network/access/qhttpnetworkconnectionchannel.cpp
+++ b/src/network/access/qhttpnetworkconnectionchannel.cpp
@@ -180,9 +180,7 @@ void QHttpNetworkConnectionChannel::init()
            sslSocket->setSslConfiguration(sslConfiguration);
     } else {
 #endif // !QT_NO_SSL
-        if (connection->connectionType() == QHttpNetworkConnection::ConnectionTypeHTTP2)
-            protocolHandler.reset(new QHttp2ProtocolHandler(this));
-        else
+        if (connection->connectionType() != QHttpNetworkConnection::ConnectionTypeHTTP2)
             protocolHandler.reset(new QHttpProtocolHandler(this));
 #ifndef QT_NO_SSL
     }
@@ -839,6 +837,9 @@ void QHttpNetworkConnectionChannel::_q_connected()
     } else {
         state = QHttpNetworkConnectionChannel::IdleState;
         if (connection->connectionType() == QHttpNetworkConnection::ConnectionTypeHTTP2) {
+            // We have to reset QHttp2ProtocolHandler's state machine, it's a new
+            // connection and the handler's state is unique per connection.
+            protocolHandler.reset(new QHttp2ProtocolHandler(this));
             if (spdyRequestsToSend.count() > 0) {
                 // wait for data from the server first (e.g. initial window, max concurrent requests)
                 QMetaObject::invokeMethod(connection, "_q_startNextRequest", Qt::QueuedConnection);
@@ -1092,6 +1093,10 @@ void QHttpNetworkConnectionChannel::_q_encrypted()
             emitFinishedWithError(QNetworkReply::SslHandshakeFailedError,
                                   "detected unknown Next Protocol Negotiation protocol");
         }
+    } else if (connection->connectionType() == QHttpNetworkConnection::ConnectionTypeHTTP2) {
+        // We have to reset QHttp2ProtocolHandler's state machine, it's a new
+        // connection and the handler's state is unique per connection.
+        protocolHandler.reset(new QHttp2ProtocolHandler(this));
     }
 
     if (!socket)
diff --git a/tests/auto/network/access/http2/http2srv.cpp b/tests/auto/network/access/http2/http2srv.cpp
index 9d68b5c798..9f77419461 100644
--- a/tests/auto/network/access/http2/http2srv.cpp
+++ b/tests/auto/network/access/http2/http2srv.cpp
@@ -41,6 +41,7 @@
 
 #include <QtNetwork/qtcpsocket.h>
 
+#include <QtCore/qtimer.h>
 #include <QtCore/qdebug.h>
 #include <QtCore/qlist.h>
 #include <QtCore/qfile.h>
@@ -117,6 +118,13 @@ void Http2Server::setResponseBody(const QByteArray &body)
     responseBody = body;
 }
 
+void Http2Server::emulateGOAWAY(int timeout)
+{
+    Q_ASSERT(timeout >= 0);
+    testingGOAWAY = true;
+    goawayTimeout = timeout;
+}
+
 void Http2Server::startServer()
 {
 #ifdef QT_NO_SSL
@@ -271,6 +279,16 @@ void Http2Server::connectionEstablished()
 {
     using namespace Http2;
 
+    if (testingGOAWAY) {
+        auto timer = new QTimer(this);
+        timer->setSingleShot(true);
+        connect(timer, &QTimer::timeout, [this]() {
+            sendGOAWAY(quint32(connectionStreamID), quint32(INTERNAL_ERROR), 0);
+        });
+        timer->start(goawayTimeout);
+        return;
+    }
+
     connect(socket.data(), SIGNAL(readyRead()),
             this, SLOT(readReady()));
 
diff --git a/tests/auto/network/access/http2/http2srv.h b/tests/auto/network/access/http2/http2srv.h
index 15a4f212c9..63a4a4c8e9 100644
--- a/tests/auto/network/access/http2/http2srv.h
+++ b/tests/auto/network/access/http2/http2srv.h
@@ -70,6 +70,7 @@ public:
     // To be called before server started:
     void enablePushPromise(bool enabled, const QByteArray &path = QByteArray());
     void setResponseBody(const QByteArray &body);
+    void emulateGOAWAY(int timeout);
 
     // Invokables, since we can call them from the main thread,
     // but server (can) work on its own thread.
@@ -162,6 +163,9 @@ private:
     quint32 lastPromisedStream = 0;
     QByteArray pushPath;
 
+    bool testingGOAWAY = false;
+    int goawayTimeout = 0;
+
 protected slots:
     void ignoreErrorSlot();
 };
diff --git a/tests/auto/network/access/http2/tst_http2.cpp b/tests/auto/network/access/http2/tst_http2.cpp
index 771ddb01be..645d28ccb3 100644
--- a/tests/auto/network/access/http2/tst_http2.cpp
+++ b/tests/auto/network/access/http2/tst_http2.cpp
@@ -69,6 +69,8 @@ private slots:
     void flowControlClientSide();
     void flowControlServerSide();
     void pushPromise();
+    void goaway_data();
+    void goaway();
 
 protected slots:
     // Slots to listen to our in-process server:
@@ -83,6 +85,7 @@ protected slots:
     void receivedData(quint32 streamID);
     void windowUpdated(quint32 streamID);
     void replyFinished();
+    void replyFinishedWithError();
 
 private:
     void clearHTTP2State();
@@ -97,6 +100,7 @@ private:
     void sendRequest(int streamNumber,
                      QNetworkRequest::Priority priority = QNetworkRequest::NormalPriority,
                      const QByteArray &payload = QByteArray());
+    QUrl requestUrl() const;
 
     quint16 serverPort = 0;
     QThread *workerThread = nullptr;
@@ -196,9 +200,8 @@ void tst_Http2::singleRequest()
 
     QVERIFY(serverPort != 0);
 
-    const QString urlAsString(clearTextHTTP2 ? QString("http://127.0.0.1:%1/index.html")
-                                             : QString("https://127.0.0.1:%1/index.html"));
-    const QUrl url(urlAsString.arg(serverPort));
+    auto url = requestUrl();
+    url.setPath("/index.html");
 
     QNetworkRequest request(url);
     request.setAttribute(QNetworkRequest::HTTP2AllowedAttribute, QVariant(true));
@@ -347,11 +350,10 @@ void tst_Http2::pushPromise()
 
     QVERIFY(serverPort != 0);
 
-    const QString urlAsString((clearTextHTTP2 ? QString("http://127.0.0.1:%1/")
-                                              : QString("https://127.0.0.1:%1/")).arg(serverPort));
-    const QUrl requestUrl(urlAsString + "index.html");
+    auto url = requestUrl();
+    url.setPath("/index.html");
 
-    QNetworkRequest request(requestUrl);
+    QNetworkRequest request(url);
     request.setAttribute(QNetworkRequest::HTTP2AllowedAttribute, QVariant(true));
 
     auto reply = manager.get(request);
@@ -374,8 +376,8 @@ void tst_Http2::pushPromise()
     // Create an additional request (let's say, we parsed reply and realized we
     // need another resource):
 
-    const QUrl promisedUrl(urlAsString + "script.js");
-    QNetworkRequest promisedRequest(promisedUrl);
+    url.setPath("/script.js");
+    QNetworkRequest promisedRequest(url);
     promisedRequest.setAttribute(QNetworkRequest::HTTP2AllowedAttribute, QVariant(true));
     reply = manager.get(promisedRequest);
     connect(reply, &QNetworkReply::finished, this, &tst_Http2::replyFinished);
@@ -391,6 +393,61 @@ void tst_Http2::pushPromise()
     QVERIFY(reply->isFinished());
 }
 
+void tst_Http2::goaway_data()
+{
+    // For now we test only basic things in two very simple scenarios:
+    // - server sends GOAWAY immediately or
+    // - server waits for some time (enough for ur to init several streams on a
+    // client side); then suddenly it replies with GOAWAY, never processing any
+    // request.
+    QTest::addColumn<int>("responseTimeoutMS");
+    QTest::newRow("ImmediateGOAWAY") << 0;
+    QTest::newRow("DelayedGOAWAY") << 1000;
+}
+
+void tst_Http2::goaway()
+{
+    using namespace Http2;
+
+    QFETCH(const int, responseTimeoutMS);
+
+    clearHTTP2State();
+
+    serverPort = 0;
+    nRequests = 3;
+
+    ServerPtr srv(newServer(defaultServerSettings, defaultClientSettings));
+    srv->emulateGOAWAY(responseTimeoutMS);
+    QMetaObject::invokeMethod(srv.data(), "startServer", Qt::QueuedConnection);
+    runEventLoop();
+
+    QVERIFY(serverPort != 0);
+
+    auto url = requestUrl();
+    // We have to store these replies, so that we can check errors later.
+    std::vector<QNetworkReply *> replies(nRequests);
+    for (int i = 0; i < nRequests; ++i) {
+        url.setPath(QString("/%1").arg(i));
+        QNetworkRequest request(url);
+        request.setAttribute(QNetworkRequest::HTTP2AllowedAttribute, QVariant(true));
+        replies[i] = manager.get(request);
+        QCOMPARE(replies[i]->error(), QNetworkReply::NoError);
+        void (QNetworkReply::*errorSignal)(QNetworkReply::NetworkError) =
+            &QNetworkReply::error;
+        connect(replies[i], errorSignal, this, &tst_Http2::replyFinishedWithError);
+        // Since we're using self-signed certificates, ignore SSL errors:
+        replies[i]->ignoreSslErrors();
+    }
+
+    runEventLoop(5000 + responseTimeoutMS);
+
+    // No request processed, no 'replyFinished' slot calls:
+    QCOMPARE(nRequests, 0);
+    // Our server did not bother to send anything except a single GOAWAY frame:
+    QVERIFY(!prefaceOK);
+    QVERIFY(!serverGotSettingsACK);
+}
+
 void tst_Http2::serverStarted(quint16 port)
 {
     serverPort = port;
@@ -445,10 +502,9 @@ void tst_Http2::sendRequest(int streamNumber,
                             QNetworkRequest::Priority priority,
                             const QByteArray &payload)
 {
-    static const QString urlAsString(clearTextHTTP2 ? "http://127.0.0.1:%1/stream%2.html"
-                                                    : "https://127.0.0.1:%1/stream%2.html");
+    auto url = requestUrl();
+    url.setPath(QString("/stream%1.html").arg(streamNumber));
 
-    const QUrl url(urlAsString.arg(serverPort).arg(streamNumber));
     QNetworkRequest request(url);
     request.setAttribute(QNetworkRequest::HTTP2AllowedAttribute, QVariant(true));
     request.setPriority(priority);
@@ -463,6 +519,14 @@ void tst_Http2::sendRequest(int streamNumber,
     connect(reply, &QNetworkReply::finished, this, &tst_Http2::replyFinished);
 }
 
+QUrl tst_Http2::requestUrl() const
+{
+    static auto url = QUrl(QLatin1String(clearTextHTTP2 ? "http://127.0.0.1" : "https://127.0.0.1"));
+    url.setPort(serverPort);
+
+    return url;
+}
+
 void tst_Http2::clientPrefaceOK()
 {
     prefaceOK = true;
@@ -532,6 +596,21 @@ void tst_Http2::replyFinished()
         stopEventLoop();
 }
 
+void tst_Http2::replyFinishedWithError()
+{
+    QVERIFY(nRequests);
+
+    if (const auto reply = qobject_cast<QNetworkReply *>(sender())) {
+        // For now this is a 'generic' code, it just verifies some error was
+        // reported without testing its type.
+        QVERIFY(reply->error() != QNetworkReply::NoError);
+    }
+
+    --nRequests;
+    if (!nRequests)
+        stopEventLoop();
+}
+
 QT_END_NAMESPACE
 
 QTEST_MAIN(tst_Http2)