Merge branch 'kitsune-job-lifetime-fixes'

3 files changed, 79 insertions, 73 deletions

diff --git a/lib/connection.cpp b/lib/connection.cpp
index 021ff5dd..1c63adeb 100644
--- a/lib/connection.cpp
+++ b/lib/connection.cpp
@@ -253,51 +253,48 @@ void Connection::resolveServer(const QString& mxid)
         return;
     }
 
-    setHomeserver(maybeBaseUrl);
-
     auto domain = maybeBaseUrl.host();
     qCDebug(MAIN) << "Finding the server" << domain;
 
+    d->data->setBaseUrl(maybeBaseUrl); // Just enough to check .well-known file
     auto getWellKnownJob = callApi<GetWellknownJob>();
-    connect(
-        getWellKnownJob, &BaseJob::finished,
+    connect(getWellKnownJob, &BaseJob::finished, this,
         [this, getWellKnownJob, maybeBaseUrl] {
-            if (getWellKnownJob->status() == BaseJob::NotFoundError)
-                qCDebug(MAIN) << "No .well-known file, IGNORE";
-            else {
+            if (getWellKnownJob->status() != BaseJob::NotFoundError) {
                 if (getWellKnownJob->status() != BaseJob::Success) {
-                    qCDebug(MAIN)
+                    qCWarning(MAIN)
                         << "Fetching .well-known file failed, FAIL_PROMPT";
-                    emit resolveError(tr("Fetching .well-known file failed"));
+                    emit resolveError(tr("Failed resolving the homeserver"));
                     return;
                 }
-                QUrl baseUrl(getWellKnownJob->data().homeserver.baseUrl);
+                QUrl baseUrl { getWellKnownJob->data().homeserver.baseUrl };
                 if (baseUrl.isEmpty()) {
-                    qCDebug(MAIN) << "base_url not provided, FAIL_PROMPT";
-                    emit resolveError(tr("base_url not provided"));
+                    qCWarning(MAIN) << "base_url not provided, FAIL_PROMPT";
+                    emit resolveError(
+                        tr("The homeserver base URL is not provided"));
                     return;
                 }
                 if (!baseUrl.isValid()) {
-                    qCDebug(MAIN) << "base_url invalid, FAIL_ERROR";
-                    emit resolveError(tr("base_url invalid"));
+                    qCWarning(MAIN) << "base_url invalid, FAIL_ERROR";
+                    emit resolveError(tr("The homeserver base URL is invalid"));
                     return;
                 }
-
-                qCDebug(MAIN) << ".well-known for" << maybeBaseUrl.host()
-                              << "is" << baseUrl.toString();
+                qCInfo(MAIN) << ".well-known URL for" << maybeBaseUrl.host()
+                             << "is" << baseUrl.authority();
                 setHomeserver(baseUrl);
+            } else {
+                qCInfo(MAIN) << "No .well-known file, using" << maybeBaseUrl
+                             << "for base URL";
+                setHomeserver(maybeBaseUrl);
             }
 
             auto getVersionsJob = callApi<GetVersionsJob>();
-
-            connect(getVersionsJob, &BaseJob::finished, [this, getVersionsJob] {
-                if (getVersionsJob->status() == BaseJob::Success) {
-                    qCDebug(MAIN) << "homeserver url is valid";
-                    emit resolved();
-                } else {
-                    qCDebug(MAIN) << "homeserver url invalid";
-                    emit resolveError(tr("homeserver url invalid"));
-                }
+            connect(getVersionsJob, &BaseJob::success, this,
+                    &Connection::resolved);
+            connect(getVersionsJob, &BaseJob::failure, this, [this] {
+                qCWarning(MAIN) << "Homeserver base URL invalid";
+                emit resolveError(tr("The homeserver base URL "
+                                     "doesn't seem to work"));
             });
         });
 }
@@ -726,7 +723,7 @@ void Connection::stopSync()
 
 QString Connection::nextBatchToken() const { return d->data->lastEvent(); }
 
-PostReceiptJob* Connection::postReceipt(Room* room, RoomEvent* event) const
+PostReceiptJob* Connection::postReceipt(Room* room, RoomEvent* event)
 {
     return callApi<PostReceiptJob>(room->id(), "m.read", event->id());
 }
@@ -776,7 +773,7 @@ inline auto splitMediaId(const QString& mediaId)
 
 MediaThumbnailJob* Connection::getThumbnail(const QString& mediaId,
                                             QSize requestedSize,
-                                            RunningPolicy policy) const
+                                            RunningPolicy policy)
 {
     auto idParts = splitMediaId(mediaId);
     return callApi<MediaThumbnailJob>(policy, idParts.front(), idParts.back(),
@@ -784,21 +781,21 @@ MediaThumbnailJob* Connection::getThumbnail(const QString& mediaId,
 }
 
 MediaThumbnailJob* Connection::getThumbnail(const QUrl& url, QSize requestedSize,
-                                            RunningPolicy policy) const
+                                            RunningPolicy policy)
 {
     return getThumbnail(url.authority() + url.path(), requestedSize, policy);
 }
 
 MediaThumbnailJob* Connection::getThumbnail(const QUrl& url, int requestedWidth,
                                             int requestedHeight,
-                                            RunningPolicy policy) const
+                                            RunningPolicy policy)
 {
     return getThumbnail(url, QSize(requestedWidth, requestedHeight), policy);
 }
 
 UploadContentJob*
 Connection::uploadContent(QIODevice* contentSource, const QString& filename,
-                          const QString& overrideContentType) const
+                          const QString& overrideContentType)
 {
     Q_ASSERT(contentSource != nullptr);
     auto contentType = overrideContentType;
@@ -823,19 +820,19 @@ UploadContentJob* Connection::uploadFile(const QString& fileName,
                          overrideContentType);
 }
 
-GetContentJob* Connection::getContent(const QString& mediaId) const
+GetContentJob* Connection::getContent(const QString& mediaId)
 {
     auto idParts = splitMediaId(mediaId);
     return callApi<GetContentJob>(idParts.front(), idParts.back());
 }
 
-GetContentJob* Connection::getContent(const QUrl& url) const
+GetContentJob* Connection::getContent(const QUrl& url)
 {
     return getContent(url.authority() + url.path());
 }
 
 DownloadFileJob* Connection::downloadFile(const QUrl& url,
-                                          const QString& localFilename) const
+                                          const QString& localFilename)
 {
     auto mediaId = url.authority() + url.path();
     auto idParts = splitMediaId(mediaId);
@@ -1014,7 +1011,7 @@ ForgetRoomJob* Connection::forgetRoom(const QString& id)
 
 SendToDeviceJob*
 Connection::sendToDevices(const QString& eventType,
-                          const UsersToDevicesToEvents& eventsMap) const
+                          const UsersToDevicesToEvents& eventsMap)
 {
     QHash<QString, QHash<QString, QJsonObject>> json;
     json.reserve(int(eventsMap.size()));
@@ -1035,7 +1032,7 @@ Connection::sendToDevices(const QString& eventType,
 }
 
 SendMessageJob* Connection::sendMessage(const QString& roomId,
-                                        const RoomEvent& event) const
+                                        const RoomEvent& event)
 {
     const auto txnId = event.transactionId().isEmpty() ? generateTxnId()
                                                        : event.transactionId();
@@ -1612,8 +1609,9 @@ void Connection::setLazyLoading(bool newValue)
     }
 }
 
-void Connection::run(BaseJob* job, RunningPolicy runningPolicy) const
+void Connection::run(BaseJob* job, RunningPolicy runningPolicy)
 {
+    job->setParent(this); // Protects from #397, #398
     connect(job, &BaseJob::failure, this, &Connection::requestFailed);
     job->initiate(d->data.get(), runningPolicy & BackgroundRequest);
 }
diff --git a/lib/connection.h b/lib/connection.h
index 350571f1..b0263732 100644
--- a/lib/connection.h
+++ b/lib/connection.h
@@ -423,7 +423,7 @@ public:
     void setLazyLoading(bool newValue);
 
     /*! Start a pre-created job object on this connection */
-    void run(BaseJob* job, RunningPolicy runningPolicy = ForegroundRequest) const;
+    void run(BaseJob* job, RunningPolicy runningPolicy = ForegroundRequest);
 
     /*! Start a job of a specified type with specified arguments and policy
      *
@@ -438,7 +438,7 @@ public:
      * \sa BaseJob::isBackground. QNetworkRequest::BackgroundRequestAttribute
      */
     template <typename JobT, typename... JobArgTs>
-    JobT* callApi(RunningPolicy runningPolicy, JobArgTs&&... jobArgs) const
+    JobT* callApi(RunningPolicy runningPolicy, JobArgTs&&... jobArgs)
     {
         auto job = new JobT(std::forward<JobArgTs>(jobArgs)...);
         run(job, runningPolicy);
@@ -450,7 +450,7 @@ public:
      * This is an overload that runs the job with "foreground" policy.
      */
     template <typename JobT, typename... JobArgTs>
-    JobT* callApi(JobArgTs&&... jobArgs) const
+    JobT* callApi(JobArgTs&&... jobArgs)
     {
         return callApi<JobT>(ForegroundRequest,
                              std::forward<JobArgTs>(jobArgs)...);
@@ -544,25 +544,24 @@ public slots:
 
     virtual MediaThumbnailJob*
     getThumbnail(const QString& mediaId, QSize requestedSize,
-                 RunningPolicy policy = BackgroundRequest) const;
-    MediaThumbnailJob*
-    getThumbnail(const QUrl& url, QSize requestedSize,
-                 RunningPolicy policy = BackgroundRequest) const;
-    MediaThumbnailJob*
-    getThumbnail(const QUrl& url, int requestedWidth, int requestedHeight,
-                 RunningPolicy policy = BackgroundRequest) const;
+                 RunningPolicy policy = BackgroundRequest);
+    MediaThumbnailJob* getThumbnail(const QUrl& url, QSize requestedSize,
+                                    RunningPolicy policy = BackgroundRequest);
+    MediaThumbnailJob* getThumbnail(const QUrl& url, int requestedWidth,
+                                    int requestedHeight,
+                                    RunningPolicy policy = BackgroundRequest);
 
     // QIODevice* should already be open
-    UploadContentJob*
-    uploadContent(QIODevice* contentSource, const QString& filename = {},
-                  const QString& overrideContentType = {}) const;
+    UploadContentJob* uploadContent(QIODevice* contentSource,
+                                    const QString& filename = {},
+                                    const QString& overrideContentType = {});
     UploadContentJob* uploadFile(const QString& fileName,
                                  const QString& overrideContentType = {});
-    GetContentJob* getContent(const QString& mediaId) const;
-    GetContentJob* getContent(const QUrl& url) const;
+    GetContentJob* getContent(const QString& mediaId);
+    GetContentJob* getContent(const QUrl& url);
     // If localFilename is empty, a temporary file will be created
     DownloadFileJob* downloadFile(const QUrl& url,
-                                  const QString& localFilename = {}) const;
+                                  const QString& localFilename = {});
 
     /**
      * \brief Create a room (generic method)
@@ -641,11 +640,10 @@ public slots:
     ForgetRoomJob* forgetRoom(const QString& id);
 
     SendToDeviceJob* sendToDevices(const QString& eventType,
-                                   const UsersToDevicesToEvents& eventsMap) const;
+                                   const UsersToDevicesToEvents& eventsMap);
 
     /** \deprecated This method is experimental and may be removed any time */
-    SendMessageJob* sendMessage(const QString& roomId,
-                                const RoomEvent& event) const;
+    SendMessageJob* sendMessage(const QString& roomId, const RoomEvent& event);
 
     /** \deprecated Do not use this directly, use Room::leaveRoom() instead */
     virtual LeaveRoomJob* leaveRoom(Room* room);
@@ -654,7 +652,7 @@ public slots:
 
     /** @deprecated Use callApi<PostReceiptJob>() or Room::postReceipt() instead
      */
-    virtual PostReceiptJob* postReceipt(Room* room, RoomEvent* event) const;
+    virtual PostReceiptJob* postReceipt(Room* room, RoomEvent* event);
 
 signals:
     /**
diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp
index 68adeaf6..65668521 100644
--- a/lib/jobs/basejob.cpp
+++ b/lib/jobs/basejob.cpp
@@ -145,6 +145,7 @@ BaseJob::BaseJob(HttpVerb verb, const QString& name, const QString& endpoint,
 BaseJob::~BaseJob()
 {
     stop();
+    d->retryTimer.stop(); // See #398
     qCDebug(d->logCat) << this << "destroyed";
 }
 
@@ -233,6 +234,7 @@ void BaseJob::Private::sendRequest()
     req.setMaximumRedirectsAllowed(10);
     req.setAttribute(QNetworkRequest::HttpPipeliningAllowedAttribute, true);
     req.setAttribute(QNetworkRequest::HTTP2AllowedAttribute, true);
+    Q_ASSERT(req.url().isValid());
     for (auto it = requestHeaders.cbegin(); it != requestHeaders.cend(); ++it)
         req.setRawHeader(it.key(), it.value());
 
@@ -260,25 +262,31 @@ void BaseJob::beforeAbandon(QNetworkReply*) {}
 
 void BaseJob::initiate(ConnectionData* connData, bool inBackground)
 {
-    Q_ASSERT(connData != nullptr);
+    if (connData && connData->baseUrl().isValid()) {
+        d->inBackground = inBackground;
+        d->connection = connData;
+        doPrepare();
 
-    d->inBackground = inBackground;
-    d->connection = connData;
-    doPrepare();
-
-    if ((d->verb == HttpVerb::Post || d->verb == HttpVerb::Put)
-        && d->requestData.source() && !d->requestData.source()->isReadable()) {
-        setStatus(FileError, "Request data not ready");
-    }
-    Q_ASSERT(status().code != Pending); // doPrepare() must NOT set this
-    if (status().code == Unprepared) {
-        d->connection->submit(this);
-    } else {
-        qDebug(d->logCat).noquote()
+        if ((d->verb == HttpVerb::Post || d->verb == HttpVerb::Put)
+            && d->requestData.source() && !d->requestData.source()->isReadable()) {
+            setStatus(FileError, "Request data not ready");
+        }
+        Q_ASSERT(status().code != Pending); // doPrepare() must NOT set this
+        if (status().code == Unprepared) {
+            d->connection->submit(this);
+            return;
+        }
+        qCWarning(d->logCat).noquote()
             << "Request failed preparation and won't be sent:"
             << d->dumpRequest();
-        QTimer::singleShot(0, this, &BaseJob::finishJob);
+    } else {
+        qCCritical(d->logCat)
+            << "Developers, ensure the Connection is valid before using it";
+        Q_ASSERT(false);
+        setStatus(IncorrectRequestError, tr("Invalid server connection"));
     }
+    // The status is no good, finalise
+    QTimer::singleShot(0, this, &BaseJob::finishJob);
 }
 
 void BaseJob::sendRequest()
@@ -337,7 +345,7 @@ bool checkContentType(const QByteArray& type, const QByteArrayList& patterns)
     // ignore possible appendixes of the content type
     const auto ctype = type.split(';').front();
 
-    for (const auto& pattern : patterns) {
+    for (const auto& pattern: patterns) {
         if (pattern.startsWith('*') || ctype == pattern) // Fast lane
             return true;
 
@@ -686,6 +694,8 @@ void BaseJob::setStatus(int code, QString message)
 void BaseJob::abandon()
 {
     beforeAbandon(d->reply ? d->reply.data() : nullptr);
+    d->timer.stop();
+    d->retryTimer.stop(); // In case abandon() was called between retries
     setStatus(Abandoned);
     if (d->reply)
         d->reply->disconnect(this);