Merge pull request #14609 from sethalves/fb-20280

fix a couple shutdown crashes and a class of asan warning

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));

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);
     }
 

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;

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<hfm::Serializer::Factory> getFactory() const override;
 

libraries/hfm/src/hfm/HFMSerializer.h

@@ -23,6 +23,7 @@ class Serializer {
 public:
     class Factory {
     public:
+        virtual ~Factory() {}
         virtual std::shared_ptr<Serializer> get() = 0;
     };
 

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<ScriptEngines>()->addScriptEngine(qSharedPointerCast<ScriptEngine>(engineSP));
+    auto scriptEngines = DependencyManager::get<ScriptEngines>();
+    scriptEngines->addScriptEngine(qSharedPointerCast<ScriptEngine>(engineSP));
+    engine->setScriptEngines(scriptEngines);
     return engineSP;
 }
 
@@ -259,7 +261,7 @@ bool ScriptEngine::isDebugMode() const {
 }
 
 ScriptEngine::~ScriptEngine() {
-    auto scriptEngines = DependencyManager::get<ScriptEngines>();
+    QSharedPointer<ScriptEngines> scriptEngines(_scriptEngines);
     if (scriptEngines) {
         scriptEngines->removeScriptEngine(qSharedPointerCast<ScriptEngine>(sharedFromThis()));
     }
@@ -555,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
@@ -1012,7 +1018,8 @@ QScriptValue ScriptEngine::evaluateInClosure(const QScriptValue& closure, const
 }
 
 QScriptValue ScriptEngine::evaluate(const QString& sourceCode, const QString& fileName, int lineNumber) {
-    if (DependencyManager::get<ScriptEngines>()->isStopped()) {
+    QSharedPointer<ScriptEngines> scriptEngines(_scriptEngines);
+    if (!scriptEngines || scriptEngines->isStopped()) {
         return QScriptValue(); // bail early
     }
 
@@ -1062,7 +1069,8 @@ void ScriptEngine::run() {
     auto name = filenameParts.size() > 0 ? filenameParts[filenameParts.size() - 1] : "unknown";
     PROFILE_SET_THREAD_NAME("Script: " + name);
 
-    if (DependencyManager::get<ScriptEngines>()->isStopped()) {
+    QSharedPointer<ScriptEngines> scriptEngines(_scriptEngines);
+    if (!scriptEngines || scriptEngines->isStopped()) {
         return; // bail early - avoid setting state in init(), as evaluate() will bail too
     }
 
@@ -1319,8 +1327,8 @@ void ScriptEngine::updateMemoryCost(const qint64& deltaSize) {
 
 void ScriptEngine::timerFired() {
     {
-        auto engine = DependencyManager::get<ScriptEngines>();
+        QSharedPointer<ScriptEngines> scriptEngines(_scriptEngines);
-        if (!engine || engine->isStopped()) {
+        if (!scriptEngines || scriptEngines->isStopped()) {
             scriptWarningMessage("Script.timerFired() while shutting down is ignored... parent script:" + getFilename());
             return; // bail early
         }
@@ -1373,7 +1381,8 @@ QObject* ScriptEngine::setupTimerWithInterval(const QScriptValue& function, int
 }
 
 QObject* ScriptEngine::setInterval(const QScriptValue& function, int intervalMS) {
-    if (DependencyManager::get<ScriptEngines>()->isStopped()) {
+    QSharedPointer<ScriptEngines> scriptEngines(_scriptEngines);
+    if (!scriptEngines || scriptEngines->isStopped()) {
         scriptWarningMessage("Script.setInterval() while shutting down is ignored... parent script:" + getFilename());
         return NULL; // bail early
     }
@@ -1382,7 +1391,8 @@ QObject* ScriptEngine::setInterval(const QScriptValue& function, int intervalMS)
 }
 
 QObject* ScriptEngine::setTimeout(const QScriptValue& function, int timeoutMS) {
-    if (DependencyManager::get<ScriptEngines>()->isStopped()) {
+    QSharedPointer<ScriptEngines> scriptEngines(_scriptEngines);
+    if (!scriptEngines || scriptEngines->isStopped()) {
         scriptWarningMessage("Script.setTimeout() while shutting down is ignored... parent script:" + getFilename());
         return NULL; // bail early
     }
@@ -1813,7 +1823,8 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac
     if (!IS_THREADSAFE_INVOCATION(thread(), __FUNCTION__)) {
         return;
     }
-    if (DependencyManager::get<ScriptEngines>()->isStopped()) {
+    QSharedPointer<ScriptEngines> 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 +1918,8 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac
 }
 
 void ScriptEngine::include(const QString& includeFile, QScriptValue callback) {
-    if (DependencyManager::get<ScriptEngines>()->isStopped()) {
+    QSharedPointer<ScriptEngines> scriptEngines(_scriptEngines);
+    if (!scriptEngines || scriptEngines->isStopped()) {
         scriptWarningMessage("Script.include() while shutting down is ignored...  includeFile:"
                     + includeFile + "parent script:" + getFilename());
         return; // bail early
@@ -1925,7 +1937,8 @@ void ScriptEngine::load(const QString& loadFile) {
     if (!IS_THREADSAFE_INVOCATION(thread(), __FUNCTION__)) {
         return;
     }
-    if (DependencyManager::get<ScriptEngines>()->isStopped()) {
+    QSharedPointer<ScriptEngines> scriptEngines(_scriptEngines);
+    if (!scriptEngines || scriptEngines->isStopped()) {
         scriptWarningMessage("Script.load() while shutting down is ignored... loadFile:"
                 + loadFile + "parent script:" + getFilename());
         return; // bail early
@@ -2073,10 +2086,11 @@ void ScriptEngine::loadEntityScript(const EntityItemID& entityID, const QString&
     }
     PROFILE_RANGE(script, __FUNCTION__);
 
-    if (isStopping() || DependencyManager::get<ScriptEngines>()->isStopped()) {
+    QSharedPointer<ScriptEngines> scriptEngines(_scriptEngines);
+    if (isStopping() || !scriptEngines || scriptEngines->isStopped()) {
         qCDebug(scriptengine) << "loadEntityScript.start " << entityID.toString()
                                      << " but isStopping==" << isStopping()
-                                     << " || engines->isStopped==" << DependencyManager::get<ScriptEngines>()->isStopped();
+                                     << " || engines->isStopped==" << scriptEngines->isStopped();
         return;
     }
 

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 = scriptEngines; }
+
 public slots:
 
     /**jsdoc
@@ -814,6 +816,8 @@ protected:
     static const QString _SETTINGS_ENABLE_EXTENDED_EXCEPTIONS;
 
     Setting::Handle<bool> _enableExtendedJSExceptions { _SETTINGS_ENABLE_EXTENDED_EXCEPTIONS, true };
+
+    QWeakPointer<ScriptEngines> _scriptEngines;
 };
 
 ScriptEnginePointer scriptEngineFactory(ScriptEngine::Context context,