Fixed XMLHttpRequest crash

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<ScriptException> exception);
 
 protected:
-    ~ScriptEngine() {}  // prevent explicit deletion of base class
+    virtual ~ScriptEngine() {}  // prevent explicit deletion of base class
 
     ScriptManager *_manager;
 };

libraries/script-engine/src/ScriptManager.cpp

@@ -980,6 +980,7 @@ void ScriptManager::run() {
                 QCoreApplication::processEvents();
             }
             processedEvents = true;
+            _engine->perManagerLoopIterationCleanup();
         }
 
         PROFILE_RANGE(script, "ScriptMainLoop");

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();
 

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;

libraries/script-engine/src/v8/ScriptEngineV8.h

@@ -45,6 +45,7 @@ class ScriptEngineV8;
 class ScriptManager;
 class ScriptObjectV8Proxy;
 class ScriptMethodV8Proxy;
+class ScriptValueV8Wrapper;
 
 template <typename T> class V8ScriptValueTemplate;
 typedef V8ScriptValueTemplate<v8::Value> 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<QObject*, QSharedPointer<ScriptObjectV8Proxy>> _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<ScriptValueV8Wrapper*> _scriptValueWrappersToDelete;
+
     // Used by ScriptObjectV8Proxy to create JS objects referencing C++ ones
     v8::Local<v8::ObjectTemplate> getObjectProxyTemplate();
     v8::Local<v8::ObjectTemplate> getMethodDataTemplate();
@@ -252,6 +261,9 @@ protected:
     v8::Persistent<v8::ObjectTemplate> _variantDataTemplate;
     v8::Persistent<v8::ObjectTemplate> _variantProxyTemplate;
 
+public:
+    volatile int _memoryCorruptionIndicator = 12345678;
+private:
     //V8TODO
     //ArrayBufferClass* _arrayBufferClass;
     // Counts how many nested evaluate calls are there at a given point

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<v8::Function> v8Function = v8::Local<v8::Function>::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<v8::Object> 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<v8::String> key = v8::String::NewFromUtf8(_engine->getIsolate(), name.toStdString().c_str(),v8::NewStringType::kNormal).ToLocalChecked();
         const v8::Local<v8::Object> object = v8::Local<v8::Object>::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<v8::Value> resultLocal;
         const v8::Local<v8::Object> object = v8::Local<v8::Object>::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<v8::String> key = v8::String::NewFromUtf8(isolate, name.toStdString().c_str(),v8::NewStringType::kNormal).ToLocalChecked();
         Q_ASSERT(_value.get()->IsObject());
         auto object = v8::Local<v8::Object>::Cast(_value.get());
+        lock.lockForRead();
         v8::Maybe<bool> 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<v8::Object>::Cast(_value.get());
         //V8TODO: I don't know which context to use here
+        lock.lockForRead();
         v8::Maybe<bool> 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<v8::Object>::Cast(_value.get());
             //V8TODO: I don't know which context to use here
+            lock.lockForRead();
             v8::Maybe<bool> retVal = object->SetPrototype(context, unwrappedPrototype->toV8Value().constGet());
+            lock.unlock();
             if (retVal.IsJust() ? !retVal.FromJust() : true){
                 qCDebug(scriptengine_v8) << "Failed to assign prototype";
             }

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)
 };