From cae1e1195d5324448709a4e77954b85eecb19f0b Mon Sep 17 00:00:00 2001 From: ksuprynowicz Date: Fri, 3 Mar 2023 00:38:40 +0100 Subject: [PATCH] V8 QObject pointer handling changes --- libraries/animation/src/AnimVariant.cpp | 2 +- .../src/v8/ScriptContextV8Wrapper.h | 4 +- .../src/v8/ScriptEngineV8_cast.cpp | 9 +- .../src/v8/ScriptObjectV8Proxy.cpp | 85 +++++++++++++------ .../src/v8/ScriptValueV8Wrapper.cpp | 4 +- libraries/script-engine/src/v8/V8Types.h | 2 - 6 files changed, 72 insertions(+), 34 deletions(-) diff --git a/libraries/animation/src/AnimVariant.cpp b/libraries/animation/src/AnimVariant.cpp index 72fca63f7f..3d38252407 100644 --- a/libraries/animation/src/AnimVariant.cpp +++ b/libraries/animation/src/AnimVariant.cpp @@ -124,7 +124,7 @@ void AnimVariantMap::animVariantMapFromScriptValue(const ScriptValue& source) { } } qCWarning(animation) << "Ignoring unrecognized data " << value.toString() << " for animation property " << property->name(); - // V8TODO this was spamming to much logs but needs to be fixed later + // V8TODO this was spamming too much logs but needs to be fixed later //Q_ASSERT(false); } } diff --git a/libraries/script-engine/src/v8/ScriptContextV8Wrapper.h b/libraries/script-engine/src/v8/ScriptContextV8Wrapper.h index 0ba83a7dd0..ddb14f1d4c 100644 --- a/libraries/script-engine/src/v8/ScriptContextV8Wrapper.h +++ b/libraries/script-engine/src/v8/ScriptContextV8Wrapper.h @@ -58,7 +58,7 @@ private: // storage const v8::FunctionCallbackInfo *_functionCallbackInfo; const v8::PropertyCallbackInfo *_propertyCallbackInfo; ScriptEngineV8* _engine; - // V8TODO: Is custom copy constructor needed for thread safety? + // V8TODO: custom destructor is needed for v8::Persistent v8::Persistent _context; ScriptContextPointer _parentContext; Q_DISABLE_COPY(ScriptContextV8Wrapper) @@ -77,7 +77,7 @@ public: // ScriptFunctionContext implementation private: // storage ScriptEngineV8* _engine; - // V8TODO: Is custom copy constructor needed for thread safety? + // V8TODO: custom destructor is needed for v8::Persistent (check is all other classes using different types of persistent handles have custom destructors too) v8::Persistent _context; //V8ScriptContextInfo _value; Q_DISABLE_COPY(ScriptFunctionContextV8Wrapper) diff --git a/libraries/script-engine/src/v8/ScriptEngineV8_cast.cpp b/libraries/script-engine/src/v8/ScriptEngineV8_cast.cpp index 7eb0cbe7b4..74016eac65 100644 --- a/libraries/script-engine/src/v8/ScriptEngineV8_cast.cpp +++ b/libraries/script-engine/src/v8/ScriptEngineV8_cast.cpp @@ -471,8 +471,9 @@ bool ScriptEngineV8::castValueToVariant(const V8ScriptValue& v8Val, QVariant& de // V8TODO: this is to diagnose a really weird segfault where it looks like only half of the QPointer is set to null upon object deletion uint64_t ptrVal = (uint64_t)(ScriptObjectV8Proxy::unwrap(v8Val)); if ((uint32_t)(ptrVal) == 0 && ptrVal != 0) { - printf("break"); - Q_ASSERT(false); + qDebug() << "ScriptEngineV8::castValueToVariant pointer bug happened"; + //Q_ASSERT(false); + return false; } } dest = QVariant::fromValue(ScriptObjectV8Proxy::unwrap(v8Val)); @@ -754,7 +755,9 @@ V8ScriptValue ScriptEngineV8::castVariantToValue(const QVariant& val) { } default: // check to see if this is a pointer to a QObject-derived object - if (QMetaType::typeFlags(valTypeId) & (QMetaType::PointerToQObject | QMetaType::TrackingPointerToQObject)) { + // V8TODO: I added WeakPointerToQObject and SharedPointerToQObject here, but maybe they need different object ownership? + if (QMetaType::typeFlags(valTypeId) & (QMetaType::PointerToQObject | QMetaType::TrackingPointerToQObject + | QMetaType::WeakPointerToQObject | QMetaType::SharedPointerToQObject)) { QObject* obj = val.value(); if (obj == nullptr) return V8ScriptValue(this, v8::Null(_v8Isolate)); return ScriptObjectV8Proxy::newQObject(this, obj); diff --git a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp index 5159184896..36f3521dd7 100644 --- a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp +++ b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp @@ -66,6 +66,7 @@ private: // storage ScriptObjectV8Proxy::ScriptObjectV8Proxy(ScriptEngineV8* engine, QObject* object, bool ownsObject, const ScriptEngine::QObjectWrapOptions& options) : _engine(engine), _wrapOptions(options), _ownsObject(ownsObject), _object(object) { //_v8ObjectTemplate(engine->getIsolate(), v8::ObjectTemplate::New(engine->getIsolate()) + Q_ASSERT(_engine != nullptr); investigate(); } @@ -451,21 +452,40 @@ void ScriptObjectV8Proxy::v8Get(v8::Local name, const v8::PropertyCall return; } V8ScriptValue object(proxy->_engine, objectV8); - Q_ASSERT(name->IsString()); - V8ScriptString nameString(proxy->_engine, v8::Local::Cast(name)); - uint id; - QueryFlags flags = proxy->queryProperty(object, nameString, HandlesReadAccess, &id); - if (flags) { - V8ScriptValue value = proxy->property(object, nameString, id); - info.GetReturnValue().Set(value.get()); + if (!name->IsString() && !name->IsSymbol()) { + QString notStringMessage("ScriptObjectV8Proxy::v8Get: " + proxy->_engine->scriptValueDebugDetailsV8(V8ScriptValue(proxy->_engine, name))); + qDebug(scriptengine) << notStringMessage; + Q_ASSERT(false); + } + v8::Local v8NameString; + /*if (name->IsString()) { + v8NameString = v8::Local::Cast(name); } else { - v8::Local property; - if(info.This()->GetInternalField(2).As()->Get(proxy->_engine->getContext(), name).ToLocal(&property)) { - info.GetReturnValue().Set(property); - } else { - qDebug(scriptengine) << "Value not found: " << *utf8Value; + if (!name->ToString(info.GetIsolate()->GetCurrentContext()).ToLocal(&v8NameString)) { + Q_ASSERT(false); } } + + if (name->IsSymbol()) { + qDebug(scriptengine) << "ScriptObjectV8Proxy::v8Set: symbol: " + nameString.toQString(); + }*/ + if (name->IsString()) { + V8ScriptString nameString(proxy->_engine, v8::Local::Cast(name)); + uint id; + QueryFlags flags = proxy->queryProperty(object, nameString, HandlesReadAccess, &id); + if (flags) { + V8ScriptValue value = proxy->property(object, nameString, id); + info.GetReturnValue().Set(value.get()); + return; + } + } + + v8::Local property; + if(info.This()->GetInternalField(2).As()->Get(proxy->_engine->getContext(), name).ToLocal(&property)) { + info.GetReturnValue().Set(property); + } else { + qDebug(scriptengine) << "Value not found: " << *utf8Value; + } } void ScriptObjectV8Proxy::v8Set(v8::Local name, v8::Local value, const v8::PropertyCallbackInfo& info) { @@ -480,21 +500,38 @@ void ScriptObjectV8Proxy::v8Set(v8::Local name, v8::Local v return; } V8ScriptValue object(proxy->_engine, objectV8); - Q_ASSERT(name->IsString()); - V8ScriptString nameString(proxy->_engine, v8::Local::Cast(name)); + if (!name->IsString() && !name->IsSymbol()) { + QString notStringMessage("ScriptObjectV8Proxy::v8Set: " + proxy->_engine->scriptValueDebugDetailsV8(V8ScriptValue(proxy->_engine, name))); + qDebug(scriptengine) << notStringMessage; + Q_ASSERT(false); + } + /*v8::Local v8NameString; + if (name->IsString()) { + v8NameString = v8::Local::Cast(name); + } else { + if (!name->ToString(info.GetIsolate()->GetCurrentContext()).ToLocal(&v8NameString)) { + Q_ASSERT(false); + } + } + if (name->IsSymbol()) { + qDebug(scriptengine) << "ScriptObjectV8Proxy::v8Set: symbol: " + nameString.toQString(); + }*/ //V8ScriptString nameString(info.GetIsolate(), name->ToString(proxy->_engine->getContext()).ToLocalChecked()); - uint id; - QueryFlags flags = proxy->queryProperty(object, nameString, HandlesWriteAccess, &id); - if (flags) { - proxy->setProperty(object, nameString, id, V8ScriptValue(proxy->_engine, value)); + if (name->IsString()) { + V8ScriptString nameString(proxy->_engine, v8::Local::Cast(name)); + uint id; + QueryFlags flags = proxy->queryProperty(object, nameString, HandlesWriteAccess, &id); + if (flags) { + proxy->setProperty(object, nameString, id, V8ScriptValue(proxy->_engine, value)); + info.GetReturnValue().Set(value); + return; + } + } + // V8TODO: Should it be v8::Object or v8::Local? + if (info.This()->GetInternalField(2).As()->Set(proxy->_engine->getContext(), name, value).FromMaybe(false)) { info.GetReturnValue().Set(value); } else { - // V8TODO: Should it be v8::Object or v8::Local? - if (info.This()->GetInternalField(2).As()->Set(proxy->_engine->getContext(), name, value).FromMaybe(false)) { - info.GetReturnValue().Set(value); - } else { - qDebug(scriptengine) << "Set failed: " << *utf8Value; - } + qDebug(scriptengine) << "Set failed: " << *utf8Value; } } diff --git a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp index b3ec666914..8bcb0cf3e2 100644 --- a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp +++ b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp @@ -554,11 +554,11 @@ QObject* ScriptValueV8Wrapper::toQObject() const { if (dest.canConvert()) { return dest.value(); } else { - Q_ASSERT(false); + //Q_ASSERT(false); return nullptr; } } else { - Q_ASSERT(false); + //Q_ASSERT(false); return nullptr; } } diff --git a/libraries/script-engine/src/v8/V8Types.h b/libraries/script-engine/src/v8/V8Types.h index 7e7aced97c..c64a807c32 100644 --- a/libraries/script-engine/src/v8/V8Types.h +++ b/libraries/script-engine/src/v8/V8Types.h @@ -131,8 +131,6 @@ public: } private: - // std::shared_ptr> _value; - // V8TODO: Persistent needs reset to release object? It may cause memory leaks here std::shared_ptr> _value; // V8TODO: maybe make it weak // does it need , CopyablePersistentTraits?