From aef39ce6fa03147543af41e939b7eab37deccc51 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Thu, 10 Nov 2016 16:06:53 -0800 Subject: [PATCH 1/3] block until follow-on includes are finished but still avoid multiple evaluation of included urls --- libraries/script-engine/src/ScriptEngine.cpp | 25 +++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 776c7cfec6..c6a9874c38 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -1187,20 +1187,15 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac thisURL = resolvePath(file); } - if (!_includedURLs.contains(thisURL)) { - if (!isStandardLibrary && !currentSandboxURL.isEmpty() && (thisURL.scheme() == "file") && - (currentSandboxURL.scheme() != "file" || - !thisURL.toString(strippingFlags).startsWith(currentSandboxURL.toString(strippingFlags), getSensitivity()))) { - qCWarning(scriptengine) << "Script.include() ignoring file path" - << thisURL << "outside of original entity script" << currentSandboxURL; - } else { - // We could also check here for CORS, but we don't yet. - // It turns out that QUrl.resolve will not change hosts and copy authority, so we don't need to check that here. - urls.append(thisURL); - _includedURLs << thisURL; - } + if (!isStandardLibrary && !currentSandboxURL.isEmpty() && (thisURL.scheme() == "file") && + (currentSandboxURL.scheme() != "file" || + !thisURL.toString(strippingFlags).startsWith(currentSandboxURL.toString(strippingFlags), getSensitivity()))) { + qCWarning(scriptengine) << "Script.include() ignoring file path" + << thisURL << "outside of original entity script" << currentSandboxURL; } else { - qCDebug(scriptengine) << "Script.include() ignoring previously included url:" << thisURL; + // We could also check here for CORS, but we don't yet. + // It turns out that QUrl.resolve will not change hosts and copy authority, so we don't need to check that here. + urls.append(thisURL); } } @@ -1219,13 +1214,15 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac QString contents = data[url]; if (contents.isNull()) { qCDebug(scriptengine) << "Error loading file: " << url << "line:" << __LINE__; - } else { + } else if (!_includedURLs.contains(url)) { + _includedURLs << url; // Set the parent url so that path resolution will be relative // to this script's url during its initial evaluation _parentURL = url.toString(); auto operation = [&]() { evaluate(contents, url.toString()); }; + doWithEnvironment(capturedEntityIdentifier, capturedSandboxURL, operation); } } From 576eed9941cfae3bc03ee82cff50aa83c7758fde Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Thu, 10 Nov 2016 17:12:06 -0800 Subject: [PATCH 2/3] use a mutex to avoid a smaller race --- libraries/script-engine/src/ScriptEngine.cpp | 21 +++++++++++--------- libraries/script-engine/src/ScriptEngine.h | 1 + 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index c6a9874c38..1569a1b9f1 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -1214,16 +1214,19 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac QString contents = data[url]; if (contents.isNull()) { qCDebug(scriptengine) << "Error loading file: " << url << "line:" << __LINE__; - } else if (!_includedURLs.contains(url)) { - _includedURLs << url; - // Set the parent url so that path resolution will be relative - // to this script's url during its initial evaluation - _parentURL = url.toString(); - auto operation = [&]() { - evaluate(contents, url.toString()); - }; + } else { + std::lock_guard lock(_lock); + if (!_includedURLs.contains(url)) { + _includedURLs << url; + // Set the parent url so that path resolution will be relative + // to this script's url during its initial evaluation + _parentURL = url.toString(); + auto operation = [&]() { + evaluate(contents, url.toString()); + }; - doWithEnvironment(capturedEntityIdentifier, capturedSandboxURL, operation); + doWithEnvironment(capturedEntityIdentifier, capturedSandboxURL, operation); + } } } _parentURL = parentURL; diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index 01088660ff..2b2cb3c81a 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -245,6 +245,7 @@ protected: std::function _emitScriptUpdates{ [](){ return true; } }; + std::recursive_mutex _lock; }; #endif // hifi_ScriptEngine_h From 5133ce054807019456403816d01dec090dec0507 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Sat, 12 Nov 2016 10:50:21 -0800 Subject: [PATCH 3/3] print a debug message when skipping evaluation of a previously included url --- libraries/script-engine/src/ScriptEngine.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 1569a1b9f1..00c48cc633 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -1226,6 +1226,8 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac }; doWithEnvironment(capturedEntityIdentifier, capturedSandboxURL, operation); + } else { + qCDebug(scriptengine) << "Script.include() skipping evaluation of previously included url:" << url; } } }