From 6bd8a8d50341846e881bcca5d3cac917b43a184c Mon Sep 17 00:00:00 2001 From: ksuprynowicz Date: Fri, 27 Jan 2023 19:06:19 +0100 Subject: [PATCH] V8 threading safetyu improvements --- .../script-engine/src/v8/ScriptEngineV8.cpp | 4 ++ .../src/v8/ScriptObjectV8Proxy.cpp | 45 ++++++++++++++++--- .../src/v8/ScriptValueV8Wrapper.cpp | 35 ++++++++++----- libraries/script-engine/src/v8/V8Types.h | 13 +++++- scripts/system/create/edit.js | 10 +++-- .../system/create/entityList/entityList.js | 4 +- tests/script-engine/src/tests/005_include.js | 4 ++ .../script-engine/src/tests/005b_included.js | 17 ++++++- 8 files changed, 107 insertions(+), 25 deletions(-) diff --git a/libraries/script-engine/src/v8/ScriptEngineV8.cpp b/libraries/script-engine/src/v8/ScriptEngineV8.cpp index 2921141311..763ab33c57 100644 --- a/libraries/script-engine/src/v8/ScriptEngineV8.cpp +++ b/libraries/script-engine/src/v8/ScriptEngineV8.cpp @@ -1107,6 +1107,10 @@ ScriptContextV8Pointer ScriptEngineV8::pushContext(v8::Local &conte Q_ASSERT(!_contexts.isEmpty()); ScriptContextPointer parent = _contexts.last(); _contexts.append(std::make_shared(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)); + debug_context_id++; return _contexts.last(); } diff --git a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp index 21c218868e..0a17345fad 100644 --- a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp +++ b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp @@ -440,7 +440,10 @@ void ScriptObjectV8Proxy::v8Set(v8::Local name, v8::Local v V8ScriptValue ScriptObjectV8Proxy::property(const V8ScriptValue& object, const V8ScriptString& name, uint id) { + v8::Locker locker(_engine->getIsolate()); + v8::Isolate::Scope isolateScope(_engine->getIsolate()); v8::HandleScope handleScope(_engine->getIsolate()); + v8::Context::Scope contextScope(_engine->getContext()); QObject* qobject = _object; if (!qobject) { _engine->getIsolate()->ThrowError("Referencing deleted native object"); @@ -509,7 +512,10 @@ V8ScriptValue ScriptObjectV8Proxy::property(const V8ScriptValue& object, const V } void ScriptObjectV8Proxy::setProperty(V8ScriptValue& object, const V8ScriptString& name, uint id, const V8ScriptValue& value) { + v8::Locker locker(_engine->getIsolate()); + v8::Isolate::Scope isolateScope(_engine->getIsolate()); v8::HandleScope handleScope(_engine->getIsolate()); + v8::Context::Scope contextScope(_engine->getContext()); if (!(id & PROPERTY_TYPE)) return; QObject* qobject = _object; if (!qobject) { @@ -548,6 +554,10 @@ ScriptVariantV8Proxy::ScriptVariantV8Proxy(ScriptEngineV8* engine, const QVarian } V8ScriptValue ScriptVariantV8Proxy::newVariant(ScriptEngineV8* engine, const QVariant& variant, V8ScriptValue proto) { + v8::Locker locker(engine->getIsolate()); + v8::Isolate::Scope isolateScope(engine->getIsolate()); + v8::HandleScope handleScope(engine->getIsolate()); + v8::Context::Scope contextScope(engine->getContext()); ScriptObjectV8Proxy* protoProxy = ScriptObjectV8Proxy::unwrapProxy(proto); if (!protoProxy) { Q_ASSERT(protoProxy); @@ -566,7 +576,7 @@ V8ScriptValue ScriptVariantV8Proxy::newVariant(ScriptEngineV8* engine, const QVa } ScriptVariantV8Proxy* ScriptVariantV8Proxy::unwrapProxy(const V8ScriptValue& val) { - // V8TODO + // V8TODO V8ScriptValue should have link to engine instead maybe? v8::HandleScope handleScope(const_cast(val.constGetIsolate())); auto v8Value = val.constGet(); if (!v8Value->IsObject()) { @@ -599,6 +609,10 @@ ScriptMethodV8Proxy::~ScriptMethodV8Proxy() { V8ScriptValue ScriptMethodV8Proxy::newMethod(ScriptEngineV8* engine, QObject* object, V8ScriptValue lifetime, const QList& metas, int numMaxParams) { + v8::Locker locker(engine->getIsolate()); + v8::Isolate::Scope isolateScope(engine->getIsolate()); + v8::HandleScope handleScope(engine->getIsolate()); + v8::Context::Scope contextScope(engine->getContext()); auto methodDataTemplate = v8::ObjectTemplate::New(engine->getIsolate()); methodDataTemplate->SetInternalFieldCount(2); auto methodData = methodDataTemplate->NewInstance(engine->getContext()).ToLocalChecked(); @@ -632,7 +646,11 @@ QString ScriptMethodV8Proxy::fullName() const { }*/ void ScriptMethodV8Proxy::callback(const v8::FunctionCallbackInfo& arguments) { + v8::Locker locker(arguments.GetIsolate()); + v8::Isolate::Scope isolateScope(arguments.GetIsolate()); v8::HandleScope handleScope(arguments.GetIsolate()); + Q_ASSERT(!arguments.GetIsolate()->GetCurrentContext().IsEmpty()); + v8::Context::Scope contextScope(arguments.GetIsolate()->GetCurrentContext()); if (!arguments.Data()->IsObject()) { arguments.GetIsolate()->ThrowError("Method value is not an object"); return; @@ -655,14 +673,19 @@ void ScriptMethodV8Proxy::callback(const v8::FunctionCallbackInfo& ar } void ScriptMethodV8Proxy::call(const v8::FunctionCallbackInfo& arguments) { + v8::Locker locker(_engine->getIsolate()); + v8::Isolate::Scope isolateScope(_engine->getIsolate()); + v8::HandleScope handleScope(_engine->getIsolate()); + v8::Context::Scope contextScope(_engine->getContext()); QObject* qobject = _object; v8::Isolate *isolate = arguments.GetIsolate(); + Q_ASSERT(isolate == _engine->getIsolate()); if (!qobject) { isolate->ThrowError("Referencing deleted native object"); return; } - v8::HandleScope handleScope(_engine->getIsolate()); + //v8::HandleScope handleScope(_engine->getIsolate()); int scriptNumArgs = arguments.Length(); int numArgs = std::min(scriptNumArgs, _numMaxParams); @@ -1052,7 +1075,8 @@ int ScriptSignalV8Proxy::qt_metacall(QMetaObject::Call call, int id, void** argu callback->Call(_engine->getContext(), v8This, numArgs, args); if (tryCatch.HasCaught()) { qCDebug(scriptengine) << "Signal proxy " << fullName() << " connection call failed: \"" - << _engine->formatErrorMessageFromTryCatch(tryCatch) << "This: " << conn.thisValue.get()->IsObject(); + << _engine->formatErrorMessageFromTryCatch(tryCatch) + << "\nThis provided: " << conn.thisValue.get()->IsObject(); } } @@ -1065,6 +1089,10 @@ int ScriptSignalV8Proxy::discoverMetaCallIdx() { } ScriptSignalV8Proxy::ConnectionList::iterator ScriptSignalV8Proxy::findConnection(V8ScriptValue thisObject, V8ScriptValue callback) { + v8::Locker locker(_engine->getIsolate()); + v8::Isolate::Scope isolateScope(_engine->getIsolate()); + v8::HandleScope handleScope(_engine->getIsolate()); + v8::Context::Scope contextScope(_engine->getContext()); auto iterOut = resultWithReadLock([&]{ ConnectionList::iterator iter; for (iter = _connections.begin(); iter != _connections.end(); ++iter) { @@ -1083,14 +1111,18 @@ ScriptSignalV8Proxy::ConnectionList::iterator ScriptSignalV8Proxy::findConnectio }*/ void ScriptSignalV8Proxy::connect(ScriptValue arg0, ScriptValue arg1) { - QObject* qobject = _object; v8::Isolate *isolate = _engine->getIsolate(); + v8::Locker locker(isolate); + v8::Isolate::Scope isolateScope(isolate); + v8::HandleScope handleScope(isolate); + v8::Context::Scope contextScope(_engine->getContext()); + QObject* qobject = _object; if (!qobject) { isolate->ThrowError("Referencing deleted native object"); return; } - v8::HandleScope handleScope(isolate); + //v8::HandleScope handleScope(isolate); // untangle the arguments V8ScriptValue callback(isolate, v8::Null(isolate)); @@ -1194,7 +1226,10 @@ void ScriptSignalV8Proxy::disconnect(ScriptValue arg0, ScriptValue arg1) { isolate->ThrowError("Referencing deleted native object"); return; } + v8::Locker locker(isolate); + v8::Isolate::Scope isolateScope(isolate); v8::HandleScope handleScope(isolate); + v8::Context::Scope contextScope(_engine->getContext()); // untangle the arguments V8ScriptValue callback(isolate, v8::Null(isolate)); diff --git a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp index c8122ac26f..7f99f709bb 100644 --- a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp +++ b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp @@ -242,7 +242,8 @@ bool ScriptValueV8Wrapper::hasProperty(const QString& name) const { v8::Local resultLocal; v8::Local key = v8::String::NewFromUtf8(_engine->getIsolate(), name.toStdString().c_str(),v8::NewStringType::kNormal).ToLocalChecked(); const v8::Local object = v8::Local::Cast(_value.constGet()); - if (object->Get(_value.constGetContext(), key).ToLocal(&resultLocal)) { + //V8TODO: Which context? + if (object->Get(_engine->getContext(), key).ToLocal(&resultLocal)) { return true; } else { return false; @@ -264,7 +265,9 @@ ScriptValue ScriptValueV8Wrapper::property(const QString& name, const ScriptValu v8::Local resultLocal; v8::Local key = v8::String::NewFromUtf8(_engine->getIsolate(), name.toStdString().c_str(),v8::NewStringType::kNormal).ToLocalChecked(); const v8::Local object = v8::Local::Cast(_value.constGet()); - if (object->Get(_value.constGetContext(), key).ToLocal(&resultLocal)) { + //V8TODO: Which context? + if (object->Get(_engine->getContext(), key).ToLocal(&resultLocal)) { + //if (object->Get(_value.constGetContext(), key).ToLocal(&resultLocal)) { V8ScriptValue result(_engine->getIsolate(), resultLocal); return ScriptValue(new ScriptValueV8Wrapper(_engine, std::move(result))); } else { @@ -374,7 +377,9 @@ void ScriptValueV8Wrapper::setProperty(quint32 arrayIndex, const ScriptValue& va V8ScriptValue unwrapped = fullUnwrap(value); if(_value.constGet()->IsObject()) { auto object = v8::Local::Cast(_value.get()); - v8::Maybe retVal(object->Set(_value.getContext(), arrayIndex, unwrapped.constGet())); + //V8TODO: I don't know which context to use here + v8::Maybe retVal(object->Set(_engine->getContext(), arrayIndex, unwrapped.constGet())); + //v8::Maybe retVal(object->Set(_value.getContext(), arrayIndex, unwrapped.constGet())); if (retVal.IsJust() ? !retVal.FromJust() : true){ qDebug(scriptengine) << "Failed to set property"; } @@ -395,7 +400,9 @@ void ScriptValueV8Wrapper::setPrototype(const ScriptValue& prototype) { if (unwrappedPrototype) { if(unwrappedPrototype->toV8Value().constGet()->IsObject() && _value.constGet()->IsObject()) { auto object = v8::Local::Cast(_value.get()); - v8::Maybe retVal = object->SetPrototype(_value.getContext(), unwrappedPrototype->toV8Value().constGet()); + //V8TODO: I don't know which context to use here + v8::Maybe retVal = object->SetPrototype(_engine->getContext(), unwrappedPrototype->toV8Value().constGet()); + //v8::Maybe retVal = object->SetPrototype(_value.getContext(), unwrappedPrototype->toV8Value().constGet()); if (retVal.IsJust() ? !retVal.FromJust() : true){ qDebug(scriptengine) << "Failed to assign prototype"; } @@ -431,7 +438,7 @@ qint32 ScriptValueV8Wrapper::toInt32() const { v8::HandleScope handleScope(isolate); v8::Context::Scope contextScope(_engine->getContext()); v8::Local integer; - if (!_value.constGet()->ToInteger(_value.constGetContext()).ToLocal(&integer)) { + if (!_value.constGet()->ToInteger(_engine->getContext()).ToLocal(&integer)) { Q_ASSERT(false); } return static_cast((integer)->Value()); @@ -444,7 +451,7 @@ double ScriptValueV8Wrapper::toInteger() const { v8::HandleScope handleScope(isolate); v8::Context::Scope contextScope(_engine->getContext()); v8::Local integer; - if (!_value.constGet()->ToInteger(_value.constGetContext()).ToLocal(&integer)) { + if (!_value.constGet()->ToInteger(_engine->getContext()).ToLocal(&integer)) { Q_ASSERT(false); } return (integer)->Value(); @@ -457,7 +464,7 @@ double ScriptValueV8Wrapper::toNumber() const { v8::HandleScope handleScope(isolate); v8::Context::Scope contextScope(_engine->getContext()); v8::Local number; - if (!_value.constGet()->ToNumber(_value.constGetContext()).ToLocal(&number)) { + if (!_value.constGet()->ToNumber(_engine->getContext()).ToLocal(&number)) { Q_ASSERT(false); } return number->Value(); @@ -481,7 +488,7 @@ quint16 ScriptValueV8Wrapper::toUInt16() const { v8::HandleScope handleScope(isolate); v8::Context::Scope contextScope(_engine->getContext()); v8::Local integer; - if (!_value.constGet()->ToUint32(_value.constGetContext()).ToLocal(&integer)) { + if (!_value.constGet()->ToUint32(_engine->getContext()).ToLocal(&integer)) { Q_ASSERT(false); } return static_cast(integer->Value()); @@ -494,7 +501,7 @@ quint32 ScriptValueV8Wrapper::toUInt32() const { v8::HandleScope handleScope(isolate); v8::Context::Scope contextScope(_engine->getContext()); v8::Local integer; - if (!_value.constGet()->ToUint32(_value.constGetContext()).ToLocal(&integer)) { + if (!_value.constGet()->ToUint32(_engine->getContext()).ToLocal(&integer)) { Q_ASSERT(false); } return integer->Value(); @@ -532,11 +539,17 @@ bool ScriptValueV8Wrapper::equals(const ScriptValue& other) const { if (!unwrappedOther) { return false; }else{ - if (_value.constGet()->Equals(unwrappedOther->toV8Value().constGetContext(), unwrappedOther->toV8Value().constGet()).IsNothing()) { + // V8TODO: which context needs to be used here? + if (_value.constGet()->Equals(_engine->getContext(), unwrappedOther->toV8Value().constGet()).IsNothing()) { + return false; + } else { + return _value.constGet()->Equals(_engine->getContext(), unwrappedOther->toV8Value().constGet()).FromJust(); + } + /*if (_value.constGet()->Equals(unwrappedOther->toV8Value().constGetContext(), unwrappedOther->toV8Value().constGet()).IsNothing()) { return false; } else { return _value.constGet()->Equals(unwrappedOther->toV8Value().constGetContext(), unwrappedOther->toV8Value().constGet()).FromJust(); - } + }*/ } } diff --git a/libraries/script-engine/src/v8/V8Types.h b/libraries/script-engine/src/v8/V8Types.h index eb18f8fedb..daa161c414 100644 --- a/libraries/script-engine/src/v8/V8Types.h +++ b/libraries/script-engine/src/v8/V8Types.h @@ -32,6 +32,11 @@ public: _context.Reset(isolate, isolate->GetCurrentContext()); _value.reset(new v8::UniquePersistent(_isolate, std::move(value))); }; + + /*V8ScriptValueTemplate(const V8ScriptValueTemplate &copied) { + ; + }*/ + v8::Local get() { v8::EscapableHandleScope handleScope(_isolate); return handleScope.Escape(_value.get()->Get(_isolate)); @@ -47,14 +52,18 @@ public: const v8::Local constGetContext() const { v8::EscapableHandleScope handleScope(_isolate); - return handleScope.Escape(_context.Get(_isolate)); + Q_ASSERT(!_isolate->GetCurrentContext().IsEmpty()); + return handleScope.Escape(_isolate->GetCurrentContext()); + //return handleScope.Escape(_context.Get(_isolate)); }; const v8::Isolate* constGetIsolate() const { return _isolate;}; v8::Isolate* getIsolate() { return _isolate;}; //v8::Persistent>& getContext() { return _context;}; v8::Local getContext() { v8::EscapableHandleScope handleScope(_isolate); - return handleScope.Escape(_context.Get(_isolate)); + Q_ASSERT(!_isolate->GetCurrentContext().IsEmpty()); + return handleScope.Escape(_isolate->GetCurrentContext()); + //return handleScope.Escape(_context.Get(_isolate)); }; void reset(v8::Isolate *isolate, v8::Local) {}; private: diff --git a/scripts/system/create/edit.js b/scripts/system/create/edit.js index eed471dc57..ad40505855 100644 --- a/scripts/system/create/edit.js +++ b/scripts/system/create/edit.js @@ -11,15 +11,14 @@ // Distributed under the Apache License, Version 2.0. // See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html // +"use strict"; /* global Script, SelectionDisplay, LightOverlayManager, CameraManager, Grid, GridTool, EditTools, EditVoxels, EntityListTool, Vec3, SelectionManager, Overlays, OverlayWebWindow, UserActivityLogger, Settings, Entities, Tablet, Toolbars, Messages, Menu, Camera, progressDialog, tooltip, MyAvatar, Quat, Controller, Clipboard, HMD, UndoStack, OverlaySystemWindow, keyUpEventFromUIWindow:true */ -(function() { // BEGIN LOCAL_SCOPE - -"use strict"; +//(function() { // BEGIN LOCAL_SCOPE var EDIT_TOGGLE_BUTTON = "com.highfidelity.interface.system.editButton"; @@ -3181,6 +3180,7 @@ function zoneSortOrder(a, b) { return 0; } +//print("getParentState added"); function getParentState(id) { var state = "NONE"; var properties = Entities.getEntityProperties(id, ["parentID"]); @@ -3199,6 +3199,8 @@ function getParentState(id) { return state; } +//print("Global object after getParentState" + JSON.stringify(globalThis)); + function getDomainOnlyChildrenIDs(id) { var allChildren = Entities.getChildrenIDs(id); var realChildren = []; @@ -3264,4 +3266,4 @@ function rotateAsNextClickedSurface() { } } -}()); // END LOCAL_SCOPE +//}()); // END LOCAL_SCOPE diff --git a/scripts/system/create/entityList/entityList.js b/scripts/system/create/entityList/entityList.js index 179239c916..47a04ad0bd 100644 --- a/scripts/system/create/entityList/entityList.js +++ b/scripts/system/create/entityList/entityList.js @@ -217,7 +217,7 @@ var EntityListTool = function(shouldUseEditTabletApp) { } else if (properties.type === "Image") { url = properties.imageURL; } - + //print("Global object before getParentState call: " + JSON.stringify(globalThis)); var parentStatus = getParentState(ids[i]); var parentState = ""; if (parentStatus === "PARENT") { @@ -297,7 +297,7 @@ var EntityListTool = function(shouldUseEditTabletApp) { } var onWebEventReceived = function(data) { - print("entityList.js onWebEventReceived: " + data); + //print("entityList.js onWebEventReceived: " + data); try { data = JSON.parse(data); } catch(e) { diff --git a/tests/script-engine/src/tests/005_include.js b/tests/script-engine/src/tests/005_include.js index 680492ebf8..eb392b0067 100644 --- a/tests/script-engine/src/tests/005_include.js +++ b/tests/script-engine/src/tests/005_include.js @@ -2,6 +2,10 @@ Script.include("./005b_included.js"); +//var requireTest = Script.require("http://oaktown.pl/scripts/004b_require_module.js"); + +var testObject = new TestObject(); + function functionInMainFile () { print("In main file"); } diff --git a/tests/script-engine/src/tests/005b_included.js b/tests/script-engine/src/tests/005b_included.js index ef2173bff3..a15b05697d 100644 --- a/tests/script-engine/src/tests/005b_included.js +++ b/tests/script-engine/src/tests/005b_included.js @@ -5,4 +5,19 @@ function functionInInclude() { functionInMainFile(); } -functionInMainFile(); \ No newline at end of file +var TestObject = function(shouldUseEditTabletApp) { + var requireTest = Script.require("http://oaktown.pl/scripts/004b_require_module.js"); + + var onEvent = function(data) { + that.thatFunction(); + } + var that = {}; + that.thatFunction = function() { + functionInMainFile(); + } + Script.update.connect(onEvent); +} + +functionInMainFile(); + +//Script.setInterval(functionInInclude, 1000);