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.

Author: adrian-niculescu

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)) {

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<char[]> 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<char[]> 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<uint8_t*>(nodes), lenNames, reinterpret_cast<uint8_t*>(names), lenValues, reinterpret_cast<uint8_t*>(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<uint8_t*>(nodes.get()), lenNames, reinterpret_cast<uint8_t*>(names.release()), lenValues, reinterpret_cast<uint8_t*>(values));
 }
 
 void MetadataNode::BuildMetadata(uint32_t nodesLength, uint8_t* nodeData, uint32_t nameLength, uint8_t* nameData, uint32_t valueLength, uint8_t* valueData) {

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<v8::Function>* m_poJsWrapperFunc;
+  v8::Persistent<v8::Function>* m_poJsWrapperFunc = nullptr;
 };
 }  // namespace tns
 

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<EventLoop> Runtime::s_mainEventLoop;
+std::atomic<Runtime*> Runtime::s_mainRuntime{nullptr};
 
 thread_local Runtime* Runtime::s_currentRuntime = nullptr;
 thread_local PendingIsolateSetup Runtime::s_pendingIsolateSetup;

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

@@ -202,6 +202,16 @@ class Runtime {
         static std::shared_ptr<EventLoop> 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<v8::Function>* m_gcFunc;
         volatile bool m_runGC;
 
-        v8::Persistent<v8::Context>* m_context;
+        v8::Persistent<v8::Context>* 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<EventLoop> s_mainEventLoop;
+        static std::atomic<Runtime*> s_mainRuntime;
 
         static thread_local Runtime* s_currentRuntime;
         static thread_local PendingIsolateSetup s_pendingIsolateSetup;