Fix V8 memory leak and persistend handle problems

libraries/entities/src/EntityScriptingInterface.cpp

@@ -1645,8 +1645,11 @@ bool EntityScriptingInterface::getServerScriptStatus(const QUuid& entityID, cons
     }
 
     connect(request, &GetScriptStatusRequest::finished, manager, [callback](GetScriptStatusRequest* request) mutable {
-        QString statusString = EntityScriptStatus_::valueToKey(request->getStatus());
         auto engine = callback.engine();
+        // V8TODO: it seems to sometimes be called on a wrong thread, reading to script engine crashes. I added an assert for now
+        Q_ASSERT(QThread::currentThread() == engine->thread());
+        Q_ASSERT(QThread::currentThread() == engine->manager()->thread());
+        QString statusString = EntityScriptStatus_::valueToKey(request->getStatus());
         ScriptValueList args { engine->newValue(request->getResponseReceived()), engine->newValue(request->getIsRunning()), engine->newValue(statusString.toLower()), engine->newValue(request->getErrorInfo()) };
         callback.call(ScriptValue(), args);
         request->deleteLater();

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

@@ -846,7 +846,7 @@ ScriptValue ScriptEngineV8::evaluateInClosure(const ScriptValue& _closure,
     v8::Local<v8::Value> closureGlobal;
     ScriptValueV8Wrapper* unwrappedClosure;
     ScriptProgramV8Wrapper* unwrappedProgram;
-    v8::Local<v8::Context> oldContext = getContext();
+    //v8::Local<v8::Context> oldContext = getContext();
 
     {
         v8::Context::Scope contextScope(getContext());
@@ -1180,7 +1180,9 @@ ScriptContextV8Pointer ScriptEngineV8::pushContext(v8::Local<v8::Context> contex
     _contexts.append(std::make_shared<ScriptContextV8Wrapper>(this, context, ScriptContextPointer()));
     v8::Context::Scope contextScope(context);
     static volatile int debug_context_id = 1;
-    context->Global()->Set(context, v8::String::NewFromUtf8(_v8Isolate, "debug_context_id").ToLocalChecked(), v8::Integer::New(_v8Isolate, debug_context_id));
+    if (!context->Global()->Set(context, v8::String::NewFromUtf8(_v8Isolate, "debug_context_id").ToLocalChecked(), v8::Integer::New(_v8Isolate, debug_context_id)).FromMaybe(false)) {
+        Q_ASSERT(false);
+    }
     debug_context_id++;
     return _contexts.last();
 }

libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp

@@ -504,12 +504,12 @@ void ScriptObjectV8Proxy::v8GetPropertyNames(const v8::PropertyCallbackInfo<v8::
         return;
     }
     V8ScriptValue object(proxy->_engine, objectV8);
-    uint id;
+    //uint id;
     v8::Local<v8::Array> properties = proxy->getPropertyNames();
     v8::Local<v8::Array> objectProperties;
     uint32_t propertiesLength = properties->Length();
     if (info.This()->GetInternalField(2).As<v8::Object>()->GetPropertyNames(context).ToLocal(&objectProperties)) {
-        for (int n = 0; n < objectProperties->Length(); n++) {
+        for (uint32_t n = 0; n < objectProperties->Length(); n++) {
             if(!properties->Set(context, propertiesLength+n, objectProperties->Get(context, n).ToLocalChecked()).FromMaybe(false)) {
                 qDebug(scriptengine) << "ScriptObjectV8Proxy::v8GetPropertyNames: Cannot add member name";
             }
@@ -1262,7 +1262,7 @@ int ScriptSignalV8Proxy::qt_metacall(QMetaObject::Call call, int id, void** argu
     }
     //});
 
-    _totalCallTime_s += callTimer.elapsed() / 1000.0;
+    _totalCallTime_s += callTimer.elapsed() / 1000.0f;
 
     return -1;
 }

libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp

@@ -550,6 +550,7 @@ QObject* ScriptValueV8Wrapper::toQObject() const {
             return dest.value<QObject*>();
         } else {
             Q_ASSERT(false);
+            return nullptr;
         }
     } else {
         Q_ASSERT(false);

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

@@ -23,16 +23,30 @@ template <typename T>
 class V8ScriptValueTemplate {
 public:
     V8ScriptValueTemplate() = delete;
+
+    /*enum class Ownership {
+        Script = 0,
+        Value = 1
+    };*/
     //V8ScriptValueTemplate(v8::Isolate *isolate, v8::Local<T> value) : _isolate(isolate) {
         //_value.reset(v8::UniquePersistent<T>::New(_isolate, value));
     //};
-    V8ScriptValueTemplate(ScriptEngineV8 *engine, const v8::Local<T> value) : _engine(engine) {
+    // V8TODO: Persistent handles need to be constructed and destructed in the same thread
+    // Adding asserts might be a good idea
+    V8ScriptValueTemplate(ScriptEngineV8 *engine, const v8::Local<T> value/*, V8ScriptValueTemplate::Ownership ownership*/) : _engine(engine) {
         v8::Locker locker(_engine->getIsolate());
         v8::Isolate::Scope isolateScope(_engine->getIsolate());
         v8::HandleScope handleScope(_engine->getIsolate());
         v8::Context::Scope(_engine->getContext());
-        _value.reset(new v8::Persistent<T>(_engine->getIsolate(), value));
+        /*if (ownership == V8ScriptValueTemplate::Ownership::Script) {
-        //_value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), value));
+            _value.reset(new v8::WeakPersistent<T>(_engine->getIsolate(), value));
+            return;
+        } else if (ownership == V8ScriptValueTemplate::Ownership::Value) {
+            _value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), value));
+            return;
+        }
+        Q_ASSERT(false);*/
+        _value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), value));
     };
 
     V8ScriptValueTemplate& operator= (const V8ScriptValueTemplate &source) {
@@ -41,7 +55,7 @@ public:
         v8::HandleScope handleScope(_engine->getIsolate());
         v8::Context::Scope(_engine->getContext());
         _engine = source.getEngine();
-        _value.reset(new v8::Persistent<T>(_engine->getIsolate(), source.constGet()));
+        _value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), source.constGet()));
         //_value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), source.constGet()));
         return *this;
     };
@@ -52,7 +66,7 @@ public:
         v8::HandleScope handleScope(_engine->getIsolate());
         v8::Context::Scope(_engine->getContext());
         //_value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), v8::Local<T>()));
-        _value.reset(new v8::Persistent<T>(_engine->getIsolate(), v8::Local<T>()));
+        _value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), v8::Local<T>()));
     };
 
     V8ScriptValueTemplate(const V8ScriptValueTemplate &copied) : _engine(copied.getEngine()) {
@@ -61,7 +75,7 @@ public:
         v8::HandleScope handleScope(_engine->getIsolate());
         v8::Context::Scope(_engine->getContext());
         //_value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), copied.constGet()));
-        _value.reset(new v8::Persistent<T>(_engine->getIsolate(), copied.constGet()));
+        _value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), copied.constGet()));
     }
 
     v8::Local<T> get() {
@@ -106,11 +120,19 @@ public:
     void reset(v8::Isolate *isolate, v8::Local<T>) {
         Q_ASSERT(false);
     };
+    // V8TODO: add thread safe destructors to all objects that have persistent handles
+    ~V8ScriptValueTemplate() {
+        v8::Locker locker(_engine->getIsolate());
+        v8::Isolate::Scope isolateScope(_engine->getIsolate());
+        v8::HandleScope handleScope(_engine->getIsolate());
+        //v8::Context::Scope(_engine->getContext());
+        _value->Reset();
+    }
 
 private:
     // std::shared_ptr<v8::UniquePersistent<T>> _value;
     // V8TODO: Persistent needs reset to release object? It may cause memory leaks here
-    std::shared_ptr<v8::Persistent<T>> _value;
+    std::shared_ptr<v8::UniquePersistent<T>> _value;
     // V8TODO: maybe make it weak
     // does it need , CopyablePersistentTraits<Value>?
     // V8TODO: is context needed at all?

scripts/system/controllers/controllerModules/inEditMode.js

@@ -203,12 +203,13 @@ Script.include("/~/system/libraries/utils.js");
 
             if ((controllerData.triggerClicks[this.hand] === 0 && controllerData.secondaryValues[this.hand] === 0)) {
                 var stopRunning = false;
-                controllerData.nearbyOverlayIDs[this.hand].forEach(function(overlayID) {
+                // V8TODO: check if this doesn't break anything
+                /*controllerData.nearbyOverlayIDs[this.hand].forEach(function(overlayID) {
                     var overlayName = Overlays.getProperty(overlayID, "name");
                     if (overlayName === "KeyboardAnchor") {
                         stopRunning = true;
                     }
-                });
+                });*/
 
                 if (stopRunning) {
                     return this.exitModule();