From e8673fc52af74b3d3a3e8f41e6f70d6235aef7ff Mon Sep 17 00:00:00 2001 From: nerix Date: Sun, 26 Nov 2023 16:54:19 +0100 Subject: [PATCH] fix: deadlock and use-after-free in tests (#4981) * fix: use-after-free in settings * refactor: put seventv api into a singleton * chore: add changelog entry * Add warning for when the 7TV load fails --- CHANGELOG.md | 1 + mocks/include/mocks/EmptyApplication.hpp | 5 ++++ src/Application.cpp | 2 ++ src/Application.hpp | 7 +++++ src/providers/seventv/SeventvAPI.cpp | 6 ----- src/providers/seventv/SeventvAPI.hpp | 34 +++++++++++++++--------- src/providers/seventv/SeventvBadges.cpp | 3 --- src/providers/seventv/SeventvEmotes.cpp | 7 ++--- src/providers/twitch/TwitchAccount.cpp | 10 ++++++- src/providers/twitch/TwitchChannel.cpp | 2 +- src/singletons/Settings.cpp | 6 ++++- src/singletons/Settings.hpp | 1 + 12 files changed, 57 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29e627f7..0590c9ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,7 @@ - Dev: Removed `Outcome` from network requests. (#4959) - Dev: Added Tests for Windows and MacOS in CI. (#4970) - Dev: Refactored the Image Uploader feature. (#4971) +- Dev: Fixed deadlock and use-after-free in tests. (#4981) ## 2.4.6 diff --git a/mocks/include/mocks/EmptyApplication.hpp b/mocks/include/mocks/EmptyApplication.hpp index 70ce3070..612aed43 100644 --- a/mocks/include/mocks/EmptyApplication.hpp +++ b/mocks/include/mocks/EmptyApplication.hpp @@ -94,6 +94,11 @@ public: { return nullptr; } + + SeventvAPI *getSeventvAPI() override + { + return nullptr; + } }; } // namespace chatterino::mock diff --git a/src/Application.cpp b/src/Application.cpp index 806d2155..84c940f6 100644 --- a/src/Application.cpp +++ b/src/Application.cpp @@ -10,6 +10,7 @@ #include "controllers/hotkeys/HotkeyController.hpp" #include "controllers/ignores/IgnoreController.hpp" #include "controllers/notifications/NotificationController.hpp" +#include "providers/seventv/SeventvAPI.hpp" #include "singletons/ImageUploader.hpp" #ifdef CHATTERINO_HAVE_PLUGINS # include "controllers/plugins/PluginController.hpp" @@ -81,6 +82,7 @@ Application::Application(Settings &_settings, Paths &_paths) , windows(&this->emplace()) , toasts(&this->emplace()) , imageUploader(&this->emplace()) + , seventvAPI(&this->emplace()) , commands(&this->emplace()) , notifications(&this->emplace()) diff --git a/src/Application.hpp b/src/Application.hpp index 3bedfd2c..38f0092c 100644 --- a/src/Application.hpp +++ b/src/Application.hpp @@ -42,6 +42,7 @@ class ChatterinoBadges; class FfzBadges; class SeventvBadges; class ImageUploader; +class SeventvAPI; class IApplication { @@ -68,6 +69,7 @@ public: virtual IUserDataController *getUserData() = 0; virtual ITwitchLiveController *getTwitchLiveController() = 0; virtual ImageUploader *getImageUploader() = 0; + virtual SeventvAPI *getSeventvAPI() = 0; }; class Application : public IApplication @@ -97,6 +99,7 @@ public: WindowManager *const windows{}; Toasts *const toasts{}; ImageUploader *const imageUploader{}; + SeventvAPI *const seventvAPI{}; CommandController *const commands{}; NotificationController *const notifications{}; @@ -174,6 +177,10 @@ public: { return this->imageUploader; } + SeventvAPI *getSeventvAPI() override + { + return this->seventvAPI; + } pajlada::Signals::NoArgSignal streamerModeChanged; diff --git a/src/providers/seventv/SeventvAPI.cpp b/src/providers/seventv/SeventvAPI.cpp index 67999a99..ba63b821 100644 --- a/src/providers/seventv/SeventvAPI.cpp +++ b/src/providers/seventv/SeventvAPI.cpp @@ -78,11 +78,5 @@ void SeventvAPI::updatePresence(const QString &twitchChannelID, .execute(); } -SeventvAPI &getSeventvAPI() -{ - static SeventvAPI instance; - return instance; -} - } // namespace chatterino // NOLINTEND(readability-convert-member-functions-to-static) diff --git a/src/providers/seventv/SeventvAPI.hpp b/src/providers/seventv/SeventvAPI.hpp index fd75345f..c47f9dd3 100644 --- a/src/providers/seventv/SeventvAPI.hpp +++ b/src/providers/seventv/SeventvAPI.hpp @@ -1,5 +1,7 @@ #pragma once +#include "common/Singleton.hpp" + #include class QString; @@ -9,25 +11,33 @@ namespace chatterino { class NetworkResult; -class SeventvAPI +class SeventvAPI : public Singleton { using ErrorCallback = std::function; template using SuccessCallback = std::function; public: - void getUserByTwitchID(const QString &twitchID, - SuccessCallback &&onSuccess, - ErrorCallback &&onError); - void getEmoteSet(const QString &emoteSet, - SuccessCallback &&onSuccess, - ErrorCallback &&onError); + SeventvAPI() = default; + ~SeventvAPI() override = default; - void updatePresence(const QString &twitchChannelID, - const QString &seventvUserID, - SuccessCallback<> &&onSuccess, ErrorCallback &&onError); + SeventvAPI(const SeventvAPI &) = delete; + SeventvAPI(SeventvAPI &&) = delete; + SeventvAPI &operator=(const SeventvAPI &) = delete; + SeventvAPI &operator=(SeventvAPI &&) = delete; + + virtual void getUserByTwitchID( + const QString &twitchID, + SuccessCallback &&onSuccess, + ErrorCallback &&onError); + virtual void getEmoteSet(const QString &emoteSet, + SuccessCallback &&onSuccess, + ErrorCallback &&onError); + + virtual void updatePresence(const QString &twitchChannelID, + const QString &seventvUserID, + SuccessCallback<> &&onSuccess, + ErrorCallback &&onError); }; -SeventvAPI &getSeventvAPI(); - } // namespace chatterino diff --git a/src/providers/seventv/SeventvBadges.cpp b/src/providers/seventv/SeventvBadges.cpp index 3ee36735..2f09bd08 100644 --- a/src/providers/seventv/SeventvBadges.cpp +++ b/src/providers/seventv/SeventvBadges.cpp @@ -2,15 +2,12 @@ #include "messages/Emote.hpp" #include "messages/Image.hpp" -#include "providers/seventv/SeventvAPI.hpp" #include "providers/seventv/SeventvEmotes.hpp" #include #include #include -#include - namespace chatterino { std::optional SeventvBadges::getBadge(const UserId &id) const diff --git a/src/providers/seventv/SeventvEmotes.cpp b/src/providers/seventv/SeventvEmotes.cpp index 4004d9ed..c7f90df6 100644 --- a/src/providers/seventv/SeventvEmotes.cpp +++ b/src/providers/seventv/SeventvEmotes.cpp @@ -1,5 +1,6 @@ #include "providers/seventv/SeventvEmotes.hpp" +#include "Application.hpp" #include "common/Literals.hpp" #include "common/NetworkResult.hpp" #include "common/QLogging.hpp" @@ -214,7 +215,7 @@ void SeventvEmotes::loadGlobalEmotes() qCDebug(chatterinoSeventv) << "Loading 7TV Global Emotes"; - getSeventvAPI().getEmoteSet( + getIApp()->getSeventvAPI()->getEmoteSet( u"global"_s, [this](const auto &json) { QJsonArray parsedEmotes = json["emotes"].toArray(); @@ -243,7 +244,7 @@ void SeventvEmotes::loadChannelEmotes( qCDebug(chatterinoSeventv) << "Reloading 7TV Channel Emotes" << channelId << manualRefresh; - getSeventvAPI().getUserByTwitchID( + getIApp()->getSeventvAPI()->getUserByTwitchID( channelId, [callback = std::move(callback), channel, channelId, manualRefresh](const auto &json) { @@ -405,7 +406,7 @@ void SeventvEmotes::getEmoteSet( { qCDebug(chatterinoSeventv) << "Loading 7TV Emote Set" << emoteSetId; - getSeventvAPI().getEmoteSet( + getIApp()->getSeventvAPI()->getEmoteSet( emoteSetId, [callback = std::move(successCallback), emoteSetId](const auto &json) { auto parsedEmotes = json["emotes"].toArray(); diff --git a/src/providers/twitch/TwitchAccount.cpp b/src/providers/twitch/TwitchAccount.cpp index 8e32d27b..df58f049 100644 --- a/src/providers/twitch/TwitchAccount.cpp +++ b/src/providers/twitch/TwitchAccount.cpp @@ -491,7 +491,15 @@ void TwitchAccount::loadSeventvUserID() return; } - getSeventvAPI().getUserByTwitchID( + auto *seventv = getIApp()->getSeventvAPI(); + if (!seventv) + { + qCWarning(chatterinoSeventv) + << "Not loading 7TV User ID because the 7TV API is not initialized"; + return; + } + + seventv->getUserByTwitchID( this->getUserId(), [this](const auto &json) { const auto id = json["user"]["id"].toString(); diff --git a/src/providers/twitch/TwitchChannel.cpp b/src/providers/twitch/TwitchChannel.cpp index 72e84da0..14670769 100644 --- a/src/providers/twitch/TwitchChannel.cpp +++ b/src/providers/twitch/TwitchChannel.cpp @@ -1646,7 +1646,7 @@ void TwitchChannel::updateSevenTVActivity() qCDebug(chatterinoSeventv) << "Sending activity in" << this->getName(); - getSeventvAPI().updatePresence( + getIApp()->getSeventvAPI()->updatePresence( this->roomId(), currentSeventvUserID, [chan = weakOf(this)]() { const auto self = diff --git a/src/singletons/Settings.cpp b/src/singletons/Settings.cpp index 8caa53c8..885f003d 100644 --- a/src/singletons/Settings.cpp +++ b/src/singletons/Settings.cpp @@ -137,6 +137,7 @@ bool Settings::toggleMutedChannel(const QString &channelName) Settings *Settings::instance_ = nullptr; Settings::Settings(const QString &settingsDirectory) + : prevInstance_(Settings::instance_) { QString settingsPath = settingsDirectory + "/settings.json"; @@ -188,7 +189,10 @@ Settings::Settings(const QString &settingsDirectory) false); } -Settings::~Settings() = default; +Settings::~Settings() +{ + Settings::instance_ = this->prevInstance_; +} void Settings::saveSnapshot() { diff --git a/src/singletons/Settings.hpp b/src/singletons/Settings.hpp index 2f953594..4a9af170 100644 --- a/src/singletons/Settings.hpp +++ b/src/singletons/Settings.hpp @@ -66,6 +66,7 @@ enum UsernameRightClickBehavior : int { class Settings { static Settings *instance_; + Settings *prevInstance_ = nullptr; public: Settings(const QString &settingsDirectory);