From 09c9da268fdc833304f6159735cfb7e80d0e69bc Mon Sep 17 00:00:00 2001 From: Rachel Powers <508861+Ryex@users.noreply.github.com> Date: Fri, 11 Oct 2024 13:26:20 -0700 Subject: [PATCH] fix: prevent inf recursion when mod icon load fails; cut max pixmapcache to 1/4 previous value Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com> --- launcher/MTPixmapCache.h | 6 +-- launcher/minecraft/mod/Mod.cpp | 41 +++++++++++-------- launcher/minecraft/mod/Mod.h | 8 ++-- .../minecraft/mod/tasks/LocalModParseTask.cpp | 41 ++++++++++--------- .../minecraft/mod/tasks/LocalModParseTask.h | 4 +- 5 files changed, 55 insertions(+), 45 deletions(-) diff --git a/launcher/MTPixmapCache.h b/launcher/MTPixmapCache.h index b6bd13045..0ba9c5ac8 100644 --- a/launcher/MTPixmapCache.h +++ b/launcher/MTPixmapCache.h @@ -101,7 +101,7 @@ class PixmapCache final : public QObject { */ bool _markCacheMissByEviciton() { - static constexpr uint maxInt = static_cast(std::numeric_limits::max()); + static constexpr uint maxCache = static_cast(std::numeric_limits::max()) / 4; static constexpr uint step = 10240; static constexpr int oneSecond = 1000; @@ -118,8 +118,8 @@ class PixmapCache final : public QObject { if (m_consecutive_fast_evicitons >= m_consecutive_fast_evicitons_threshold) { // increase the cache size uint newSize = _cacheLimit() + step; - if (newSize >= maxInt) { // increase it until you overflow :D - newSize = maxInt; + if (newSize >= maxCache) { // increase it until you overflow :D + newSize = maxCache; qDebug() << m_consecutive_fast_evicitons << tr("pixmap cache misses by eviction happened too fast, doing nothing as the cache size reached it's limit"); } else { diff --git a/launcher/minecraft/mod/Mod.cpp b/launcher/minecraft/mod/Mod.cpp index 5442df7fe..9663026cd 100644 --- a/launcher/minecraft/mod/Mod.cpp +++ b/launcher/minecraft/mod/Mod.cpp @@ -36,6 +36,7 @@ */ #include "Mod.h" +#include #include #include @@ -241,7 +242,7 @@ void Mod::finishResolvingWithDetails(ModDetails&& details) if (metadata) setMetadata(std::move(metadata)); if (!iconPath().isEmpty()) { - m_pack_image_cache_key.was_read_attempt = false; + m_packImageCacheKey.wasReadAttempt = false; } } @@ -290,45 +291,53 @@ auto Mod::issueTracker() const -> QString return details().issue_tracker; } -void Mod::setIcon(QImage new_image) const +QPixmap Mod::setIcon(QImage new_image) const { QMutexLocker locker(&m_data_lock); Q_ASSERT(!new_image.isNull()); - if (m_pack_image_cache_key.key.isValid()) - PixmapCache::remove(m_pack_image_cache_key.key); + if (m_packImageCacheKey.key.isValid()) + PixmapCache::remove(m_packImageCacheKey.key); // scale the image to avoid flooding the pixmapcache auto pixmap = QPixmap::fromImage(new_image.scaled({ 64, 64 }, Qt::AspectRatioMode::KeepAspectRatioByExpanding, Qt::SmoothTransformation)); - m_pack_image_cache_key.key = PixmapCache::insert(pixmap); - m_pack_image_cache_key.was_ever_used = true; - m_pack_image_cache_key.was_read_attempt = true; + m_packImageCacheKey.key = PixmapCache::insert(pixmap); + m_packImageCacheKey.wasEverUsed = true; + m_packImageCacheKey.wasReadAttempt = true; + return pixmap; } QPixmap Mod::icon(QSize size, Qt::AspectRatioMode mode) const { - QPixmap cached_image; - if (PixmapCache::find(m_pack_image_cache_key.key, &cached_image)) { + auto pixmap_transform = [&size, &mode](QPixmap pixmap) { if (size.isNull()) - return cached_image; - return cached_image.scaled(size, mode, Qt::SmoothTransformation); + return pixmap; + return pixmap.scaled(size, mode, Qt::SmoothTransformation); + }; + + QPixmap cached_image; + if (PixmapCache::find(m_packImageCacheKey.key, &cached_image)) { + return pixmap_transform(cached_image); } // No valid image we can get - if ((!m_pack_image_cache_key.was_ever_used && m_pack_image_cache_key.was_read_attempt) || iconPath().isEmpty()) + if ((!m_packImageCacheKey.wasEverUsed && m_packImageCacheKey.wasReadAttempt) || iconPath().isEmpty()) return {}; - if (m_pack_image_cache_key.was_ever_used) { + if (m_packImageCacheKey.wasEverUsed) { qDebug() << "Mod" << name() << "Had it's icon evicted from the cache. reloading..."; PixmapCache::markCacheMissByEviciton(); } // Image got evicted from the cache or an attempt to load it has not been made. load it and retry. - m_pack_image_cache_key.was_read_attempt = true; - ModUtils::loadIconFile(*this); - return icon(size); + m_packImageCacheKey.wasReadAttempt = true; + if (ModUtils::loadIconFile(*this, &cached_image)) { + return pixmap_transform(cached_image); + } + // Image failed to load + return {}; } bool Mod::valid() const diff --git a/launcher/minecraft/mod/Mod.h b/launcher/minecraft/mod/Mod.h index 9bd76c2fd..a0d9797ed 100644 --- a/launcher/minecraft/mod/Mod.h +++ b/launcher/minecraft/mod/Mod.h @@ -82,7 +82,7 @@ class Mod : public Resource { /** Gets the icon of the mod, converted to a QPixmap for drawing, and scaled to size. */ [[nodiscard]] QPixmap icon(QSize size, Qt::AspectRatioMode mode = Qt::AspectRatioMode::IgnoreAspectRatio) const; /** Thread-safe. */ - void setIcon(QImage new_image) const; + QPixmap setIcon(QImage new_image) const; auto metadata() -> std::shared_ptr; auto metadata() const -> const std::shared_ptr; @@ -111,7 +111,7 @@ class Mod : public Resource { struct { QPixmapCache::Key key; - bool was_ever_used = false; - bool was_read_attempt = false; - } mutable m_pack_image_cache_key; + bool wasEverUsed = false; + bool wasReadAttempt = false; + } mutable m_packImageCacheKey; }; diff --git a/launcher/minecraft/mod/tasks/LocalModParseTask.cpp b/launcher/minecraft/mod/tasks/LocalModParseTask.cpp index 60257ce0c..d456211f8 100644 --- a/launcher/minecraft/mod/tasks/LocalModParseTask.cpp +++ b/launcher/minecraft/mod/tasks/LocalModParseTask.cpp @@ -647,11 +647,11 @@ bool validate(QFileInfo file) return ModUtils::process(mod, ProcessingLevel::BasicInfoOnly) && mod.valid(); } -bool processIconPNG(const Mod& mod, QByteArray&& raw_data) +bool processIconPNG(const Mod& mod, QByteArray&& raw_data, QPixmap* pixmap) { auto img = QImage::fromData(raw_data); if (!img.isNull()) { - mod.setIcon(img); + *pixmap = mod.setIcon(img); } else { qWarning() << "Failed to parse mod logo:" << mod.iconPath() << "from" << mod.name(); return false; @@ -659,15 +659,15 @@ bool processIconPNG(const Mod& mod, QByteArray&& raw_data) return true; } -bool loadIconFile(const Mod& mod) +bool loadIconFile(const Mod& mod, QPixmap* pixmap) { if (mod.iconPath().isEmpty()) { qWarning() << "No Iconfile set, be sure to parse the mod first"; return false; } - auto png_invalid = [&mod]() { - qWarning() << "Mod at" << mod.fileinfo().filePath() << "does not have a valid icon"; + auto png_invalid = [&mod](const QString& reason) { + qWarning() << "Mod at" << mod.fileinfo().filePath() << "does not have a valid icon:" << reason; return false; }; @@ -676,24 +676,26 @@ bool loadIconFile(const Mod& mod) QFileInfo icon_info(FS::PathCombine(mod.fileinfo().filePath(), mod.iconPath())); if (icon_info.exists() && icon_info.isFile()) { QFile icon(icon_info.filePath()); - if (!icon.open(QIODevice::ReadOnly)) - return false; + if (!icon.open(QIODevice::ReadOnly)) { + return png_invalid("failed to open file " + icon_info.filePath()); + } auto data = icon.readAll(); - bool icon_result = ModUtils::processIconPNG(mod, std::move(data)); + bool icon_result = ModUtils::processIconPNG(mod, std::move(data), pixmap); icon.close(); if (!icon_result) { - return png_invalid(); // icon invalid + return png_invalid("invalid png image"); // icon invalid } + return true; } - return false; + return png_invalid("file '" + icon_info.filePath() + "' does not exists or is not a file"); } case ResourceType::ZIPFILE: { QuaZip zip(mod.fileinfo().filePath()); if (!zip.open(QuaZip::mdUnzip)) - return false; + return png_invalid("failed to open '" + mod.fileinfo().filePath() + "' as a zip archive"); QuaZipFile file(&zip); @@ -701,28 +703,27 @@ bool loadIconFile(const Mod& mod) if (!file.open(QIODevice::ReadOnly)) { qCritical() << "Failed to open file in zip."; zip.close(); - return png_invalid(); + return png_invalid("Failed to open '" + mod.iconPath() + "' in zip archive"); } auto data = file.readAll(); - bool icon_result = ModUtils::processIconPNG(mod, std::move(data)); + bool icon_result = ModUtils::processIconPNG(mod, std::move(data), pixmap); file.close(); if (!icon_result) { - return png_invalid(); // icon png invalid + return png_invalid("invalid png image"); // icon png invalid } - } else { - return png_invalid(); // could not set icon as current file. + return true; } - return false; + return png_invalid("Failed to set '" + mod.iconPath() + + "' as current file in zip archive"); // could not set icon as current file. } case ResourceType::LITEMOD: { - return false; // can lightmods even have icons? + return png_invalid("litemods do not have icons"); // can lightmods even have icons? } default: - qWarning() << "Invalid type for mod, can not load icon."; - return false; + return png_invalid("Invalid type for mod, can not load icon."); } } diff --git a/launcher/minecraft/mod/tasks/LocalModParseTask.h b/launcher/minecraft/mod/tasks/LocalModParseTask.h index a03217093..91ee6f253 100644 --- a/launcher/minecraft/mod/tasks/LocalModParseTask.h +++ b/launcher/minecraft/mod/tasks/LocalModParseTask.h @@ -26,8 +26,8 @@ bool processLitemod(Mod& mod, ProcessingLevel level = ProcessingLevel::Full); /** Checks whether a file is valid as a mod or not. */ bool validate(QFileInfo file); -bool processIconPNG(const Mod& mod, QByteArray&& raw_data); -bool loadIconFile(const Mod& mod); +bool processIconPNG(const Mod& mod, QByteArray&& raw_data, QPixmap* pixmap); +bool loadIconFile(const Mod& mod, QPixmap* pixmap); } // namespace ModUtils class LocalModParseTask : public Task {