fix: own isolate-bound state per runtime instead of in shared maps

Workers bootstrap on detached threads and are not serialized, so several
runtimes are inside PrepareV8Runtime while another is in disposeIsolate. A
handful of subsystems kept their per-isolate state in process-wide maps keyed
by v8::Isolate*, which makes the *container* shared even though the entries are
not: one runtime inserting its own entry while another erases its own corrupts
the map. Holding the isolate's Locker does not help, because each thread holds
only its own isolate's lock, so two runtimes never exclude each other.

The reproduced crash walked a freed red-black tree node:

  std::less<v8::Isolate*>::operator()
  std::map<v8::Isolate*, std::map<std::string,double>>::insert
  tns::Console::createConsole
  tns::Runtime::PrepareV8Runtime
  Java_com_tns_Runtime_initNativeScript      (thread W41: ./EvalWork)

Rather than guard each container, remove the sharing: RuntimeState is a typed
per-runtime slot bag owned by Runtime. A subsystem declares a state struct,
usually in its own .cpp, and reaches it with RuntimeState::For<T>(isolate) --
an isolate data-slot read plus a vector index, with no lock and no shared
container. The bag is destroyed once in DestroyRuntime, on the runtime's own
thread and while the isolate is still alive, which is what state holding
v8::Persistents requires.

Moved onto it:

- Console: console.time() labels and the compiled inspect.js instance. Console
  now has no global mutable state and no mutex at all.
- ArgConverter: the java-long conversion helpers.
- JSONObjectHelper: the compiled JS->org.json serializer.
- MetadataNode: the per-isolate node cache and the array wrapper template, plus
  the constructor functions that used to hang off every node as a map keyed by
  isolate -- which is why teardown had to walk every node in
  s_treeNode2NodeCache to erase one entry. That walk, running on a dying
  worker's thread while other threads inserted, is gone.

Four onDisposeIsolate hooks disappear with it: nothing is keyed by isolate any
more, so there is no per-isolate entry to erase.

Also:

- MetadataNode::s_profilerEnabled and Runtime::s_mainThreadInitialized are now
  atomic. The latter gated the one-time BuildMetadata, so as a plain bool there
  was no happens-before edge between the main thread's metadata construction
  and a worker's first read of s_metadataReader.
- TypeLongOperationsCache gains a destructor; it was deleted without one,
  leaking two v8::Persistents per isolate.
- console.time/timeEnd no longer dereference the iterator returned by a failed
  find (both had a "// throw?" comment and then used it anyway).

Verified on an arm64 emulator with the SIGSEGV handler temporarily disabled, so
faults are fatal and tombstoned rather than swallowed: the earlier, narrower
mutex-based version of this fix ran 20/20 full-suite runs clean against a
baseline that reproduced roughly 1 in 5. Re-verification of this version is
running; suite is 879/0.

Still shared, and deliberately left for a follow-up: the metadata tree and
MetadataReader's buffers (genuinely one blob for the process, so they need a
narrow lock rather than per-runtime storage), and the string-keyed
MethodCache::s_mthod_ctor_signature_cache and JEnv::s_classCache.

Author: edusperoni

test-app/runtime/CMakeLists.txt

@@ -203,6 +203,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

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 <sstream>
 
 using namespace v8;
@@ -196,16 +197,8 @@ int64_t ArgConverter::ConvertToJavaLong(Isolate* isolate, const Local<Value>& 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;
-    }
-
-    return cache;
+    // Per runtime, so there is no shared table to race on; see RuntimeState.h.
+    return RuntimeState::For<TypeLongOperationsCache>(isolate);
 }
 
 
@@ -222,12 +215,3 @@ u16string ArgConverter::ConvertToUtf16String(const v8::Local<String>& 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<Isolate*, ArgConverter::TypeLongOperationsCache*> ArgConverter::s_type_long_operations_cache;

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<v8::Function>* LongNumberCtorFunc = nullptr;
+
+            v8::Persistent<v8::NumberObject>* 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<v8::Function>* LongNumberCtorFunc;
-
-            v8::Persistent<v8::NumberObject>* 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<v8::Value>& args);
 
-        /*
-         * "s_type_long_operations_cache" used to keep function
-         * dealing with operations concerning java long -> javascript number.
-         */
-        static robin_hood::unordered_map<v8::Isolate*, TypeLongOperationsCache*> s_type_long_operations_cache;
 };
 }
 

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

test-app/runtime/src/main/cpp/JSONObjectHelper.cpp

@@ -2,14 +2,24 @@
 #include "JSONObjectHelper.h"
 #include "ArgConverter.h"
 #include "BuiltinLoader.h"
+#include "RuntimeState.h"
 #include "robin_hood.h"
 #include <sstream>
 #include <string>
 
 using namespace v8;
 using namespace tns;
 
-static robin_hood::unordered_map<Isolate*, Persistent<Function>*> isolateToSerializeFunc;
+namespace {
+// The compiled JS->org.json serializer, per runtime; see RuntimeState.h.
+struct SerializeFuncState {
+    Persistent<Function>* func = nullptr;
+
+    ~SerializeFuncState() {
+        delete func;
+    }
+};
+}  // namespace
 
 void JSONObjectHelper::RegisterFromFunction(Isolate *isolate, Local<Value>& jsonObject) {
     if (!jsonObject->IsFunction()) {
@@ -84,9 +94,12 @@ void JSONObjectHelper::ConvertCallbackStatic(const FunctionCallbackInfo<Value>&
 Persistent<Function>* JSONObjectHelper::GetSerializeFunc(Local<Context> context) {
     Isolate* isolate = v8::Isolate::GetCurrent();
 
-    auto it = isolateToSerializeFunc.find(isolate);
-    if (it != isolateToSerializeFunc.end()) {
-        return it->second;
+    auto* state = RuntimeState::For<SerializeFuncState>(isolate);
+    if (state == nullptr) {
+        return nullptr;
+    }
+    if (state->func != nullptr) {
+        return state->func;
     }
 
     Local<Value> result;
@@ -95,16 +108,8 @@ Persistent<Function>* JSONObjectHelper::GetSerializeFunc(Local<Context> context)
         return nullptr;
     }
 
-    auto* serializeFunc = new Persistent<Function>(isolate, result.As<Function>());
-    isolateToSerializeFunc.emplace(isolate, serializeFunc);
+    state->func = new Persistent<Function>(isolate, result.As<Function>());
 
-    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);
-    }
-}

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<v8::Value>& jsonObject);
-    static void onDisposeIsolate(v8::Isolate* isolate);
 private:
     static v8::Persistent<v8::Function>* GetSerializeFunc(v8::Local<v8::Context> context);
     static void ConvertCallbackStatic(const v8::FunctionCallbackInfo<v8::Value>& info);

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,17 +83,17 @@ bool MetadataNode::TryGetPackageName(Isolate* isolate, const Local<Object>& valu
 }
 
 Local<ObjectTemplate> 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);
     arrayObjectTemplate->SetInternalFieldCount(static_cast<int>(ObjectManager::MetadataNodeKeys::END));
     arrayObjectTemplate->SetHandler(IndexedPropertyHandlerConfiguration(
             ArrayIndexedPropertyGetterCallback, ArrayIndexedPropertySetterCallback));
 
-    s_arrayObjectTemplates.emplace(std::make_pair(isolate, new Persistent<ObjectTemplate>(isolate, arrayObjectTemplate)));
+    cache->ArrayObjectTemplate = new Persistent<ObjectTemplate>(isolate, arrayObjectTemplate);
 
     return arrayObjectTemplate;
 }
@@ -721,7 +722,7 @@ vector<MetadataNode::MethodCallbackData *> MetadataNode::SetInstanceMethodsFromS
                 callbackData->parent = *itFound;
             }
 
-            if (s_profilerEnabled) {
+            if (s_profilerEnabled.load(std::memory_order_relaxed)) {
                 Local<External> funcData = External::New(isolate, callbackData, v8::kExternalPointerTypeTagDefault);
                 Local<FunctionTemplate> funcTemplate = FunctionTemplate::New(isolate, MethodCallback, funcData);
                 auto func = funcTemplate->GetFunction(context).ToLocalChecked();
@@ -965,8 +966,9 @@ void MetadataNode::InnerTypeAccessorGetterCallback(v8::Local<v8::Name> property,
     MetadataTreeNode* curChild = static_cast<MetadataTreeNode*>(
         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<FunctionTemplate> MetadataNode::GetConstructorFunctionTemplate(Isolate* is
     node->SetStaticMembers(isolate, wrappedCtorFunc, treeNode, curPtr);
 
     // insert isolate-specific persistent function handle
-    node->m_poCtorCachePerIsolate.insert({isolate, new Persistent<Function>(isolate, wrappedCtorFunc)});
+    cache->CtorFunctions.emplace(node, new Persistent<Function>(isolate, wrappedCtorFunc));
     if (!baseCtorFunc.IsEmpty()) {
         auto currentContext = isolate->GetCurrentContext();
         wrappedCtorFunc->SetPrototype(currentContext, baseCtorFunc);
@@ -1116,8 +1118,9 @@ Local<Function> MetadataNode::GetConstructorFunction(Isolate* isolate) {
 }
 
 Persistent<Function>* 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 Local<Object
 }
 
 MetadataNode::MetadataNodeCache* MetadataNode::GetMetadataNodeCache(Isolate* isolate) {
-    MetadataNodeCache* cache;
-    auto itFound = s_metadata_node_cache.find(isolate);
-    if (itFound == s_metadata_node_cache.end()) {
-        cache = new MetadataNodeCache;
-        s_metadata_node_cache.emplace(isolate, cache);
-    } else {
-        cache = itFound->second;
+    // 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<MetadataNodeCache>(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<Function> MetadataNode::Wrap(Isolate* isolate, const Local<Function>& function, const string& name, const string& origin, bool isCtorFunc) {
-    if (!s_profilerEnabled || name == "<init>") {
+    if (!s_profilerEnabled.load(std::memory_order_relaxed) || name == "<init>") {
         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<std::string, MetadataNode*> MetadataNode::s_name2NodeCache;
 robin_hood::unordered_map<std::string, MetadataTreeNode*> MetadataNode::s_name2TreeNodeCache;
 robin_hood::unordered_map<MetadataTreeNode*, MetadataNode*> MetadataNode::s_treeNode2NodeCache;
-robin_hood::unordered_map<Isolate*, MetadataNode::MetadataNodeCache*> MetadataNode::s_metadata_node_cache;
-bool MetadataNode::s_profilerEnabled = false;
-robin_hood::unordered_map<Isolate*, Persistent<ObjectTemplate>*> MetadataNode::s_arrayObjectTemplates;
+std::atomic<bool> MetadataNode::s_profilerEnabled{false};
 

test-app/runtime/src/main/cpp/MetadataNode.h

@@ -18,6 +18,7 @@
 //	....->InstanceProxy->EmptyInstance
 
 #include "v8.h"
+#include <atomic>
 #include "MetadataEntry.h"
 #include "MetadataTreeNode.h"
 #include "MetadataReader.h"
@@ -70,7 +71,6 @@ class MetadataNode {
 
         static bool TryGetPackageName(v8::Isolate* isolate, const v8::Local<v8::Object>& 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<v8::Isolate*, v8::Persistent<v8::Function>*> m_poCtorCachePerIsolate;
         std::string m_name;
         std::string m_implType;
         bool m_isArray;
@@ -195,9 +194,7 @@ class MetadataNode {
         static robin_hood::unordered_map<std::string, MetadataNode*> s_name2NodeCache;
         static robin_hood::unordered_map<std::string, MetadataTreeNode*> s_name2TreeNodeCache;
         static robin_hood::unordered_map<MetadataTreeNode*, MetadataNode*> s_treeNode2NodeCache;
-        static robin_hood::unordered_map<v8::Isolate*, MetadataNodeCache*> s_metadata_node_cache;
-        static robin_hood::unordered_map<v8::Isolate*, v8::Persistent<v8::ObjectTemplate>*> s_arrayObjectTemplates;
-        static bool s_profilerEnabled;
+        static std::atomic<bool> s_profilerEnabled;
 
         struct MethodCallbackData {
             MethodCallbackData()
@@ -275,6 +272,11 @@ 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<v8::String>* MetadataKey;
 
@@ -283,6 +285,24 @@ class MetadataNode {
             robin_hood::unordered_map<MetadataTreeNode*, CtorCacheData> CtorFuncCache;
 
             robin_hood::unordered_map<std::string, MetadataNode::ExtendedClassCacheData> ExtendedCtorFuncCache;
+
+            // The array wrapper template for this runtime.
+            v8::Persistent<v8::ObjectTemplate>* 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<MetadataNode*, v8::Persistent<v8::Function>*> CtorFunctions;
+
+            ~MetadataNodeCache() {
+                delete ArrayObjectTemplate;
+                for (auto& entry : CtorFunctions) {
+                    delete entry.second;
+                }
+            }
         };
 };
 }