Merge remote-tracking branch 'upstream/2.3' into main

2 files changed, 61 insertions, 52 deletions

diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp
index cc41984431..02d2f037b6 100644
--- a/src/network/webtask.cpp
+++ b/src/network/webtask.cpp
@@ -75,11 +75,10 @@ WebTask::WebTask(
         QNetworkAccessManager* networkAccessManager,
         QObject* parent)
         : QObject(parent),
-          m_networkAccessManager(networkAccessManager),
+          m_networkAccessManagerWeakPtr(networkAccessManager),
           m_timeoutTimerId(kInvalidTimerId),
           m_state(State::Idle) {
     std::call_once(registerMetaTypesOnceFlag, registerMetaTypesOnce);
-    DEBUG_ASSERT(m_networkAccessManager);
     s_instanceCounter.increment(1);
 }
 
@@ -161,10 +160,11 @@ void WebTask::slotStart(int timeoutMillis) {
     VERIFY_OR_DEBUG_ASSERT(m_state != State::Pending) {
         return;
     }
-    DEBUG_ASSERT(!m_pendingNetworkReply);
+    DEBUG_ASSERT(!m_pendingNetworkReplyWeakPtr);
     m_state = State::Idle;
 
-    VERIFY_OR_DEBUG_ASSERT(m_networkAccessManager) {
+    auto* const pNetworkAccessManager = m_networkAccessManagerWeakPtr.data();
+    VERIFY_OR_DEBUG_ASSERT(pNetworkAccessManager) {
         m_state = State::Pending;
         onNetworkError(
                 QUrl(),
@@ -173,19 +173,20 @@ void WebTask::slotStart(int timeoutMillis) {
                 QByteArray());
         return;
     }
+    DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(pNetworkAccessManager);
 
     kLogger.debug()
             << this
             << "Starting...";
 
-    m_pendingNetworkReply = doStartNetworkRequest(
-            m_networkAccessManager,
+    m_pendingNetworkReplyWeakPtr = doStartNetworkRequest(
+            pNetworkAccessManager,
             timeoutMillis);
     // Still idle, because we are in the same thread.
     // The callee is not supposed to abort a request
     // before it has beeen started successfully.
     DEBUG_ASSERT(m_state == State::Idle);
-    if (!m_pendingNetworkReply) {
+    if (!m_pendingNetworkReplyWeakPtr) {
         kLogger.debug()
                 << "Network task has not been started";
         return;
@@ -200,7 +201,7 @@ void WebTask::slotStart(int timeoutMillis) {
 
     // It is not necessary to connect the QNetworkReply::errorOccurred signal.
     // Network errors are also received through the QNetworkReply::finished signal.
-    connect(m_pendingNetworkReply,
+    connect(m_pendingNetworkReplyWeakPtr.data(),
             &QNetworkReply::finished,
             this,
             &WebTask::slotNetworkReplyFinished,
@@ -213,9 +214,10 @@ void WebTask::abort() {
         DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId);
         return;
     }
-    VERIFY_OR_DEBUG_ASSERT(m_pendingNetworkReply) {
-        return;
+    auto* const pPendingNetworkReply = m_pendingNetworkReplyWeakPtr.data();
+    VERIFY_OR_DEBUG_ASSERT(pPendingNetworkReply) {
         DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId);
+        return;
     }
     if (m_timeoutTimerId != kInvalidTimerId) {
         killTimer(m_timeoutTimerId);
@@ -225,15 +227,15 @@ void WebTask::abort() {
     kLogger.debug()
             << this
             << "Aborting...";
-    if (m_pendingNetworkReply->isRunning()) {
-        m_pendingNetworkReply->abort();
-        doNetworkReplyAborted(m_pendingNetworkReply);
+    if (pPendingNetworkReply->isRunning()) {
+        pPendingNetworkReply->abort();
+        doNetworkReplyAborted(pPendingNetworkReply);
         // Suspend and await finished signal
         return;
     }
-    doNetworkReplyAborted(m_pendingNetworkReply);
+    doNetworkReplyAborted(pPendingNetworkReply);
     m_state = State::Aborted;
-    const auto requestUrl = m_pendingNetworkReply->request().url();
+    const auto requestUrl = pPendingNetworkReply->request().url();
     VERIFY_OR_DEBUG_ASSERT(
             isSignalFuncConnected(&WebTask::aborted)) {
         kLogger.warning()
@@ -263,60 +265,57 @@ void WebTask::timerEvent(QTimerEvent* event) {
     VERIFY_OR_DEBUG_ASSERT(m_state == State::Pending) {
         return;
     }
-    VERIFY_OR_DEBUG_ASSERT(m_pendingNetworkReply) {
+    auto* const pPendingNetworkReply = m_pendingNetworkReplyWeakPtr.data();
+    VERIFY_OR_DEBUG_ASSERT(pPendingNetworkReply) {
         return;
     }
-    if (m_pendingNetworkReply->isFinished()) {
+    if (pPendingNetworkReply->isFinished()) {
         // Nothing to do
     }
     kLogger.info()
             << this
             << "Aborting after timed out";
-    DEBUG_ASSERT(m_pendingNetworkReply->isRunning());
-    m_pendingNetworkReply->abort();
-    // Aborting the network reply might finish it
-    // immediately. It will be destroyed when handling
-    // the finished signal, i.e. m_pendingNetworkReply
-    // could be nullptr here!
+    DEBUG_ASSERT(pPendingNetworkReply->isRunning());
+    pPendingNetworkReply->abort();
 }
 
 void WebTask::slotNetworkReplyFinished() {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
 
-    auto* const finishedNetworkReply = qobject_cast<QNetworkReply*>(sender());
-    VERIFY_OR_DEBUG_ASSERT(finishedNetworkReply) {
+    auto* const pFinishedNetworkReply = qobject_cast<QNetworkReply*>(sender());
+    VERIFY_OR_DEBUG_ASSERT(pFinishedNetworkReply) {
         return;
     }
-    finishedNetworkReply->deleteLater();
+    const auto deleteFinishedNetworkReply = ScopedDeleteLater(pFinishedNetworkReply);
+
     if (kLogger.debugEnabled()) {
-        if (finishedNetworkReply->url() == finishedNetworkReply->request().url()) {
+        if (pFinishedNetworkReply->url() == pFinishedNetworkReply->request().url()) {
             kLogger.debug()
                     << this
                     << "Received reply for request"
-                    << finishedNetworkReply->url();
+                    << pFinishedNetworkReply->url();
         } else {
             // Redirected
             kLogger.debug()
                     << this
                     << "Received reply for redirected request"
-                    << finishedNetworkReply->request().url()
+                    << pFinishedNetworkReply->request().url()
                     << "->"
-                    << finishedNetworkReply->url();
+                    << pFinishedNetworkReply->url();
         }
     }
 
-    if (!m_pendingNetworkReply) {
-        DEBUG_ASSERT(m_state == State::Aborted || m_state == State::TimedOut);
-        DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId);
-        kLogger.debug()
+    // Check correlation between pending and finished reply
+    auto* const pPendingNetworkReply = m_pendingNetworkReplyWeakPtr.data();
+    VERIFY_OR_DEBUG_ASSERT(pPendingNetworkReply == pFinishedNetworkReply) {
+        // Another or no reply is pending
+        kLogger.warning()
                 << this
-                << "Ignoring obsolete network reply";
-        return;
-    }
-    VERIFY_OR_DEBUG_ASSERT(m_pendingNetworkReply == finishedNetworkReply) {
+                << "Discarding unexpected network reply:"
+                << "finished =" << pFinishedNetworkReply
+                << "pending =" << pPendingNetworkReply;
         return;
     }
-    m_pendingNetworkReply = nullptr;
 
     VERIFY_OR_DEBUG_ASSERT(m_state == State::Pending) {
         DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId);
@@ -328,18 +327,18 @@ void WebTask::slotNetworkReplyFinished() {
         m_timeoutTimerId = kInvalidTimerId;
     }
 
-    if (finishedNetworkReply->error() != QNetworkReply::NetworkError::NoError) {
+    if (pFinishedNetworkReply->error() != QNetworkReply::NetworkError::NoError) {
         onNetworkError(
-                finishedNetworkReply->request().url(),
-                finishedNetworkReply->error(),
-                finishedNetworkReply->errorString(),
-                finishedNetworkReply->readAll());
+                pFinishedNetworkReply->request().url(),
+                pFinishedNetworkReply->error(),
+                pFinishedNetworkReply->errorString(),
+                pFinishedNetworkReply->readAll());
         return;
     }
 
     m_state = State::Finished;
-    const auto statusCode = readStatusCode(finishedNetworkReply);
-    doNetworkReplyFinished(finishedNetworkReply, statusCode);
+    const auto statusCode = readStatusCode(pFinishedNetworkReply);
+    doNetworkReplyFinished(pFinishedNetworkReply, statusCode);
 }
 
 } // namespace network
diff --git a/src/network/webtask.h b/src/network/webtask.h
index 48a3942ebc..a1a5d16409 100644
--- a/src/network/webtask.h
+++ b/src/network/webtask.h
@@ -8,6 +8,7 @@
 
 #include "network/httpstatuscode.h"
 #include "util/optional.h"
+#include "util/qt.h"
 
 namespace mixxx {
 
@@ -104,17 +105,20 @@ class WebTask : public QObject {
             QObject* parent = nullptr);
     ~WebTask() override;
 
+    /// Start a new task by sending a network request.
+    ///
     /// timeoutMillis <= 0: No timeout (unlimited)
     /// timeoutMillis > 0: Implicitly aborted after timeout expired
+    ///
+    /// This function is thread-safe and could be invoked from any thread.
     void invokeStart(
             int timeoutMillis = 0);
 
-    /// Cancel a pending request.
+    /// Cancel the task by aborting the pending network request.
+    ///
+    /// This function is thread-safe and could be invoked from any thread.
     void invokeAbort();
 
-    /// Cancel a pending request from the event loop thread.
-    void abort();
-
   public slots:
     void slotStart(
             int timeoutMillis);
@@ -138,6 +142,12 @@ class WebTask : public QObject {
             const QByteArray& errorContent);
 
   protected:
+    /// Cancel the task by aborting the pending network request.
+    ///
+    /// This function is NOT thread-safe and must only be called from
+    /// the event loop thread.
+    void abort();
+
     template<typename S>
     bool isSignalFuncConnected(
             S signalFunc) {
@@ -191,12 +201,12 @@ class WebTask : public QObject {
 
     /// All member variables must only be accessed from
     /// the event loop thread!!
-    const QPointer<QNetworkAccessManager> m_networkAccessManager;
+    const SafeQPointer<QNetworkAccessManager> m_networkAccessManagerWeakPtr;
 
     int m_timeoutTimerId;
     State m_state;
 
-    QPointer<QNetworkReply> m_pendingNetworkReply;
+    SafeQPointer<QNetworkReply> m_pendingNetworkReplyWeakPtr;
 };
 
 } // namespace network