Merge pull request #7310 from jherico/crashes

Addressing thread safety issues

libraries/controllers/src/controllers/ScriptingInterface.h

@@ -126,10 +126,10 @@ namespace controller {
         QVariantMap _actions;
         QVariantMap _standard;
 
-        bool _mouseCaptured{ false };
+        std::atomic<bool> _mouseCaptured{ false };
-        bool _touchCaptured{ false };
+        std::atomic<bool> _touchCaptured { false };
-        bool _wheelCaptured{ false };
+        std::atomic<bool> _wheelCaptured { false };
-        bool _actionsCaptured{ false };
+        std::atomic<bool> _actionsCaptured { false };
     };
 
 

libraries/script-engine/src/ScriptCache.cpp

@@ -28,21 +28,26 @@ ScriptCache::ScriptCache(QObject* parent) {
 }
 
 void ScriptCache::clearCache() {
+    Lock lock(_containerLock);
     _scriptCache.clear();
 }
 
 QString ScriptCache::getScript(const QUrl& unnormalizedURL, ScriptUser* scriptUser, bool& isPending, bool reload) {
     QUrl url = ResourceManager::normalizeURL(unnormalizedURL);
     QString scriptContents;
+
+    Lock lock(_containerLock);
     if (_scriptCache.contains(url) && !reload) {
         qCDebug(scriptengine) << "Found script in cache:" << url.toString();
         scriptContents = _scriptCache[url];
+        lock.unlock();
         scriptUser->scriptContentsAvailable(url, scriptContents);
         isPending = false;
     } else {
         isPending = true;
         bool alreadyWaiting = _scriptUsers.contains(url);
         _scriptUsers.insert(url, scriptUser);
+        lock.unlock();
 
         if (alreadyWaiting) {
             qCDebug(scriptengine) << "Already downloading script at:" << url.toString();
@@ -58,6 +63,7 @@ QString ScriptCache::getScript(const QUrl& unnormalizedURL, ScriptUser* scriptUs
 
 void ScriptCache::deleteScript(const QUrl& unnormalizedURL) {
     QUrl url = ResourceManager::normalizeURL(unnormalizedURL);
+    Lock lock(_containerLock);
     if (_scriptCache.contains(url)) {
         qCDebug(scriptengine) << "Delete script from cache:" << url.toString();
         _scriptCache.remove(url);
@@ -67,17 +73,22 @@ void ScriptCache::deleteScript(const QUrl& unnormalizedURL) {
 void ScriptCache::scriptDownloaded() {
     ResourceRequest* req = qobject_cast<ResourceRequest*>(sender());
     QUrl url = req->getUrl();
+
+    Lock lock(_containerLock);
     QList<ScriptUser*> scriptUsers = _scriptUsers.values(url);
     _scriptUsers.remove(url);
 
     if (req->getResult() == ResourceRequest::Success) {
-        _scriptCache[url] = req->getData();
+        auto scriptContents = req->getData();
+        _scriptCache[url] = scriptContents;
+        lock.unlock();
         qCDebug(scriptengine) << "Done downloading script at:" << url.toString();
 
         foreach(ScriptUser* user, scriptUsers) {
-            user->scriptContentsAvailable(url, _scriptCache[url]);
+            user->scriptContentsAvailable(url, scriptContents);
         }
     } else {
+        lock.unlock();
         qCWarning(scriptengine) << "Error loading script from URL " << url;
         foreach(ScriptUser* user, scriptUsers) {
             user->errorInLoadingScript(url);
@@ -99,15 +110,19 @@ void ScriptCache::getScriptContents(const QString& scriptOrURL, contentAvailable
         return;
     }
 
+    Lock lock(_containerLock);
     if (_scriptCache.contains(url) && !forceDownload) {
+        auto scriptContent = _scriptCache[url];
+        lock.unlock();
         qCDebug(scriptengine) << "Found script in cache:" << url.toString();
         #if 1 // def THREAD_DEBUGGING
         qCDebug(scriptengine) << "ScriptCache::getScriptContents() about to call contentAvailable() on thread [" << QThread::currentThread() << "] expected thread [" << thread() << "]";
         #endif
-        contentAvailable(url.toString(), _scriptCache[url], true, true);
+        contentAvailable(url.toString(), scriptContent, true, true);
     } else {
         bool alreadyWaiting = _contentCallbacks.contains(url);
         _contentCallbacks.insert(url, contentAvailable);
+        lock.unlock();
 
         if (alreadyWaiting) {
             qCDebug(scriptengine) << "Already downloading script at:" << url.toString();
@@ -129,19 +144,29 @@ void ScriptCache::scriptContentAvailable() {
     #endif
     ResourceRequest* req = qobject_cast<ResourceRequest*>(sender());
     QUrl url = req->getUrl();
-    QList<contentAvailableCallback> allCallbacks = _contentCallbacks.values(url);
-    _contentCallbacks.remove(url);
 
-    bool success = req->getResult() == ResourceRequest::Success;
+    QString scriptContent;
+    QList<contentAvailableCallback> allCallbacks;
+    bool success { false };
 
-    if (success) {
+    {
-        _scriptCache[url] = req->getData();
+        Lock lock(_containerLock);
-        qCDebug(scriptengine) << "Done downloading script at:" << url.toString();
+        allCallbacks = _contentCallbacks.values(url);
-    } else {
+        _contentCallbacks.remove(url);
-        qCWarning(scriptengine) << "Error loading script from URL " << url;
+        success = req->getResult() == ResourceRequest::Success;
+
+        if (success) {
+            _scriptCache[url] = scriptContent = req->getData();
+            qCDebug(scriptengine) << "Done downloading script at:" << url.toString();
+        } else {
+            // Dubious, but retained here because it matches the behavior before fixing the threading
+            scriptContent = _scriptCache[url];
+            qCWarning(scriptengine) << "Error loading script from URL " << url;
+        }
     }
+
     foreach(contentAvailableCallback thisCallback, allCallbacks) {
-        thisCallback(url.toString(), _scriptCache[url], true, success);
+        thisCallback(url.toString(), scriptContent, true, success);
     }
     req->deleteLater();
 }

libraries/script-engine/src/ScriptCache.h

@@ -12,6 +12,7 @@
 #ifndef hifi_ScriptCache_h
 #define hifi_ScriptCache_h
 
+#include <mutex>
 #include <ResourceCache.h>
 
 class ScriptUser {
@@ -27,6 +28,9 @@ class ScriptCache : public QObject, public Dependency {
     Q_OBJECT
     SINGLETON_DEPENDENCY
 
+    using Mutex = std::mutex;
+    using Lock = std::unique_lock<Mutex>;
+
 public:
     void clearCache();
     void getScriptContents(const QString& scriptOrURL, contentAvailableCallback contentAvailable, bool forceDownload = false);
@@ -46,6 +50,7 @@ private slots:
 private:
     ScriptCache(QObject* parent = NULL);
     
+    Mutex _containerLock;
     QMultiMap<QUrl, contentAvailableCallback> _contentCallbacks;
     
     QHash<QUrl, QString> _scriptCache;