From 6466d39c05e121f13f34d73df74b904bbf41d564 Mon Sep 17 00:00:00 2001 From: ksuprynowicz Date: Sat, 29 Apr 2023 15:02:14 +0200 Subject: [PATCH] V8 memory leak fixes --- .../src/ScriptManagerScriptingInterface.cpp | 4 +- .../src/v8/ScriptObjectV8Proxy.cpp | 96 +++++++++++++++---- .../src/v8/ScriptObjectV8Proxy.h | 31 +++--- .../src/v8/ScriptValueV8Wrapper.h | 2 +- 4 files changed, 101 insertions(+), 32 deletions(-) diff --git a/libraries/script-engine/src/ScriptManagerScriptingInterface.cpp b/libraries/script-engine/src/ScriptManagerScriptingInterface.cpp index 93ad37d6ca..a5b9ebb13b 100644 --- a/libraries/script-engine/src/ScriptManagerScriptingInterface.cpp +++ b/libraries/script-engine/src/ScriptManagerScriptingInterface.cpp @@ -69,5 +69,7 @@ QVariantMap ScriptManagerScriptingInterface::getMemoryUsageStatistics() { } ScriptValue ScriptManagerScriptingInterface::createGarbageCollectorDebuggingObject() { + //auto value = _manager->engine()->newQObject(new TestQObject, ScriptEngine::ScriptOwnership); return _manager->engine()->newQObject(new TestQObject, ScriptEngine::ScriptOwnership); -} \ No newline at end of file + //return _manager->engine()->newValue(1); +} diff --git a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp index 95fbd15f10..e6ff31863c 100644 --- a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp +++ b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp @@ -146,7 +146,7 @@ V8ScriptValue ScriptObjectV8Proxy::newQObject(ScriptEngineV8* engine, QObject* o if (lookupV8 != enginePtr->_qobjectWrapperMapV8.end()) { enginePtr->_qobjectWrapperMapV8.erase(lookupV8); } - qDebug() << "ScriptObjectV8Proxy::newQObject object deleted, object count: " << enginePtr->_qobjectWrapperMapV8.size(); + //qDebug() << "ScriptObjectV8Proxy::newQObject object deleted, object count: " << enginePtr->_qobjectWrapperMapV8.size(); }); //qDebug() << "ScriptObjectV8Proxy::newQObject object count: " << engine->_qobjectWrapperMapV8.size(); } @@ -218,19 +218,26 @@ QObject* ScriptObjectV8Proxy::unwrap(const V8ScriptValue& val) { } ScriptObjectV8Proxy::~ScriptObjectV8Proxy() { - auto isolate = _engine->getIsolate(); - v8::Locker locker(isolate); - v8::Isolate::Scope isolateScope(isolate); - v8::HandleScope handleScope(isolate); - if(_object) qCDebug(scriptengine_v8) << "Deleting object proxy: " << name(); - // V8TODO: once WeakPersistent pointer is added we should check if it's valid before deleting - Q_ASSERT(!_v8Object.Get(isolate)->IsNullOrUndefined()); - // This prevents unwrap function from unwrapping proxy that was deleted - _v8Object.Get(isolate)->SetAlignedPointerInInternalField(0, const_cast(internalPointsToDeletedQObjectProxy)); - _v8Object.Reset(); if (_ownsObject) { + auto isolate = _engine->getIsolate(); + v8::Locker locker(isolate); + v8::Isolate::Scope isolateScope(isolate); + v8::HandleScope handleScope(isolate); + _v8Object.Reset(); QObject* qobject = _object; if(qobject) qobject->deleteLater(); + } else { + auto isolate = _engine->getIsolate(); + v8::Locker locker(isolate); + v8::Isolate::Scope isolateScope(isolate); + v8::HandleScope handleScope(isolate); + if (_object) + qCDebug(scriptengine_v8) << "Deleting object proxy: " << name(); + // V8TODO: once WeakPersistent pointer is added we should check if it's valid before deleting + Q_ASSERT(!_v8Object.Get(isolate)->IsNullOrUndefined()); + // This prevents unwrap function from unwrapping proxy that was deleted + _v8Object.Get(isolate)->SetAlignedPointerInInternalField(0, const_cast(internalPointsToDeletedQObjectProxy)); + _v8Object.Reset(); } } @@ -382,6 +389,7 @@ void ScriptObjectV8Proxy::investigate() { // Add all the methods objects as properties - this allows adding properties to a given method later. Is used by Script.request. // V8TODO: Should these be deleted when the script-owned object is destroyed? It needs checking if script-owned objects will be garbage-collected, or will self-referencing prevent it. for (auto i = _methods.begin(); i != _methods.end(); i++) { + //V8TODO: lifetime may prevent garbage collection? V8ScriptValue method = ScriptMethodV8Proxy::newMethod(_engine, qobject, V8ScriptValue(_engine, v8Object), i.value().methods, i.value().numMaxParms); if(!propertiesObject->Set(_engine->getContext(), i.value().name.constGet(), method.get()).FromMaybe(false)) { @@ -393,7 +401,9 @@ void ScriptObjectV8Proxy::investigate() { void ScriptObjectV8Proxy::weakHandleCallback(const v8::WeakCallbackInfo& info) { //V8TODO: does the object need to be moved to script engine thread? //V8TODO: why does this never get called? - qDebug(scriptengine_v8) << "ScriptObjectV8Proxy::weakHandleCallback"; + //qDebug(scriptengine_v8) << "ScriptObjectV8Proxy::weakHandleCallback"; + auto proxy = info.GetParameter(); + proxy->_v8Object.Reset(); info.GetParameter()->_object->deleteLater(); } @@ -775,6 +785,7 @@ ScriptVariantV8Proxy::~ScriptVariantV8Proxy() { } V8ScriptValue ScriptVariantV8Proxy::newVariant(ScriptEngineV8* engine, const QVariant& variant, V8ScriptValue proto) { + qDebug() << "ScriptVariantV8Proxy::newVariant"; auto isolate = engine->getIsolate(); v8::Locker locker(isolate); v8::Isolate::Scope isolateScope(isolate); @@ -960,12 +971,30 @@ QVariant ScriptVariantV8Proxy::unwrap(const V8ScriptValue& val) { ScriptMethodV8Proxy::ScriptMethodV8Proxy(ScriptEngineV8* engine, QObject* object, V8ScriptValue lifetime, const QList& metas, int numMaxParams) : - _numMaxParams(numMaxParams), _engine(engine), _object(object), _objectLifetime(lifetime), _metas(metas) { + _numMaxParams(numMaxParams), _engine(engine), _object(object), /*_objectLifetime(lifetime),*/ _metas(metas) { + auto isolate = engine->getIsolate(); + v8::Locker locker(isolate); + v8::Isolate::Scope isolateScope(isolate); + v8::HandleScope handleScope(isolate); + v8::Context::Scope contextScope(engine->getContext()); + _objectLifetime.Reset(isolate, lifetime.get()); + _objectLifetime.SetWeak(this, weakHandleCallback, v8::WeakCallbackType::kParameter); } ScriptMethodV8Proxy::~ScriptMethodV8Proxy() { - qCDebug(scriptengine_v8) << "ScriptMethodV8Proxy destroyed"; - printf("ScriptMethodV8Proxy destroyed"); + //qCDebug(scriptengine_v8) << "ScriptMethodV8Proxy destroyed"; + auto isolate = _engine->getIsolate(); + v8::Locker locker(isolate); + v8::Isolate::Scope isolateScope(isolate); + v8::HandleScope handleScope(isolate); + _objectLifetime.Reset(); +} + +void ScriptMethodV8Proxy::weakHandleCallback(const v8::WeakCallbackInfo& info) { + //qDebug(scriptengine_v8) << "ScriptMethodV8Proxy::weakHandleCallback"; + auto proxy = info.GetParameter(); + proxy->_objectLifetime.Reset(); + info.GetParameter()->deleteLater(); } V8ScriptValue ScriptMethodV8Proxy::newMethod(ScriptEngineV8* engine, QObject* object, V8ScriptValue lifetime, @@ -980,6 +1009,7 @@ V8ScriptValue ScriptMethodV8Proxy::newMethod(ScriptEngineV8* engine, QObject* ob auto methodData = methodDataTemplate->NewInstance(engine->getContext()).ToLocalChecked(); methodData->SetAlignedPointerInInternalField(0, const_cast(internalPointsToMethodProxy)); // V8TODO it needs to be deleted somehow on object destruction + // weak persistent callback would do this methodData->SetAlignedPointerInInternalField(1, reinterpret_cast(new ScriptMethodV8Proxy(engine, object, lifetime, metas, numMaxParams))); auto v8Function = v8::Function::New(engine->getContext(), callback, methodData, numMaxParams).ToLocalChecked(); return V8ScriptValue(engine, v8Function); @@ -1068,6 +1098,7 @@ void ScriptMethodV8Proxy::call(const v8::FunctionCallbackInfo& argume int bestMeta = 0; int bestConversionPenaltyScore = 0; + for (int i = 0; i < num_metas; i++) { const QMetaMethod& meta = _metas[i]; int methodNumArgs = meta.parameterCount(); @@ -1162,14 +1193,25 @@ void ScriptMethodV8Proxy::call(const v8::FunctionCallbackInfo& argume return; } else if (returnTypeId == scriptValueTypeId) { ScriptValue result; - bool success = meta.invoke(qobject, Qt::DirectConnection, Q_RETURN_ARG(ScriptValue, result), qGenArgs[0], + /*if (_metas.front().name() == "createGarbageCollectorDebuggingObject") { + //qDebug() << "createGarbageCollectorDebuggingObject"; + return; + }*/ + /*const char* typeName = meta.typeName(); + QVariant qRetVal(returnTypeId, static_cast(NULL)); + QGenericReturnArgument sRetVal(typeName, const_cast(qRetVal.constData())); + bool success = meta.invoke(qobject, Qt::DirectConnection, sRetVal, qGenArgs[0], qGenArgs[1], qGenArgs[2], qGenArgs[3], qGenArgs[4], qGenArgs[5], qGenArgs[6], - qGenArgs[7], qGenArgs[8], qGenArgs[9]); + qGenArgs[7], qGenArgs[8], qGenArgs[9]);*/ + bool success = meta.invoke(qobject, Qt::DirectConnection, Q_RETURN_ARG(ScriptValue, result), qGenArgs[0], + qGenArgs[1], qGenArgs[2], qGenArgs[3], qGenArgs[4], qGenArgs[5], qGenArgs[6], + qGenArgs[7], qGenArgs[8], qGenArgs[9]); if (!success) { isolate->ThrowError(v8::String::NewFromUtf8(isolate, QString("Unexpected: Native call of %1 failed").arg(fullName()).toStdString().c_str()).ToLocalChecked()); return; } V8ScriptValue v8Result = ScriptValueV8Wrapper::fullUnwrap(_engine, result); + //V8ScriptValue v8Result = _engine->castVariantToValue(qRetVal); arguments.GetReturnValue().Set(v8Result.get()); return; } else { @@ -1396,14 +1438,34 @@ void ScriptMethodV8Proxy::call(const v8::FunctionCallbackInfo& argume return QVariant(); }*/ +ScriptSignalV8Proxy::ScriptSignalV8Proxy(ScriptEngineV8* engine, QObject* object, V8ScriptValue lifetime, const QMetaMethod& meta) : + _engine(engine), _object(object), _meta(meta), _metaCallId(discoverMetaCallIdx()) { + auto isolate = _engine->getIsolate(); + v8::Locker locker(isolate); + v8::Isolate::Scope isolateScope(isolate); + v8::HandleScope handleScope(isolate); + v8::Context::Scope contextScope(_engine->getContext()); + _objectLifetime.Reset(isolate, lifetime.get()); + _objectLifetime.SetWeak(this, weakHandleCallback, v8::WeakCallbackType::kParameter); + _v8Context.Reset(isolate, _engine->getContext()); +} + ScriptSignalV8Proxy::~ScriptSignalV8Proxy() { auto isolate = _engine->getIsolate(); v8::Locker locker(isolate); v8::Isolate::Scope isolateScope(isolate); v8::HandleScope handleScope(isolate); + _objectLifetime.Reset(); _v8Context.Reset(); } +void ScriptSignalV8Proxy::weakHandleCallback(const v8::WeakCallbackInfo& info) { + //qDebug(scriptengine_v8) << "ScriptSignalV8Proxy::weakHandleCallback"; + auto proxy = info.GetParameter(); + proxy->_objectLifetime.Reset(); + proxy->deleteLater(); +} + QString ScriptSignalV8Proxy::fullName() const { Q_ASSERT(_object); if (!_object) return ""; diff --git a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h index 7151e5010a..a0c0f875ed 100644 --- a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h +++ b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h @@ -33,6 +33,11 @@ class ScriptEngineV8; class ScriptSignalV8Proxy; +// V8TODO: Current implementation relies on weak handle callbacks for destroying objects on C++ side +// this is fine for avoiding memory leaks while script engine runs, but there's no guarantee that these will be called +// when script engine shutd down, so memory leaks may happen +// To avoid this handle visitor needs to be added (it's a feature of V8) + /// [V8] (re-)implements the translation layer between ScriptValue and QObject. This object /// will focus exclusively on property get/set until function calls appear to be a problem class ScriptObjectV8Proxy final { @@ -108,11 +113,11 @@ public: static void v8Get(v8::Local name, const v8::PropertyCallbackInfo& info); static void v8Set(v8::Local name, v8::Local value_obj, const v8::PropertyCallbackInfo& info); static void v8GetPropertyNames(const v8::PropertyCallbackInfo& info); - // This gets called when script-owned object is being garbage-collected - static void weakHandleCallback(const v8::WeakCallbackInfo &info); private: // implementation void investigate(); + // This gets called when script-owned object is being garbage-collected + static void weakHandleCallback(const v8::WeakCallbackInfo &info); private: // storage ScriptEngineV8* _engine; @@ -150,6 +155,7 @@ private: // storage new AnimationObject(), ScriptEngine::ScriptOwnership)); * */ + // V8TODO: there may be memory leaks in these class ScriptVariantV8Proxy final { public: // construction ScriptVariantV8Proxy(ScriptEngineV8* engine, const QVariant& variant, V8ScriptValue scriptProto, ScriptObjectV8Proxy* proto); @@ -204,7 +210,8 @@ private: Q_DISABLE_COPY(ScriptVariantV8Proxy) }; -class ScriptMethodV8Proxy final { +class ScriptMethodV8Proxy final : public QObject { + Q_OBJECT public: // construction ScriptMethodV8Proxy(ScriptEngineV8* engine, QObject* object, V8ScriptValue lifetime, const QList& metas, int numMaxParams); @@ -220,13 +227,15 @@ public: // QScriptClass implementation const QList& metas, int numMaxParams); private: + static void weakHandleCallback(const v8::WeakCallbackInfo &info); QString fullName() const; private: // storage const int _numMaxParams; ScriptEngineV8* _engine; QPointer _object; - V8ScriptValue _objectLifetime; + v8::Persistent _objectLifetime; + //V8ScriptValue _objectLifetime; const QList _metas; Q_DISABLE_COPY(ScriptMethodV8Proxy) @@ -257,14 +266,7 @@ private: // storage using ConnectionList = QList; public: // construction - inline ScriptSignalV8Proxy(ScriptEngineV8* engine, QObject* object, V8ScriptValue lifetime, const QMetaMethod& meta) : - _engine(engine), _object(object), _objectLifetime(lifetime), _meta(meta), _metaCallId(discoverMetaCallIdx()) { - v8::Locker locker(_engine->getIsolate()); - v8::Isolate::Scope isolateScope(_engine->getIsolate()); - v8::HandleScope handleScope(_engine->getIsolate()); - v8::Context::Scope contextScope(_engine->getContext()); - _v8Context.Reset(_engine->getIsolate(), _engine->getContext()); - } + inline ScriptSignalV8Proxy(ScriptEngineV8* engine, QObject* object, V8ScriptValue lifetime, const QMetaMethod& meta); ~ScriptSignalV8Proxy(); @@ -273,6 +275,7 @@ private: // implementation int discoverMetaCallIdx(); ConnectionList::iterator findConnection(V8ScriptValue thisObject, V8ScriptValue callback); //QString fullName() const; + static void weakHandleCallback(const v8::WeakCallbackInfo &info); public: // API // arg1 was had Null default value, but that needs isolate pointer to create Null in V8 @@ -285,9 +288,11 @@ public: // API //virtual void disconnect(V8ScriptValue arg0) override; private: // storage + ScriptEngineV8* _engine; QPointer _object; - V8ScriptValue _objectLifetime; + v8::Persistent _objectLifetime; + const QMetaMethod _meta; const int _metaCallId; ConnectionList _connections; diff --git a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.h b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.h index f7d4ba8eb1..e9ccb3a02d 100644 --- a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.h +++ b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.h @@ -38,7 +38,7 @@ public: // construction static ScriptValueV8Wrapper* unwrap(const ScriptValue& val); inline const V8ScriptValue& toV8Value() const { return _value; } static V8ScriptValue fullUnwrap(ScriptEngineV8* engine, const ScriptValue& value); - ScriptEngineV8* getV8Engine() {return _engine;}; + ScriptEngineV8* getV8Engine() {return _engine;} public: virtual void release() override;