fix: memory leak with NetJob and responce not getting cleaned up

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

launcher/InstanceCreationTask.h

@@ -34,7 +34,7 @@ class InstanceCreationTask : public InstanceTask {
     QString getError() const { return m_error_message; }
 
    protected:
-    void setError(QString message) { m_error_message = message; };
+    void setError(const QString& message) { m_error_message = message; };
 
    protected:
     bool m_abort = false;

launcher/InstanceImportTask.cpp

@@ -41,6 +41,7 @@
 #include "MMCZip.h"
 #include "NullInstance.h"
 
+#include "QObjectPtr.h"
 #include "icons/IconList.h"
 #include "icons/IconUtils.h"
 
@@ -260,7 +261,7 @@ void InstanceImportTask::extractFinished()
 
 void InstanceImportTask::processFlame()
 {
-    FlameCreationTask* inst_creation_task = nullptr;
+    shared_qobject_ptr<FlameCreationTask> inst_creation_task = nullptr;
     if (!m_extra_info.isEmpty()) {
         auto pack_id_it = m_extra_info.constFind("pack_id");
         Q_ASSERT(pack_id_it != m_extra_info.constEnd());
@@ -275,10 +276,10 @@ void InstanceImportTask::processFlame()
         if (original_instance_id_it != m_extra_info.constEnd())
             original_instance_id = original_instance_id_it.value();
 
-        inst_creation_task = new FlameCreationTask(m_stagingPath, m_globalSettings, m_parent, pack_id, pack_version_id, original_instance_id);
+        inst_creation_task = makeShared<FlameCreationTask>(m_stagingPath, m_globalSettings, m_parent, pack_id, pack_version_id, original_instance_id);
     } else {
         // FIXME: Find a way to get IDs in directly imported ZIPs
-        inst_creation_task = new FlameCreationTask(m_stagingPath, m_globalSettings, m_parent, {}, {});
+        inst_creation_task = makeShared<FlameCreationTask>(m_stagingPath, m_globalSettings, m_parent, QString(), QString());
     }
 
     inst_creation_task->setName(*this);
@@ -286,20 +287,19 @@ void InstanceImportTask::processFlame()
     inst_creation_task->setGroup(m_instGroup);
     inst_creation_task->setConfirmUpdate(shouldConfirmUpdate());
     
-    connect(inst_creation_task, &Task::succeeded, this, [this, inst_creation_task] {
+    connect(inst_creation_task.get(), &Task::succeeded, this, [this, inst_creation_task] {
         setOverride(inst_creation_task->shouldOverride(), inst_creation_task->originalInstanceID());
         emitSucceeded();
     });
-    connect(inst_creation_task, &Task::failed, this, &InstanceImportTask::emitFailed);
+    connect(inst_creation_task.get(), &Task::failed, this, &InstanceImportTask::emitFailed);
-    connect(inst_creation_task, &Task::progress, this, &InstanceImportTask::setProgress);
+    connect(inst_creation_task.get(), &Task::progress, this, &InstanceImportTask::setProgress);
-    connect(inst_creation_task, &Task::stepProgress, this, &InstanceImportTask::propogateStepProgress);
+    connect(inst_creation_task.get(), &Task::stepProgress, this, &InstanceImportTask::propogateStepProgress);
-    connect(inst_creation_task, &Task::status, this, &InstanceImportTask::setStatus);
+    connect(inst_creation_task.get(), &Task::status, this, &InstanceImportTask::setStatus);
-    connect(inst_creation_task, &Task::details, this, &InstanceImportTask::setDetails);
+    connect(inst_creation_task.get(), &Task::details, this, &InstanceImportTask::setDetails);
-    connect(inst_creation_task, &Task::finished, inst_creation_task, &InstanceCreationTask::deleteLater);
 
-    connect(this, &Task::aborted, inst_creation_task, &InstanceCreationTask::abort);
+    connect(this, &Task::aborted, inst_creation_task.get(), &InstanceCreationTask::abort);
-    connect(inst_creation_task, &Task::aborted, this, &Task::abort);
+    connect(inst_creation_task.get(), &Task::aborted, this, &Task::abort);
-    connect(inst_creation_task, &Task::abortStatusChanged, this, &Task::setAbortable);
+    connect(inst_creation_task.get(), &Task::abortStatusChanged, this, &Task::setAbortable);
 
     inst_creation_task->start();
 }

launcher/modplatform/flame/FileResolvingTask.cpp

@@ -35,7 +35,29 @@ void Flame::FileResolvingTask::executeTask()
     QByteArray data = Json::toText(object);
     auto dl = Net::Upload::makeByteArray(QUrl("https://api.curseforge.com/v1/mods/files"), result.get(), data);
     m_dljob->addNetAction(dl);
-    connect(m_dljob.get(), &NetJob::finished, this, &Flame::FileResolvingTask::netJobFinished);
+
+    auto step_progress = std::make_shared<TaskStepProgress>();
+    connect(m_dljob.get(), &NetJob::finished, this, [this, step_progress]() {
+        step_progress->state = TaskStepState::Succeeded;
+        stepProgress(*step_progress);
+        netJobFinished();
+    });
+    connect(m_dljob.get(), &NetJob::failed, this, [this, step_progress](QString reason) {
+        step_progress->state = TaskStepState::Failed;
+        stepProgress(*step_progress);
+        emitFailed(reason);
+    });
+    connect(m_dljob.get(), &NetJob::stepProgress, this, &FileResolvingTask::propogateStepProgress);
+    connect(m_dljob.get(), &NetJob::progress, this, [this, step_progress](qint64 current, qint64 total) {
+        qDebug() << "Resolve slug progress" << current << total;
+        step_progress->update(current, total);
+        stepProgress(*step_progress);
+    });
+    connect(m_dljob.get(), &NetJob::status, this, [this, step_progress](QString status) {
+        step_progress->status = status;
+        stepProgress(*step_progress);
+    });
+
     m_dljob->start();
 }
 
@@ -44,7 +66,7 @@ void Flame::FileResolvingTask::netJobFinished()
     setProgress(1, 3);
     // job to check modrinth for blocked projects
     m_checkJob.reset(new NetJob("Modrinth check", m_network));
-    blockedProjects = QMap<File *,QByteArray *>();
+    blockedProjects = QMap<File*, std::shared_ptr<QByteArray>>();
 
     QJsonDocument doc;
     QJsonArray array;
@@ -71,8 +93,8 @@ void Flame::FileResolvingTask::netJobFinished()
             auto hash = out.hash;
             if(!hash.isEmpty()) {
                 auto url = QString("https://api.modrinth.com/v2/version_file/%1?algorithm=sha1").arg(hash);
-                auto output = new QByteArray();
+                auto output = std::make_shared<QByteArray>();
-                auto dl = Net::Download::makeByteArray(QUrl(url), output);
+                auto dl = Net::Download::makeByteArray(QUrl(url), output.get());
                 QObject::connect(dl.get(), &Net::Download::succeeded, [&out]() {
                     out.resolved = true;
                 });
@@ -82,7 +104,27 @@ void Flame::FileResolvingTask::netJobFinished()
             }
         }
     }
-    connect(m_checkJob.get(), &NetJob::finished, this, &Flame::FileResolvingTask::modrinthCheckFinished);
+    auto step_progress = std::make_shared<TaskStepProgress>();
+    connect(m_checkJob.get(), &NetJob::finished, this, [this, step_progress]() {
+        step_progress->state = TaskStepState::Succeeded;
+        stepProgress(*step_progress);
+        modrinthCheckFinished();
+    });
+    connect(m_checkJob.get(), &NetJob::failed, this, [this, step_progress](QString reason) {
+        step_progress->state = TaskStepState::Failed;
+        stepProgress(*step_progress);
+        emitFailed(reason);
+    });
+    connect(m_checkJob.get(), &NetJob::stepProgress, this, &FileResolvingTask::propogateStepProgress);
+    connect(m_checkJob.get(), &NetJob::progress, this, [this, step_progress](qint64 current, qint64 total) {
+        qDebug() << "Resolve slug progress" << current << total;
+        step_progress->update(current, total);
+        stepProgress(*step_progress);
+    });
+    connect(m_checkJob.get(), &NetJob::status, this, [this, step_progress](QString status) {
+        step_progress->status = status;
+        stepProgress(*step_progress);
+    });
 
     m_checkJob->start();
 }
@@ -95,7 +137,6 @@ void Flame::FileResolvingTask::modrinthCheckFinished() {
         auto &out = *it;
         auto bytes = blockedProjects[out];
         if (!out->resolved) {
-            delete bytes;
             continue;
         }
 
@@ -112,11 +153,9 @@ void Flame::FileResolvingTask::modrinthCheckFinished() {
         } else {
             out->resolved = false;
         }
-
-        delete bytes;
     }
     //copy to an output list and filter out projects found on modrinth
-    auto block = new QList<File *>();
+    auto block = std::make_shared<QList<File*>>();
     auto it = blockedProjects.keys();
     std::copy_if(it.begin(), it.end(), std::back_inserter(*block), [](File *f) {
         return !f->resolved;
@@ -124,32 +163,48 @@ void Flame::FileResolvingTask::modrinthCheckFinished() {
     //Display not found mods early
     if (!block->empty()) {
         //blocked mods found, we need the slug for displaying.... we need another job :D !
-        auto slugJob = new NetJob("Slug Job", m_network);
+        m_slugJob.reset(new NetJob("Slug Job", m_network));
-        auto slugs = QVector<QByteArray>(block->size());
+        int index = 0;
-        auto index = 0;
+        for (auto mod : *block) {
-        for (auto fileInfo: *block) {
+            auto projectId = mod->projectId;
-            auto projectId = fileInfo->projectId;
+            auto output = std::make_shared<QByteArray>();
-            slugs[index] = QByteArray();
             auto url = QString("https://api.curseforge.com/v1/mods/%1").arg(projectId);
-            auto dl = Net::Download::makeByteArray(url, &slugs[index]);
+            auto dl = Net::Download::makeByteArray(url, output.get());
-            slugJob->addNetAction(dl);
+            qDebug() << "Fetching url slug for file:" << mod->fileName;
-            index++;
+            QObject::connect(dl.get(), &Net::Download::succeeded, [block, index, output]() {
-        }
+                auto mod = block->at(index);  // use the shared_ptr so it is captured and only freed when we are done
-        connect(slugJob, &NetJob::succeeded, this, [slugs, this, slugJob, block]() {
+                auto json = QJsonDocument::fromJson(*output);
-            slugJob->deleteLater();
-            auto index = 0;
-            for (const auto &slugResult: slugs) {
-                auto json = QJsonDocument::fromJson(slugResult);
                 auto base = Json::requireString(Json::requireObject(Json::requireObject(Json::requireObject(json),"data"),"links"),
                         "websiteUrl");
-                auto mod = block->at(index);
                 auto link = QString("%1/download/%2").arg(base, QString::number(mod->fileId));
                 mod->websiteUrl = link;
+            });
+            m_slugJob->addNetAction(dl);
             index++;
         }
+        auto step_progress = std::make_shared<TaskStepProgress>();
+        connect(m_slugJob.get(), &NetJob::succeeded, this, [this, step_progress]() {
+            step_progress->state = TaskStepState::Succeeded;
+            stepProgress(*step_progress);
             emitSucceeded();
         });
-        slugJob->start();
+        connect(m_slugJob.get(), &NetJob::failed, this, [this, step_progress](QString reason) {
+            step_progress->state = TaskStepState::Failed;
+            stepProgress(*step_progress);
+            emitFailed(reason);
+        });
+        connect(m_slugJob.get(), &NetJob::stepProgress, this, &FileResolvingTask::propogateStepProgress);
+        connect(m_slugJob.get(), &NetJob::progress, this, [this, step_progress](qint64 current, qint64 total) {
+            qDebug() << "Resolve slug progress" << current << total;
+            step_progress->update(current, total);
+            stepProgress(*step_progress);
+        });
+        connect(m_slugJob.get(), &NetJob::status, this, [this, step_progress](QString status) {
+            step_progress->status = status;
+            stepProgress(*step_progress);
+        });
+
+        m_slugJob->start();
     } else {
         emitSucceeded();
     }

launcher/modplatform/flame/FileResolvingTask.h

@@ -1,13 +1,11 @@
 #pragma once
 
-#include "tasks/Task.h"
-#include "net/NetJob.h"
 #include "PackManifest.h"
+#include "net/NetJob.h"
+#include "tasks/Task.h"
 
-namespace Flame
+namespace Flame {
-{
+class FileResolvingTask : public Task {
-class FileResolvingTask : public Task
-{
     Q_OBJECT
    public:
     explicit FileResolvingTask(const shared_qobject_ptr<QNetworkAccessManager>& network, Flame::Manifest& toProcess);
@@ -16,10 +14,7 @@ public:
     bool canAbort() const override { return true; }
     bool abort() override;
 
-    const Flame::Manifest &getResults() const
+    const Flame::Manifest& getResults() const { return m_toProcess; }
-    {
-        return m_toProcess;
-    }
 
    protected:
     virtual void executeTask() override;
@@ -33,9 +28,10 @@ private: /* data */
     std::shared_ptr<QByteArray> result;
     NetJob::Ptr m_dljob;
     NetJob::Ptr m_checkJob;
+    NetJob::Ptr m_slugJob;
 
     void modrinthCheckFinished();
 
-    QMap<File *, QByteArray *> blockedProjects;
+    QMap<File*, std::shared_ptr<QByteArray>> blockedProjects;
 };
-}
+}  // namespace Flame

launcher/modplatform/flame/FlameInstanceCreationTask.cpp

@@ -384,6 +384,7 @@ bool FlameCreationTask::createInstance()
     connect(m_mod_id_resolver.get(), &Flame::FileResolvingTask::progress, this, &FlameCreationTask::setProgress);
     connect(m_mod_id_resolver.get(), &Flame::FileResolvingTask::status, this, &FlameCreationTask::setStatus);
     connect(m_mod_id_resolver.get(), &Flame::FileResolvingTask::stepProgress, this, &FlameCreationTask::propogateStepProgress);
+    connect(m_mod_id_resolver.get(), &Flame::FileResolvingTask::details, this, &FlameCreationTask::setDetails);
     m_mod_id_resolver->start();
 
     loop.exec();

launcher/tasks/ConcurrentTask.cpp

@@ -138,7 +138,7 @@ void ConcurrentTask::startNext()
     connect(next.get(), &Task::progress, this, [this, next](qint64 current, qint64 total) { subTaskProgress(next, current, total); });
 
     m_doing.insert(next.get(), next);
-    auto task_progress = std::make_shared<TaskStepProgress>(TaskStepProgress({ next->getUid() }));
+    auto task_progress = std::make_shared<TaskStepProgress>(next->getUid());
     m_task_progress.insert(next->getUid(), task_progress);
 
     updateState();
@@ -166,9 +166,9 @@ void ConcurrentTask::subTaskSucceeded(Task::Ptr task)
 
     disconnect(task.get(), 0, this, 0);
 
-    emit stepProgress(*task_progress.get());
+    emit stepProgress(*task_progress);
     updateState();
-    updateStepProgress(*task_progress.get(), Operation::REMOVED);
+    updateStepProgress(*task_progress, Operation::REMOVED);
     startNext();
 }
 
@@ -184,9 +184,9 @@ void ConcurrentTask::subTaskFailed(Task::Ptr task, const QString& msg)
 
     disconnect(task.get(), 0, this, 0);
 
-    emit stepProgress(*task_progress.get());
+    emit stepProgress(*task_progress);
     updateState();
-    updateStepProgress(*task_progress.get(), Operation::REMOVED);
+    updateStepProgress(*task_progress, Operation::REMOVED);
     startNext();
 }
 
@@ -196,7 +196,7 @@ void ConcurrentTask::subTaskStatus(Task::Ptr task, const QString& msg)
     task_progress->status = msg;
     task_progress->state = TaskStepState::Running;
     
-    emit stepProgress(*task_progress.get());
+    emit stepProgress(*task_progress);
 
     if (totalSize() == 1) {
         setStatus(msg);
@@ -209,7 +209,7 @@ void ConcurrentTask::subTaskDetails(Task::Ptr task, const QString& msg)
     task_progress->details = msg;
     task_progress->state = TaskStepState::Running;
     
-    emit stepProgress(*task_progress.get());
+    emit stepProgress(*task_progress);
 
     if (totalSize() == 1) {
         setDetails(msg);
@@ -220,15 +220,10 @@ void ConcurrentTask::subTaskProgress(Task::Ptr task, qint64 current, qint64 tota
 {
     auto task_progress = m_task_progress.value(task->getUid());
 
-    task_progress->old_current = task_progress->current;
+    task_progress->update(current, total);
-    task_progress->old_total = task_progress->old_total;
         
-    task_progress->current = current;
+    emit stepProgress(*task_progress);
-    task_progress->total = total;
+    updateStepProgress(*task_progress, Operation::CHANGED);
-    task_progress->state = TaskStepState::Running;
-    
-    emit stepProgress(*task_progress.get());
-    updateStepProgress(*task_progress.get(), Operation::CHANGED);
     updateState();
 
     if (totalSize() == 1) {

launcher/tasks/Task.cpp

@@ -109,7 +109,7 @@ void Task::start()
             return;
         }
     }
-    // NOTE: only fall thorugh to here in end states
+    // NOTE: only fall through to here in end states
     m_state = State::Running;
     emit started();
     executeTask();

launcher/tasks/Task.h

@@ -64,7 +64,21 @@ struct TaskStepProgress {
     QString status = "";
     QString details = "";
     TaskStepState state = TaskStepState::Waiting;
+    TaskStepProgress() {
+        this->uid = QUuid::createUuid();
+    }
+    TaskStepProgress(QUuid uid) {
+        this->uid = uid;
+    }
     bool isDone() const { return (state == TaskStepState::Failed) || (state == TaskStepState::Succeeded); }
+    void update(qint64 current, qint64 total) {
+        this->old_current = this->current;
+        this->old_total = this->total;
+
+        this->current = current;
+        this->total = total;
+        this->state = TaskStepState::Running;
+    }
 };
 
 Q_DECLARE_METATYPE(TaskStepProgress)