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.

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;
   }
 
@@ -735,16 +740,39 @@ void Runtime::SignalMainRuntimeReady(bool failed) {
   {
     std::lock_guard<std::mutex> 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);
     }
   }
   s_mainInitReady.notify_all();
 }
 
+void Runtime::UnwindFailedBootstrap(int runtimeId) {
+  Runtime* runtime = nullptr;
+  {
+    std::lock_guard<std::mutex> 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<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

@@ -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<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 +369,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;
@@ -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<v8::Value>& value, int classReturnType);
@@ -422,6 +441,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;

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 {

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;
         }