From 6c66f5a37bb0209ddd9d8d7dbd5d6e6855c7c5dc Mon Sep 17 00:00:00 2001 From: Anthony Thibault Date: Mon, 1 Jul 2019 17:37:57 -0700 Subject: [PATCH] Fix for DependencyManager crash on shutdown on Mac On Mac, it is possible to crash when shutting down, it is not clear if this is due to shutting down the app on another thread during logout or something that can happen during normal shutdown, because it is so difficult to reproduce. However, from looking at the stack traces it is possible for a [NSApplication terminate:] event to get processed while Appliction::aboutToQuit() is calling ScriptEngine::waitTillDoneRunning() This causes AppKit to invoke the static destructors too early. Which in turn, causes the DependencyManager destructor to fire while there are still many dependencies running. Unfortunatly, the order of destruction is not determinstic, causing them to get shutdown in an incorrect order. To workaround this, we delay the call to QCoreApplication::processEvents() as late as possible, in the Application destructor. Theoretically, this will be a safe time for the static destructors to be invoked, because it is after all of the DependencyManager's dependencies have been manually destroyed. However, this is only a speculative fix, because this is so difficult to reproduce. --- interface/src/Application.cpp | 17 ++++++++++-- libraries/baking/src/MaterialBaker.cpp | 4 +-- .../src/RenderableMaterialEntityItem.cpp | 4 +-- .../src/material-networking/MaterialCache.cpp | 7 +---- .../src/material-networking/MaterialCache.h | 7 ++--- .../model-baker/ParseMaterialMappingTask.cpp | 2 +- libraries/networking/src/AccountManager.cpp | 3 ++- libraries/script-engine/src/ScriptEngine.cpp | 26 +++++++++++++++++-- 8 files changed, 51 insertions(+), 19 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 3c80d5fa0f..8ecb16b65f 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -104,6 +104,7 @@ #include #include #include +#include #include #include #include @@ -877,6 +878,7 @@ bool setupEssentials(int& argc, char** argv, bool runningMarkerExisted) { DependencyManager::set(); DependencyManager::set(); DependencyManager::set(); + DependencyManager::set(); DependencyManager::set(); DependencyManager::set(); DependencyManager::set(); @@ -2852,6 +2854,7 @@ Application::~Application() { DependencyManager::destroy(); DependencyManager::destroy(); DependencyManager::destroy(); + DependencyManager::destroy(); DependencyManager::destroy(); DependencyManager::destroy(); DependencyManager::destroy(); @@ -2887,6 +2890,16 @@ Application::~Application() { // Can't log to file past this point, FileLogger about to be deleted qInstallMessageHandler(LogHandler::verboseMessageHandler); + +#ifdef Q_OS_MAC + // Clear the event queue before application is totally destructed. + // This will drain the messasge queue of pending "deleteLaters" queued up + // during shutdown of the script engines. + // We do this here because there is a possiblty that [NSApplication terminate:] + // will be called during processEvents which will invoke all static destructors. + // We want to postpone this utill the last possible moment. + QCoreApplication::processEvents(); +#endif } void Application::initializeGL() { @@ -5960,7 +5973,7 @@ void Application::reloadResourceCaches() { DependencyManager::get()->clear(); DependencyManager::get()->refreshAll(); DependencyManager::get()->refreshAll(); - MaterialCache::instance().refreshAll(); + DependencyManager::get()->refreshAll(); DependencyManager::get()->refreshAll(); ShaderCache::instance().refreshAll(); DependencyManager::get()->refreshAll(); @@ -7146,7 +7159,7 @@ void Application::clearDomainOctreeDetails(bool clearAll) { DependencyManager::get()->clearUnusedResources(); DependencyManager::get()->clearUnusedResources(); - MaterialCache::instance().clearUnusedResources(); + DependencyManager::get()->clearUnusedResources(); DependencyManager::get()->clearUnusedResources(); ShaderCache::instance().clearUnusedResources(); DependencyManager::get()->clearUnusedResources(); diff --git a/libraries/baking/src/MaterialBaker.cpp b/libraries/baking/src/MaterialBaker.cpp index e23b76f73a..9a1b1b2d24 100644 --- a/libraries/baking/src/MaterialBaker.cpp +++ b/libraries/baking/src/MaterialBaker.cpp @@ -72,7 +72,7 @@ void MaterialBaker::loadMaterial() { _materialResource->parsedMaterials = NetworkMaterialResource::parseJSONMaterials(QJsonDocument::fromJson(_materialData.toUtf8()), QUrl()); } else { qCDebug(material_baking) << "Downloading material" << _materialData; - _materialResource = MaterialCache::instance().getMaterial(_materialData); + _materialResource = DependencyManager::get()->getMaterial(_materialData); } if (_materialResource) { @@ -280,4 +280,4 @@ void MaterialBaker::setMaterials(const QHash& materials, void MaterialBaker::setMaterials(const NetworkMaterialResourcePointer& materialResource) { _materialResource = materialResource; -} \ No newline at end of file +} diff --git a/libraries/entities-renderer/src/RenderableMaterialEntityItem.cpp b/libraries/entities-renderer/src/RenderableMaterialEntityItem.cpp index 01eac7b203..22e3205531 100644 --- a/libraries/entities-renderer/src/RenderableMaterialEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableMaterialEntityItem.cpp @@ -172,7 +172,7 @@ void MaterialEntityRenderer::doRenderUpdateAsynchronousTyped(const TypedEntityPo } if (urlChanged && !usingMaterialData) { - _networkMaterial = MaterialCache::instance().getMaterial(_materialURL); + _networkMaterial = DependencyManager::get()->getMaterial(_materialURL); auto onMaterialRequestFinished = [this, oldParentID, oldParentMaterialName, newCurrentMaterialName](bool success) { if (success) { deleteMaterial(oldParentID, oldParentMaterialName); @@ -412,4 +412,4 @@ void MaterialEntityRenderer::applyMaterial() { // if we've reached this point, we couldn't find our parent, so we need to try again later _retryApply = true; -} \ No newline at end of file +} diff --git a/libraries/material-networking/src/material-networking/MaterialCache.cpp b/libraries/material-networking/src/material-networking/MaterialCache.cpp index cfe3b73e49..fd218fe074 100644 --- a/libraries/material-networking/src/material-networking/MaterialCache.cpp +++ b/libraries/material-networking/src/material-networking/MaterialCache.cpp @@ -441,11 +441,6 @@ std::pair> NetworkMaterialResource return std::pair>(name, material); } -MaterialCache& MaterialCache::instance() { - static MaterialCache _instance; - return _instance; -} - NetworkMaterialResourcePointer MaterialCache::getMaterial(const QUrl& url) { return ResourceCache::getResource(url).staticCast(); } @@ -761,4 +756,4 @@ void NetworkMaterial::checkResetOpacityMap() { if (albedoTexture.texture) { resetOpacityMap(); } -} \ No newline at end of file +} diff --git a/libraries/material-networking/src/material-networking/MaterialCache.h b/libraries/material-networking/src/material-networking/MaterialCache.h index 66e6b8250b..18aa5e5994 100644 --- a/libraries/material-networking/src/material-networking/MaterialCache.h +++ b/libraries/material-networking/src/material-networking/MaterialCache.h @@ -110,10 +110,11 @@ using NetworkMaterialResourcePointer = QSharedPointer; using MaterialMapping = std::vector>; Q_DECLARE_METATYPE(MaterialMapping) -class MaterialCache : public ResourceCache { -public: - static MaterialCache& instance(); +class MaterialCache : public ResourceCache, public Dependency { + Q_OBJECT + SINGLETON_DEPENDENCY +public: NetworkMaterialResourcePointer getMaterial(const QUrl& url); protected: diff --git a/libraries/model-baker/src/model-baker/ParseMaterialMappingTask.cpp b/libraries/model-baker/src/model-baker/ParseMaterialMappingTask.cpp index 17b62d0915..c4c4f7f74b 100644 --- a/libraries/model-baker/src/model-baker/ParseMaterialMappingTask.cpp +++ b/libraries/model-baker/src/model-baker/ParseMaterialMappingTask.cpp @@ -61,7 +61,7 @@ void processMaterialMapping(MaterialMapping& materialMapping, const QJsonObject& } else if (mappingJSON.isString()) { auto mappingValue = mappingJSON.toString(); materialMapping.push_back(std::pair(mapping.toStdString(), - MaterialCache::instance().getMaterial(url.resolved(mappingValue)))); + DependencyManager::get()->getMaterial(url.resolved(mappingValue)))); } } } diff --git a/libraries/networking/src/AccountManager.cpp b/libraries/networking/src/AccountManager.cpp index e2e9d33eb6..96cfa66013 100644 --- a/libraries/networking/src/AccountManager.cpp +++ b/libraries/networking/src/AccountManager.cpp @@ -92,6 +92,7 @@ const QString DOUBLE_SLASH_SUBSTITUTE = "slashslash"; const QString ACCOUNT_MANAGER_REQUESTED_SCOPE = "owner"; void AccountManager::logout() { + // a logout means we want to delete the DataServerAccountInfo we currently have for this URL, in-memory and in file _accountInfo = DataServerAccountInfo(); @@ -959,4 +960,4 @@ void AccountManager::saveLoginStatus(bool isLoggedIn) { QMetaObject::invokeMethod(qApp, "quit", Qt::QueuedConnection); } } -} \ No newline at end of file +} diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 02dcde3695..5e20f06a7f 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -224,7 +224,7 @@ ScriptEngine::ScriptEngine(Context context, const QString& scriptContents, const if (_type == Type::ENTITY_CLIENT || _type == Type::ENTITY_SERVER) { QObject::connect(this, &ScriptEngine::update, this, [this]() { // process pending entity script content - if (!_contentAvailableQueue.empty()) { + if (!_contentAvailableQueue.empty() && !(_isFinished || _isStopping)) { EntityScriptContentAvailableMap pending; std::swap(_contentAvailableQueue, pending); for (auto& pair : pending) { @@ -343,7 +343,7 @@ void ScriptEngine::runDebuggable() { // we check for 'now' in the past in case people set their clock back if (_lastUpdate < now) { float deltaTime = (float)(now - _lastUpdate) / (float)USECS_PER_SECOND; - if (!_isFinished) { + if (!(_isFinished || _isStopping)) { emit update(deltaTime); } } @@ -411,6 +411,27 @@ void ScriptEngine::waitTillDoneRunning() { // We should never be waiting (blocking) on our own thread assert(workerThread != QThread::currentThread()); +#ifdef Q_OS_MAC + // On mac, don't call QCoreApplication::processEvents() here. This is to prevent + // [NSApplication terminate:] from prematurely destroying the static destructors + // while we are waiting for the scripts to shutdown. We will pump the message + // queue later in the Application destructor. + if (workerThread->isRunning()) { + workerThread->quit(); + + if (isEvaluating()) { + qCWarning(scriptengine) << "Script Engine has been running too long, aborting:" << getFilename(); + abortEvaluation(); + } + + // Wait for the scripting thread to stop running, as + // flooding it with aborts/exceptions will persist it longer + static const auto MAX_SCRIPT_QUITTING_TIME = 0.5 * MSECS_PER_SECOND; + if (!workerThread->wait(MAX_SCRIPT_QUITTING_TIME)) { + workerThread->terminate(); + } + } +#else auto startedWaiting = usecTimestampNow(); while (workerThread->isRunning()) { // If the final evaluation takes too long, then tell the script engine to stop running @@ -448,6 +469,7 @@ void ScriptEngine::waitTillDoneRunning() { // Avoid a pure busy wait QThread::yieldCurrentThread(); } +#endif scriptInfoMessage("Script Engine has stopped:" + getFilename()); }