From 3ad3fd472e5720f1a67d202657ababab98b88d51 Mon Sep 17 00:00:00 2001 From: humbletim Date: Thu, 23 Feb 2017 19:46:28 -0500 Subject: [PATCH] * Inline / duplicate exception checking per CR feedback * Coding standards --- .../script-engine/src/BaseScriptEngine.cpp | 36 +-------------- .../script-engine/src/BaseScriptEngine.h | 19 -------- libraries/script-engine/src/ScriptEngine.cpp | 45 +++++++++---------- 3 files changed, 24 insertions(+), 76 deletions(-) diff --git a/libraries/script-engine/src/BaseScriptEngine.cpp b/libraries/script-engine/src/BaseScriptEngine.cpp index fd4d829d54..16308c0650 100644 --- a/libraries/script-engine/src/BaseScriptEngine.cpp +++ b/libraries/script-engine/src/BaseScriptEngine.cpp @@ -210,10 +210,9 @@ QScriptValue BaseScriptEngine::evaluateInClosure(const QScriptValue& closure, co qCDebug(scriptengine) << QString("[%1] evaluateInClosure %2").arg(isEvaluating()).arg(shortName); #endif { - ExceptionEmitter tryCatch(this, __FUNCTION__); result = BaseScriptEngine::evaluate(program); - if (tryCatch.hasPending()) { - auto err = tryCatch.pending(); + if (hasUncaughtException()) { + auto err = cloneUncaughtException(__FUNCTION__); #ifdef DEBUG_JS_EXCEPTIONS qCWarning(scriptengine) << __FUNCTION__ << "---------- hasCaught:" << err.toString() << result.toString(); err.setProperty("_result", result); @@ -266,37 +265,6 @@ QScriptValue Lambda::call() { return operation(engine->currentContext(), engine); } -// BaseScriptEngine::ExceptionEmitter - -void BaseScriptEngine::_emitUnhandledException(const QScriptValue& exception) { - emit unhandledException(exception); -} -BaseScriptEngine::ExceptionEmitter::ExceptionEmitter(BaseScriptEngine* engine, const QString& debugName) - : _engine(engine), _debugName(debugName) { -} -bool BaseScriptEngine::ExceptionEmitter::hasPending() { - return _engine->hasUncaughtException(); -} -QScriptValue BaseScriptEngine::ExceptionEmitter::pending() { - return _engine->cloneUncaughtException(_debugName); -} -QScriptValue BaseScriptEngine::ExceptionEmitter::consume() { - if (hasPending()) { - _consumedException = pending(); - _engine->clearExceptions(); - } - return _consumedException; -} -bool BaseScriptEngine::ExceptionEmitter::wouldEmit() { - return !_engine->isEvaluating() && _engine->hasUncaughtException(); -} -BaseScriptEngine::ExceptionEmitter::~ExceptionEmitter() { - if (wouldEmit()) { - _engine->_emitUnhandledException(consume()); - } -} - - #ifdef DEBUG_JS void BaseScriptEngine::_debugDump(const QString& header, const QScriptValue& object, const QString& footer) { if (!header.isEmpty()) { diff --git a/libraries/script-engine/src/BaseScriptEngine.h b/libraries/script-engine/src/BaseScriptEngine.h index 620c0915ea..27a6eff33d 100644 --- a/libraries/script-engine/src/BaseScriptEngine.h +++ b/libraries/script-engine/src/BaseScriptEngine.h @@ -46,25 +46,6 @@ protected: #ifdef DEBUG_JS static void _debugDump(const QString& header, const QScriptValue& object, const QString& footer = QString()); #endif - - // ExceptionEmitter can be placed above calls that might throw JS exceptions and it'll - // automatically signal BaseScriptEngine when falling out of scope (recursion aware). - class ExceptionEmitter { - public: - ExceptionEmitter(BaseScriptEngine* engine, const QString& debugName = QString()); - // is there a pending exception? - bool hasPending(); - QScriptValue pending(); - // would it be emitted at scope's end? - bool wouldEmit(); - // consume and return the pending exception - QScriptValue consume(); - ~ExceptionEmitter(); - protected: - BaseScriptEngine* _engine; - QString _debugName; - QScriptValue _consumedException; - }; }; // Lambda helps create callable QScriptValues out of std::functions: diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index ff0ddc2ce3..06cbc8177a 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -899,9 +899,11 @@ QScriptValue ScriptEngine::evaluate(const QString& sourceCode, const QString& fi QScriptValue result; { - // catch any exceptions throw by nested operations - ExceptionEmitter tryCatch(this, __FUNCTION__); result = BaseScriptEngine::evaluate(program); + if (!isEvaluating() && hasUncaughtException()) { + emit unhandledException(cloneUncaughtException(__FUNCTION__)); + clearExceptions(); + } } return result; } @@ -923,9 +925,11 @@ void ScriptEngine::run() { emit runningStateChanged(); { - // catch any exceptions throw by nested operations - ExceptionEmitter tryCatch(this, __FUNCTION__); evaluate(_scriptContents, _fileNameString); + if (!isEvaluating() && hasUncaughtException()) { + emit unhandledException(cloneUncaughtException(__FUNCTION__)); + clearExceptions(); + } } #ifdef _WIN32 // VS13 does not sleep_until unless it uses the system_clock, see: @@ -1361,16 +1365,10 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac evaluate(contents, url.toString()); }; - ExceptionEmitter tryCatch(this, "include"); doWithEnvironment(capturedEntityIdentifier, capturedSandboxURL, operation); - if (tryCatch.hasPending()) { - // match previous include behavior where possible by logging, not propagating, exceptions. - auto err = tryCatch.pending(); - emit unhandledException(err); - // FIXME: to be revisited with next PR 21114-part3 for compat with require/module - //if (tryCatch.wouldEmit()) { - tryCatch.consume(); - //} + if (hasUncaughtException()) { + emit unhandledException(cloneUncaughtException(__FUNCTION__)); + clearExceptions(); } } else { scriptWarningMessage("Script.include() skipping evaluation of previously included url:" + url.toString()); @@ -1508,7 +1506,7 @@ void ScriptEngine::deferLoadEntityScript(const EntityItemID& entityID, const QSt for (int i=0; i < RETRY_ATTEMPT_BATCH && !_pendingLoadEntities.isEmpty(); i++) { auto retry = _pendingLoadEntities.takeFirst(); qCDebug(scriptengine) << "deferLoadEntityScript.retry[" << i << "]:" << retry.entityID << retry.entityScript; - if(!_entityScripts.contains(retry.entityID)) { + if (!_entityScripts.contains(retry.entityID)) { qCDebug(scriptengine) << "deferLoadEntityScript.retry -- entity details gone (entity deleted?)"; } else { auto details = _entityScripts[retry.entityID]; @@ -1715,7 +1713,6 @@ void ScriptEngine::entityScriptContentAvailable(const EntityItemID& entityID, co sandbox.setProcessEventsInterval(SANDBOX_TIMEOUT); QScriptValue testConstructor, exception; { - ExceptionEmitter tryCatch(&sandbox, QString("(preflight %1)").arg(entityID.toString())); QTimer timeout; timeout.setSingleShot(true); timeout.start(SANDBOX_TIMEOUT); @@ -1731,8 +1728,9 @@ void ScriptEngine::entityScriptContentAvailable(const EntityItemID& entityID, co testConstructor = sandbox.evaluate(program); - if (tryCatch.hasPending()) { - exception = tryCatch.consume(); + if (sandbox.hasUncaughtException()) { + exception = sandbox.cloneUncaughtException(QString("(preflight %1)").arg(entityID.toString())); + sandbox.clearExceptions(); } else if (testConstructor.isError()) { exception = testConstructor; } @@ -1780,13 +1778,12 @@ void ScriptEngine::entityScriptContentAvailable(const EntityItemID& entityID, co QScriptValue entityScriptConstructor, entityScriptObject; QUrl sandboxURL = currentSandboxURL.isEmpty() ? scriptOrURL : currentSandboxURL; auto initialization = [&]{ - ExceptionEmitter tryCatch(this, "(construct " + entityID.toString() + ")"); - entityScriptConstructor = evaluate(contents, fileName); entityScriptObject = entityScriptConstructor.construct(); - if (tryCatch.hasPending()) { - entityScriptObject = tryCatch.consume(); + if (hasUncaughtException()) { + entityScriptObject = cloneUncaughtException("(construct " + entityID.toString() + ")"); + clearExceptions(); } }; @@ -1914,8 +1911,6 @@ void ScriptEngine::doWithEnvironment(const EntityItemID& entityID, const QUrl& s currentEntityIdentifier = entityID; currentSandboxURL = sandboxURL; - ExceptionEmitter tryCatcher(this, !entityID.isNull() ? entityID.toString() : __FUNCTION__); - #if DEBUG_CURRENT_ENTITY QScriptValue oldData = this->globalObject().property("debugEntityID"); this->globalObject().setProperty("debugEntityID", entityID.toScriptValue(this)); // Make the entityID available to javascript as a global. @@ -1924,6 +1919,10 @@ void ScriptEngine::doWithEnvironment(const EntityItemID& entityID, const QUrl& s #else operation(); #endif + if (!isEvaluating() && hasUncaughtException()) { + emit unhandledException(cloneUncaughtException(!entityID.isNull() ? entityID.toString() : __FUNCTION__)); + clearExceptions(); + } currentEntityIdentifier = oldIdentifier; currentSandboxURL = oldSandboxURL; }