Mirrors/overte
3
0
Fork 0
mirror of https://github.com/overte-org/overte.git synced 2025-04-07 19:12:28 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

Increased thread safety in V8

Browse source
This commit is contained in:
ksuprynowicz 2023-02-11 20:42:25 +01:00
parent 57a2dccc6a
commit c3417b807c
14 changed files with 102 additions and 30 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
libraries/script-engine/src/ScriptManager.cpp
View file

@ -1322,7 +1322,7 @@ ScriptValue ScriptManager::newModule(const QString& modulePath, const ScriptValu
auto closure = _engine->newObject();
auto exports = _engine->newObject();
auto module = _engine->newObject();
qCDebug(scriptengine_module) << "newModule" << parent.property("filename").toString();
//qCDebug(scriptengine_module) << "newModule" << parent.property("filename").toString();
closure.setProperty("module", module, READONLY_PROP_FLAGS);
@ -1530,7 +1530,7 @@ ScriptValue ScriptManager::require(const QString& moduleId) {
// set up a new reference point for detecting cache key deletion
cacheMeta.setProperty(modulePath, module);
qCDebug(scriptengine_module) << "//ScriptManager::require(" << moduleId << ")";
//qCDebug(scriptengine_module) << "//ScriptManager::require(" << moduleId << ")";
_engine->maybeEmitUncaughtException(__FUNCTION__);
//qCDebug(scriptengine_module) << "Exports: " << _engine->scriptValueDebugDetails(module.property("exports"));

3
libraries/script-engine/src/ScriptValueUtils.cpp
View file

@ -459,6 +459,9 @@ ScriptValue quatToScriptValue(ScriptEngine* engine, const glm::quat& quat) {
// V8TODO: add similar check to other conversions
bool quatFromScriptValue(const ScriptValue& object, glm::quat& quat) {
if (!object.isValid()) {
return false;
}
if (!object.isObject()) {
return false;
}

5
libraries/script-engine/src/v8/ScriptContextV8Wrapper.cpp
View file

@ -178,7 +178,8 @@ ScriptValue ScriptContextV8Wrapper::throwError(const QString& text) {
return ScriptValue(new ScriptValueV8Wrapper(_engine, std::move(result)));
} else {
qWarning() << "throwError on a different thread not implemented yet, error value: " << text;
return _engine->undefinedValue();
//return _engine->undefinedValue();
return ScriptValue();
}
}
@ -197,7 +198,7 @@ ScriptValue ScriptContextV8Wrapper::throwValue(const ScriptValue& value) {
}
ScriptFunctionContextV8Wrapper::ScriptFunctionContextV8Wrapper(ScriptEngineV8* engine, const v8::Local<v8::Context> context) : _engine(engine) {
v8::Locker locker();
v8::Locker locker(engine->getIsolate());
v8::Isolate::Scope isolateScope(engine->getIsolate());
v8::HandleScope handleScope(engine->getIsolate());
_context.Reset(engine->getIsolate(), context);

2
libraries/script-engine/src/v8/ScriptContextV8Wrapper.h
View file

@ -60,6 +60,7 @@ private: // storage
// V8TODO: Is custom copy constructor needed for thread safety?
v8::Persistent<v8::Context> _context;
ScriptContextPointer _parentContext;
Q_DISABLE_COPY(ScriptContextV8Wrapper)
};
class ScriptFunctionContextV8Wrapper final : public ScriptFunctionContext {
@ -78,6 +79,7 @@ private: // storage
// V8TODO: Is custom copy constructor needed for thread safety?
v8::Persistent<v8::Context> _context;
//V8ScriptContextInfo _value;
Q_DISABLE_COPY(ScriptFunctionContextV8Wrapper)
};
#endif // hifi_ScriptContextV8Wrapper_h

6
libraries/script-engine/src/v8/ScriptEngineV8.cpp
View file

@ -339,7 +339,7 @@ Lambda::~Lambda() {
}
Lambda::Lambda(ScriptEngineV8* engine,
std::function<V8ScriptValue(V8ScriptContext*, ScriptEngineV8*)> operation,
std::function<V8ScriptValue(ScriptEngineV8*)> operation,
V8ScriptValue data) :
_engine(engine),
_operation(operation), _data(data) {
@ -1056,7 +1056,7 @@ ScriptValue ScriptEngineV8::evaluate(const QString& sourceCode, const QString& f
_evaluatingCounter--;
return err;
}
qCDebug(scriptengine) << "Script compilation succesful: " << fileName;
//qCDebug(scriptengine) << "Script compilation succesful: " << fileName;
//V8TODO
/*auto syntaxError = lintScript(sourceCode, fileName);
@ -1599,7 +1599,7 @@ int ScriptEngineV8::uncaughtExceptionLineNumber() const {
bool ScriptEngineV8::raiseException(const ScriptValue& exception) {
//V8TODO
//Q_ASSERT(false);
qCritical() << "Script exception occured: " << exception.toString();
qCritical() << "Script exception occurred: " << exception.toString();
/*ScriptValueV8Wrapper* unwrapped = ScriptValueV8Wrapper::unwrap(exception);
V8ScriptValue qException = unwrapped ? unwrapped->toV8Value() : QScriptEngine::newVariant(exception.toVariant());
return raiseException(qException);*/

11
libraries/script-engine/src/v8/ScriptEngineV8_cast.cpp
View file

@ -343,7 +343,8 @@ bool ScriptEngineV8::castValueToVariant(const V8ScriptValue& v8Val, QVariant& de
_customTypeProtect.unlock();
}
if (demarshalFunc) {
dest = QVariant(destTypeId, static_cast<void*>(NULL));
//dest = QVariant(destTypeId, static_cast<void*>(NULL));
dest = QVariant();
ScriptValue wrappedVal(new ScriptValueV8Wrapper(this, v8Val));
//Q_ASSERT(dest.constData() != nullptr);
bool success = demarshalFunc(wrappedVal, dest);
@ -464,6 +465,14 @@ bool ScriptEngineV8::castValueToVariant(const V8ScriptValue& v8Val, QVariant& de
dest = QVariant::fromValue(static_cast<uint16_t>(val->ToUint32(context).ToLocalChecked()->Value()));
break;
case QMetaType::QObjectStar:
{
// 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);
}
}
dest = QVariant::fromValue(ScriptObjectV8Proxy::unwrap(v8Val));
break;
case QMetaType::QVariant:

25
libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp
View file

@ -21,9 +21,11 @@
#include "ScriptContextV8Wrapper.h"
#include "ScriptValueV8Wrapper.h"
Q_DECLARE_METATYPE(V8ScriptContext*)
//V8TODO: is this needed for anything? It could cause trouble with multithreading if V8ScriptContext is v8::Persistent
//Q_DECLARE_METATYPE(V8ScriptContext*)
Q_DECLARE_METATYPE(ScriptValue)
//V8TODO?
//V8TODO: default constructor would be needed
//Q_DECLARE_METATYPE(V8ScriptValue)
Q_DECLARE_METATYPE(QSharedPointer<ScriptObjectV8Proxy>)
@ -60,8 +62,8 @@ 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())) {
_engine(engine), _wrapOptions(options), _ownsObject(ownsObject), _object(object) {
//_v8ObjectTemplate(engine->getIsolate(), v8::ObjectTemplate::New(engine->getIsolate())
investigate();
}
@ -147,6 +149,9 @@ ScriptObjectV8Proxy* ScriptObjectV8Proxy::unwrapProxy(const V8ScriptValue& val)
//V8TODO This shouldn't cause problems but I'm not sure if it's ok
//v8::HandleScope handleScope(const_cast<v8::Isolate*>(val.constGetIsolate()));
auto v8Value = val.constGet();
if (v8Value->IsNullOrUndefined()) {
return nullptr;
}
if (!v8Value->IsObject()) {
//qDebug(scriptengine) << "Cannot unwrap proxy - value is not an object";
return nullptr;
@ -170,6 +175,10 @@ ScriptObjectV8Proxy* ScriptObjectV8Proxy::unwrapProxy(v8::Isolate* isolate, v8::
//V8TODO: shouldn't there be context scope here?
//v8::Local<v8::Context> context = val.constGetContext();
//v8::Context::Scope contextScope(context);
if (value->IsNullOrUndefined()) {
//qDebug(scriptengine) << "Cannot unwrap proxy - value is not an object";
return nullptr;
}
if (!value->IsObject()) {
//qDebug(scriptengine) << "Cannot unwrap proxy - value is not an object";
return nullptr;
@ -201,7 +210,10 @@ ScriptObjectV8Proxy::~ScriptObjectV8Proxy() {
void ScriptObjectV8Proxy::investigate() {
QObject* qobject = _object;
Q_ASSERT(qobject);
if (!qobject) {
QStringList backtrace = _engine->currentContext()->backtrace();
qDebug(scriptengine) << "ScriptObjectV8Proxy::investigate: Object pointer is NULL, " << backtrace;
}
if (!qobject) return;
auto isolate = _engine->getIsolate();
@ -210,7 +222,8 @@ void ScriptObjectV8Proxy::investigate() {
v8::HandleScope handleScope(_engine->getIsolate());
v8::Context::Scope contextScope(_engine->getContext());
auto objectTemplate = _v8ObjectTemplate.Get(_engine->getIsolate());
//auto objectTemplate = _v8ObjectTemplate.Get(_engine->getIsolate());
auto objectTemplate = v8::ObjectTemplate::New(_engine->getIsolate());
objectTemplate->SetInternalFieldCount(3);
objectTemplate->SetHandler(v8::NamedPropertyHandlerConfiguration(v8Get, v8Set));

2
libraries/script-engine/src/v8/ScriptObjectV8Proxy.h
View file

@ -113,7 +113,7 @@ private: // storage
const bool _ownsObject;
QPointer<QObject> _object;
// V8TODO Is this necessary?
v8::UniquePersistent<v8::ObjectTemplate> _v8ObjectTemplate;
// v8::UniquePersistent<v8::ObjectTemplate> _v8ObjectTemplate;
// Handle for its own object
v8::UniquePersistent<v8::Object> _v8Object;
int pointerCorruptionTest = 12345678;

1
libraries/script-engine/src/v8/ScriptValueIteratorV8Wrapper.h
View file

@ -37,6 +37,7 @@ private:
int _length;
int _currentIndex;
ScriptEngineV8 *_engine;
Q_DISABLE_COPY(V8ScriptValueIterator)
};
/// [V8] Implements ScriptValueIterator for V8 and translates calls for V8ScriptValueIterator

37
libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp
View file

@ -237,15 +237,15 @@ ScriptValueIteratorPointer ScriptValueV8Wrapper::newIterator() const {
bool ScriptValueV8Wrapper::hasProperty(const QString& name) const {
auto isolate = _engine->getIsolate();
v8::Locker locker(_engine->getIsolate());
v8::Isolate::Scope isolateScope(_engine->getIsolate());
v8::Locker locker(isolate);
v8::Isolate::Scope isolateScope(isolate);
v8::HandleScope handleScope(isolate);
v8::Context::Scope contextScope(_engine->getContext());
//V8TODO: does function return true on IsObject too?
if (_value.constGet()->IsObject()) {
//V8TODO: what about flags?
v8::Local<v8::Value> resultLocal;
v8::Local<v8::String> key = v8::String::NewFromUtf8(_engine->getIsolate(), name.toStdString().c_str(),v8::NewStringType::kNormal).ToLocalChecked();
v8::Local<v8::String> key = v8::String::NewFromUtf8(isolate, name.toStdString().c_str(),v8::NewStringType::kNormal).ToLocalChecked();
const v8::Local<v8::Object> object = v8::Local<v8::Object>::Cast(_value.constGet());
//V8TODO: Which context?
if (object->Get(_engine->getContext(), key).ToLocal(&resultLocal)) {
@ -265,6 +265,9 @@ ScriptValue ScriptValueV8Wrapper::property(const QString& name, const ScriptValu
v8::Isolate::Scope isolateScope(_engine->getIsolate());
v8::HandleScope handleScope(isolate);
v8::Context::Scope contextScope(_engine->getContext());
if (_value.constGet()->IsNullOrUndefined()) {
return _engine->undefinedValue();
}
if (_value.constGet()->IsObject()) {
//V8TODO: what about flags?
v8::Local<v8::Value> resultLocal;
@ -304,6 +307,10 @@ ScriptValue ScriptValueV8Wrapper::property(quint32 arrayIndex, const ScriptValue
v8::Isolate::Scope isolateScope(isolate);
v8::HandleScope handleScope(isolate);
v8::Context::Scope contextScope(_engine->getContext());
if (_value.constGet()->IsNullOrUndefined()) {
qDebug() << "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();
}
if (_value.constGet()->IsObject()) {
//V8TODO: what about flags?
v8::Local<v8::Value> resultLocal;
@ -315,9 +322,6 @@ ScriptValue ScriptValueV8Wrapper::property(quint32 arrayIndex, const ScriptValue
}
qDebug() << "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();
/*v8::Local<v8::Value> nullValue = v8::Null(_engine->getIsolate());
V8ScriptValue nullScriptValue(_engine->getIsolate(), std::move(nullValue));
return ScriptValue(new ScriptValueV8Wrapper(_engine, nullScriptValue));*/
}
void ScriptValueV8Wrapper::setData(const ScriptValue& value) {
@ -329,6 +333,10 @@ void ScriptValueV8Wrapper::setData(const ScriptValue& value) {
V8ScriptValue unwrapped = fullUnwrap(value);
// V8TODO Check if it uses same isolate. Would pointer check be enough?
// Private properties are an experimental feature for now on V8, so we are using regular value for now
if (_value.constGet()->IsNullOrUndefined()) {
qDebug() << "ScriptValueV8Wrapper::setData() was called on a value that is null or undefined";
return;
}
if (_value.constGet()->IsObject()) {
auto v8Object = v8::Local<v8::Object>::Cast(_value.constGet());
if( !v8Object->Set(_engine->getContext(), v8::String::NewFromUtf8(isolate, "__data").ToLocalChecked(), unwrapped.constGet()).FromMaybe(false)) {
@ -349,6 +357,10 @@ void ScriptValueV8Wrapper::setProperty(const QString& name, const ScriptValue& v
v8::HandleScope handleScope(isolate);
v8::Context::Scope contextScope(_engine->getContext());
V8ScriptValue unwrapped = fullUnwrap(value);
if (_value.constGet()->IsNullOrUndefined()) {
qDebug() << "ScriptValueV8Wrapper::setProperty() was called on a value that is null or undefined";
return;
}
if(_value.constGet()->IsObject()) {
v8::Local<v8::String> key = v8::String::NewFromUtf8(isolate, name.toStdString().c_str(),v8::NewStringType::kNormal).ToLocalChecked();
Q_ASSERT(_value.get()->IsObject());
@ -381,6 +393,10 @@ void ScriptValueV8Wrapper::setProperty(quint32 arrayIndex, const ScriptValue& va
v8::HandleScope handleScope(isolate);
v8::Context::Scope contextScope(_engine->getContext());
V8ScriptValue unwrapped = fullUnwrap(value);
if (_value.constGet()->IsNullOrUndefined()) {
qDebug() << "ScriptValueV8Wrapper::setProperty() was called on a value that is null or undefined";
return;
}
if(_value.constGet()->IsObject()) {
auto object = v8::Local<v8::Object>::Cast(_value.get());
//V8TODO: I don't know which context to use here
@ -404,6 +420,9 @@ void ScriptValueV8Wrapper::setPrototype(const ScriptValue& prototype) {
v8::Context::Scope contextScope(_engine->getContext());
ScriptValueV8Wrapper* unwrappedPrototype = unwrap(prototype);
if (unwrappedPrototype) {
if(unwrappedPrototype->toV8Value().constGet()->IsNullOrUndefined() && _value.constGet()->IsNullOrUndefined()) {
qDebug(scriptengine) << "Failed to assign prototype - one of values is null or undefined";
}
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
@ -526,7 +545,11 @@ QVariant ScriptValueV8Wrapper::toVariant() const {
QObject* ScriptValueV8Wrapper::toQObject() const {
QVariant dest;
if (_engine->castValueToVariant(_value, dest, QMetaType::QObjectStar)) {
return dest.value<QObject*>();
if (dest.canConvert<QObject*>()) {
return dest.value<QObject*>();
} else {
Q_ASSERT(false);
}
} else {
Q_ASSERT(false);
return nullptr;

1
libraries/script-engine/src/v8/ScriptValueV8Wrapper.h
View file

@ -30,7 +30,6 @@ class ScriptValueV8Wrapper final : public ScriptValueProxy {
public: // construction
inline ScriptValueV8Wrapper(ScriptEngineV8* engine, const V8ScriptValue& value) :
_engine(engine), _value(value) {}
// _engine(engine), _value(std::move(value.copy())) {}
inline ScriptValueV8Wrapper(ScriptEngineV8* engine, V8ScriptValue&& value) :
_engine(engine), _value(std::move(value)) {}
static ScriptValueV8Wrapper* unwrap(const ScriptValue& val);

4
libraries/script-engine/src/v8/V8Lambda.h
View file

@ -25,7 +25,7 @@ class Lambda : public QObject {
Q_OBJECT
public:
Lambda(ScriptEngineV8* engine,
std::function<V8ScriptValue(V8ScriptContext* context, ScriptEngineV8* engine)> operation,
std::function<V8ScriptValue(ScriptEngineV8* engine)> operation,
V8ScriptValue data);
~Lambda();
public slots:
@ -34,7 +34,7 @@ public slots:
private:
ScriptEngineV8* _engine;
std::function<V8ScriptValue(V8ScriptContext* context, ScriptEngineV8* engine)> _operation;
std::function<V8ScriptValue(ScriptEngineV8* engine)> _operation;
V8ScriptValue _data;
};

27
libraries/script-engine/src/v8/V8Types.h
View file

@ -31,7 +31,17 @@ public:
v8::Isolate::Scope isolateScope(_engine->getIsolate());
v8::HandleScope handleScope(_engine->getIsolate());
v8::Context::Scope(_engine->getContext());
_value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), std::move(value)));
_value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), value));
};
V8ScriptValueTemplate& operator= (const V8ScriptValueTemplate &source) {
v8::Locker locker(_engine->getIsolate());
v8::Isolate::Scope isolateScope(_engine->getIsolate());
v8::HandleScope handleScope(_engine->getIsolate());
v8::Context::Scope(_engine->getContext());
_engine = source.getEngine();
_value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), source.constGet()));
return *this;
};
V8ScriptValueTemplate(ScriptEngineV8 *engine) : _engine(engine) {
@ -47,7 +57,7 @@ public:
v8::Isolate::Scope isolateScope(_engine->getIsolate());
v8::HandleScope handleScope(_engine->getIsolate());
v8::Context::Scope(_engine->getContext());
_value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), std::move(copied.constGet())));
_value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), copied.constGet()));
}
v8::Local<T> get() {
@ -92,6 +102,7 @@ public:
void reset(v8::Isolate *isolate, v8::Local<T>) {
Q_ASSERT(false);
};
private:
std::shared_ptr<v8::UniquePersistent<T>> _value;
// V8TODO: maybe make it weak
@ -105,19 +116,25 @@ private:
//typedef V8ScriptValueTemplate<v8::Value> V8ScriptValue;
//typedef V8ScriptValueTemplate<v8::Script> V8ScriptProgram;
// V8TODO: Maybe weak?
typedef v8::Persistent<v8::Context> V8ScriptContext;
//typedef v8::Persistent<v8::Context> V8ScriptContext;
class V8ScriptString : public V8ScriptValueTemplate<v8::String> {
public:
V8ScriptString() = delete;
V8ScriptString(ScriptEngineV8 *engine, const v8::Local<v8::String> value) : V8ScriptValueTemplate<v8::String>(engine, value) {};
const QString toQString() const {
Q_ASSERT(constGetIsolate()->IsCurrent());
v8::Locker locker(getEngine()->getIsolate());
v8::Isolate::Scope isolateScope(getEngine()->getIsolate());
v8::HandleScope handleScope(getEngine()->getIsolate());
v8::Context::Scope(getEngine()->getContext());
Q_ASSERT(constGet()->IsString());
return QString(*v8::String::Utf8Value(const_cast<v8::Isolate*>(constGetIsolate()), constGet()));
};
bool operator==(const V8ScriptString& string) const {
Q_ASSERT(constGetIsolate()->IsCurrent());
v8::Locker locker(getEngine()->getIsolate());
v8::Isolate::Scope isolateScope(getEngine()->getIsolate());
v8::HandleScope handleScope(getEngine()->getIsolate());
v8::Context::Scope(getEngine()->getContext());
Q_ASSERT(constGet()->IsString());
return constGet()->StringEquals(string.constGet());
}

4
scripts/system/controllers/controllerModules/inEditMode.js
View file

@ -72,14 +72,17 @@ Script.include("/~/system/libraries/utils.js");
if (controllerData.triggerClicks[this.hand]) {
var hand = this.hand === RIGHT_HAND ? Controller.Standard.RightHand : Controller.Standard.LeftHand;
if (!this.triggerClicked) {
print("inEditMode click");
this.selectedTarget = controllerData.rayPicks[this.hand];
if (!this.selectedTarget.intersects) {
print("inEditMode no intersect");
Messages.sendLocalMessage(this.ENTITY_TOOL_UPDATES_CHANNEL, JSON.stringify({
method: "clearSelection",
hand: hand
}));
} else {
if (this.selectedTarget.type === Picks.INTERSECTED_ENTITY) {
print("inEditMode select entity");
Messages.sendLocalMessage(this.ENTITY_TOOL_UPDATES_CHANNEL, JSON.stringify({
method: "selectEntity",
entityID: this.selectedTarget.objectID,
@ -88,6 +91,7 @@ Script.include("/~/system/libraries/utils.js");
intersection: this.selectedTarget.intersection
}));
} else if (this.selectedTarget.type === Picks.INTERSECTED_OVERLAY) {
print("inEditMode select overlay");
Messages.sendLocalMessage(this.ENTITY_TOOL_UPDATES_CHANNEL, JSON.stringify({
method: "selectOverlay",
overlayID: this.selectedTarget.objectID,