From 6589db628affa3e37bfdaf96cf49bc238d378a46 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Mon, 17 Dec 2018 12:03:16 -0800 Subject: [PATCH 1/7] fix a couple shutdown crashes and a class of asan warning --- .../src/scripting/MenuScriptingInterface.cpp | 8 ++--- libraries/networking/src/LimitedNodeList.cpp | 9 ----- libraries/networking/src/LimitedNodeList.h | 9 +++++ libraries/script-engine/src/ScriptEngine.cpp | 34 ++++++++++++------- libraries/script-engine/src/ScriptEngine.h | 4 +++ 5 files changed, 39 insertions(+), 25 deletions(-) diff --git a/interface/src/scripting/MenuScriptingInterface.cpp b/interface/src/scripting/MenuScriptingInterface.cpp index d6dc2fa703..ae6a7c7d67 100644 --- a/interface/src/scripting/MenuScriptingInterface.cpp +++ b/interface/src/scripting/MenuScriptingInterface.cpp @@ -43,7 +43,7 @@ bool MenuScriptingInterface::menuExists(const QString& menu) { if (QThread::currentThread() == qApp->thread()) { return Menu::getInstance()->menuExists(menu); } - bool result; + bool result { false }; BLOCKING_INVOKE_METHOD(Menu::getInstance(), "menuExists", Q_RETURN_ARG(bool, result), Q_ARG(const QString&, menu)); @@ -86,7 +86,7 @@ bool MenuScriptingInterface::menuItemExists(const QString& menu, const QString& if (QThread::currentThread() == qApp->thread()) { return Menu::getInstance()->menuItemExists(menu, menuitem); } - bool result; + bool result { false }; BLOCKING_INVOKE_METHOD(Menu::getInstance(), "menuItemExists", Q_RETURN_ARG(bool, result), Q_ARG(const QString&, menu), @@ -98,7 +98,7 @@ bool MenuScriptingInterface::isOptionChecked(const QString& menuOption) { if (QThread::currentThread() == qApp->thread()) { return Menu::getInstance()->isOptionChecked(menuOption); } - bool result; + bool result { false }; BLOCKING_INVOKE_METHOD(Menu::getInstance(), "isOptionChecked", Q_RETURN_ARG(bool, result), Q_ARG(const QString&, menuOption)); @@ -115,7 +115,7 @@ bool MenuScriptingInterface::isMenuEnabled(const QString& menuOption) { if (QThread::currentThread() == qApp->thread()) { return Menu::getInstance()->isOptionChecked(menuOption); } - bool result; + bool result { false }; BLOCKING_INVOKE_METHOD(Menu::getInstance(), "isMenuEnabled", Q_RETURN_ARG(bool, result), Q_ARG(const QString&, menuOption)); diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index 8b9e37569c..87ffb0b396 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -40,15 +40,6 @@ static Setting::Handle LIMITED_NODELIST_LOCAL_PORT("LimitedNodeList.LocalPort", 0); -const std::set SOLO_NODE_TYPES = { - NodeType::AvatarMixer, - NodeType::AudioMixer, - NodeType::AssetServer, - NodeType::EntityServer, - NodeType::MessagesMixer, - NodeType::EntityScriptServer -}; - LimitedNodeList::LimitedNodeList(int socketListenPort, int dtlsListenPort) : _nodeSocket(this), _packetReceiver(new PacketReceiver(this)) diff --git a/libraries/networking/src/LimitedNodeList.h b/libraries/networking/src/LimitedNodeList.h index 450fad96a9..ca75514761 100644 --- a/libraries/networking/src/LimitedNodeList.h +++ b/libraries/networking/src/LimitedNodeList.h @@ -444,6 +444,15 @@ private: int _outboundPPS { 0 }; float _inboundKbps { 0.0f }; float _outboundKbps { 0.0f }; + + const std::set SOLO_NODE_TYPES = { + NodeType::AvatarMixer, + NodeType::AudioMixer, + NodeType::AssetServer, + NodeType::EntityServer, + NodeType::MessagesMixer, + NodeType::EntityScriptServer + }; }; #endif // hifi_LimitedNodeList_h diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 8dad5932be..7b493dbe05 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -164,7 +164,9 @@ ScriptEnginePointer scriptEngineFactory(ScriptEngine::Context context, const QString& fileNameString) { ScriptEngine* engine = new ScriptEngine(context, scriptContents, fileNameString); ScriptEnginePointer engineSP = ScriptEnginePointer(engine); - DependencyManager::get()->addScriptEngine(qSharedPointerCast(engineSP)); + auto scriptEngines = DependencyManager::get(); + scriptEngines->addScriptEngine(qSharedPointerCast(engineSP)); + engine->setScriptEngines(scriptEngines); return engineSP; } @@ -259,7 +261,7 @@ bool ScriptEngine::isDebugMode() const { } ScriptEngine::~ScriptEngine() { - auto scriptEngines = DependencyManager::get(); + QSharedPointer scriptEngines(_scriptEngines); if (scriptEngines) { scriptEngines->removeScriptEngine(qSharedPointerCast(sharedFromThis())); } @@ -1012,7 +1014,8 @@ QScriptValue ScriptEngine::evaluateInClosure(const QScriptValue& closure, const } QScriptValue ScriptEngine::evaluate(const QString& sourceCode, const QString& fileName, int lineNumber) { - if (DependencyManager::get()->isStopped()) { + QSharedPointer scriptEngines(_scriptEngines); + if (!scriptEngines || scriptEngines->isStopped()) { return QScriptValue(); // bail early } @@ -1062,7 +1065,8 @@ void ScriptEngine::run() { auto name = filenameParts.size() > 0 ? filenameParts[filenameParts.size() - 1] : "unknown"; PROFILE_SET_THREAD_NAME("Script: " + name); - if (DependencyManager::get()->isStopped()) { + QSharedPointer scriptEngines(_scriptEngines); + if (!scriptEngines || scriptEngines->isStopped()) { return; // bail early - avoid setting state in init(), as evaluate() will bail too } @@ -1319,7 +1323,7 @@ void ScriptEngine::updateMemoryCost(const qint64& deltaSize) { void ScriptEngine::timerFired() { { - auto engine = DependencyManager::get(); + QSharedPointer engine(_scriptEngines); if (!engine || engine->isStopped()) { scriptWarningMessage("Script.timerFired() while shutting down is ignored... parent script:" + getFilename()); return; // bail early @@ -1373,7 +1377,8 @@ QObject* ScriptEngine::setupTimerWithInterval(const QScriptValue& function, int } QObject* ScriptEngine::setInterval(const QScriptValue& function, int intervalMS) { - if (DependencyManager::get()->isStopped()) { + QSharedPointer scriptEngines(_scriptEngines); + if (!scriptEngines || scriptEngines->isStopped()) { scriptWarningMessage("Script.setInterval() while shutting down is ignored... parent script:" + getFilename()); return NULL; // bail early } @@ -1382,7 +1387,8 @@ QObject* ScriptEngine::setInterval(const QScriptValue& function, int intervalMS) } QObject* ScriptEngine::setTimeout(const QScriptValue& function, int timeoutMS) { - if (DependencyManager::get()->isStopped()) { + QSharedPointer scriptEngines(_scriptEngines); + if (!scriptEngines || scriptEngines->isStopped()) { scriptWarningMessage("Script.setTimeout() while shutting down is ignored... parent script:" + getFilename()); return NULL; // bail early } @@ -1813,7 +1819,8 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac if (!IS_THREADSAFE_INVOCATION(thread(), __FUNCTION__)) { return; } - if (DependencyManager::get()->isStopped()) { + QSharedPointer scriptEngines(_scriptEngines); + if (!scriptEngines || scriptEngines->isStopped()) { scriptWarningMessage("Script.include() while shutting down is ignored... includeFiles:" + includeFiles.join(",") + "parent script:" + getFilename()); return; // bail early @@ -1907,7 +1914,8 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac } void ScriptEngine::include(const QString& includeFile, QScriptValue callback) { - if (DependencyManager::get()->isStopped()) { + QSharedPointer scriptEngines(_scriptEngines); + if (!scriptEngines || scriptEngines->isStopped()) { scriptWarningMessage("Script.include() while shutting down is ignored... includeFile:" + includeFile + "parent script:" + getFilename()); return; // bail early @@ -1925,7 +1933,8 @@ void ScriptEngine::load(const QString& loadFile) { if (!IS_THREADSAFE_INVOCATION(thread(), __FUNCTION__)) { return; } - if (DependencyManager::get()->isStopped()) { + QSharedPointer scriptEngines(_scriptEngines); + if (!scriptEngines || scriptEngines->isStopped()) { scriptWarningMessage("Script.load() while shutting down is ignored... loadFile:" + loadFile + "parent script:" + getFilename()); return; // bail early @@ -2073,10 +2082,11 @@ void ScriptEngine::loadEntityScript(const EntityItemID& entityID, const QString& } PROFILE_RANGE(script, __FUNCTION__); - if (isStopping() || DependencyManager::get()->isStopped()) { + QSharedPointer scriptEngines(_scriptEngines); + if (isStopping() || !scriptEngines || scriptEngines->isStopped()) { qCDebug(scriptengine) << "loadEntityScript.start " << entityID.toString() << " but isStopping==" << isStopping() - << " || engines->isStopped==" << DependencyManager::get()->isStopped(); + << " || engines->isStopped==" << scriptEngines->isStopped(); return; } diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index 8fe50aee78..0d6d45e594 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -563,6 +563,8 @@ public: bool getEntityScriptDetails(const EntityItemID& entityID, EntityScriptDetails &details) const; bool hasEntityScriptDetails(const EntityItemID& entityID) const; + void setScriptEngines(QSharedPointer& scriptEngines) { _scriptEngines = scriptEngines; } + public slots: /**jsdoc @@ -814,6 +816,8 @@ protected: static const QString _SETTINGS_ENABLE_EXTENDED_EXCEPTIONS; Setting::Handle _enableExtendedJSExceptions { _SETTINGS_ENABLE_EXTENDED_EXCEPTIONS, true }; + + QWeakPointer _scriptEngines; }; ScriptEnginePointer scriptEngineFactory(ScriptEngine::Context context, From 36040597ba8d6c6d20cef8fd5de36caea7527203 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Sat, 22 Dec 2018 06:56:20 -0800 Subject: [PATCH 2/7] rename variable for consistency --- libraries/script-engine/src/ScriptEngine.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 7b493dbe05..1396ee02fc 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -1323,8 +1323,8 @@ void ScriptEngine::updateMemoryCost(const qint64& deltaSize) { void ScriptEngine::timerFired() { { - QSharedPointer engine(_scriptEngines); - if (!engine || engine->isStopped()) { + QSharedPointer scriptEngines(_scriptEngines); + if (!scriptEngines || scriptEngines->isStopped()) { scriptWarningMessage("Script.timerFired() while shutting down is ignored... parent script:" + getFilename()); return; // bail early } From 114bcc003a6fc53c58ddba61261a6b3d5a5d44b4 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Mon, 7 Jan 2019 07:36:39 -0800 Subject: [PATCH 3/7] quiet build warnings --- libraries/fbx/src/FBXSerializer.h | 2 ++ libraries/hfm/src/hfm/HFMSerializer.h | 1 + 2 files changed, 3 insertions(+) diff --git a/libraries/fbx/src/FBXSerializer.h b/libraries/fbx/src/FBXSerializer.h index 7227ebac17..31ca301522 100644 --- a/libraries/fbx/src/FBXSerializer.h +++ b/libraries/fbx/src/FBXSerializer.h @@ -96,6 +96,8 @@ class ExtractedMesh; class FBXSerializer : public HFMSerializer { public: + virtual ~FBXSerializer() {} + MediaType getMediaType() const override; std::unique_ptr getFactory() const override; diff --git a/libraries/hfm/src/hfm/HFMSerializer.h b/libraries/hfm/src/hfm/HFMSerializer.h index 868ec3dd45..d0be588d60 100644 --- a/libraries/hfm/src/hfm/HFMSerializer.h +++ b/libraries/hfm/src/hfm/HFMSerializer.h @@ -23,6 +23,7 @@ class Serializer { public: class Factory { public: + virtual ~Factory() {} virtual std::shared_ptr get() = 0; }; From ecb34450bdd100779353552cb18e980428b90272 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Wed, 9 Jan 2019 08:03:24 -0800 Subject: [PATCH 4/7] quiet uneeded logging --- libraries/animation/src/AnimClip.cpp | 6 +++--- libraries/animation/src/Rig.cpp | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/libraries/animation/src/AnimClip.cpp b/libraries/animation/src/AnimClip.cpp index 9dcf5822cd..32914e31ef 100644 --- a/libraries/animation/src/AnimClip.cpp +++ b/libraries/animation/src/AnimClip.cpp @@ -109,9 +109,9 @@ void AnimClip::copyFromNetworkAnim() { jointMap.reserve(animJointCount); for (int i = 0; i < animJointCount; i++) { int skeletonJoint = _skeleton->nameToJointIndex(animSkeleton.getJointName(i)); - if (skeletonJoint == -1) { - qCWarning(animation) << "animation contains joint =" << animSkeleton.getJointName(i) << " which is not in the skeleton"; - } + // if (skeletonJoint == -1) { + // qCWarning(animation) << "animation contains joint =" << animSkeleton.getJointName(i) << " which is not in the skeleton"; + // } jointMap.push_back(skeletonJoint); } diff --git a/libraries/animation/src/Rig.cpp b/libraries/animation/src/Rig.cpp index 44fdd8797f..bfa051435b 100644 --- a/libraries/animation/src/Rig.cpp +++ b/libraries/animation/src/Rig.cpp @@ -425,7 +425,7 @@ int Rig::indexOfJoint(const QString& jointName) const { // This is a content error, so we should issue a warning. if (result < 0 && _jointNameWarningCount < MAX_JOINT_NAME_WARNING_COUNT) { - qCWarning(animation) << "Rig: Missing joint" << jointName << "in avatar model"; + // qCWarning(animation) << "Rig: Missing joint" << jointName << "in avatar model"; _jointNameWarningCount++; } return result; From 6ca9ab6fba8fd81bae4ac3718481087d7f6f5c51 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Wed, 9 Jan 2019 17:15:38 -0800 Subject: [PATCH 5/7] avoid shutdown crash --- libraries/script-engine/src/ScriptEngine.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 1396ee02fc..7039f8bc94 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -557,6 +557,10 @@ using ScriptableResourceRawPtr = ScriptableResource*; static QScriptValue scriptableResourceToScriptValue(QScriptEngine* engine, const ScriptableResourceRawPtr& resource) { + if (!resource) { + return QScriptValue(); // probably shutting down + } + // The first script to encounter this resource will track its memory. // In this way, it will be more likely to GC. // This fails in the case that the resource is used across many scripts, but From b71f4e0204ff9fc473daea1e25eb99cdd6a522b5 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Thu, 10 Jan 2019 09:18:27 -0800 Subject: [PATCH 6/7] remove commented-out code --- libraries/animation/src/AnimClip.cpp | 3 --- libraries/animation/src/Rig.cpp | 1 - 2 files changed, 4 deletions(-) diff --git a/libraries/animation/src/AnimClip.cpp b/libraries/animation/src/AnimClip.cpp index 32914e31ef..1adc04ee1b 100644 --- a/libraries/animation/src/AnimClip.cpp +++ b/libraries/animation/src/AnimClip.cpp @@ -109,9 +109,6 @@ void AnimClip::copyFromNetworkAnim() { jointMap.reserve(animJointCount); for (int i = 0; i < animJointCount; i++) { int skeletonJoint = _skeleton->nameToJointIndex(animSkeleton.getJointName(i)); - // if (skeletonJoint == -1) { - // qCWarning(animation) << "animation contains joint =" << animSkeleton.getJointName(i) << " which is not in the skeleton"; - // } jointMap.push_back(skeletonJoint); } diff --git a/libraries/animation/src/Rig.cpp b/libraries/animation/src/Rig.cpp index bfa051435b..6e27bee06f 100644 --- a/libraries/animation/src/Rig.cpp +++ b/libraries/animation/src/Rig.cpp @@ -425,7 +425,6 @@ int Rig::indexOfJoint(const QString& jointName) const { // This is a content error, so we should issue a warning. if (result < 0 && _jointNameWarningCount < MAX_JOINT_NAME_WARNING_COUNT) { - // qCWarning(animation) << "Rig: Missing joint" << jointName << "in avatar model"; _jointNameWarningCount++; } return result; From 1ef047a3296eac3a036e76013f0dcab2a6725948 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Fri, 11 Jan 2019 16:49:09 -0800 Subject: [PATCH 7/7] revert unneeded change to LimitedNodeList --- libraries/networking/src/LimitedNodeList.cpp | 9 +++++++++ libraries/networking/src/LimitedNodeList.h | 9 --------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index 87ffb0b396..8b9e37569c 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -40,6 +40,15 @@ static Setting::Handle LIMITED_NODELIST_LOCAL_PORT("LimitedNodeList.LocalPort", 0); +const std::set SOLO_NODE_TYPES = { + NodeType::AvatarMixer, + NodeType::AudioMixer, + NodeType::AssetServer, + NodeType::EntityServer, + NodeType::MessagesMixer, + NodeType::EntityScriptServer +}; + LimitedNodeList::LimitedNodeList(int socketListenPort, int dtlsListenPort) : _nodeSocket(this), _packetReceiver(new PacketReceiver(this)) diff --git a/libraries/networking/src/LimitedNodeList.h b/libraries/networking/src/LimitedNodeList.h index ca75514761..450fad96a9 100644 --- a/libraries/networking/src/LimitedNodeList.h +++ b/libraries/networking/src/LimitedNodeList.h @@ -444,15 +444,6 @@ private: int _outboundPPS { 0 }; float _inboundKbps { 0.0f }; float _outboundKbps { 0.0f }; - - const std::set SOLO_NODE_TYPES = { - NodeType::AvatarMixer, - NodeType::AudioMixer, - NodeType::AssetServer, - NodeType::EntityServer, - NodeType::MessagesMixer, - NodeType::EntityScriptServer - }; }; #endif // hifi_LimitedNodeList_h