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>

2 files changed, 26 insertions, 12 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)