From 80c0f2a21e6f549fff129a460d80b15a3f47d48f Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Fri, 2 Feb 2018 11:28:25 -0800 Subject: [PATCH] Fix crash when passing --checkMinSpec flag That flag caused a DLL to be loaded before Application was instanced. This triggers a Qt bug inside Q_COREAPP_STARTUP_FUNC that causes the previous registration pointing the startup function in the main executable to be overridden with the address of the function in the DLL (Since they both link the same static library) This leads to the correct function running in the wrong address space (the DLLs), hence not initializing some global variables correctly. --- assignment-client/src/main.cpp | 3 + domain-server/src/main.cpp | 2 + interface/src/main.cpp | 2 + libraries/shared/src/SettingInterface.cpp | 69 ++++++++++++++--------- libraries/shared/src/SettingManager.cpp | 6 +- libraries/shared/src/SharedUtil.cpp | 9 ++- libraries/shared/src/SharedUtil.h | 34 +---------- tools/ac-client/src/main.cpp | 5 +- tools/atp-client/src/main.cpp | 3 +- tools/oven/src/main.cpp | 8 ++- 10 files changed, 68 insertions(+), 73 deletions(-) diff --git a/assignment-client/src/main.cpp b/assignment-client/src/main.cpp index 4f64bf8f7f..bf4497984f 100644 --- a/assignment-client/src/main.cpp +++ b/assignment-client/src/main.cpp @@ -23,9 +23,12 @@ int main(int argc, char* argv[]) { QCoreApplication::setOrganizationDomain(BuildInfo::ORGANIZATION_DOMAIN); QCoreApplication::setApplicationVersion(BuildInfo::VERSION); + setupGlobalInstances(); + qInstallMessageHandler(LogHandler::verboseMessageHandler); qInfo() << "Starting."; + AssignmentClientApp app(argc, argv); int acReturn = app.exec(); diff --git a/domain-server/src/main.cpp b/domain-server/src/main.cpp index dc3ee54fe7..e258626223 100644 --- a/domain-server/src/main.cpp +++ b/domain-server/src/main.cpp @@ -29,6 +29,8 @@ int main(int argc, char* argv[]) { QCoreApplication::setOrganizationDomain(BuildInfo::ORGANIZATION_DOMAIN); QCoreApplication::setApplicationVersion(BuildInfo::VERSION); + setupGlobalInstances(); + Setting::init(); #ifndef WIN32 diff --git a/interface/src/main.cpp b/interface/src/main.cpp index 30e8439985..0f8134b253 100644 --- a/interface/src/main.cpp +++ b/interface/src/main.cpp @@ -62,6 +62,8 @@ int main(int argc, const char* argv[]) { QCoreApplication::setOrganizationDomain(BuildInfo::ORGANIZATION_DOMAIN); QCoreApplication::setApplicationVersion(BuildInfo::VERSION); + setupGlobalInstances(); + Setting::init(); // Instance UserActivityLogger now that the settings are loaded diff --git a/libraries/shared/src/SettingInterface.cpp b/libraries/shared/src/SettingInterface.cpp index 327668574e..62f116795e 100644 --- a/libraries/shared/src/SettingInterface.cpp +++ b/libraries/shared/src/SettingInterface.cpp @@ -23,43 +23,53 @@ #include "SharedUtil.h" namespace Setting { - static QSharedPointer globalManager; - // cleans up the settings private instance. Should only be run once at closing down. void cleanupPrivateInstance() { + auto globalManager = DependencyManager::get(); + Q_ASSERT(qApp && globalManager); + // grab the thread before we nuke the instance - QThread* settingsManagerThread = DependencyManager::get()->thread(); + QThread* settingsManagerThread = globalManager->thread(); - // tell the private instance to clean itself up on its thread - DependencyManager::destroy(); - - globalManager.reset(); - - // quit the settings manager thread and wait on it to make sure it's gone + // quit the settings manager thread settingsManagerThread->quit(); settingsManagerThread->wait(); + + // Save all settings + globalManager->saveAll(); + + qCDebug(shared) << "Settings thread stopped."; } void setupPrivateInstance() { - // Ensure Setting::init has already ran and qApp exists - if (qApp && globalManager) { - // Let's set up the settings Private instance on its own thread - QThread* thread = new QThread(); - Q_CHECK_PTR(thread); - thread->setObjectName("Settings Thread"); + auto globalManager = DependencyManager::get(); + Q_ASSERT(qApp && globalManager); - QObject::connect(thread, SIGNAL(started()), globalManager.data(), SLOT(startTimer())); - QObject::connect(thread, SIGNAL(finished()), thread, SLOT(deleteLater())); - QObject::connect(thread, SIGNAL(finished()), globalManager.data(), SLOT(deleteLater())); - globalManager->moveToThread(thread); - thread->start(); - qCDebug(shared) << "Settings thread started."; + // Let's set up the settings private instance on its own thread + QThread* thread = new QThread(qApp); + Q_CHECK_PTR(thread); + thread->setObjectName("Settings Thread"); - // Register cleanupPrivateInstance to run inside QCoreApplication's destructor. - qAddPostRoutine(cleanupPrivateInstance); - } + // Setup setting periodical save timer + QObject::connect(thread, &QThread::started, globalManager.data(), &Manager::startTimer); + QObject::connect(thread, &QThread::finished, globalManager.data(), &Manager::stopTimer); + + // Setup manager threading affinity + globalManager->moveToThread(thread); + QObject::connect(thread, &QThread::finished, globalManager.data(), [] { + auto globalManager = DependencyManager::get(); + Q_ASSERT(qApp && globalManager); + + // Move manager back to the main thread (has to be done on owning thread) + globalManager->moveToThread(qApp->thread()); + }); + + thread->start(); + qCDebug(shared) << "Settings thread started."; + + // Register cleanupPrivateInstance to run inside QCoreApplication's destructor. + qAddPostRoutine(cleanupPrivateInstance); } - FIXED_Q_COREAPP_STARTUP_FUNCTION(setupPrivateInstance) // Sets up the settings private instance. Should only be run once at startup. preInit() must be run beforehand, void init() { @@ -68,6 +78,7 @@ namespace Setting { QSettings settings; qCDebug(shared) << "Settings file:" << settings.fileName(); + // Backward compatibility for old settings file if (settings.allKeys().isEmpty()) { loadOldINIFile(settings); } @@ -80,11 +91,13 @@ namespace Setting { qCDebug(shared) << (deleted ? "Deleted" : "Failed to delete") << "settings lock file" << settingsLockFilename; } - globalManager = DependencyManager::set(); + // Setup settings manager + DependencyManager::set(); - setupPrivateInstance(); + // Add pre-routine to setup threading + qAddPreRoutine(setupPrivateInstance); } - + void Interface::init() { if (!DependencyManager::isSet()) { // WARNING: As long as we are using QSettings this should always be triggered for each Setting::Handle diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp index 6c246d4cea..2e0850255a 100644 --- a/libraries/shared/src/SettingManager.cpp +++ b/libraries/shared/src/SettingManager.cpp @@ -23,11 +23,7 @@ namespace Setting { // Cleanup timer stopTimer(); delete _saveTimer; - - // Save all settings before exit - saveAll(); - - // sync will be called in the QSettings destructor + _saveTimer = nullptr; } // Custom deleter does nothing, because we need to shutdown later than the dependency manager diff --git a/libraries/shared/src/SharedUtil.cpp b/libraries/shared/src/SharedUtil.cpp index 8e5c30711c..772340b631 100644 --- a/libraries/shared/src/SharedUtil.cpp +++ b/libraries/shared/src/SharedUtil.cpp @@ -63,12 +63,12 @@ extern "C" FILE * __cdecl __iob_func(void) { #include "OctalCode.h" #include "SharedLogging.h" +static std::mutex stagedGlobalInstancesMutex; static std::unordered_map stagedGlobalInstances; std::mutex& globalInstancesMutex() { - static std::mutex mutex; - return mutex; + return stagedGlobalInstancesMutex; } static void commitGlobalInstances() { @@ -78,7 +78,10 @@ static void commitGlobalInstances() { } stagedGlobalInstances.clear(); } -FIXED_Q_COREAPP_STARTUP_FUNCTION(commitGlobalInstances) + +void setupGlobalInstances() { + qAddPreRoutine(commitGlobalInstances); +} QVariant getGlobalInstance(const char* propertyName) { if (qApp) { diff --git a/libraries/shared/src/SharedUtil.h b/libraries/shared/src/SharedUtil.h index 5a1e48d9c0..64bbc6585b 100644 --- a/libraries/shared/src/SharedUtil.h +++ b/libraries/shared/src/SharedUtil.h @@ -25,23 +25,6 @@ #include #include -// Workaround for https://bugreports.qt.io/browse/QTBUG-54479 -// Wrap target function inside another function that holds -// a unique string identifier and uses it to ensure it only runs once -// by storing a state within the qApp -// We cannot used std::call_once with a static once_flag because -// this is used in shared libraries that are linked by several DLLs -// (ie. plugins), meaning the static will be useless in that case -#define FIXED_Q_COREAPP_STARTUP_FUNCTION(AFUNC) \ - static void AFUNC ## _fixed() { \ - const auto propertyName = std::string(Q_FUNC_INFO) + __FILE__; \ - if (!qApp->property(propertyName.c_str()).toBool()) { \ - AFUNC(); \ - qApp->setProperty(propertyName.c_str(), QVariant(true)); \ - } \ - } \ - Q_COREAPP_STARTUP_FUNCTION(AFUNC ## _fixed) - // When writing out avatarEntities to a QByteArray, if the parentID is the ID of MyAvatar, use this ID instead. This allows // the value to be reset when the sessionID changes. const QUuid AVATAR_SELF_ID = QUuid("{00000000-0000-0000-0000-000000000001}"); @@ -53,21 +36,7 @@ std::unique_ptr& globalInstancePointer() { return instancePtr; } -template -void setGlobalInstance(const char* propertyName, T* instance) { - globalInstancePointer().reset(instance); -} - -template -bool destroyGlobalInstance() { - std::unique_ptr& instancePtr = globalInstancePointer(); - if (instancePtr.get()) { - instancePtr.reset(); - return true; - } - return false; -} - +void setupGlobalInstances(); std::mutex& globalInstancesMutex(); QVariant getGlobalInstance(const char* propertyName); void setGlobalInstance(const char* propertyName, const QVariant& variant); @@ -78,7 +47,6 @@ void setGlobalInstance(const char* propertyName, const QVariant& variant); template T* globalInstance(const char* propertyName, Args&&... args) { static T* resultInstance { nullptr }; - static std::mutex mutex; if (!resultInstance) { std::unique_lock lock(globalInstancesMutex()); if (!resultInstance) { diff --git a/tools/ac-client/src/main.cpp b/tools/ac-client/src/main.cpp index c9affde3b5..139e44bc9a 100644 --- a/tools/ac-client/src/main.cpp +++ b/tools/ac-client/src/main.cpp @@ -19,15 +19,16 @@ using namespace std; -int main(int argc, char * argv[]) { +int main(int argc, char* argv[]) { QCoreApplication::setApplicationName(BuildInfo::AC_CLIENT_SERVER_NAME); QCoreApplication::setOrganizationName(BuildInfo::MODIFIED_ORGANIZATION); QCoreApplication::setOrganizationDomain(BuildInfo::ORGANIZATION_DOMAIN); QCoreApplication::setApplicationVersion(BuildInfo::VERSION); + setupGlobalInstances(); + Setting::init(); ACClientApp app(argc, argv); - return app.exec(); } diff --git a/tools/atp-client/src/main.cpp b/tools/atp-client/src/main.cpp index 830c049bc7..0a8274fedd 100644 --- a/tools/atp-client/src/main.cpp +++ b/tools/atp-client/src/main.cpp @@ -25,9 +25,10 @@ int main(int argc, char * argv[]) { QCoreApplication::setOrganizationDomain(BuildInfo::ORGANIZATION_DOMAIN); QCoreApplication::setApplicationVersion(BuildInfo::VERSION); + setupGlobalInstances(); + Setting::init(); ATPClientApp app(argc, argv); - return app.exec(); } diff --git a/tools/oven/src/main.cpp b/tools/oven/src/main.cpp index 788470b75e..be0c22a0c6 100644 --- a/tools/oven/src/main.cpp +++ b/tools/oven/src/main.cpp @@ -10,11 +10,17 @@ #include "Oven.h" +#include #include +#include int main (int argc, char** argv) { - QCoreApplication::setOrganizationName("High Fidelity"); QCoreApplication::setApplicationName("Oven"); + QCoreApplication::setOrganizationName(BuildInfo::MODIFIED_ORGANIZATION); + QCoreApplication::setOrganizationDomain(BuildInfo::ORGANIZATION_DOMAIN); + QCoreApplication::setApplicationVersion(BuildInfo::VERSION); + + setupGlobalInstances(); // init the settings interface so we can save and load settings Setting::init();