From 4107f40c7c20c0704a7e523ea47c40a5ed672db8 Mon Sep 17 00:00:00 2001 From: ksuprynowicz Date: Fri, 12 May 2023 16:11:54 +0200 Subject: [PATCH] Fixed XMLHttpRequest crash --- libraries/script-engine/src/ScriptEngine.h | 4 +-- libraries/script-engine/src/ScriptManager.cpp | 1 + .../script-engine/src/XMLHttpRequestClass.cpp | 2 -- .../script-engine/src/v8/ScriptEngineV8.cpp | 15 ++++++++++ .../script-engine/src/v8/ScriptEngineV8.h | 12 ++++++++ .../src/v8/ScriptValueV8Wrapper.cpp | 28 +++++++++++++++++-- .../src/v8/ScriptValueV8Wrapper.h | 21 ++++++++++++++ 7 files changed, 76 insertions(+), 7 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index dfff5b93c4..bc7ce06dbc 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -433,7 +433,7 @@ public: // not for public use, but I don't like how Qt strings this along with p virtual QVariant convert(const ScriptValue& value, int type) = 0; virtual void registerCustomType(int type, MarshalFunction mf, DemarshalFunction df) = 0; virtual QStringList getCurrentScriptURLs() const = 0; - + virtual void perManagerLoopIterationCleanup() = 0; signals: /** @@ -444,7 +444,7 @@ signals: void exception(std::shared_ptr exception); protected: - ~ScriptEngine() {} // prevent explicit deletion of base class + virtual ~ScriptEngine() {} // prevent explicit deletion of base class ScriptManager *_manager; }; diff --git a/libraries/script-engine/src/ScriptManager.cpp b/libraries/script-engine/src/ScriptManager.cpp index 454c8fc7ef..07df59bf7b 100644 --- a/libraries/script-engine/src/ScriptManager.cpp +++ b/libraries/script-engine/src/ScriptManager.cpp @@ -980,6 +980,7 @@ void ScriptManager::run() { QCoreApplication::processEvents(); } processedEvents = true; + _engine->perManagerLoopIterationCleanup(); } PROFILE_RANGE(script, "ScriptMainLoop"); diff --git a/libraries/script-engine/src/XMLHttpRequestClass.cpp b/libraries/script-engine/src/XMLHttpRequestClass.cpp index f6b38c11ed..caca759096 100644 --- a/libraries/script-engine/src/XMLHttpRequestClass.cpp +++ b/libraries/script-engine/src/XMLHttpRequestClass.cpp @@ -234,8 +234,6 @@ void XMLHttpRequestClass::requestFinished() { } } - qDebug() << "XMLHttpRequestClass::requestFinished : ec: " << _errorCode << " rt: " << _responseType << " URL: " << _url - << " method: " << _method << "data" << QString(_rawResponseData.data()); setReadyState(DONE); emit requestComplete(); diff --git a/libraries/script-engine/src/v8/ScriptEngineV8.cpp b/libraries/script-engine/src/v8/ScriptEngineV8.cpp index f9744999c4..c62ae13aaa 100644 --- a/libraries/script-engine/src/v8/ScriptEngineV8.cpp +++ b/libraries/script-engine/src/v8/ScriptEngineV8.cpp @@ -245,6 +245,21 @@ ScriptEngineV8::ScriptEngineV8(ScriptManager *manager) : ScriptEngine(manager), //} } +ScriptEngineV8::~ScriptEngineV8() { + deleteUnusedValueWrappers(); +} + +void ScriptEngineV8::perManagerLoopIterationCleanup() { + deleteUnusedValueWrappers(); +} + +void ScriptEngineV8::deleteUnusedValueWrappers() { + while (!_scriptValueWrappersToDelete.empty()) { + auto wrapper = _scriptValueWrappersToDelete.dequeue(); + wrapper->release(); + } +} + void ScriptEngineV8::registerEnum(const QString& enumName, QMetaEnum newEnum) { if (!newEnum.isValid()) { qCCritical(scriptengine_v8) << "registerEnum called on invalid enum with name " << enumName; diff --git a/libraries/script-engine/src/v8/ScriptEngineV8.h b/libraries/script-engine/src/v8/ScriptEngineV8.h index fd1536f33a..2dc91aebf4 100644 --- a/libraries/script-engine/src/v8/ScriptEngineV8.h +++ b/libraries/script-engine/src/v8/ScriptEngineV8.h @@ -45,6 +45,7 @@ class ScriptEngineV8; class ScriptManager; class ScriptObjectV8Proxy; class ScriptMethodV8Proxy; +class ScriptValueV8Wrapper; template class V8ScriptValueTemplate; typedef V8ScriptValueTemplate V8ScriptValue; @@ -63,6 +64,7 @@ class ScriptEngineV8 final : public ScriptEngine, public: // construction ScriptEngineV8(ScriptManager *manager = nullptr); + virtual ~ScriptEngineV8(); //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // NOTE - these are NOT intended to be public interfaces available to scripts, the are only Q_INVOKABLE so we can @@ -135,6 +137,9 @@ public: // ScriptEngine implementation virtual ScriptEngineMemoryStatistics getMemoryUsageStatistics() override; virtual void startCollectingObjectStatistics() override; virtual void dumpHeapObjectStatistics() override; + void scheduleValueWrapperForDeletion(ScriptValueV8Wrapper* wrapper) {_scriptValueWrappersToDelete.enqueue(wrapper);} + void deleteUnusedValueWrappers(); + virtual void perManagerLoopIterationCleanup() override; // helper to detect and log warnings when other code invokes QScriptEngine/BaseScriptEngine in thread-unsafe ways inline bool IS_THREADSAFE_INVOCATION(const QString& method) { return ScriptEngine::IS_THREADSAFE_INVOCATION(method); } @@ -187,6 +192,10 @@ public: // not for public use, but I don't like how Qt strings this along with p QMap> _qobjectWrapperMapV8; // V8TODO: maybe just a single map can be used instead to increase performance? + // Sometimes ScriptValueV8Wrapper::release() is called from inside ScriptValueV8Wrapper. + // Then wrapper needs to be deleted in the event loop + QQueue _scriptValueWrappersToDelete; + // Used by ScriptObjectV8Proxy to create JS objects referencing C++ ones v8::Local getObjectProxyTemplate(); v8::Local getMethodDataTemplate(); @@ -252,6 +261,9 @@ protected: v8::Persistent _variantDataTemplate; v8::Persistent _variantProxyTemplate; +public: + volatile int _memoryCorruptionIndicator = 12345678; +private: //V8TODO //ArrayBufferClass* _arrayBufferClass; // Counts how many nested evaluate calls are there at a given point diff --git a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp index 72e4d71a46..c4c172d218 100644 --- a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp +++ b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp @@ -20,9 +20,15 @@ #include "ScriptEngineLoggingV8.h" void ScriptValueV8Wrapper::release() { - // V8TODO: maybe add an assert to check if it happens on script engine thread? - // With v8::Locker in V8ScriptValue such requirement shouldn't be necessary but could deleting it on different thread cause deadlocks sometimes? - delete this; + // Check if ScriptValueV8Wrapper::release was called from inside ScriptValueV8Wrapper functions, and if so, delete it later + // This prevents access-after-delete crashes when ScriptValueV8Wrapper::release is called from inside JS executed in + // ScriptValueV8Wrapper::call, ScriptValueV8Wrapper::construct and others + if (lock.tryLockForWrite()) { + lock.unlock(); + delete this; + } else { + _engine->scheduleValueWrapperForDeletion(this); + } } ScriptValueProxy* ScriptValueV8Wrapper::copy() const { @@ -91,7 +97,10 @@ ScriptValue ScriptValueV8Wrapper::call(const ScriptValue& thisObject, const Scri }else{ recv = _engine->getContext()->Global(); } + + lock.lockForRead(); auto maybeResult = v8Function->Call(_engine->getContext(), recv, args.length(), v8Args); + lock.unlock(); if (tryCatch.HasCaught()) { qCDebug(scriptengine_v8) << "Function call failed: \"" << _engine->formatErrorMessageFromTryCatch(tryCatch); } @@ -155,7 +164,9 @@ ScriptValue ScriptValueV8Wrapper::construct(const ScriptValueList& args) { v8::Local v8Function = v8::Local::Cast(_value.get()); // V8TODO: I'm not sure if this is correct, maybe use CallAsConstructor instead? // Maybe it's CallAsConstructor for function and NewInstance for class? + lock.lockForRead(); auto maybeResult = v8Function->NewInstance(_engine->getContext(), args.length(), v8Args); + lock.unlock(); v8::Local result; if (maybeResult.ToLocal(&result)) { return ScriptValue(new ScriptValueV8Wrapper(_engine, V8ScriptValue(_engine, result))); @@ -279,8 +290,10 @@ ScriptValue ScriptValueV8Wrapper::property(const QString& name, const ScriptValu v8::Local key = v8::String::NewFromUtf8(_engine->getIsolate(), name.toStdString().c_str(),v8::NewStringType::kNormal).ToLocalChecked(); const v8::Local object = v8::Local::Cast(_value.constGet()); //V8TODO: Which context? + lock.lockForRead(); if (object->Get(_engine->getContext(), key).ToLocal(&resultLocal)) { V8ScriptValue result(_engine, resultLocal); + lock.unlock(); return ScriptValue(new ScriptValueV8Wrapper(_engine, std::move(result))); } else { QString parentValueQString(""); @@ -311,10 +324,13 @@ ScriptValue ScriptValueV8Wrapper::property(quint32 arrayIndex, const ScriptValue //V8TODO: what about flags? v8::Local resultLocal; const v8::Local object = v8::Local::Cast(_value.constGet()); + lock.lockForRead(); if (object->Get(_value.constGetContext(), arrayIndex).ToLocal(&resultLocal)) { V8ScriptValue result(_engine, resultLocal); + lock.unlock(); return ScriptValue(new ScriptValueV8Wrapper(_engine, std::move(result))); } + lock.unlock(); } qCDebug(scriptengine_v8) << "Failed to get property, parent of value: " << arrayIndex << " is not a V8 object, reported type: " << QString(*v8::String::Utf8Value(isolate, _value.constGet()->TypeOf(isolate))); return _engine->undefinedValue(); @@ -378,7 +394,9 @@ void ScriptValueV8Wrapper::setProperty(const QString& name, const ScriptValue& v v8::Local key = v8::String::NewFromUtf8(isolate, name.toStdString().c_str(),v8::NewStringType::kNormal).ToLocalChecked(); Q_ASSERT(_value.get()->IsObject()); auto object = v8::Local::Cast(_value.get()); + lock.lockForRead(); v8::Maybe retVal = object->Set(isolate->GetCurrentContext(), key, unwrapped.constGet()); + lock.unlock(); if (retVal.IsJust() ? !retVal.FromJust() : true){ qCDebug(scriptengine_v8) << "Failed to set property"; } @@ -410,7 +428,9 @@ void ScriptValueV8Wrapper::setProperty(quint32 arrayIndex, const ScriptValue& va if(_value.constGet()->IsObject()) { auto object = v8::Local::Cast(_value.get()); //V8TODO: I don't know which context to use here + lock.lockForRead(); v8::Maybe retVal(object->Set(_engine->getContext(), arrayIndex, unwrapped.constGet())); + lock.unlock(); if (retVal.IsJust() ? !retVal.FromJust() : true){ qCDebug(scriptengine_v8) << "Failed to set property"; } @@ -436,7 +456,9 @@ void ScriptValueV8Wrapper::setPrototype(const ScriptValue& prototype) { if(unwrappedPrototype->toV8Value().constGet()->IsObject() && _value.constGet()->IsObject()) { auto object = v8::Local::Cast(_value.get()); //V8TODO: I don't know which context to use here + lock.lockForRead(); v8::Maybe retVal = object->SetPrototype(context, unwrappedPrototype->toV8Value().constGet()); + lock.unlock(); if (retVal.IsJust() ? !retVal.FromJust() : true){ qCDebug(scriptengine_v8) << "Failed to assign prototype"; } diff --git a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.h b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.h index aff0685e58..14340dc2f4 100644 --- a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.h +++ b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.h @@ -26,6 +26,8 @@ #include "ScriptEngineV8.h" #include "V8Types.h" +//#define OVERTE_V8_SCRIPT_VALUE_WRAPPER_DELETE_GUARD + /// [V8] Implements ScriptValue for V8 and translates calls for V8ScriptValue class ScriptValueV8Wrapper final : public ScriptValueProxy { public: // construction @@ -102,10 +104,23 @@ public: // ScriptValue implementation virtual QVariant toVariant() const override; virtual QObject* toQObject() const override; +#ifdef OVERTE_V8_SCRIPT_VALUE_WRAPPER_DELETE_GUARD + // These can be used for debugging crashes caused access after delete + // If delete guard is enabled, deleting wrapper will cause a crash and thus trigger debugger and reveal location where object was deleted. + void enableDeleteGuard() { deleteGuard = true;} + void disableDeleteGuard() { deleteGuard = false;} +#endif + protected: virtual ~ScriptValueV8Wrapper() { #ifdef OVERTE_V8_MEMORY_DEBUG _engine->decrementScriptValueProxyCounter(); +#endif +#ifdef OVERTE_V8_SCRIPT_VALUE_WRAPPER_DELETE_GUARD + if (deleteGuard) { + uint32_t* crashTrigger = nullptr; + *crashTrigger = 0x12345678; + } #endif }; @@ -116,6 +131,12 @@ private: // storage ScriptEngineV8 *_engine; V8ScriptValue _value; +#ifdef OVERTE_V8_SCRIPT_VALUE_WRAPPER_DELETE_GUARD + bool deleteGuard{false}; +#endif + // This is to prevent proxy being deleted when it is in use for example during callbacks from inside it + mutable QReadWriteLock lock; + Q_DISABLE_COPY(ScriptValueV8Wrapper) };