diff --git a/test-app/runtime/CMakeLists.txt b/test-app/runtime/CMakeLists.txt index a092d3058..594afc2da 100644 --- a/test-app/runtime/CMakeLists.txt +++ b/test-app/runtime/CMakeLists.txt @@ -176,6 +176,7 @@ add_library( src/main/cpp/File.cpp src/main/cpp/Interop.cpp src/main/cpp/IsolateDisposer.cpp + src/main/cpp/IsolateTracked.cpp src/main/cpp/JEnv.cpp src/main/cpp/DesugaredInterfaceCompanionClassNameResolver.cpp src/main/cpp/JType.cpp @@ -203,6 +204,7 @@ add_library( src/main/cpp/Profiler.cpp src/main/cpp/ReadWriteLock.cpp src/main/cpp/Runtime.cpp + src/main/cpp/RuntimeState.cpp src/main/cpp/SimpleAllocator.cpp src/main/cpp/SimpleProfiler.cpp src/main/cpp/StructuredClone.cpp diff --git a/test-app/runtime/src/main/cpp/ArgConverter.cpp b/test-app/runtime/src/main/cpp/ArgConverter.cpp index 18cc680b1..6ebcb4a64 100644 --- a/test-app/runtime/src/main/cpp/ArgConverter.cpp +++ b/test-app/runtime/src/main/cpp/ArgConverter.cpp @@ -7,6 +7,7 @@ #include "Runtime.h" #include "V8GlobalHelpers.h" #include "NativeScriptAssert.h" +#include "RuntimeState.h" #include using namespace v8; @@ -196,15 +197,11 @@ int64_t ArgConverter::ConvertToJavaLong(Isolate* isolate, const Local& va } ArgConverter::TypeLongOperationsCache* ArgConverter::GetTypeLongCache(v8::Isolate* isolate) { - TypeLongOperationsCache* cache; - auto itFound = s_type_long_operations_cache.find(isolate); - if (itFound == s_type_long_operations_cache.end()) { - cache = new TypeLongOperationsCache; - s_type_long_operations_cache.emplace(isolate, cache); - } else { - cache = itFound->second; + // Per runtime, so there is no shared table to race on; see RuntimeState.h. + auto* cache = RuntimeState::For(isolate); + if (cache == nullptr) { + throw NativeScriptException("Long conversion cache requested after the runtime was torn down"); } - return cache; } @@ -222,12 +219,3 @@ u16string ArgConverter::ConvertToUtf16String(const v8::Local& s) { -void ArgConverter::onDisposeIsolate(Isolate* isolate) { - auto itFound = s_type_long_operations_cache.find(isolate); - if (itFound != s_type_long_operations_cache.end()) { - delete itFound->second; - s_type_long_operations_cache.erase(itFound); - } -} - -robin_hood::unordered_map ArgConverter::s_type_long_operations_cache; \ No newline at end of file diff --git a/test-app/runtime/src/main/cpp/ArgConverter.h b/test-app/runtime/src/main/cpp/ArgConverter.h index a9878e211..263a6ba3d 100644 --- a/test-app/runtime/src/main/cpp/ArgConverter.h +++ b/test-app/runtime/src/main/cpp/ArgConverter.h @@ -115,20 +115,27 @@ class ArgConverter { return v8::String::NewFromTwoByte(isolate, ((const uint16_t*) utf16string.data())).ToLocalChecked(); } - static void onDisposeIsolate(v8::Isolate* isolate); + /* + * Per-runtime state (RuntimeState owns one of these per runtime, so it + * has to be constructible from outside ArgConverter). Destroyed with + * the runtime, while its isolate is still alive. + */ + struct TypeLongOperationsCache { + v8::Persistent* LongNumberCtorFunc = nullptr; + + v8::Persistent* NanNumberObject = nullptr; + + ~TypeLongOperationsCache() { + delete LongNumberCtorFunc; + delete NanNumberObject; + } + }; private: // TODO: plamen5kov: rewrite logic for java long number operations in javascript (java long -> javascript number operations check) static const long long JS_LONG_LIMIT = ((long long) 1) << 53; - struct TypeLongOperationsCache { - v8::Persistent* LongNumberCtorFunc; - - v8::Persistent* NanNumberObject; - }; - // - static TypeLongOperationsCache* GetTypeLongCache(v8::Isolate* isolate); inline static jstring ObjectToString(jobject object) { @@ -146,11 +153,6 @@ class ArgConverter { static void NativeScriptLongToStringFunctionCallback(const v8::FunctionCallbackInfo& args); - /* - * "s_type_long_operations_cache" used to keep function - * dealing with operations concerning java long -> javascript number. - */ - static robin_hood::unordered_map s_type_long_operations_cache; }; } diff --git a/test-app/runtime/src/main/cpp/ErrorEvents.cpp b/test-app/runtime/src/main/cpp/ErrorEvents.cpp index b41ff1486..895a5ab83 100644 --- a/test-app/runtime/src/main/cpp/ErrorEvents.cpp +++ b/test-app/runtime/src/main/cpp/ErrorEvents.cpp @@ -10,15 +10,6 @@ using namespace std; using namespace tns; using namespace v8; -/* - * Non-throwing runtime lookup, safe from V8 callbacks that may fire while a - * runtime is being torn down (Runtime::GetRuntime throws in that window). - */ -static Runtime* GetRuntimeOrNull(Isolate* isolate) { - return static_cast( - isolate->GetData((uint32_t) Runtime::IsolateData::RUNTIME)); -} - /* * Native function handed to internal/error-events.js as `nativeReportFatal(error, * stackString)`. It runs the terminal tail (shim + log) WITHOUT re-dispatching @@ -38,7 +29,7 @@ static void NativeReportFatalCallback(const FunctionCallbackInfo& info) { void ErrorEvents::Init(Local context) { auto isolate = v8::Isolate::GetCurrent(); - auto runtime = GetRuntimeOrNull(isolate); + auto runtime = Runtime::TryGetRuntime(isolate); if (runtime == nullptr) { throw NativeScriptException("ErrorEvents::Init: no runtime for isolate"); } @@ -81,7 +72,7 @@ void ErrorEvents::Init(Local context) { bool ErrorEvents::DispatchError(Isolate* isolate, Local error, const string& messageString, const string& stack) { - auto runtime = GetRuntimeOrNull(isolate); + auto runtime = Runtime::TryGetRuntime(isolate); if (runtime == nullptr || runtime->DispatchErrorEventFunc().IsEmpty()) { return false; } @@ -104,7 +95,7 @@ bool ErrorEvents::DispatchError(Isolate* isolate, Local error, bool ErrorEvents::DispatchUnhandledRejection(Isolate* isolate, Local promise, Local reason) { - auto runtime = GetRuntimeOrNull(isolate); + auto runtime = Runtime::TryGetRuntime(isolate); if (runtime == nullptr || runtime->DispatchUnhandledRejectionFunc().IsEmpty()) { return false; } @@ -126,7 +117,7 @@ bool ErrorEvents::DispatchNativeUncaughtError(Isolate* isolate, Local error, const string& messageString, const string& stack) { - auto runtime = GetRuntimeOrNull(isolate); + auto runtime = Runtime::TryGetRuntime(isolate); if (runtime == nullptr || runtime->DispatchNativeUncaughtErrorFunc().IsEmpty()) { return false; } @@ -149,7 +140,7 @@ bool ErrorEvents::DispatchNativeUncaughtError(Isolate* isolate, void ErrorEvents::DispatchRejectionHandled(Isolate* isolate, Local promise, Local reason) { - auto runtime = GetRuntimeOrNull(isolate); + auto runtime = Runtime::TryGetRuntime(isolate); if (runtime == nullptr || runtime->DispatchRejectionHandledFunc().IsEmpty()) { return; } diff --git a/test-app/runtime/src/main/cpp/Events.cpp b/test-app/runtime/src/main/cpp/Events.cpp index 7aaeffecd..f1f2fe60e 100644 --- a/test-app/runtime/src/main/cpp/Events.cpp +++ b/test-app/runtime/src/main/cpp/Events.cpp @@ -10,8 +10,7 @@ using namespace v8; void Events::Init(Local context) { auto isolate = v8::Isolate::GetCurrent(); - auto runtime = static_cast( - isolate->GetData((uint32_t) Runtime::IsolateData::RUNTIME)); + auto runtime = Runtime::TryGetRuntime(isolate); if (runtime == nullptr) { throw NativeScriptException("Events::Init: no runtime for isolate"); } diff --git a/test-app/runtime/src/main/cpp/FrameCallbacks.cpp b/test-app/runtime/src/main/cpp/FrameCallbacks.cpp index 661edbe6b..ff96c3ab7 100644 --- a/test-app/runtime/src/main/cpp/FrameCallbacks.cpp +++ b/test-app/runtime/src/main/cpp/FrameCallbacks.cpp @@ -258,8 +258,7 @@ void Dispatch(EntryId id, int64_t frameTimeNanos) { } Isolate* isolate = entry->isolate_; - Runtime* runtime = static_cast( - isolate->GetData((uint32_t) Runtime::IsolateData::RUNTIME)); + Runtime* runtime = Runtime::TryGetRuntime(isolate); if (runtime == nullptr) { return; } diff --git a/test-app/runtime/src/main/cpp/IsolateDisposer.cpp b/test-app/runtime/src/main/cpp/IsolateDisposer.cpp index fede75cad..954911ce7 100644 --- a/test-app/runtime/src/main/cpp/IsolateDisposer.cpp +++ b/test-app/runtime/src/main/cpp/IsolateDisposer.cpp @@ -15,10 +15,6 @@ namespace tns { void disposeIsolate(v8::Isolate *isolate) { - tns::ArgConverter::onDisposeIsolate(isolate); - tns::MetadataNode::onDisposeIsolate(isolate); - tns::Console::onDisposeIsolate(isolate); - tns::JSONObjectHelper::onDisposeIsolate(isolate); tns::NsBuiltinModules::onDisposeIsolate(isolate); tns::BuiltinLoader::onDisposeIsolate(isolate); // clear all isolate bound objects diff --git a/test-app/runtime/src/main/cpp/IsolateTracked.cpp b/test-app/runtime/src/main/cpp/IsolateTracked.cpp new file mode 100644 index 000000000..54f935223 --- /dev/null +++ b/test-app/runtime/src/main/cpp/IsolateTracked.cpp @@ -0,0 +1,56 @@ +#include "IsolateTracked.h" + +#include "RuntimeState.h" +#include "robin_hood.h" + +namespace tns { + +namespace { +// The live instances of one runtime; see RuntimeState.h. Touched only on that +// runtime's own thread -- both the GC finalizer and the teardown sweep run +// there -- so it needs no synchronization. +struct TrackedInstances { + robin_hood::unordered_set live; +}; +} // namespace + +void IsolateTracked::BindFinalizer(v8::Isolate* isolate, + const v8::Local& object) { + v8::HandleScope scopedHandle(isolate); + weakHandle_.Reset(isolate, object); + weakHandle_.SetWeak(this, Finalizer, v8::WeakCallbackType::kParameter); + + auto* tracked = RuntimeState::For(isolate); + if (tracked != nullptr) { + tracked->live.insert(this); + } +} + +void IsolateTracked::Finalizer(const v8::WeakCallbackInfo& data) { + IsolateTracked* self = data.GetParameter(); + + auto* tracked = RuntimeState::For(data.GetIsolate()); + if (tracked != nullptr) { + tracked->live.erase(self); + } + + delete self; +} + +void IsolateTracked::SweepAll(v8::Isolate* isolate) { + auto* tracked = RuntimeState::For(isolate); + if (tracked == nullptr) { + return; + } + + // Detached first: a destructor that somehow reached back into the registry + // must not mutate the set being walked. + auto survivors = std::move(tracked->live); + tracked->live.clear(); + + for (IsolateTracked* instance : survivors) { + delete instance; + } +} + +} // namespace tns diff --git a/test-app/runtime/src/main/cpp/IsolateTracked.h b/test-app/runtime/src/main/cpp/IsolateTracked.h new file mode 100644 index 000000000..fda6cccb8 --- /dev/null +++ b/test-app/runtime/src/main/cpp/IsolateTracked.h @@ -0,0 +1,44 @@ +#ifndef TEST_APP_ISOLATETRACKED_H +#define TEST_APP_ISOLATETRACKED_H + +#include "v8.h" + +namespace tns { + +/* + * Base for self-owned native objects whose lifetime is bound to a single JS + * object through a weak handle (URL, URLSearchParams, URLPattern). + * + * An instance dies in exactly two places: the GC finalizer, or SweepAll at + * runtime teardown. The sweep is the point of this class -- V8 does not run + * weak callbacks when an isolate is disposed, so every instance still alive + * when a runtime goes away (a worker shutting down, say) would otherwise leak + * its native state, and URLPattern would leak its compiled v8::Global handles + * with it. + * + * Never delete a bound instance directly: both deletion paths own the registry + * bookkeeping. Subclasses only need a destructor that releases their own + * state; it runs while the isolate is still alive, so it may reset v8::Global + * handles, but it must not create new ones. + */ +class IsolateTracked { + public: + virtual ~IsolateTracked() = default; + + void BindFinalizer(v8::Isolate* isolate, const v8::Local& object); + + /* + * Deletes every instance the GC never got to. Runs on the runtime's own + * thread while the isolate is alive. + */ + static void SweepAll(v8::Isolate* isolate); + + private: + static void Finalizer(const v8::WeakCallbackInfo& data); + + v8::Global weakHandle_; +}; + +} // namespace tns + +#endif // TEST_APP_ISOLATETRACKED_H diff --git a/test-app/runtime/src/main/cpp/JEnv.cpp b/test-app/runtime/src/main/cpp/JEnv.cpp index dda0241b4..a172f3dde 100644 --- a/test-app/runtime/src/main/cpp/JEnv.cpp +++ b/test-app/runtime/src/main/cpp/JEnv.cpp @@ -1,10 +1,22 @@ #include "JEnv.h" + +#include #include #include "Util.h" #include "NativeScriptException.h" #include "DesugaredInterfaceCompanionClassNameResolver.h" using namespace tns; + +/* + * The class caches are process-wide and string-keyed, holding JNI global refs, + * which is correct to share -- but every runtime resolves classes from its own + * thread and workers run concurrently. Shared for lookups, exclusive only to + * publish. NewGlobalRef/DeleteLocalRef stay outside the lock; if two threads + * resolve the same class, the first published wins and the loser releases its + * ref rather than leaking it. + */ +static std::shared_mutex classCacheMutex; using namespace std; JEnv::JEnv() @@ -758,6 +770,8 @@ jclass JEnv::FindClass(const string &className) { jclass JEnv::CheckForClassInCache(const string &className) { jclass global_class = nullptr; + + std::shared_lock lock(classCacheMutex); auto itFound = s_classCache.find(className); if (itFound != s_classCache.end()) { @@ -769,14 +783,30 @@ jclass JEnv::CheckForClassInCache(const string &className) { jclass JEnv::InsertClassIntoCache(const string &className, jclass &tmp) { auto global_class = reinterpret_cast(m_env->NewGlobalRef(tmp)); - s_classCache.emplace(className, global_class); m_env->DeleteLocalRef(tmp); + jclass published = nullptr; + { + std::unique_lock lock(classCacheMutex); + auto result = s_classCache.emplace(className, global_class); + if (!result.second) { + published = result.first->second; + } + } + + if (published != nullptr) { + // Another thread resolved the same class first; keep its ref. + m_env->DeleteGlobalRef(global_class); + return published; + } + return global_class; } jthrowable JEnv::CheckForClassMissingCache(const string &className) { jthrowable throwable = nullptr; + + std::shared_lock lock(classCacheMutex); auto itFound = s_missingClasses.find(className); if (itFound != s_missingClasses.end()) { @@ -788,9 +818,22 @@ jthrowable JEnv::CheckForClassMissingCache(const string &className) { jthrowable JEnv::InsertClassIntoMissingCache(const string &className,const jthrowable &tmp) { auto throwable = reinterpret_cast(m_env->NewGlobalRef(tmp)); - s_missingClasses.emplace(className, throwable); m_env->DeleteLocalRef(tmp); + jthrowable published = nullptr; + { + std::unique_lock lock(classCacheMutex); + auto result = s_missingClasses.emplace(className, throwable); + if (!result.second) { + published = result.first->second; + } + } + + if (published != nullptr) { + m_env->DeleteGlobalRef(throwable); + return published; + } + return throwable; } diff --git a/test-app/runtime/src/main/cpp/JSONObjectHelper.cpp b/test-app/runtime/src/main/cpp/JSONObjectHelper.cpp index 88cb8f9f3..b509add49 100644 --- a/test-app/runtime/src/main/cpp/JSONObjectHelper.cpp +++ b/test-app/runtime/src/main/cpp/JSONObjectHelper.cpp @@ -2,6 +2,7 @@ #include "JSONObjectHelper.h" #include "ArgConverter.h" #include "BuiltinLoader.h" +#include "RuntimeState.h" #include "robin_hood.h" #include #include @@ -9,7 +10,16 @@ using namespace v8; using namespace tns; -static robin_hood::unordered_map*> isolateToSerializeFunc; +namespace { +// The compiled JS->org.json serializer, per runtime; see RuntimeState.h. +struct SerializeFuncState { + Persistent* func = nullptr; + + ~SerializeFuncState() { + delete func; + } +}; +} // namespace void JSONObjectHelper::RegisterFromFunction(Isolate *isolate, Local& jsonObject) { if (!jsonObject->IsFunction()) { @@ -84,9 +94,12 @@ void JSONObjectHelper::ConvertCallbackStatic(const FunctionCallbackInfo& Persistent* JSONObjectHelper::GetSerializeFunc(Local context) { Isolate* isolate = v8::Isolate::GetCurrent(); - auto it = isolateToSerializeFunc.find(isolate); - if (it != isolateToSerializeFunc.end()) { - return it->second; + auto* state = RuntimeState::For(isolate); + if (state == nullptr) { + return nullptr; + } + if (state->func != nullptr) { + return state->func; } Local result; @@ -95,16 +108,8 @@ Persistent* JSONObjectHelper::GetSerializeFunc(Local context) return nullptr; } - auto* serializeFunc = new Persistent(isolate, result.As()); - isolateToSerializeFunc.emplace(isolate, serializeFunc); + state->func = new Persistent(isolate, result.As()); - return serializeFunc; + return state->func; } -void JSONObjectHelper::onDisposeIsolate(Isolate* isolate) { - auto it = isolateToSerializeFunc.find(isolate); - if (it != isolateToSerializeFunc.end()) { - delete it->second; - isolateToSerializeFunc.erase(it); - } -} diff --git a/test-app/runtime/src/main/cpp/JSONObjectHelper.h b/test-app/runtime/src/main/cpp/JSONObjectHelper.h index 8aeb4d02f..b0479a652 100644 --- a/test-app/runtime/src/main/cpp/JSONObjectHelper.h +++ b/test-app/runtime/src/main/cpp/JSONObjectHelper.h @@ -8,7 +8,6 @@ namespace tns { class JSONObjectHelper { public: static void RegisterFromFunction(v8::Isolate *isolate, v8::Local& jsonObject); - static void onDisposeIsolate(v8::Isolate* isolate); private: static v8::Persistent* GetSerializeFunc(v8::Local context); static void ConvertCallbackStatic(const v8::FunctionCallbackInfo& info); diff --git a/test-app/runtime/src/main/cpp/MetadataNode.cpp b/test-app/runtime/src/main/cpp/MetadataNode.cpp index 449fc7722..4188ef618 100644 --- a/test-app/runtime/src/main/cpp/MetadataNode.cpp +++ b/test-app/runtime/src/main/cpp/MetadataNode.cpp @@ -1,4 +1,5 @@ #include "MetadataNode.h" +#include "RuntimeState.h" #include "NativeScriptAssert.h" #include "Constants.h" #include "Util.h" @@ -82,9 +83,9 @@ bool MetadataNode::TryGetPackageName(Isolate* isolate, const Local& valu } Local MetadataNode::GetOrCreateArrayObjectTemplate(Isolate* isolate) { - auto it = s_arrayObjectTemplates.find(isolate); - if (it != s_arrayObjectTemplates.end()) { - return it->second->Get(isolate); + auto cache = GetMetadataNodeCache(isolate); + if (cache->ArrayObjectTemplate != nullptr) { + return cache->ArrayObjectTemplate->Get(isolate); } auto arrayObjectTemplate = ObjectTemplate::New(isolate); @@ -92,7 +93,7 @@ Local MetadataNode::GetOrCreateArrayObjectTemplate(Isolate* isol arrayObjectTemplate->SetHandler(IndexedPropertyHandlerConfiguration( ArrayIndexedPropertyGetterCallback, ArrayIndexedPropertySetterCallback)); - s_arrayObjectTemplates.emplace(std::make_pair(isolate, new Persistent(isolate, arrayObjectTemplate))); + cache->ArrayObjectTemplate = new Persistent(isolate, arrayObjectTemplate); return arrayObjectTemplate; } @@ -721,7 +722,7 @@ vector MetadataNode::SetInstanceMethodsFromS callbackData->parent = *itFound; } - if (s_profilerEnabled) { + if (s_profilerEnabled.load(std::memory_order_relaxed)) { Local funcData = External::New(isolate, callbackData, v8::kExternalPointerTypeTagDefault); Local funcTemplate = FunctionTemplate::New(isolate, MethodCallback, funcData); auto func = funcTemplate->GetFunction(context).ToLocalChecked(); @@ -965,8 +966,9 @@ void MetadataNode::InnerTypeAccessorGetterCallback(v8::Local property, MetadataTreeNode* curChild = static_cast( v8::External::Cast(*info.Data())->Value(v8::kExternalPointerTypeTagDefault)); auto childNode = GetOrCreateInternal(curChild); - auto itFound = childNode->m_poCtorCachePerIsolate.find(isolate); - if (itFound != childNode->m_poCtorCachePerIsolate.end()) { + auto innerCache = GetMetadataNodeCache(isolate); + auto itFound = innerCache->CtorFunctions.find(childNode); + if (itFound != innerCache->CtorFunctions.end()) { info.GetReturnValue().Set(itFound->second->Get(isolate)); return; } @@ -1086,7 +1088,7 @@ Local MetadataNode::GetConstructorFunctionTemplate(Isolate* is node->SetStaticMembers(isolate, wrappedCtorFunc, treeNode, curPtr); // insert isolate-specific persistent function handle - node->m_poCtorCachePerIsolate.insert({isolate, new Persistent(isolate, wrappedCtorFunc)}); + cache->CtorFunctions.emplace(node, new Persistent(isolate, wrappedCtorFunc)); if (!baseCtorFunc.IsEmpty()) { auto currentContext = isolate->GetCurrentContext(); wrappedCtorFunc->SetPrototype(currentContext, baseCtorFunc); @@ -1116,8 +1118,9 @@ Local MetadataNode::GetConstructorFunction(Isolate* isolate) { } Persistent* MetadataNode::GetPersistentConstructorFunction(Isolate* isolate) { - auto itFound = m_poCtorCachePerIsolate.find(isolate); - if (itFound != m_poCtorCachePerIsolate.end()) { + auto cache = GetMetadataNodeCache(isolate); + auto itFound = cache->CtorFunctions.find(this); + if (itFound != cache->CtorFunctions.end()) { auto& constrFunction = itFound->second; return constrFunction; @@ -2063,19 +2066,17 @@ void MetadataNode::CreateTopLevelNamespaces(Isolate* isolate, const Localsecond; + // Per runtime; see RuntimeState.h. Null only once the runtime has begun + // tearing down, which no caller here can legitimately reach. + auto* cache = RuntimeState::For(isolate); + if (cache == nullptr) { + throw NativeScriptException("Metadata cache requested after the runtime was torn down"); } return cache; } void MetadataNode::EnableProfiler(bool enableProfiler) { - s_profilerEnabled = enableProfiler; + s_profilerEnabled.store(enableProfiler, std::memory_order_relaxed); } bool MetadataNode::IsJavascriptKeyword(const std::string &word) { @@ -2095,7 +2096,7 @@ bool MetadataNode::IsJavascriptKeyword(const std::string &word) { } Local MetadataNode::Wrap(Isolate* isolate, const Local& function, const string& name, const string& origin, bool isCtorFunc) { - if (!s_profilerEnabled || name == "") { + if (!s_profilerEnabled.load(std::memory_order_relaxed) || name == "") { return function; } @@ -2305,32 +2306,6 @@ std::string MetadataNode::GetJniClassName(MetadataEntry& entry) { return fullClassName; } -void MetadataNode::onDisposeIsolate(Isolate* isolate) { - { - auto it = s_metadata_node_cache.find(isolate); - if (it != s_metadata_node_cache.end()) { - delete it->second; - s_metadata_node_cache.erase(it); - } - } - { - auto it = s_arrayObjectTemplates.find(isolate); - if (it != s_arrayObjectTemplates.end()) { - delete it->second; - s_arrayObjectTemplates.erase(it); - } - } - { - for (auto it = s_treeNode2NodeCache.begin(); it != s_treeNode2NodeCache.end(); it++) { - auto it2 = it->second->m_poCtorCachePerIsolate.find(isolate); - if(it2 != it->second->m_poCtorCachePerIsolate.end()) { - delete it2->second; - it->second->m_poCtorCachePerIsolate.erase(it2); - } - } - } -} - MetadataReader* MetadataNode::getMetadataReader() { return &MetadataNode::s_metadataReader; } @@ -2340,7 +2315,5 @@ MetadataReader MetadataNode::s_metadataReader; robin_hood::unordered_map MetadataNode::s_name2NodeCache; robin_hood::unordered_map MetadataNode::s_name2TreeNodeCache; robin_hood::unordered_map MetadataNode::s_treeNode2NodeCache; -robin_hood::unordered_map MetadataNode::s_metadata_node_cache; -bool MetadataNode::s_profilerEnabled = false; -robin_hood::unordered_map*> MetadataNode::s_arrayObjectTemplates; +std::atomic MetadataNode::s_profilerEnabled{false}; diff --git a/test-app/runtime/src/main/cpp/MetadataNode.h b/test-app/runtime/src/main/cpp/MetadataNode.h index 2b36fa92e..117208e50 100644 --- a/test-app/runtime/src/main/cpp/MetadataNode.h +++ b/test-app/runtime/src/main/cpp/MetadataNode.h @@ -18,6 +18,7 @@ // ....->InstanceProxy->EmptyInstance #include "v8.h" +#include #include "MetadataEntry.h" #include "MetadataTreeNode.h" #include "MetadataReader.h" @@ -70,7 +71,6 @@ class MetadataNode { static bool TryGetPackageName(v8::Isolate* isolate, const v8::Local& value, std::string& out); - static void onDisposeIsolate(v8::Isolate* isolate); static MetadataReader* getMetadataReader(); private: @@ -185,7 +185,6 @@ class MetadataNode { PrototypeTemplateFiller& protoFiller); MetadataTreeNode* m_treeNode; - robin_hood::unordered_map*> m_poCtorCachePerIsolate; std::string m_name; std::string m_implType; bool m_isArray; @@ -195,9 +194,7 @@ class MetadataNode { static robin_hood::unordered_map s_name2NodeCache; static robin_hood::unordered_map s_name2TreeNodeCache; static robin_hood::unordered_map s_treeNode2NodeCache; - static robin_hood::unordered_map s_metadata_node_cache; - static robin_hood::unordered_map*> s_arrayObjectTemplates; - static bool s_profilerEnabled; + static std::atomic s_profilerEnabled; struct MethodCallbackData { MethodCallbackData() @@ -275,14 +272,55 @@ class MetadataNode { MetadataNode* node; }; + /* + * Metadata state for one runtime. Owned by RuntimeState, so it is + * reached without a shared container and destroyed with the runtime, + * while its isolate is still alive. + */ struct MetadataNodeCache { - v8::Persistent* MetadataKey; + // Initialized rather than left indeterminate: the cache is created + // on first use, which may precede MetadataNode::Init populating + // these, and the destructor below releases them. + v8::Persistent* MetadataKey = nullptr; - v8::Persistent* PackageKey; + v8::Persistent* PackageKey = nullptr; robin_hood::unordered_map CtorFuncCache; robin_hood::unordered_map ExtendedCtorFuncCache; + + // The array wrapper template for this runtime. + v8::Persistent* ArrayObjectTemplate = nullptr; + + /* + * This runtime's constructor function per node. The nodes + * themselves are shared between runtimes, so this cannot live on + * them -- it used to, as a map keyed by isolate, which meant every + * runtime's teardown walked every node to erase its entry. + */ + robin_hood::unordered_map*> CtorFunctions; + + ~MetadataNodeCache() { + delete MetadataKey; + delete PackageKey; + delete ArrayObjectTemplate; + for (auto& entry : CtorFunctions) { + delete entry.second; + } + /* + * Freed from the maps rather than from CtorCacheData and + * ExtendedClassCacheData themselves: both are held by value and + * handed out by value (GetCachedExtendedClassData returns a + * copy), and the copies share these raw pointers. A destructor + * on either struct would turn every copy into a double free. + */ + for (auto& entry : CtorFuncCache) { + delete entry.second.ft; + } + for (auto& entry : ExtendedCtorFuncCache) { + delete entry.second.extendedCtorFunction; + } + } }; }; } diff --git a/test-app/runtime/src/main/cpp/MethodCache.cpp b/test-app/runtime/src/main/cpp/MethodCache.cpp index 18e78978b..4b19d9d95 100644 --- a/test-app/runtime/src/main/cpp/MethodCache.cpp +++ b/test-app/runtime/src/main/cpp/MethodCache.cpp @@ -1,4 +1,6 @@ #include "MethodCache.h" + +#include #include "JniLocalRef.h" #include "JsArgToArrayConverter.h" #include "MetadataNode.h" @@ -13,6 +15,16 @@ #include using namespace v8; + +/* + * s_mthod_ctor_signature_cache is process-wide and string-keyed: it holds only + * JNI handles, so sharing it across runtimes is correct -- but every runtime + * resolves into it from its own thread, and workers run concurrently. Shared + * for lookups (the common case by far), exclusive only to publish a new entry. + * Never held across the JNI resolution itself: two threads may resolve the + * same signature, which is idempotent, and the first one published wins. + */ +static std::shared_mutex signatureCacheMutex; using namespace std; using namespace tns; @@ -33,31 +45,34 @@ MethodCache::CacheMethodInfo MethodCache::ResolveMethodSignature(const string& c CacheMethodInfo method_info; auto encoded_method_signature = EncodeSignature(className, methodName, args, isStatic); - auto it = s_mthod_ctor_signature_cache.find(encoded_method_signature); - - if (it == s_mthod_ctor_signature_cache.end()) { - auto signature = ResolveJavaMethod(args, className, methodName); - - DEBUG_WRITE("ResolveMethodSignature %s='%s'", encoded_method_signature.c_str(), signature.c_str()); - - if (!signature.empty()) { - JEnv env; - auto clazz = env.FindClass(className); - assert(clazz != nullptr); - method_info.clazz = clazz; - method_info.signature = signature; - method_info.returnType = MetadataReader::ParseReturnType(method_info.signature); - method_info.retType = MetadataReader::GetReturnType(method_info.returnType); - method_info.isStatic = isStatic; - method_info.mid = isStatic - ? env.GetStaticMethodID(clazz, methodName, signature) - : - env.GetMethodID(clazz, methodName, signature); - - s_mthod_ctor_signature_cache.emplace(encoded_method_signature, method_info); + { + std::shared_lock lock(signatureCacheMutex); + auto it = s_mthod_ctor_signature_cache.find(encoded_method_signature); + if (it != s_mthod_ctor_signature_cache.end()) { + return it->second; } - } else { - method_info = (*it).second; + } + + auto signature = ResolveJavaMethod(args, className, methodName); + + DEBUG_WRITE("ResolveMethodSignature %s='%s'", encoded_method_signature.c_str(), signature.c_str()); + + if (!signature.empty()) { + JEnv env; + auto clazz = env.FindClass(className); + assert(clazz != nullptr); + method_info.clazz = clazz; + method_info.signature = signature; + method_info.returnType = MetadataReader::ParseReturnType(method_info.signature); + method_info.retType = MetadataReader::GetReturnType(method_info.returnType); + method_info.isStatic = isStatic; + method_info.mid = isStatic + ? env.GetStaticMethodID(clazz, methodName, signature) + : + env.GetMethodID(clazz, methodName, signature); + + std::unique_lock lock(signatureCacheMutex); + s_mthod_ctor_signature_cache.emplace(encoded_method_signature, method_info); } return method_info; @@ -68,23 +83,26 @@ MethodCache::CacheMethodInfo MethodCache::ResolveConstructorSignature(const Args auto& args = argWrapper.args; auto encoded_ctor_signature = EncodeSignature(fullClassName, "", args, false); - auto it = s_mthod_ctor_signature_cache.find(encoded_ctor_signature); + { + std::shared_lock lock(signatureCacheMutex); + auto it = s_mthod_ctor_signature_cache.find(encoded_ctor_signature); + if (it != s_mthod_ctor_signature_cache.end()) { + return it->second; + } + } - if (it == s_mthod_ctor_signature_cache.end()) { - auto signature = ResolveConstructor(args, javaClass, isInterface); + auto signature = ResolveConstructor(args, javaClass, isInterface); - DEBUG_WRITE("ResolveConstructorSignature %s='%s'", encoded_ctor_signature.c_str(), signature.c_str()); + DEBUG_WRITE("ResolveConstructorSignature %s='%s'", encoded_ctor_signature.c_str(), signature.c_str()); - if (!signature.empty()) { - JEnv env; - constructor_info.clazz = javaClass; - constructor_info.signature = signature; - constructor_info.mid = env.GetMethodID(javaClass, "", signature); + if (!signature.empty()) { + JEnv env; + constructor_info.clazz = javaClass; + constructor_info.signature = signature; + constructor_info.mid = env.GetMethodID(javaClass, "", signature); - s_mthod_ctor_signature_cache.emplace(encoded_ctor_signature, constructor_info); - } - } else { - constructor_info = (*it).second; + std::unique_lock lock(signatureCacheMutex); + s_mthod_ctor_signature_cache.emplace(encoded_ctor_signature, constructor_info); } return constructor_info; diff --git a/test-app/runtime/src/main/cpp/NativeScriptException.cpp b/test-app/runtime/src/main/cpp/NativeScriptException.cpp index c38a6456f..c76d57f1b 100644 --- a/test-app/runtime/src/main/cpp/NativeScriptException.cpp +++ b/test-app/runtime/src/main/cpp/NativeScriptException.cpp @@ -281,15 +281,6 @@ void NativeScriptException::OnUncaughtError(Local message, e.ReThrowToJava(); } -/* - * Non-throwing runtime lookup, safe from V8 callbacks that may fire while a - * runtime is being torn down (Runtime::GetRuntime throws in that window). - */ -static Runtime* GetRuntimeOrNull(Isolate* isolate) { - return static_cast( - isolate->GetData((uint32_t)Runtime::IsolateData::RUNTIME)); -} - static string ToDetailString(Isolate* isolate, Local value) { auto context = isolate->GetCurrentContext(); Local str; @@ -326,7 +317,7 @@ static void LogLines(const string& text) { void NativeScriptException::OnPromiseRejected(v8::PromiseRejectMessage message) { auto promise = message.GetPromise(); auto isolate = v8::Isolate::GetCurrent(); - auto runtime = GetRuntimeOrNull(isolate); + auto runtime = Runtime::TryGetRuntime(isolate); if (runtime == nullptr || runtime->PromiseRejections() == nullptr) { return; } @@ -363,7 +354,7 @@ void NativeScriptException::ReportUnhandledRejection(Isolate* isolate, return; } - auto runtime = GetRuntimeOrNull(isolate); + auto runtime = Runtime::TryGetRuntime(isolate); bool discard = runtime != nullptr && runtime->GetDiscardUncaughtJsExceptions(); ReportFatalTail(isolate, reason, stackTrace, "Unhandled promise rejection:", @@ -458,7 +449,7 @@ static bool IsMarkedReportedToJs(Isolate* isolate, Local error) { bool NativeScriptException::ContainUncaughtCallbackException(Isolate* isolate, v8::TryCatch& tc) { - auto runtime = GetRuntimeOrNull(isolate); + auto runtime = Runtime::TryGetRuntime(isolate); if (runtime == nullptr) { return false; } diff --git a/test-app/runtime/src/main/cpp/Performance.cpp b/test-app/runtime/src/main/cpp/Performance.cpp index 45edccbe6..9d9b01941 100644 --- a/test-app/runtime/src/main/cpp/Performance.cpp +++ b/test-app/runtime/src/main/cpp/Performance.cpp @@ -11,15 +11,6 @@ namespace tns { namespace { -/* - * Non-throwing runtime lookup, safe from V8 callbacks that may fire while a - * runtime is being torn down (Runtime::GetRuntime throws in that window). - */ -Runtime* GetRuntimeOrNull(Isolate* isolate) { - return static_cast( - isolate->GetData((uint32_t) Runtime::IsolateData::RUNTIME)); -} - } // namespace void Performance::Init(Local context) { @@ -55,7 +46,7 @@ void Performance::Init(Local context) { } double Performance::NowMillis(Isolate* isolate) { - Runtime* runtime = GetRuntimeOrNull(isolate); + Runtime* runtime = Runtime::TryGetRuntime(isolate); if (runtime == nullptr) { return 0.0; } @@ -64,7 +55,7 @@ double Performance::NowMillis(Isolate* isolate) { } double Performance::TimeOriginMillis(Isolate* isolate) { - Runtime* runtime = GetRuntimeOrNull(isolate); + Runtime* runtime = Runtime::TryGetRuntime(isolate); if (runtime == nullptr) { return 0.0; } @@ -74,7 +65,7 @@ double Performance::TimeOriginMillis(Isolate* isolate) { double Performance::MonotonicNanosToTimelineMillis(Isolate* isolate, int64_t nanos) { - Runtime* runtime = GetRuntimeOrNull(isolate); + Runtime* runtime = Runtime::TryGetRuntime(isolate); if (runtime == nullptr) { return 0.0; } diff --git a/test-app/runtime/src/main/cpp/Runtime.cpp b/test-app/runtime/src/main/cpp/Runtime.cpp index c218dcdf0..e2a450527 100644 --- a/test-app/runtime/src/main/cpp/Runtime.cpp +++ b/test-app/runtime/src/main/cpp/Runtime.cpp @@ -20,6 +20,7 @@ #include "File.h" #include "FrameCallbacks.h" #include "Interop.h" +#include "IsolateTracked.h" #include "IsolateDisposer.h" #include "JType.h" #include "JsArgConverter.h" @@ -32,6 +33,7 @@ #include "NativeScriptAssert.h" #include "NativeScriptException.h" #include "NativeScriptPlatform.h" +#include "RuntimeState.h" #include "napi/NapiEnv.h" #include "Performance.h" #include "SimpleAllocator.h" @@ -147,6 +149,7 @@ Runtime::Runtime(JNIEnv* env, jobject runtime, int id) m_runGC(false) { m_runtime = env->NewGlobalRef(runtime); m_objectManager = new ObjectManager(m_runtime); + m_state = std::make_unique(); { std::lock_guard lock(s_runtimeCacheMutex); s_id2RuntimeCache.emplace(id, this); @@ -185,8 +188,7 @@ Runtime* Runtime::GetRuntime(v8::Isolate* isolate) { * is written once in PrepareV8Runtime before the isolate runs any JS, so * reading it requires no lock. */ - auto runtime = static_cast( - isolate->GetData((uint32_t)Runtime::IsolateData::RUNTIME)); + auto runtime = TryGetRuntime(isolate); if (runtime != nullptr) { return runtime; } @@ -208,9 +210,7 @@ Runtime* Runtime::GetRuntime(v8::Isolate* isolate) { } Runtime* Runtime::GetRuntimeFromIsolateData(v8::Isolate* isolate) { - void* maybeRuntime = - isolate->GetData((uint32_t)Runtime::IsolateData::RUNTIME); - auto runtime = static_cast(maybeRuntime); + auto runtime = TryGetRuntime(isolate); if (runtime == nullptr) { stringstream ss; @@ -309,8 +309,16 @@ Runtime::~Runtime() { if (platformInstance != nullptr && m_isolate != nullptr && m_eventLoop != nullptr) { platformInstance->IsolateDisposed(m_isolate, m_eventLoop); } - CallbackHandlers::RemoveIsolateEntries(m_isolate); - FrameCallbacks::RemoveIsolateEntries(m_isolate); + + // Last: ObjectManager calls Java through this same object above, so the ref + // has to outlive it. Without this the com.tns.Runtime instance -- and every + // Java object the runtime ever strongly registered through it -- stays + // reachable for the life of the process. + if (m_runtime != nullptr) { + JEnv env; + env.DeleteGlobalRef(m_runtime); + m_runtime = nullptr; + } } std::string Runtime::ReadFileText(const std::string& filePath) { @@ -615,7 +623,7 @@ Isolate* Runtime::PrepareV8Runtime(const string& filesPath, * Setup the V8Platform only once per process - once for the application * lifetime Don't execute again if main thread has already been initialized */ - if (!s_mainThreadInitialized) { + if (!s_mainThreadInitialized.load(std::memory_order_acquire)) { InitializeV8(); } @@ -743,7 +751,7 @@ Isolate* Runtime::PrepareV8Runtime(const string& filesPath, globalTemplate->Set(ArgConverter::ConvertToV8String(isolate, "URLPattern"), URLPatternImpl::GetCtor(isolate)); - if (!s_mainThreadInitialized) { + if (!s_mainThreadInitialized.load(std::memory_order_acquire)) { m_isMainThread = true; // __runOnMainThread closures from any runtime's thread land on this loop s_mainEventLoop = m_eventLoop; @@ -847,7 +855,7 @@ Isolate* Runtime::PrepareV8Runtime(const string& filesPath, m_weakRef.Init(isolate, context); // Do not set 'self' accessor to main thread JavaScript - if (s_mainThreadInitialized) { + if (s_mainThreadInitialized.load(std::memory_order_acquire)) { global->DefineOwnProperty(context, ArgConverter::ConvertToV8String(isolate, "self"), global, readOnlyFlags); @@ -877,7 +885,7 @@ Isolate* Runtime::PrepareV8Runtime(const string& filesPath, // Do not build metadata (which should be static for the process) for non-main // threads - if (!s_mainThreadInitialized) { + if (!s_mainThreadInitialized.load(std::memory_order_acquire)) { MetadataNode::BuildMetadata(filesPath); } @@ -895,7 +903,7 @@ Isolate* Runtime::PrepareV8Runtime(const string& filesPath, this->m_napiEnv = NapiEnv::Create(context, m_eventLoop); s_currentRuntime = this; - s_mainThreadInitialized = true; + s_mainThreadInitialized.store(true, std::memory_order_release); return isolate; } @@ -959,7 +967,50 @@ void Runtime::DestroyRuntime() { m_dispatchUnhandledRejectionFunc.Reset(); m_dispatchRejectionHandledFunc.Reset(); m_dispatchNativeUncaughtErrorFunc.Reset(); + + // Both hold v8::Global handles to JS callbacks, so their entries must be + // dropped here rather than in ~Runtime, which runs after Isolate::Dispose -- + // resetting a Global then writes into a freed handle table. Doing it here + // also closes a window in which the main thread could take a Locker on this + // isolate (RunMainThreadEntry) after it had already been disposed. + CallbackHandlers::RemoveIsolateEntries(m_isolate); + FrameCallbacks::RemoveIsolateEntries(m_isolate); + tns::disposeIsolate(m_isolate); + + // V8 does not run weak callbacks when an isolate is disposed, so anything + // still bound to one has to be deleted explicitly, here, while the isolate + // is alive and its destructors can still touch v8::Global handles. + IsolateTracked::SweepAll(m_isolate); + + // Everything below still needs the isolate alive -- the caller disposes it + // only after this returns -- but runs after the hooks above so nothing they + // touch is pulled out from under them. + + // Cached per-isolate string constants; its destructor resets 19 handles. The + // slot is cleared so a stray lookup during the rest of teardown reads null + // rather than freed memory. + auto* consts = static_cast( + m_isolate->GetData((uint32_t)Runtime::IsolateData::CONSTANTS)); + delete consts; + m_isolate->SetData((uint32_t)Runtime::IsolateData::CONSTANTS, nullptr); + + if (m_gcFunc != nullptr) { + m_gcFunc->Reset(); + delete m_gcFunc; + m_gcFunc = nullptr; + } + if (m_context != nullptr) { + m_context->Reset(); + delete m_context; + m_context = nullptr; + } + + // Last, so anything above still finds its state: this state holds + // v8::Persistents, which have to be released while the isolate is alive. + if (m_state != nullptr) { + m_state->Clear(); + } } Local Runtime::GetContext() { @@ -974,7 +1025,7 @@ jmethodID Runtime::GET_USED_MEMORY_METHOD_ID = nullptr; robin_hood::unordered_map Runtime::s_id2RuntimeCache; robin_hood::unordered_map Runtime::s_isolate2RuntimesCache; std::mutex Runtime::s_runtimeCacheMutex; -bool Runtime::s_mainThreadInitialized = false; +std::atomic Runtime::s_mainThreadInitialized{false}; v8::Platform* Runtime::platform = nullptr; int Runtime::m_androidVersion = Runtime::GetAndroidVersion(); std::shared_ptr Runtime::s_mainEventLoop; diff --git a/test-app/runtime/src/main/cpp/Runtime.h b/test-app/runtime/src/main/cpp/Runtime.h index 8cecb1c26..e9bf656aa 100644 --- a/test-app/runtime/src/main/cpp/Runtime.h +++ b/test-app/runtime/src/main/cpp/Runtime.h @@ -12,6 +12,7 @@ #include "File.h" #include "Timers.h" #include "EventLoop.h" +#include #include #include #include @@ -23,6 +24,7 @@ typedef struct napi_env__* napi_env; namespace tns { class PromiseRejectionTracker; +class RuntimeState; class Runtime { public: @@ -52,6 +54,18 @@ class Runtime { static Runtime* GetRuntimeFromIsolateData(v8::Isolate* isolate); + /* + * The runtime for this isolate, or null. Unlike the accessors above it + * neither throws nor locks -- it only reads the isolate's own data + * slot -- which is what makes it usable from the places that must not + * throw: GC weak callbacks, teardown paths, and the error handlers + * that run while a runtime is going away. + */ + static Runtime* TryGetRuntime(v8::Isolate* isolate) { + return static_cast( + isolate->GetData((uint32_t)IsolateData::RUNTIME)); + } + /* * The runtime whose home thread is the calling thread, or null. Set at * the end of PrepareV8Runtime; may be stale after a Runtime destroyed @@ -93,6 +107,15 @@ class Runtime { v8::Isolate* GetIsolate() const; + /* + * Per-runtime storage for subsystem state bound to this isolate; see + * RuntimeState.h. Released in DestroyRuntime while the isolate is + * still alive, so state holding v8::Persistents can be torn down. + */ + RuntimeState* GetState() const { + return m_state.get(); + } + jobject GetJavaRuntime() const; ObjectManager* GetObjectManager() const; @@ -256,6 +279,8 @@ class Runtime { std::shared_ptr m_eventLoop; + std::unique_ptr m_state; + napi_env m_napiEnv = nullptr; v8::Global m_globalEventTarget; @@ -303,7 +328,7 @@ class Runtime { static jmethodID GET_USED_MEMORY_METHOD_ID; - static bool s_mainThreadInitialized; + static std::atomic s_mainThreadInitialized; static std::shared_ptr s_mainEventLoop; diff --git a/test-app/runtime/src/main/cpp/RuntimeState.cpp b/test-app/runtime/src/main/cpp/RuntimeState.cpp new file mode 100644 index 000000000..fbfef1652 --- /dev/null +++ b/test-app/runtime/src/main/cpp/RuntimeState.cpp @@ -0,0 +1,13 @@ +#include "RuntimeState.h" + +namespace tns { + +size_t RuntimeState::NextSlotIndex() { + // Slot indices are claimed the first time a state type is used, which can + // happen on any runtime's thread -- two workers touching two different + // subsystems for the first time race here. + static std::atomic next{0}; + return next.fetch_add(1, std::memory_order_relaxed); +} + +} // namespace tns diff --git a/test-app/runtime/src/main/cpp/RuntimeState.h b/test-app/runtime/src/main/cpp/RuntimeState.h new file mode 100644 index 000000000..7723db7b0 --- /dev/null +++ b/test-app/runtime/src/main/cpp/RuntimeState.h @@ -0,0 +1,115 @@ +#ifndef TEST_APP_RUNTIMESTATE_H +#define TEST_APP_RUNTIMESTATE_H + +#include +#include +#include +#include + +#include "Runtime.h" +#include "v8.h" + +namespace tns { + +/* + * Per-runtime storage for subsystem state that belongs to a single isolate. + * + * Such state used to live in process-wide maps keyed by v8::Isolate*. Keying by + * isolate does not make the *container* private: runtimes start and tear down on + * their own threads, so one runtime inserting its entry while another erases its + * own corrupts the shared container. Holding the isolate's Locker does not help + * -- each thread holds only its own isolate's lock, so two runtimes are never + * excluded from each other. + * + * Hanging the state off the Runtime removes the sharing instead of guarding it: + * nothing is shared, so there is nothing to race on and no lock on the access + * path (a lookup is an isolate data-slot read plus a vector index). It also + * removes the per-isolate erase at teardown -- the whole bag is destroyed once, + * on the runtime's own thread, while the isolate is still alive, which is what + * v8::Persistent members require. + * + * A subsystem declares a state struct -- typically in its own .cpp, so nothing + * leaks into headers -- and reaches it with: + * + * auto* state = RuntimeState::For(isolate); + * if (state == nullptr) return; // runtime is tearing down + * + * The first call for a runtime default-constructs the state; it is destroyed + * with the runtime. + */ +class RuntimeState { + public: + /* + * This runtime's instance of T, created on first use, or null once the + * runtime has started tearing down (callers must not resurrect state that + * teardown has already released). + */ + template + static T* For(v8::Isolate* isolate); + + /* + * Destroys every state object. Runs on the runtime's own thread from + * DestroyRuntime, before the isolate is disposed. + */ + void Clear() { + disposed_ = true; + slots_.clear(); + } + + private: + // One slot index per state type, handed out on first use from any thread. + static size_t NextSlotIndex(); + + template + static size_t SlotIndexFor() { + static const size_t index = NextSlotIndex(); + return index; + } + + // Type-erased so Runtime need not know any subsystem's state type; the + // deleter restores the type at destruction. + using Slot = std::unique_ptr; + + template + T* GetOrCreate() { + if (disposed_) { + return nullptr; + } + + size_t index = SlotIndexFor(); + while (slots_.size() <= index) { + slots_.emplace_back(nullptr, [](void*) {}); + } + + Slot& slot = slots_[index]; + if (slot == nullptr) { + slot = Slot(new T(), [](void* value) { delete static_cast(value); }); + } + + return static_cast(slot.get()); + } + + std::vector slots_; + bool disposed_ = false; +}; + +template +T* RuntimeState::For(v8::Isolate* isolate) { + // TryGetRuntime, not GetRuntime: this runs from GC weak callbacks, where + // throwing is not an option. + auto* runtime = Runtime::TryGetRuntime(isolate); + if (runtime == nullptr) { + return nullptr; + } + + RuntimeState* state = runtime->GetState(); + if (state == nullptr) { + return nullptr; + } + + return state->GetOrCreate(); +} + +} // namespace tns + +#endif // TEST_APP_RUNTIMESTATE_H diff --git a/test-app/runtime/src/main/cpp/URLImpl.h b/test-app/runtime/src/main/cpp/URLImpl.h index 3f11ee3b6..8ad166da3 100644 --- a/test-app/runtime/src/main/cpp/URLImpl.h +++ b/test-app/runtime/src/main/cpp/URLImpl.h @@ -5,13 +5,14 @@ #pragma once #include +#include "IsolateTracked.h" #include "ada/ada.h" #include "v8.h" #include "ArgConverter.h" using namespace ada; namespace tns { - class URLImpl { + class URLImpl : public IsolateTracked { public: URLImpl(url_aggregator url); @@ -110,20 +111,8 @@ namespace tns { static void CanParse(const v8::FunctionCallbackInfo &args); - void BindFinalizer(v8::Isolate *isolate, const v8::Local &object) { - v8::HandleScope scopedHandle(isolate); - weakHandle_.Reset(isolate, object); - weakHandle_.SetWeak(this, Finalizer, v8::WeakCallbackType::kParameter); - } - - static void Finalizer(const v8::WeakCallbackInfo &data) { - auto *pThis = data.GetParameter(); - pThis->weakHandle_.Reset(); - delete pThis; - } private: url_aggregator url_; - v8::Global weakHandle_; }; } diff --git a/test-app/runtime/src/main/cpp/URLPatternImpl.h b/test-app/runtime/src/main/cpp/URLPatternImpl.h index 46bebb61f..d8987408f 100644 --- a/test-app/runtime/src/main/cpp/URLPatternImpl.h +++ b/test-app/runtime/src/main/cpp/URLPatternImpl.h @@ -6,6 +6,7 @@ #define TEST_APP_URLPATTERNIMPL_H #include "ada/ada.h" +#include "IsolateTracked.h" #include "v8.h" #include "ArgConverter.h" #include "NativeScriptAssert.h" @@ -27,7 +28,7 @@ namespace tns { static bool regex_match(std::string_view input, const regex_type &pattern); }; - class URLPatternImpl { + class URLPatternImpl : public IsolateTracked { public: URLPatternImpl(url_pattern pattern); @@ -72,21 +73,9 @@ namespace tns { static void Exec(const v8::FunctionCallbackInfo &args); - void BindFinalizer(v8::Isolate *isolate, const v8::Local &object) { - v8::HandleScope scopedHandle(isolate); - weakHandle_.Reset(isolate, object); - weakHandle_.SetWeak(this, Finalizer, v8::WeakCallbackType::kParameter); - } - - static void Finalizer(const v8::WeakCallbackInfo &data) { - auto *pThis = data.GetParameter(); - pThis->weakHandle_.Reset(); - delete pThis; - } private: url_pattern pattern_; - v8::Global weakHandle_; static std::optional ParseInput(v8::Isolate *isolate, const v8::Local &input); diff --git a/test-app/runtime/src/main/cpp/URLSearchParamsImpl.h b/test-app/runtime/src/main/cpp/URLSearchParamsImpl.h index 5faabd691..d8f225ac2 100644 --- a/test-app/runtime/src/main/cpp/URLSearchParamsImpl.h +++ b/test-app/runtime/src/main/cpp/URLSearchParamsImpl.h @@ -4,12 +4,13 @@ #pragma once #include "ada/ada.h" +#include "IsolateTracked.h" #include "v8.h" #include "ArgConverter.h" namespace tns { - class URLSearchParamsImpl { + class URLSearchParamsImpl : public IsolateTracked { public: URLSearchParamsImpl(ada::url_search_params params); @@ -49,21 +50,9 @@ namespace tns { static void Values(const v8::FunctionCallbackInfo &args); - void BindFinalizer(v8::Isolate *isolate, const v8::Local &object) { - v8::HandleScope scopedHandle(isolate); - weakHandle_.Reset(isolate, object); - weakHandle_.SetWeak(this, Finalizer, v8::WeakCallbackType::kParameter); - } - - static void Finalizer(const v8::WeakCallbackInfo &data) { - auto *pThis = data.GetParameter(); - pThis->weakHandle_.Reset(); - delete pThis; - } private: ada::url_search_params params_; - v8::Global weakHandle_; }; } // tns diff --git a/test-app/runtime/src/main/cpp/V8StringConstants.h b/test-app/runtime/src/main/cpp/V8StringConstants.h index eb707ea51..0b895bacc 100644 --- a/test-app/runtime/src/main/cpp/V8StringConstants.h +++ b/test-app/runtime/src/main/cpp/V8StringConstants.h @@ -149,47 +149,62 @@ class V8StringConstants { }; ~PerIsolateV8Constants() { - CLASS_IMPLEMENTATION_OBJECT_PERSISTENT->Reset(); - DEBUG_NAME_PERSISTENT->Reset(); - EXTEND_PERSISTENT->Reset(); - NULL_OBJECT_PERSISTENT->Reset(); - NULL_NODE_NAME_PERSISTENT->Reset(); - IS_PROTOTYPE_IMPLEMENTATION_OBJECT_PERSISTENT->Reset(); - NATIVE_EXCEPTION_PERSISTENT->Reset(); - STACK_PERSISTENT->Reset(); - STACK_TRACE_PERSISTENT->Reset(); - LONG_NUMBER_PERSISTENT->Reset(); - PROTOTYPE_PERSISTENT->Reset(); - SUPER_PERSISTENT->Reset(); - TARGET_PERSISTENT->Reset(); - TO_STRING_PERSISTENT->Reset(); - JAVA_LONG_PERSISTENT->Reset(); - VALUE_OF_PERSISTENT->Reset(); - VALUE_PERSISTENT->Reset(); - UNCAUGHT_ERROR_PERSISTENT->Reset(); - IMPLEMENTATION_OBJECT_PERSISTENT->Reset(); + // Persistent's traits do not reset in the destructor, so each + // handle is reset explicitly (requires a live isolate) and then + // freed. Every member is default-initialized because not all of + // them are allocated by the constructor -- DEBUG_NAME_PERSISTENT + // never is, and the previous version of this destructor reset it + // unconditionally, which would have faulted had it ever run. + ResetAndDelete(CLASS_IMPLEMENTATION_OBJECT_PERSISTENT); + ResetAndDelete(DEBUG_NAME_PERSISTENT); + ResetAndDelete(DISCARDED_ERROR_PERSISTENT); + ResetAndDelete(EXTEND_PERSISTENT); + ResetAndDelete(IMPLEMENTATION_OBJECT_PERSISTENT); + ResetAndDelete(IS_PROTOTYPE_IMPLEMENTATION_OBJECT_PERSISTENT); + ResetAndDelete(JAVA_LONG_PERSISTENT); + ResetAndDelete(LONG_NUMBER_PERSISTENT); + ResetAndDelete(NATIVE_EXCEPTION_PERSISTENT); + ResetAndDelete(NULL_NODE_NAME_PERSISTENT); + ResetAndDelete(NULL_OBJECT_PERSISTENT); + ResetAndDelete(PROTOTYPE_PERSISTENT); + ResetAndDelete(STACK_PERSISTENT); + ResetAndDelete(STACK_TRACE_PERSISTENT); + ResetAndDelete(SUPER_PERSISTENT); + ResetAndDelete(TARGET_PERSISTENT); + ResetAndDelete(TO_STRING_PERSISTENT); + ResetAndDelete(UNCAUGHT_ERROR_PERSISTENT); + ResetAndDelete(VALUE_OF_PERSISTENT); + ResetAndDelete(VALUE_PERSISTENT); } - v8::Persistent* CLASS_IMPLEMENTATION_OBJECT_PERSISTENT; - v8::Persistent* DEBUG_NAME_PERSISTENT; - v8::Persistent* EXTEND_PERSISTENT; - v8::Persistent* NULL_OBJECT_PERSISTENT; - v8::Persistent* NULL_NODE_NAME_PERSISTENT; - v8::Persistent* IS_PROTOTYPE_IMPLEMENTATION_OBJECT_PERSISTENT; - v8::Persistent* NATIVE_EXCEPTION_PERSISTENT; - v8::Persistent* STACK_PERSISTENT; - v8::Persistent* STACK_TRACE_PERSISTENT; - v8::Persistent* LONG_NUMBER_PERSISTENT; - v8::Persistent* PROTOTYPE_PERSISTENT; - v8::Persistent* SUPER_PERSISTENT; - v8::Persistent* TARGET_PERSISTENT; - v8::Persistent* TO_STRING_PERSISTENT; - v8::Persistent* JAVA_LONG_PERSISTENT; - v8::Persistent* VALUE_OF_PERSISTENT; - v8::Persistent* VALUE_PERSISTENT; - v8::Persistent* UNCAUGHT_ERROR_PERSISTENT; - v8::Persistent* DISCARDED_ERROR_PERSISTENT; - v8::Persistent* IMPLEMENTATION_OBJECT_PERSISTENT; + static void ResetAndDelete(v8::Persistent*& handle) { + if (handle != nullptr) { + handle->Reset(); + delete handle; + handle = nullptr; + } + } + + v8::Persistent* CLASS_IMPLEMENTATION_OBJECT_PERSISTENT = nullptr; + v8::Persistent* DEBUG_NAME_PERSISTENT = nullptr; + v8::Persistent* EXTEND_PERSISTENT = nullptr; + v8::Persistent* NULL_OBJECT_PERSISTENT = nullptr; + v8::Persistent* NULL_NODE_NAME_PERSISTENT = nullptr; + v8::Persistent* IS_PROTOTYPE_IMPLEMENTATION_OBJECT_PERSISTENT = nullptr; + v8::Persistent* NATIVE_EXCEPTION_PERSISTENT = nullptr; + v8::Persistent* STACK_PERSISTENT = nullptr; + v8::Persistent* STACK_TRACE_PERSISTENT = nullptr; + v8::Persistent* LONG_NUMBER_PERSISTENT = nullptr; + v8::Persistent* PROTOTYPE_PERSISTENT = nullptr; + v8::Persistent* SUPER_PERSISTENT = nullptr; + v8::Persistent* TARGET_PERSISTENT = nullptr; + v8::Persistent* TO_STRING_PERSISTENT = nullptr; + v8::Persistent* JAVA_LONG_PERSISTENT = nullptr; + v8::Persistent* VALUE_OF_PERSISTENT = nullptr; + v8::Persistent* VALUE_PERSISTENT = nullptr; + v8::Persistent* UNCAUGHT_ERROR_PERSISTENT = nullptr; + v8::Persistent* DISCARDED_ERROR_PERSISTENT = nullptr; + v8::Persistent* IMPLEMENTATION_OBJECT_PERSISTENT = nullptr; }; private: diff --git a/test-app/runtime/src/main/cpp/console/Console.cpp b/test-app/runtime/src/main/cpp/console/Console.cpp index 6402c385f..ae57213bb 100644 --- a/test-app/runtime/src/main/cpp/console/Console.cpp +++ b/test-app/runtime/src/main/cpp/console/Console.cpp @@ -19,22 +19,36 @@ #include "BuiltinLoader.h" #include "Console.h" #include "NsBuiltinModules.h" +#include "RuntimeState.h" #include "robin_hood.h" namespace tns { -// internal/inspect.js, one compiled instance per isolate. Worker runtimes -// initialize on their own threads, so every access is under the mutex. -static std::mutex inspectMutex; -static robin_hood::unordered_map*> isolateToInspect; +namespace { +/* + * Console state belonging to one runtime: the console.time() labels and the + * compiled internal/inspect.js instance for this realm. Every access is on the + * owning runtime's own thread, so none of it needs synchronization -- which is + * exactly what a process-wide map keyed by v8::Isolate* could not give, since + * the container was shared even though the entries were not. See RuntimeState.h. + */ +struct ConsoleState { + robin_hood::unordered_map timerLabels; + + v8::Persistent* inspect = nullptr; + + ~ConsoleState() { + delete inspect; + } +}; +} // namespace static v8::Local getInspectFunction(v8::Isolate* isolate) { - std::lock_guard lock(inspectMutex); - auto it = isolateToInspect.find(isolate); - if (it == isolateToInspect.end()) { + auto* state = tns::RuntimeState::For(isolate); + if (state == nullptr || state->inspect == nullptr) { return v8::Local(); } - return it->second->Get(isolate); + return state->inspect->Get(isolate); } v8::Local Console::createConsole(v8::Local context, ConsoleCallback callback, const int maxLogcatObjectSize, const bool forceLog) { @@ -48,9 +62,6 @@ v8::Local Console::createConsole(v8::Local context, Con assert(success); - std::map timersMap; - Console::s_isolateToConsoleTimersMap.insert( - std::make_pair(v8::Isolate::GetCurrent(), timersMap)); bindFunctionProperty(context, console, "assert", assertCallback); bindFunctionProperty(context, console, "error", errorCallback); @@ -118,10 +129,12 @@ void Console::initInspect(v8::Local context) { return; } - std::lock_guard lock(inspectMutex); - auto& slot = isolateToInspect[isolate]; - delete slot; - slot = new v8::Persistent(isolate, result.As()); + auto* state = tns::RuntimeState::For(isolate); + if (state == nullptr) { + return; + } + delete state->inspect; + state->inspect = new v8::Persistent(isolate, result.As()); } v8::Local Console::getInspect(v8::Local context) { @@ -525,15 +538,17 @@ void Console::timeCallback(const v8::FunctionCallbackInfo& info) { label = ArgConverter::ConvertToString(argString); } - auto it = Console::s_isolateToConsoleTimersMap.find(isolate); - if (it == Console::s_isolateToConsoleTimersMap.end()) { - // throw? - } - auto nano = std::chrono::time_point_cast(std::chrono::system_clock::now()); double timeStamp = nano.time_since_epoch().count(); - it->second.insert(std::make_pair(label, timeStamp)); + auto* timers = tns::RuntimeState::For(isolate); + if (timers == nullptr) { + return; + } + + // emplace, not assignment: a second console.time() with the same label + // keeps the original start stamp, as before. + timers->timerLabels.emplace(label, timeStamp); } catch (NativeScriptException& e) { e.ReThrowToV8(); } catch (std::exception e) { @@ -559,15 +574,20 @@ void Console::timeEndCallback(const v8::FunctionCallbackInfo& info) { label = ArgConverter::ConvertToString(argString); } - auto it = Console::s_isolateToConsoleTimersMap.find(isolate); - if (it == Console::s_isolateToConsoleTimersMap.end()) { - // throw? - } + double startTimeStamp = 0; + bool started = false; - std::map timersMap = it->second; + auto* timers = tns::RuntimeState::For(isolate); + if (timers != nullptr) { + auto itTimersMap = timers->timerLabels.find(label); + if (itTimersMap != timers->timerLabels.end()) { + startTimeStamp = itTimersMap->second; + timers->timerLabels.erase(itTimersMap); + started = true; + } + } - auto itTimersMap = timersMap.find(label); - if (itTimersMap == timersMap.end()) { + if (!started) { std::string warning = std::string("No such label '" + label + "' for console.timeEnd()"); __android_log_write(ANDROID_LOG_WARN, LOG_TAG, warning.c_str()); @@ -578,9 +598,6 @@ void Console::timeEndCallback(const v8::FunctionCallbackInfo& info) { auto nano = std::chrono::time_point_cast(std::chrono::system_clock::now()); double endTimeStamp = nano.time_since_epoch().count(); - double startTimeStamp = itTimersMap->second; - - it->second.erase(label); double diffMicroseconds = endTimeStamp - startTimeStamp; double diffMilliseconds = diffMicroseconds / 1000.0; @@ -604,19 +621,7 @@ void Console::timeEndCallback(const v8::FunctionCallbackInfo& info) { } } -void Console::onDisposeIsolate(v8::Isolate* isolate) { - s_isolateToConsoleTimersMap.erase(isolate); - - std::lock_guard lock(inspectMutex); - auto it = isolateToInspect.find(isolate); - if (it != isolateToInspect.end()) { - delete it->second; - isolateToInspect.erase(it); - } -} - const char* Console::LOG_TAG = "JS"; -std::map> Console::s_isolateToConsoleTimersMap; ConsoleCallback Console::m_callback = nullptr; int Console::m_maxLogcatObjectSize; } \ No newline at end of file diff --git a/test-app/runtime/src/main/cpp/console/Console.h b/test-app/runtime/src/main/cpp/console/Console.h index afe715b3e..a4ce86f49 100644 --- a/test-app/runtime/src/main/cpp/console/Console.h +++ b/test-app/runtime/src/main/cpp/console/Console.h @@ -31,7 +31,6 @@ class Console { static void timeCallback(const v8::FunctionCallbackInfo& info); static void timeEndCallback(const v8::FunctionCallbackInfo& info); - static void onDisposeIsolate(v8::Isolate* isolate); // Builds this realm's inspect function if it isn't there yet. Public // so ns:util can re-export the same instance. @@ -43,7 +42,6 @@ class Console { static int m_maxLogcatObjectSize; static ConsoleCallback m_callback; static const char* LOG_TAG; - static std::map> s_isolateToConsoleTimersMap; static void initInspect(v8::Local context); diff --git a/test-app/runtime/src/main/cpp/napi/NapiEnv.cpp b/test-app/runtime/src/main/cpp/napi/NapiEnv.cpp index 6b250d423..89aeaa450 100644 --- a/test-app/runtime/src/main/cpp/napi/NapiEnv.cpp +++ b/test-app/runtime/src/main/cpp/napi/NapiEnv.cpp @@ -47,8 +47,7 @@ NapiEnv* NapiEnv::ForIsolate(Isolate* isolate) { // Read the isolate slot directly: the Runtime::GetRuntime* accessors throw // NativeScriptException when the slot is unset, and a C++ exception must // not cross the extern "C" Node-API surface this is called under. - Runtime* runtime = static_cast( - isolate->GetData((uint32_t)Runtime::IsolateData::RUNTIME)); + Runtime* runtime = Runtime::TryGetRuntime(isolate); if (runtime == nullptr) { return nullptr; }