From 5b27ea6670da89b6fad3febfa0fae8b99f6a5caa Mon Sep 17 00:00:00 2001 From: Adrian Niculescu <15037449+adrian-niculescu@users.noreply.github.com> Date: Thu, 17 Sep 2026 13:22:14 +0300 Subject: [PATCH] fix: tear down a failed runtime bootstrap so it can be retried A bootstrap can fail inside native initialization or afterwards, when ts_helpers.js throws. Either way the whole native runtime is unwound: the isolate, its event loop entry, its crash breadcrumb slot and the BuildMetadata buffers and directory handle. A failed main runtime hands the election back, including its readiness, so a retry becomes the main runtime again. The unwind leaves never-initialized Persistents alone, V8 and the metadata tree are initialized once per process, and the inspector finds the main runtime whatever id its attempt was given. --- .../src/main/cpp/JsV8InspectorClient.cpp | 6 +- .../runtime/src/main/cpp/MetadataNode.cpp | 20 ++++--- test-app/runtime/src/main/cpp/ObjectManager.h | 2 +- test-app/runtime/src/main/cpp/Runtime.cpp | 57 +++++++++++++++++-- test-app/runtime/src/main/cpp/Runtime.h | 30 ++++++++-- .../runtime/src/main/cpp/com_tns_Runtime.cpp | 16 ++++++ .../src/main/java/com/tns/Runtime.java | 11 ++++ 7 files changed, 121 insertions(+), 21 deletions(-) diff --git a/test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp b/test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp index e553bec6e..ea957448f 100644 --- a/test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp +++ b/test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp @@ -909,7 +909,11 @@ JsV8InspectorClient* JsV8InspectorClient::GetInstance() { // handleMessageOnSocketThread also calls this from the socket thread, so a // concurrent first call is possible: construct, then publish with a CAS and // discard our copy if another thread won the race. - auto* created = new JsV8InspectorClient(Runtime::GetRuntime(0)->GetIsolate()); + Runtime* mainRuntime = Runtime::GetMainRuntime(); + if (mainRuntime == nullptr) { + throw NativeScriptException("Cannot create the inspector: the main runtime is not initialized"); + } + auto* created = new JsV8InspectorClient(mainRuntime->GetIsolate()); JsV8InspectorClient* expected = nullptr; if (!instance.compare_exchange_strong(expected, created, std::memory_order_acq_rel, std::memory_order_acquire)) { diff --git a/test-app/runtime/src/main/cpp/MetadataNode.cpp b/test-app/runtime/src/main/cpp/MetadataNode.cpp index 83e77fa34..f8660949d 100644 --- a/test-app/runtime/src/main/cpp/MetadataNode.cpp +++ b/test-app/runtime/src/main/cpp/MetadataNode.cpp @@ -2042,6 +2042,8 @@ void MetadataNode::BuildMetadata(const string& filesPath) { throw NativeScriptException(ss.str()); } } + // Only opened to tell a missing folder from a missing file. + closedir(dir); string nodesFile = baseDir + "/treeNodeStream.dat"; string namesFile = baseDir + "/treeStringsStream.dat"; @@ -2068,9 +2070,11 @@ void MetadataNode::BuildMetadata(const string& filesPath) { << "-byte records. The metadata is truncated or corrupt."; throw NativeScriptException(ss.str()); } - char* nodes = new char[lenNodes]; + // Owned until the reader takes them, so a file that fails to open further + // down does not strand the buffers already read. + std::unique_ptr nodes(new char[lenNodes]); rewind(f); - fread(nodes, 1, lenNodes, f); + fread(nodes.get(), 1, lenNodes, f); fclose(f); const int _512KB = 524288; @@ -2085,9 +2089,9 @@ void MetadataNode::BuildMetadata(const string& filesPath) { } fseek(f, 0, SEEK_END); int lenNames = ftell(f); - char* names = new char[lenNames + _512KB]; + std::unique_ptr names(new char[lenNames + _512KB]); rewind(f); - fread(names, 1, lenNames, f); + fread(names.get(), 1, lenNames, f); fclose(f); f = fopen(valuesFile.c_str(), "rb"); @@ -2115,11 +2119,9 @@ void MetadataNode::BuildMetadata(const string& filesPath) { DEBUG_WRITE("time=%ld", (millis2 - millis1)); - BuildMetadata(lenNodes, reinterpret_cast(nodes), lenNames, reinterpret_cast(names), lenValues, reinterpret_cast(values)); - - delete[] nodes; - //delete[] names; - //delete[] values; + // The reader keeps the names and values buffers for the life of the + // process and only reads the nodes buffer while it builds the tree. + BuildMetadata(lenNodes, reinterpret_cast(nodes.get()), lenNames, reinterpret_cast(names.release()), lenValues, reinterpret_cast(values)); } void MetadataNode::BuildMetadata(uint32_t nodesLength, uint8_t* nodeData, uint32_t nameLength, uint8_t* nameData, uint32_t valueLength, uint8_t* valueData) { diff --git a/test-app/runtime/src/main/cpp/ObjectManager.h b/test-app/runtime/src/main/cpp/ObjectManager.h index 23194133a..ae4003dc4 100644 --- a/test-app/runtime/src/main/cpp/ObjectManager.h +++ b/test-app/runtime/src/main/cpp/ObjectManager.h @@ -260,7 +260,7 @@ class ObjectManager { static jmethodID CHECK_WEAK_OBJECTS_ARE_ALIVE_METHOD_ID; - v8::Persistent* m_poJsWrapperFunc; + v8::Persistent* m_poJsWrapperFunc = nullptr; }; } // namespace tns diff --git a/test-app/runtime/src/main/cpp/Runtime.cpp b/test-app/runtime/src/main/cpp/Runtime.cpp index d7ab015e8..107a4332b 100644 --- a/test-app/runtime/src/main/cpp/Runtime.cpp +++ b/test-app/runtime/src/main/cpp/Runtime.cpp @@ -313,6 +313,9 @@ Runtime::~Runtime() { s_isolate2RuntimesCache.erase(it); } } + // Same backstop for the breadcrumb slot Init took: the table is small and + // fixed, so slots lost to failed bootstraps would crowd out live runtimes. + CrashBreadcrumbs::UnregisterRuntime(m_id); delete this->m_objectManager; // idempotent backstop for the matched erase WorkerWrapper does right after @@ -712,9 +715,11 @@ void Runtime::ElectMainRuntime() { s_mainRuntimeElected = true; s_mainRuntimeFailed = false; m_isMainThread = true; - // Once per process: V8::Initialize freezes the flag list, and setting a - // flag afterwards aborts. - InitializeV8(); + // Once per process, not once per election: a main runtime that failed + // hands the election back, and V8 aborts both on a second + // InitializePlatform and on a flag set after V8::Initialize froze the list. + static std::once_flag v8Initialized; + std::call_once(v8Initialized, InitializeV8); return; } @@ -735,9 +740,13 @@ void Runtime::SignalMainRuntimeReady(bool failed) { { std::lock_guard lock(s_mainInitMutex); if (failed) { - // Hand the election back so a later bootstrap can retry. + // Hand the election back so a later bootstrap can retry. A main runtime + // that already signalled readiness and failed afterwards withdraws it + // too, so nothing waiting on the next main runtime starts against this + // one. s_mainRuntimeElected = false; s_mainRuntimeFailed = true; + s_mainThreadInitialized.store(false, std::memory_order_release); } else { s_mainThreadInitialized.store(true, std::memory_order_release); } @@ -745,6 +754,25 @@ void Runtime::SignalMainRuntimeReady(bool failed) { s_mainInitReady.notify_all(); } +void Runtime::UnwindFailedBootstrap(int runtimeId) { + Runtime* runtime = nullptr; + { + std::lock_guard lock(s_runtimeCacheMutex); + auto it = s_id2RuntimeCache.find(runtimeId); + if (it != s_id2RuntimeCache.end()) { + runtime = it->second; + } + } + if (runtime == nullptr) { + return; + } + // Only the bootstrapping thread can reach this runtime: no application JS + // has run on it, so nothing has handed it to another thread or started a + // worker from it. + runtime->UnwindFailedInit(); + delete runtime; +} + void Runtime::UnwindFailedInit() { /* * Reuses the two teardown windows rather than adding a third cleanup path. @@ -760,6 +788,12 @@ void Runtime::UnwindFailedInit() { DestroyRuntime(); } m_isolate->Dispose(); + // The ~Runtime backstop keys on m_isolate, which is cleared below, so the + // platform's loop entry has to go here. Left behind, it would hand the + // stopped loop to the next isolate allocated at this address. + if (m_eventLoop != nullptr) { + NativeScriptPlatform::Instance()->IsolateDisposed(m_isolate, m_eventLoop); + } m_isolate = nullptr; } @@ -1071,7 +1105,15 @@ Isolate* Runtime::PrepareV8Runtime(const string& filesPath, // Do not build metadata (which should be static for the process) for non-main // threads if (m_isMainThread) { - MetadataNode::BuildMetadata(filesPath); + // Once per process, like V8 itself: the tree is process-wide state that + // outlives the runtime that built it, so a main runtime elected after an + // earlier one failed past this point reads the tree already there. Only + // the elected main runtime gets here, one at a time. + static bool metadataBuilt = false; + if (!metadataBuilt) { + MetadataNode::BuildMetadata(filesPath); + metadataBuilt = true; + } } auto enableProfiler = !profilerOutputDir.empty(); @@ -1089,6 +1131,7 @@ Isolate* Runtime::PrepareV8Runtime(const string& filesPath, s_currentRuntime = this; if (m_isMainThread) { + s_mainRuntime.store(this, std::memory_order_release); // Releases any runtime waiting in ElectMainRuntime: the metadata tree and // the main event loop they depend on are published by now. SignalMainRuntimeReady(false /* failed */); @@ -1164,6 +1207,9 @@ void Runtime::DestroyRuntime() { if (s_currentRuntime == this) { s_currentRuntime = nullptr; } + Runtime* self = this; + s_mainRuntime.compare_exchange_strong(self, nullptr, + std::memory_order_acq_rel); // The events state holds v8::Global handles (backing event target, dispatch // closures and tracked promise rejections) - reset them while the isolate // is still alive. @@ -1251,6 +1297,7 @@ bool Runtime::s_mainRuntimeFailed = false; v8::Platform* Runtime::platform = nullptr; int Runtime::m_androidVersion = Runtime::GetAndroidVersion(); std::shared_ptr Runtime::s_mainEventLoop; +std::atomic Runtime::s_mainRuntime{nullptr}; thread_local Runtime* Runtime::s_currentRuntime = nullptr; thread_local PendingIsolateSetup Runtime::s_pendingIsolateSetup; diff --git a/test-app/runtime/src/main/cpp/Runtime.h b/test-app/runtime/src/main/cpp/Runtime.h index 22f0436fd..cf3dd2a78 100644 --- a/test-app/runtime/src/main/cpp/Runtime.h +++ b/test-app/runtime/src/main/cpp/Runtime.h @@ -82,6 +82,14 @@ class Runtime { */ static void SetPendingIsolateSetup(PendingIsolateSetup setup); + /* + * Tears down the native runtime of a bootstrap that failed on the + * Java side after initNativeScript returned, such as a throwing + * ts_helpers.js. No-op when no native runtime is registered under the + * id, which is the case for a bootstrap that failed inside Init. + */ + static void UnwindFailedBootstrap(int runtimeId); + static Runtime* GetRuntime(int runtimeId); static Runtime* GetRuntime(v8::Isolate* isolate); @@ -202,6 +210,16 @@ class Runtime { static std::shared_ptr GetMainEventLoop() { return s_mainEventLoop; } + + /* + * The main runtime, or null while there is none: before it finishes + * initializing and after it is destroyed. Its id is whatever its + * bootstrap attempt was handed, which is 0 only when the first attempt + * succeeded. + */ + static Runtime* GetMainRuntime() { + return s_mainRuntime.load(std::memory_order_acquire); + } static JavaVM* GetJVM() { return s_jvm; } @@ -351,7 +369,7 @@ class Runtime { v8::Persistent* m_gcFunc; volatile bool m_runGC; - v8::Persistent* m_context; + v8::Persistent* m_context = nullptr; // Decided by ElectMainRuntime, before anything can read it. bool m_isMainThread = false; @@ -383,10 +401,11 @@ class Runtime { static void SignalMainRuntimeReady(bool failed); /* - * Unwinds an initialization that threw after the isolate existed. The - * Java-side rollback only unwinds Java state, which would otherwise - * leave the isolate in the runtime caches and the half-built Runtime - * holding everything it had allocated. + * Unwinds an initialization that threw after the isolate existed, or + * one that finished and then failed on the Java side. The Java-side + * rollback only unwinds Java state, which would otherwise leave the + * isolate in the runtime caches and the Runtime holding everything it + * had allocated. */ void UnwindFailedInit(); jobject ConvertJsValueToJavaObject(JEnv& env, const v8::Local& value, int classReturnType); @@ -422,6 +441,7 @@ class Runtime { static bool s_mainRuntimeFailed; static std::shared_ptr s_mainEventLoop; + static std::atomic s_mainRuntime; static thread_local Runtime* s_currentRuntime; static thread_local PendingIsolateSetup s_pendingIsolateSetup; diff --git a/test-app/runtime/src/main/cpp/com_tns_Runtime.cpp b/test-app/runtime/src/main/cpp/com_tns_Runtime.cpp index 71800c0d9..23df206b2 100644 --- a/test-app/runtime/src/main/cpp/com_tns_Runtime.cpp +++ b/test-app/runtime/src/main/cpp/com_tns_Runtime.cpp @@ -107,6 +107,22 @@ extern "C" JNIEXPORT void Java_com_tns_Runtime_initNativeScript(JNIEnv* _env, jo } } +extern "C" JNIEXPORT void Java_com_tns_Runtime_unwindFailedBootstrap(JNIEnv* _env, jclass clazz, jint runtimeId) { + try { + Runtime::UnwindFailedBootstrap(runtimeId); + } catch (NativeScriptException& e) { + e.ReThrowToJava(); + } catch (std::exception e) { + stringstream ss; + ss << "Error: c++ exception: " << e.what() << endl; + NativeScriptException nsEx(ss.str()); + nsEx.ReThrowToJava(); + } catch (...) { + NativeScriptException nsEx(std::string("Error: c++ exception!")); + nsEx.ReThrowToJava(); + } +} + Runtime* TryGetRuntime(int runtimeId) { Runtime* runtime = nullptr; try { diff --git a/test-app/runtime/src/main/java/com/tns/Runtime.java b/test-app/runtime/src/main/java/com/tns/Runtime.java index b21263154..a7db4b548 100644 --- a/test-app/runtime/src/main/java/com/tns/Runtime.java +++ b/test-app/runtime/src/main/java/com/tns/Runtime.java @@ -42,6 +42,8 @@ private native void initNativeScript(int runtimeId, String filesPath, String nat private native Object runScript(int runtimeId, String filePath) throws NativeScriptException; + private static native void unwindFailedBootstrap(int runtimeId); + private native Object callJSMethodNative(int runtimeId, int javaObjectID, String methodName, int retType, boolean isConstructor, Object... packagedArgs) throws NativeScriptException; private native void createJSInstanceNative(int runtimeId, Object javaObject, int javaObjectID, String canonicalName); @@ -602,6 +604,15 @@ private static Runtime initRuntime(DynamicConfiguration dynamicConfiguration) { runtimeCache.remove(runtime.getRuntimeId()); currentRuntime.remove(); GcListener.unsubscribe(runtime); + // ts_helpers.js runs after the native runtime is fully built, so a + // failure there leaves the isolate behind unless it is torn down + // here as well; after the unsubscribe, so no GC notification can + // still be running against it + try { + unwindFailedBootstrap(runtime.getRuntimeId()); + } catch (Throwable unwindError) { + t.addSuppressed(unwindError); + } throw t; }