From 9e35230467267691ce745429de8d7e0b18095084 Mon Sep 17 00:00:00 2001 From: flow Date: Fri, 16 Sep 2022 19:22:37 -0300 Subject: [PATCH 1/4] fix: memory leak when getting mods from the mods folder friendly reminder to always delete your news. Signed-off-by: flow --- launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp b/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp index 3a857740..afe892f4 100644 --- a/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp +++ b/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp @@ -57,6 +57,8 @@ void ModFolderLoadTask::executeTask() if (mod->enabled()) { if (m_result->mods.contains(mod->internal_id())) { m_result->mods[mod->internal_id()]->setStatus(ModStatus::Installed); + // Delete the object we just created, since a valid one is already in the mods list. + delete mod; } else { m_result->mods[mod->internal_id()] = mod; From 10493bd44ab59171ac4f2e3ab7b600bcff8e4af6 Mon Sep 17 00:00:00 2001 From: flow Date: Fri, 16 Sep 2022 19:25:53 -0300 Subject: [PATCH 2/4] fix: move newly allocated resources to the main thread This avoids them getting deleted when the worker thread exits, due to thread affinity on the created thread. Signed-off-by: flow --- .../minecraft/mod/tasks/BasicFolderLoadTask.h | 12 ++++++++++-- .../minecraft/mod/tasks/ModFolderLoadTask.cpp | 16 +++++++++++++++- launcher/minecraft/mod/tasks/ModFolderLoadTask.h | 3 +++ 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h b/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h index be0e752d..2fce2942 100644 --- a/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h +++ b/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h @@ -3,6 +3,7 @@ #include #include #include +#include #include @@ -23,14 +24,14 @@ class BasicFolderLoadTask : public Task { [[nodiscard]] ResultPtr result() const { return m_result; } public: - BasicFolderLoadTask(QDir dir) : Task(nullptr, false), m_dir(dir), m_result(new Result) + BasicFolderLoadTask(QDir dir) : Task(nullptr, false), m_dir(dir), m_result(new Result), m_thread_to_spawn_into(thread()) { m_create_func = [](QFileInfo const& entry) -> Resource* { return new Resource(entry); }; } BasicFolderLoadTask(QDir dir, std::function create_function) - : Task(nullptr, false), m_dir(dir), m_result(new Result), m_create_func(std::move(create_function)) + : Task(nullptr, false), m_dir(dir), m_result(new Result), m_create_func(std::move(create_function)), m_thread_to_spawn_into(thread()) {} [[nodiscard]] bool canAbort() const override { return true; } @@ -42,9 +43,13 @@ class BasicFolderLoadTask : public Task { void executeTask() override { + if (thread() != m_thread_to_spawn_into) + connect(this, &Task::finished, this->thread(), &QThread::quit); + m_dir.refresh(); for (auto entry : m_dir.entryInfoList()) { auto resource = m_create_func(entry); + resource->moveToThread(m_thread_to_spawn_into); m_result->resources.insert(resource->internal_id(), resource); } @@ -61,4 +66,7 @@ private: std::atomic m_aborted = false; std::function m_create_func; + + /** This is the thread in which we should put new mod objects */ + QThread* m_thread_to_spawn_into; }; diff --git a/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp b/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp index afe892f4..40a6ba18 100644 --- a/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp +++ b/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp @@ -38,12 +38,23 @@ #include "minecraft/mod/MetadataHandler.h" +#include + ModFolderLoadTask::ModFolderLoadTask(QDir mods_dir, QDir index_dir, bool is_indexed, bool clean_orphan) - : Task(nullptr, false), m_mods_dir(mods_dir), m_index_dir(index_dir), m_is_indexed(is_indexed), m_clean_orphan(clean_orphan), m_result(new Result()) + : Task(nullptr, false) + , m_mods_dir(mods_dir) + , m_index_dir(index_dir) + , m_is_indexed(is_indexed) + , m_clean_orphan(clean_orphan) + , m_result(new Result()) + , m_thread_to_spawn_into(thread()) {} void ModFolderLoadTask::executeTask() { + if (thread() != m_thread_to_spawn_into) + connect(this, &Task::finished, this->thread(), &QThread::quit); + if (m_is_indexed) { // Read metadata first getFromMetadata(); @@ -98,6 +109,9 @@ void ModFolderLoadTask::executeTask() } } + for (auto mod : m_result->mods) + mod->moveToThread(m_thread_to_spawn_into); + if (m_aborted) emit finished(); else diff --git a/launcher/minecraft/mod/tasks/ModFolderLoadTask.h b/launcher/minecraft/mod/tasks/ModFolderLoadTask.h index 0f18b8b9..af5f58a5 100644 --- a/launcher/minecraft/mod/tasks/ModFolderLoadTask.h +++ b/launcher/minecraft/mod/tasks/ModFolderLoadTask.h @@ -79,4 +79,7 @@ private: ResultPtr m_result; std::atomic m_aborted = false; + + /** This is the thread in which we should put new mod objects */ + QThread* m_thread_to_spawn_into; }; From c9eb584ac80956730dd56068945f6791e29716b3 Mon Sep 17 00:00:00 2001 From: flow Date: Fri, 16 Sep 2022 20:00:36 -0300 Subject: [PATCH 3/4] fix: prevent deletes by shared pointer accidental creation This fixes the launcher crashing when opening the game :iea: Signed-off-by: flow --- launcher/minecraft/MinecraftInstance.cpp | 2 +- launcher/minecraft/mod/ResourceFolderModel.cpp | 2 +- launcher/minecraft/mod/ResourceFolderModel.h | 6 +++--- launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/launcher/minecraft/MinecraftInstance.cpp b/launcher/minecraft/MinecraftInstance.cpp index 9478b1b8..47f53948 100644 --- a/launcher/minecraft/MinecraftInstance.cpp +++ b/launcher/minecraft/MinecraftInstance.cpp @@ -706,7 +706,7 @@ QStringList MinecraftInstance::verboseDescription(AuthSessionPtr session, Minecr { out << QString("%1:").arg(label); auto modList = model.allMods(); - std::sort(modList.begin(), modList.end(), [](Mod::Ptr a, Mod::Ptr b) { + std::sort(modList.begin(), modList.end(), [](auto a, auto b) { auto aName = a->fileinfo().completeBaseName(); auto bName = b->fileinfo().completeBaseName(); return aName.localeAwareCompare(bName) < 0; diff --git a/launcher/minecraft/mod/ResourceFolderModel.cpp b/launcher/minecraft/mod/ResourceFolderModel.cpp index 45d1db59..95bd5648 100644 --- a/launcher/minecraft/mod/ResourceFolderModel.cpp +++ b/launcher/minecraft/mod/ResourceFolderModel.cpp @@ -251,7 +251,7 @@ bool ResourceFolderModel::update() return true; } -void ResourceFolderModel::resolveResource(Resource::Ptr res) +void ResourceFolderModel::resolveResource(Resource* res) { if (!res->shouldResolve()) { return; diff --git a/launcher/minecraft/mod/ResourceFolderModel.h b/launcher/minecraft/mod/ResourceFolderModel.h index 5652c156..7edbe030 100644 --- a/launcher/minecraft/mod/ResourceFolderModel.h +++ b/launcher/minecraft/mod/ResourceFolderModel.h @@ -68,7 +68,7 @@ class ResourceFolderModel : public QAbstractListModel { virtual bool update(); /** Creates a new parse task, if needed, for 'res' and start it.*/ - virtual void resolveResource(Resource::Ptr res); + virtual void resolveResource(Resource* res); [[nodiscard]] size_t size() const { return m_resources.size(); }; [[nodiscard]] bool empty() const { return size() == 0; } @@ -265,7 +265,7 @@ void ResourceFolderModel::applyUpdates(QSet& current_set, QSet } m_resources[row].reset(new_resource); - resolveResource(m_resources.at(row)); + resolveResource(m_resources[row].get()); emit dataChanged(index(row, 0), index(row, columnCount(QModelIndex()) - 1)); } } @@ -313,7 +313,7 @@ void ResourceFolderModel::applyUpdates(QSet& current_set, QSet for (auto& added : added_set) { auto res = new_resources[added]; m_resources.append(res); - resolveResource(res); + resolveResource(m_resources.last().get()); } endInsertRows(); diff --git a/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp b/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp index 40a6ba18..78ef4386 100644 --- a/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp +++ b/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp @@ -99,7 +99,7 @@ void ModFolderLoadTask::executeTask() // Remove orphan metadata to prevent issues // See https://github.com/PolyMC/PolyMC/issues/996 if (m_clean_orphan) { - QMutableMapIterator iter(m_result->mods); + QMutableMapIterator iter(m_result->mods); while (iter.hasNext()) { auto mod = iter.next().value(); if (mod->status() == ModStatus::NotInstalled) { From 0873b8d3046db38a8dbdde70a3d7f294c14dcf86 Mon Sep 17 00:00:00 2001 From: flow Date: Fri, 16 Sep 2022 20:02:21 -0300 Subject: [PATCH 4/4] fix: prevent container detaching in ResourceFolderModel and use const accessors whenever we can! Signed-off-by: flow --- launcher/minecraft/mod/ModFolderModel.cpp | 2 +- launcher/minecraft/mod/ResourceFolderModel.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/launcher/minecraft/mod/ModFolderModel.cpp b/launcher/minecraft/mod/ModFolderModel.cpp index 13fed1c9..9aca686f 100644 --- a/launcher/minecraft/mod/ModFolderModel.cpp +++ b/launcher/minecraft/mod/ModFolderModel.cpp @@ -234,7 +234,7 @@ auto ModFolderModel::allMods() -> QList { QList mods; - for (auto& res : m_resources) { + for (auto& res : qAsConst(m_resources)) { mods.append(static_cast(res.get())); } diff --git a/launcher/minecraft/mod/ResourceFolderModel.h b/launcher/minecraft/mod/ResourceFolderModel.h index 7edbe030..25095a45 100644 --- a/launcher/minecraft/mod/ResourceFolderModel.h +++ b/launcher/minecraft/mod/ResourceFolderModel.h @@ -247,7 +247,7 @@ void ResourceFolderModel::applyUpdates(QSet& current_set, QSet auto row = row_it.value(); auto& new_resource = new_resources[kept]; - auto const& current_resource = m_resources[row]; + auto const& current_resource = m_resources.at(row); if (new_resource->dateTimeChanged() == current_resource->dateTimeChanged()) { // no significant change, ignore... @@ -265,7 +265,7 @@ void ResourceFolderModel::applyUpdates(QSet& current_set, QSet } m_resources[row].reset(new_resource); - resolveResource(m_resources[row].get()); + resolveResource(m_resources.at(row).get()); emit dataChanged(index(row, 0), index(row, columnCount(QModelIndex()) - 1)); } } @@ -324,7 +324,7 @@ void ResourceFolderModel::applyUpdates(QSet& current_set, QSet { m_resources_index.clear(); int idx = 0; - for (auto const& mod : m_resources) { + for (auto const& mod : qAsConst(m_resources)) { m_resources_index[mod->internal_id()] = idx; idx++; }