fix: close breadcrumb races and restore enclosing module after nested loads

Address review findings:

- Stop rendering once a crash is recorded: a thread that kept running
  could otherwise flip the double buffer twice and rewrite the buffer
  the handler is copying out.
- Make g_storeFd atomic with release/acquire ordering; handlers are
  installed at JNI_OnLoad, before OpenStore publishes the fd.
- Replace SetCurrentModule with a scoped ModuleScope that restores the
  enclosing module on every return and throw path, so a crash after a
  nested require no longer blames the inner module.

Author: edusperoni

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

@@ -39,7 +39,7 @@ char g_rendered[2][kBufferMax];
 size_t g_renderedLength[2];
 std::atomic<int> g_active{-1};
 
-int g_storeFd = -1;
+std::atomic<int> g_storeFd{-1};
 std::atomic_flag g_recorded = ATOMIC_FLAG_INIT;
 struct sigaction g_previous[NSIG];
 
@@ -84,6 +84,11 @@ void Append(char* out, size_t& length, const char* format, ...) {
 }
 
 void RenderLocked() {
+  // Once a crash is recorded the handler may be reading either buffer; a
+  // second flip after that point would rewrite the one it is copying out.
+  if (g_recorded.test(std::memory_order_acquire)) {
+    return;
+  }
   int next = g_active.load(std::memory_order_relaxed) == 0 ? 1 : 0;
   char* out = g_rendered[next];
   size_t length = 0;
@@ -138,7 +143,7 @@ void AppendRawInt(char* out, size_t capacity, size_t& length, int value) {
 void Handler(int signalNumber, siginfo_t* info, void* context) {
   // Only the first thread to fault records; the rest are already doomed.
   if (!g_recorded.test_and_set()) {
-    int fd = g_storeFd;
+    int fd = g_storeFd.load(std::memory_order_acquire);
     if (fd >= 0) {
       char header[kHeaderMax];
       size_t length = 0;
@@ -219,7 +224,7 @@ void CrashBreadcrumbs::OpenStore(const std::string& filesRoot) {
       ftruncate(fd, 0);
     }
 
-    g_storeFd = fd;
+    g_storeFd.store(fd, std::memory_order_release);
   });
 }
 
@@ -273,14 +278,26 @@ void CrashBreadcrumbs::SetWorkerScript(int runtimeId, const char* script) {
   RenderLocked();
 }
 
-void CrashBreadcrumbs::SetCurrentModule(const char* modulePath) {
+CrashBreadcrumbs::ModuleScope::ModuleScope(const char* modulePath) {
   Slot* slot = t_slot;
   if (slot == nullptr) {
     return;
   }
   std::lock_guard<std::mutex> lock(g_mutex);
+  previous_ = slot->module;
+  restore_ = true;
   CopyField(slot->module, modulePath);
   RenderLocked();
 }
 
+CrashBreadcrumbs::ModuleScope::~ModuleScope() {
+  Slot* slot = t_slot;
+  if (!restore_ || slot == nullptr) {
+    return;
+  }
+  std::lock_guard<std::mutex> lock(g_mutex);
+  CopyField(slot->module, previous_.c_str());
+  RenderLocked();
+}
+
 }  // namespace tns

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

@@ -36,8 +36,22 @@ class CrashBreadcrumbs {
   /* Marks a registered runtime as a worker started from `script`. */
   static void SetWorkerScript(int runtimeId, const char* script);
 
-  /* Records the module the calling runtime is about to execute. */
-  static void SetCurrentModule(const char* modulePath);
+  /*
+   * Records the module the calling runtime is executing for the lifetime of
+   * the scope. Module loads nest (`require` inside a module body), so the
+   * enclosing module is restored on destruction, on throw paths included.
+   */
+  class ModuleScope {
+   public:
+    explicit ModuleScope(const char* modulePath);
+    ~ModuleScope();
+    ModuleScope(const ModuleScope&) = delete;
+    ModuleScope& operator=(const ModuleScope&) = delete;
+
+   private:
+    std::string previous_;
+    bool restore_ = false;
+  };
 };
 
 }  // namespace tns

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

@@ -364,7 +364,7 @@ Local<Object> ModuleInternal::LoadImpl(Isolate* isolate, const string& moduleNam
 Local<Object> ModuleInternal::LoadModule(Isolate* isolate, const string& modulePath, const string& moduleCacheKey) {
     string frameName("LoadModule " + modulePath);
     tns::instrumentation::Frame frame(frameName);
-    CrashBreadcrumbs::SetCurrentModule(modulePath.c_str());
+    CrashBreadcrumbs::ModuleScope moduleBreadcrumb(modulePath.c_str());
     Local<Object> result;
 
     auto context = isolate->GetCurrentContext();