From 10e6157ab9517ac1f58cfb8d263dac041b5026ba Mon Sep 17 00:00:00 2001
From: Ryan Huffman <ryanhuffman@gmail.com>
Date: Wed, 26 Oct 2016 15:16:29 -0700
Subject: [PATCH] Fix race condition in BatchLoader

---
 libraries/script-engine/src/BatchLoader.cpp | 27 +++++++++++++--------
 libraries/script-engine/src/BatchLoader.h   | 11 ++++++++-
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/libraries/script-engine/src/BatchLoader.cpp b/libraries/script-engine/src/BatchLoader.cpp
index 605d7e95bd..ef8cf90656 100644
--- a/libraries/script-engine/src/BatchLoader.cpp
+++ b/libraries/script-engine/src/BatchLoader.cpp
@@ -44,33 +44,40 @@ void BatchLoader::start() {
         return;
     }
 
+
     for (const auto& rawURL : _urls) {
         QUrl url = expandScriptUrl(normalizeScriptURL(rawURL));
 
         qCDebug(scriptengine) << "Loading script at " << url;
 
-        QPointer<BatchLoader> self = this;
-        DependencyManager::get<ScriptCache>()->getScriptContents(url.toString(), [this, self](const QString& url, const QString& contents, bool isURL, bool success) {
-            if (!self) {
-                return;
-            }
+		auto scriptCache = DependencyManager::get<ScriptCache>();
 
-            // Because the ScriptCache may call this callback from differents threads,
-            // we need to make sure this is thread-safe.
-            std::lock_guard<std::mutex> lock(_dataLock);
+		// Use a proxy callback to handle the call and emit the signal in a thread-safe way.
+		// If BatchLoader is deleted before the callback is called, the subsequent "emit" call will not do
+		// anything.
+		ScriptCacheSignalProxy* proxy = new ScriptCacheSignalProxy(scriptCache.data());
+		scriptCache->getScriptContents(url.toString(), [proxy](const QString& url, const QString& contents, bool isURL, bool success) {
+			proxy->receivedContent(url, contents, isURL, success);
+			proxy->deleteLater();
+		}, false);
 
+		connect(proxy, &ScriptCacheSignalProxy::contentAvailable, this, [this](const QString& url, const QString& contents, bool isURL, bool success) {
             if (isURL && success) {
                 _data.insert(url, contents);
                 qCDebug(scriptengine) << "Loaded: " << url;
             } else {
                 _data.insert(url, QString());
-                qCDebug(scriptengine) << "Could not load" << url;
+                qCDebug(scriptengine) << "Could not load: " << url;
             }
 
             if (!_finished && _urls.size() == _data.size()) {
                 _finished = true;
                 emit finished(_data);
             }
-        }, false);
+		});
     }
 }
+
+void ScriptCacheSignalProxy::receivedContent(const QString& url, const QString& contents, bool isURL, bool success) {
+	emit contentAvailable(url, contents, isURL, success);
+}
diff --git a/libraries/script-engine/src/BatchLoader.h b/libraries/script-engine/src/BatchLoader.h
index 40b43d23b6..55acc11c26 100644
--- a/libraries/script-engine/src/BatchLoader.h
+++ b/libraries/script-engine/src/BatchLoader.h
@@ -21,6 +21,16 @@
 
 #include <mutex>
 
+class ScriptCacheSignalProxy : public QObject {
+	Q_OBJECT
+public:
+	ScriptCacheSignalProxy(QObject* parent) : QObject(parent) { }
+   void receivedContent(const QString& url, const QString& contents, bool isURL, bool success);
+
+signals:
+   void contentAvailable(const QString& url, const QString& contents, bool isURL, bool success);
+};
+
 class BatchLoader : public QObject {
     Q_OBJECT
 public:
@@ -39,7 +49,6 @@ private:
     bool _finished;
     QSet<QUrl> _urls;
     QMap<QUrl, QString> _data;
-    std::mutex _dataLock;
 };
 
 #endif // hifi_BatchLoader_h