From 857834d9c904df979bda156cf0134ffb2a3ce02b 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 1/2] fix: let a failed main runtime bootstrap be retried A main runtime whose initialization throws hands the election back so a later bootstrap can retry, but the retry crashed: the unwind reset two never-initialized Persistent pointers, the second election initialized V8 again, and in debug builds the inspector looked the main runtime up by id 0. The unwind also left the platform's event loop entry, a crash breadcrumb slot and BuildMetadata's buffers and directory handle behind, and a retry past the metadata step rebuilt the process-wide tree. --- .../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 | 32 ++++++++++++++++--- test-app/runtime/src/main/cpp/Runtime.h | 13 +++++++- 5 files changed, 57 insertions(+), 16 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..527096186 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; } @@ -760,6 +765,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 +1082,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 +1108,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 +1184,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 +1274,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..40a5ff3d4 100644 --- a/test-app/runtime/src/main/cpp/Runtime.h +++ b/test-app/runtime/src/main/cpp/Runtime.h @@ -202,6 +202,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 +361,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; @@ -422,6 +432,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; From 602de509846a8f197ef52e9ccfff85d6ff16fbbc Mon Sep 17 00:00:00 2001 From: Adrian Niculescu <15037449+adrian-niculescu@users.noreply.github.com> Date: Thu, 17 Sep 2026 13:25:35 +0300 Subject: [PATCH 2/2] ci: stop installing the obsolete 'tools' SDK package setup-android installs 'tools platform-tools' by default. The SDK repository only lists 'tools' as obsolete now, and the sdkmanager on the current runner image fails on it, so the Setup Android SDK step fails before anything is built. --- .github/workflows/npm_release.yml | 8 ++++++++ .github/workflows/pull_request.yml | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/.github/workflows/npm_release.yml b/.github/workflows/npm_release.yml index ac56f2dd1..022861902 100644 --- a/.github/workflows/npm_release.yml +++ b/.github/workflows/npm_release.yml @@ -52,6 +52,10 @@ jobs: cache: gradle - name: Setup Android SDK uses: android-actions/setup-android@651bceb6f9ca583f16b8d75b62c36ded2ae6fc9c # v4.0.0 + with: + # The action's default also asks for 'tools', which the SDK repository + # only lists as obsolete and current sdkmanager versions refuse. + packages: platform-tools - name: Setup NDK run: | echo "y" | sdkmanager "cmake;$CMAKE_VERSION" @@ -151,6 +155,10 @@ jobs: cache: gradle - name: Setup Android SDK uses: android-actions/setup-android@651bceb6f9ca583f16b8d75b62c36ded2ae6fc9c # v4.0.0 + with: + # The action's default also asks for 'tools', which the SDK repository + # only lists as obsolete and current sdkmanager versions refuse. + packages: platform-tools - name: Setup NDK run: | echo "y" | sdkmanager "cmake;$CMAKE_VERSION" diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index 455b99fb7..7e4d330a6 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -41,6 +41,10 @@ jobs: cache: gradle - name: Setup Android SDK uses: android-actions/setup-android@651bceb6f9ca583f16b8d75b62c36ded2ae6fc9c # v4.0.0 + with: + # The action's default also asks for 'tools', which the SDK repository + # only lists as obsolete and current sdkmanager versions refuse. + packages: platform-tools - name: Setup NDK run: | echo "y" | sdkmanager "cmake;$CMAKE_VERSION" @@ -123,6 +127,10 @@ jobs: cache: gradle - name: Setup Android SDK uses: android-actions/setup-android@651bceb6f9ca583f16b8d75b62c36ded2ae6fc9c # v4.0.0 + with: + # The action's default also asks for 'tools', which the SDK repository + # only lists as obsolete and current sdkmanager versions refuse. + packages: platform-tools - name: Setup NDK run: | echo "y" | sdkmanager "cmake;3.6.4111459"