From 75b0e8d59999d91f1828e464166f6ad829d74d7b Mon Sep 17 00:00:00 2001 From: Lorn Potter Date: Tue, 27 Oct 2020 14:32:12 +1000 Subject: [PATCH] wasm: fix network crash on abort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also work around emscripten bug #8238 emcripten_fetch_close() does not abort the network request, but instead just free’s the emscripten_fetch_t object. onsuccess or onerror will still be called, but now with a stale pointer to the deleted emscripten_fetch_t object. See https://github.com/emscripten-core/emscripten/issues/8234 Work around this by setting the userData to null when we want to abort or are done with the request. The onerror and onsuccess callbacks can then check this field (on the the still valid emscripten_fetch_t), and bail out on the (from the Qt side) aborted request. Call emcripten_fetch_close() from on error and onsuccess; this should be the point when the emscripten request is done, and there will be no more callbacks. Pick-to: 5.15 Fixes: QTBUG-87813 Change-Id: Ie9b8a29037eb150c23741683588b0f0bfd5d8c63 Reviewed-by: Edward Welbourne Reviewed-by: Morten Johan Sørvig --- src/network/access/qnetworkreplywasmimpl.cpp | 84 ++++++++++---------- src/network/access/qnetworkreplywasmimpl_p.h | 2 - 2 files changed, 40 insertions(+), 46 deletions(-) diff --git a/src/network/access/qnetworkreplywasmimpl.cpp b/src/network/access/qnetworkreplywasmimpl.cpp index 0fc5bbf5a82..2cae6847b8f 100644 --- a/src/network/access/qnetworkreplywasmimpl.cpp +++ b/src/network/access/qnetworkreplywasmimpl.cpp @@ -67,10 +67,6 @@ QNetworkReplyWasmImplPrivate::QNetworkReplyWasmImplPrivate() QNetworkReplyWasmImplPrivate::~QNetworkReplyWasmImplPrivate() { - if (m_fetch) { - emscripten_fetch_close(m_fetch); - m_fetch = 0; - } } QNetworkReplyWasmImpl::QNetworkReplyWasmImpl(QObject *parent) @@ -115,12 +111,14 @@ void QNetworkReplyWasmImpl::close() void QNetworkReplyWasmImpl::abort() { - Q_D( QNetworkReplyWasmImpl); + Q_D(QNetworkReplyWasmImpl); if (d->state == QNetworkReplyPrivate::Finished || d->state == QNetworkReplyPrivate::Aborted) return; d->state = QNetworkReplyPrivate::Aborted; - d->doAbort(); + d->m_fetch->userData = nullptr; + + d->emitReplyError(QNetworkReply::OperationCanceledError, QStringLiteral("Operation canceled")); close(); } @@ -198,11 +196,6 @@ void QNetworkReplyWasmImplPrivate::setReplyAttributes(quintptr data, int statusC handler->q_func()->setAttribute(QNetworkRequest::HttpReasonPhraseAttribute, statusReason); } -void QNetworkReplyWasmImplPrivate::doAbort() const -{ - emscripten_fetch_close(m_fetch); -} - constexpr int getArraySize (int factor) { return 2 * factor + 1; } @@ -283,7 +276,6 @@ void QNetworkReplyWasmImplPrivate::emitReplyError(QNetworkReply::NetworkError er q->setError(errorCode, errorString); emit q->errorOccurred(errorCode); - emit q->finished(); } void QNetworkReplyWasmImplPrivate::emitDataReadProgress(qint64 bytesReceived, qint64 bytesTotal) @@ -316,6 +308,12 @@ void QNetworkReplyWasmImplPrivate::dataReceived(const QByteArray &buffer, int bu downloadBuffer.append(buffer, bufferSize); emit q->readyRead(); + + if (downloadBufferCurrentSize == totalDownloadSize) { + q->setFinished(true); + emit q->readChannelFinished(); + emit q->finished(); + } } //taken from qnetworkrequest.cpp @@ -447,16 +445,14 @@ void QNetworkReplyWasmImplPrivate::_q_bufferOutgoingData() void QNetworkReplyWasmImplPrivate::downloadSucceeded(emscripten_fetch_t *fetch) { - QNetworkReplyWasmImplPrivate *reply = - reinterpret_cast(fetch->userData); - if (reply) { - QByteArray buffer(fetch->data, fetch->numBytes); - reply->dataReceived(buffer, buffer.size()); + auto reply = reinterpret_cast(fetch->userData); + if (!reply || reply->state == QNetworkReplyPrivate::Aborted) + return; + QByteArray buffer(fetch->data, fetch->numBytes); + reply->dataReceived(buffer, buffer.size()); - QByteArray statusText(fetch->statusText); - reply->setStatusCode(fetch->status, statusText); - reply->setReplyFinished(); - } + emscripten_fetch_close(fetch); + reply->m_fetch = nullptr; } void QNetworkReplyWasmImplPrivate::setStatusCode(int status, const QByteArray &statusText) @@ -466,32 +462,29 @@ void QNetworkReplyWasmImplPrivate::setStatusCode(int status, const QByteArray &s q->setAttribute(QNetworkRequest::HttpReasonPhraseAttribute, statusText); } -void QNetworkReplyWasmImplPrivate::setReplyFinished() -{ - Q_Q(QNetworkReplyWasmImpl); - q->setFinished(true); - emit q->readChannelFinished(); - emit q->finished(); -} - void QNetworkReplyWasmImplPrivate::stateChange(emscripten_fetch_t *fetch) { - if (fetch->readyState == /*HEADERS_RECEIVED*/ 2) { - size_t headerLength = emscripten_fetch_get_response_headers_length(fetch); - QByteArray str(headerLength, Qt::Uninitialized); - emscripten_fetch_get_response_headers(fetch, str.data(), str.size()); - QNetworkReplyWasmImplPrivate *reply = - reinterpret_cast(fetch->userData); - reply->headersReceived(str); + if (fetch) { + if (!quintptr(fetch->userData)) + return; + auto reply = reinterpret_cast(fetch->userData); + if (reply->state != QNetworkReplyPrivate::Aborted) { + if (fetch->readyState == /*HEADERS_RECEIVED*/ 2) { + size_t headerLength = emscripten_fetch_get_response_headers_length(fetch); + QByteArray str(headerLength, Qt::Uninitialized); + emscripten_fetch_get_response_headers(fetch, str.data(), str.size()); + + reply->headersReceived(str); + } + } } } void QNetworkReplyWasmImplPrivate::downloadProgress(emscripten_fetch_t *fetch) { - QNetworkReplyWasmImplPrivate *reply = - reinterpret_cast(fetch->userData); - Q_ASSERT(reply); - + auto reply = reinterpret_cast(fetch->userData); + if (!reply || reply->state == QNetworkReplyPrivate::Aborted) + return; if (fetch->status < 400) { uint64_t bytes = fetch->dataOffset + fetch->numBytes; uint64_t tBytes = fetch->totalBytes; // totalBytes can be 0 if server not reporting content length @@ -503,10 +496,13 @@ void QNetworkReplyWasmImplPrivate::downloadProgress(emscripten_fetch_t *fetch) void QNetworkReplyWasmImplPrivate::downloadFailed(emscripten_fetch_t *fetch) { - QNetworkReplyWasmImplPrivate *reply = reinterpret_cast(fetch->userData); + + auto reply = reinterpret_cast(fetch->userData); + if (reply) { + QString reasonStr; - if (fetch->status > 600 || reply->state == QNetworkReplyPrivate::Aborted) + if (fetch->status > 600) reasonStr = QStringLiteral("Operation canceled"); else reasonStr = QString::fromUtf8(fetch->statusText); @@ -516,8 +512,8 @@ void QNetworkReplyWasmImplPrivate::downloadFailed(emscripten_fetch_t *fetch) reply->emitReplyError(reply->statusCodeFromHttp(fetch->status, reply->request.url()), reasonStr); } - if (fetch->status >= 400) - emscripten_fetch_close(fetch); // Also free data on failure. + emscripten_fetch_close(fetch); + reply->m_fetch = nullptr; } //taken from qhttpthreaddelegate.cpp diff --git a/src/network/access/qnetworkreplywasmimpl_p.h b/src/network/access/qnetworkreplywasmimpl_p.h index 53e9bc33757..db9fd5657e3 100644 --- a/src/network/access/qnetworkreplywasmimpl_p.h +++ b/src/network/access/qnetworkreplywasmimpl_p.h @@ -137,8 +137,6 @@ public: QSharedPointer outgoingDataBuffer; QByteArray requestData; - void doAbort() const; - static void downloadProgress(emscripten_fetch_t *fetch); static void downloadFailed(emscripten_fetch_t *fetch); static void downloadSucceeded(emscripten_fetch_t *fetch);