fix(runtime): strong Worker wrapper lifetime while the thread runs

NativeScript/runtime/DataWrapper.h

@@ -20,6 +20,8 @@ class WorkerInspectorClient;
 namespace tns {
 
 class PrimitiveDataWrapper;
+struct ObjectWeakCallbackState;
+class EventLoop;
 
 enum class WrapperType {
   Base = 1 << 0,
@@ -598,6 +600,19 @@ class WorkerWrapper : public BaseDataWrapper {
   inline bool HeapLimitExceeded() const {
     return heapLimitExceeded_.load(std::memory_order_acquire);
   }
+  // The JS Worker object is a GC root from a successful start until the worker
+  // ends, so a running worker is reachable the way a browser's is rather than
+  // depending on its finalizer to keep it. Both of these run on the main
+  // isolate's thread only -- they re-arm that isolate's global handle -- and
+  // the unroot is idempotent, so an end reached by more than one path re-arms
+  // the finalizer once.
+  void RootWorkerObject();
+  void UnrootWorkerObject();
+  // Dispatches the end-of-worker event and unroots. Main isolate's thread,
+  // with the isolate entered and locked by the caller.
+  void EndWrapperLifetime();
+
+  ~WorkerWrapper();
 
   const WrapperType Type();
   const int Id();
@@ -606,6 +621,8 @@ class WorkerWrapper : public BaseDataWrapper {
   const bool IsClosing();
   const int WorkerId();
   const inline v8::Isolate* GetMainIsolate() { return mainIsolate_; }
+  // The only route from the worker thread to the parent: see mainLoop_.
+  std::weak_ptr<EventLoop> MainLoop() const { return mainLoop_; }
   const inline v8::Isolate* GetWorkerIsolate() { return workerIsolate_; }
   const inline void MakeWeak() { isWeak_ = true; }
   const inline bool IsWeak() { return isWeak_; }
@@ -625,6 +642,12 @@ class WorkerWrapper : public BaseDataWrapper {
                      std::shared_ptr<worker::Message>)>
       onMessage_;
   std::shared_ptr<v8::Persistent<v8::Value>> poWorker_;
+  // The parent's event loop, taken on the parent's thread at construction.
+  // Every worker-thread post to the parent goes through it and never through
+  // the parent isolate: the parent runtime may be mid-teardown or its isolate
+  // already disposed when the post runs, whereas a loop that has shut down
+  // drops the post, and an expired pointer means the parent is gone entirely.
+  std::weak_ptr<EventLoop> mainLoop_;
   ConcurrentQueue queue_;
   static std::atomic<int> nextId_;
   int workerId_;
@@ -645,6 +668,15 @@ class WorkerWrapper : public BaseDataWrapper {
   // handle may be created.
   static size_t OnNearHeapLimit(void* data, size_t current_heap_limit,
                                 size_t initial_heap_limit);
+  // Parked while the Worker object is rooted, so the unroot can re-arm the very
+  // finalizer ObjectManager::Register installed. Main isolate's thread only.
+  ObjectWeakCallbackState* weakCallbackState_ = nullptr;
+  bool workerObjectRooted_ = false;
+  // Cleared by the destructor, so a task posted from the worker thread can tell
+  // whether this wrapper still exists once it reaches the main isolate. The
+  // wrapper is only ever destroyed with that isolate locked, which is what the
+  // task takes before reading this.
+  std::shared_ptr<std::atomic<WorkerWrapper*>> selfRef_;
 
   void BackgroundLooper(std::function<v8::Isolate*()> func);
   void DrainPendingTasks();

NativeScript/runtime/ObjectManager.mm

@@ -304,7 +304,16 @@ void DisposeHandle(v8::Isolate* isolate,
     case WrapperType::Worker: {
       WorkerWrapper* worker = static_cast<WorkerWrapper*>(wrapper);
       if (!worker->isDisposed()) {
-        // during final disposal, inform the worker it should delete itself
+        // A running worker's Worker object is rooted (WorkerWrapper::
+        // RootWorkerObject), so a weak callback should not reach a live worker
+        // at all. This refusal stays as the floor under that: re-arming keeps
+        // the wrapper alive for another cycle, which is safe, whereas freeing
+        // it while the thread still posts through it is not. Reaching it is not
+        // free either -- a re-armed handle that is also a weak-collection key
+        // can corrupt the collector's ephemeron bookkeeping -- so it is a
+        // fallback, not a mechanism to rely on.
+        //
+        // During final disposal, inform the worker it should delete itself.
         if (isFinalDisposal) {
           worker->MakeWeak();
         }

NativeScript/runtime/Worker.h

@@ -30,6 +30,13 @@ class Worker {
                         const std::string& message, const std::string& source,
                         const std::string& stackTrace, int lineNumber);
 
+  // Dispatches `nsworkerended` on `receiver` (the Worker object, on the parent
+  // isolate) once the worker's thread has finished. Internal and non-standard:
+  // the web has no end-of-worker event, and the node:worker_threads shim is
+  // what turns this into an 'exit'. A listener that throws leaves the exception
+  // pending for the caller's TryCatch. No-op before InitEvents has run.
+  static void EmitEnded(v8::Isolate* isolate, v8::Local<v8::Object> receiver);
+
   static std::vector<std::string> GlobalFunctions;
 
  private:

NativeScript/runtime/Worker.mm

@@ -22,11 +22,12 @@
 namespace {
 
 // The worker-events builtin's delivery callouts for this isolate. Both message
-// directions share emitMessage; only the receiver differs. emitError is
-// parent-side only.
+// directions share emitMessage; only the receiver differs. emitError and
+// emitEnded are parent-side only.
 struct WorkerEventsState {
   Global<v8::Function> emitMessage;
   Global<v8::Function> emitError;
+  Global<v8::Function> emitEnded;
 };
 
 }  // namespace
@@ -294,10 +295,16 @@ bool ParseResourceLimits(Isolate* isolate, Local<Context> context, Local<Object>
             emitError->IsFunction();
   tns::Assert(success, isolate);
 
+  Local<Value> emitEnded;
+  success = exports->Get(context, tns::ToV8String(isolate, "emitEnded")).ToLocal(&emitEnded) &&
+            emitEnded->IsFunction();
+  tns::Assert(success, isolate);
+
   WorkerEventsState* state = Caches::StateFor<WorkerEventsState>(isolate);
   tns::Assert(state != nullptr, isolate);
   state->emitMessage.Reset(isolate, emitMessage.As<v8::Function>());
   state->emitError.Reset(isolate, emitError.As<v8::Function>());
+  state->emitEnded.Reset(isolate, emitEnded.As<v8::Function>());
 }
 
 void Worker::ConstructorCallback(const FunctionCallbackInfo<Value>& info) {
@@ -596,6 +603,10 @@ throw NativeScriptException(
     Caches::Workers->Insert(worker->Id(), state);
 
     worker->Start(poWorker, func, qos);
+    // The thread is away, so from here the Worker object is a GC root. The
+    // parent's loop cannot run before this returns, so the thread-exit
+    // notification can never overtake this root.
+    worker->RootWorkerObject();
   } catch (NativeScriptException& ex) {
     ex.ReThrowToV8(isolate);
   }
@@ -625,8 +636,8 @@ throw NativeScriptException(
     // Resolved before anything is serialized: serializing a transfer list
     // detaches the caller's buffers, so bailing out afterwards would destroy
     // their contents without ever delivering the message.
-    auto runtime = static_cast<Runtime*>(state->GetIsolate()->GetData(Constants::RUNTIME_SLOT));
-    if (runtime == nullptr) {
+    std::shared_ptr<EventLoop> mainLoop = worker->MainLoop().lock();
+    if (mainLoop == nullptr) {
       return;
     }
 
@@ -642,7 +653,7 @@ throw NativeScriptException(
       return;
     }
 
-    runtime->GetEventLoop()->PostInternal([state, message]() {
+    mainLoop->PostInternal([state, message]() {
       Isolate* isolate = state->GetIsolate();
       v8::Locker locker(isolate);
       Isolate::Scope isolate_scope(isolate);
@@ -752,6 +763,16 @@ throw NativeScriptException(
   return result->BooleanValue(isolate);
 }
 
+void Worker::EmitEnded(Isolate* isolate, Local<Object> receiver) {
+  WorkerEventsState* state = Caches::StateFor<WorkerEventsState>(isolate);
+  if (state == nullptr || state->emitEnded.IsEmpty()) {
+    return;
+  }
+  Local<Context> context = Caches::Get(isolate)->GetContext();
+  Local<Value> result;
+  (void)state->emitEnded.Get(isolate)->Call(context, receiver, 0, nullptr).ToLocal(&result);
+}
+
 void Worker::CloseWorkerCallback(const FunctionCallbackInfo<Value>& info) {
   Isolate* isolate = info.GetIsolate();
   int workerId = Worker::GetWorkerId(isolate, info.This());
@@ -789,6 +810,10 @@ throw NativeScriptException(
 
   WorkerWrapper* worker = static_cast<WorkerWrapper*>(wrapper);
   worker->Terminate();
+  // The root is NOT released here: the wrapper stays strong until the thread
+  // has actually wound down and the thread-exit notification releases it, so
+  // no GC can condemn a wrapper whose thread is still draining — the
+  // ObjectManager resurrection fallback stays unreachable for workers.
 }
 
 void Worker::SetWorkerId(Isolate* isolate, int workerId) {

NativeScript/runtime/WorkerWrapper.mm

@@ -4,6 +4,7 @@
 #include "DataWrapper.h"
 #include "ErrorEvents.h"
 #include "Helpers.h"
+#include "ObjectManager.h"
 #include "Runtime.h"
 #include "RuntimeConfig.h"
 #include "Worker.h"
@@ -24,11 +25,8 @@
 // Posts to the target runtime's internal lane from the worker thread. When
 // async is false, blocks until the entry ran - or until it is destroyed
 // unrun by a shutdown that raced the post, which must release the waiter too.
-static void PostToRuntimeLoop(Runtime* runtime, std::function<void()> fn, bool async) {
-  auto loop = runtime->GetEventLoop();
-  if (loop == nullptr) {
-    return;
-  }
+static void PostToLoop(const std::shared_ptr<EventLoop>& loop, std::function<void()> fn,
+                       bool async) {
   if (async) {
     loop->PostInternal(std::move(fn));
     return;
@@ -57,7 +55,11 @@ static void PostToRuntimeLoop(Runtime* runtime, std::function<void()> fn, bool a
       isWeak_(false),
       messagesEnabled_(false),
       onMessage_(onMessage),
-      workerId_(nextId_.fetch_add(1, std::memory_order_relaxed) + 1) {}
+      mainLoop_(Runtime::GetRuntime(mainIsolate)->GetEventLoop()),
+      workerId_(nextId_.fetch_add(1, std::memory_order_relaxed) + 1),
+      selfRef_(std::make_shared<std::atomic<WorkerWrapper*>>(this)) {}
+
+WorkerWrapper::~WorkerWrapper() { this->selfRef_->store(nullptr, std::memory_order_release); }
 
 const WrapperType WorkerWrapper::Type() { return WrapperType::Worker; }
 
@@ -94,6 +96,44 @@ static void PostToRuntimeLoop(Runtime* runtime, std::function<void()> fn, bool a
   [workers_ addOperation:op];
 }
 
+void WorkerWrapper::RootWorkerObject() {
+  if (this->workerObjectRooted_ || this->poWorker_ == nullptr || this->poWorker_->IsEmpty() ||
+      !this->poWorker_->IsWeak()) {
+    return;
+  }
+  this->weakCallbackState_ = this->poWorker_->ClearWeak<ObjectWeakCallbackState>();
+  this->workerObjectRooted_ = true;
+}
+
+void WorkerWrapper::UnrootWorkerObject() {
+  if (!this->workerObjectRooted_) {
+    return;
+  }
+  this->workerObjectRooted_ = false;
+  ObjectWeakCallbackState* state = this->weakCallbackState_;
+  this->weakCallbackState_ = nullptr;
+  if (state == nullptr || this->poWorker_ == nullptr || this->poWorker_->IsEmpty()) {
+    return;
+  }
+  this->poWorker_->SetWeak(state, ObjectManager::FinalizerCallback,
+                           v8::WeakCallbackType::kFinalizer);
+}
+
+void WorkerWrapper::EndWrapperLifetime() {
+  Local<Value> worker =
+      this->poWorker_ != nullptr ? this->poWorker_->Get(this->mainIsolate_) : Local<Value>();
+  if (!worker.IsEmpty() && worker->IsObject()) {
+    TryCatch tc(this->mainIsolate_);
+    Worker::EmitEnded(this->mainIsolate_, worker.As<Object>());
+    if (tc.HasCaught()) {
+      Local<Value> error = tc.Exception();
+      Log(@"%s", tns::ToString(this->mainIsolate_, error).c_str());
+      this->mainIsolate_->ThrowException(error);
+    }
+  }
+  this->UnrootWorkerObject();
+}
+
 void WorkerWrapper::DrainPendingTasks() {
   // The drain source is armed (and can be signaled by a main-thread
   // PostMessage) BEFORE `workerIsolate_` is assigned in BackgroundLooper, and
@@ -154,6 +194,33 @@ static void PostToRuntimeLoop(Runtime* runtime, std::function<void()> fn, bool a
   }
 }
 
+// Hands the parent isolate the end-of-worker notification: the `nsworkerended`
+// dispatch and the unroot that makes the Worker object collectable again.
+// Takes only primitives plus the liveness token, because the wrapper it acts on
+// may already be gone by the time the parent's loop gets here -- and, when the
+// parent is shutting down, the post is dropped and the parent's teardown
+// cascade owns disposal instead.
+static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptr<EventLoop> mainLoop,
+                                        std::shared_ptr<std::atomic<WorkerWrapper*>> selfRef) {
+  std::shared_ptr<EventLoop> loop = mainLoop.lock();
+  if (loop == nullptr) {
+    return;
+  }
+  PostToLoop(
+      loop,
+      [mainIsolate, selfRef]() {
+        v8::Locker locker(mainIsolate);
+        Isolate::Scope isolate_scope(mainIsolate);
+        HandleScope handle_scope(mainIsolate);
+        WorkerWrapper* self = selfRef->load(std::memory_order_acquire);
+        if (self == nullptr) {
+          return;
+        }
+        self->EndWrapperLifetime();
+      },
+      true);
+}
+
 void WorkerWrapper::BackgroundLooper(std::function<Isolate*()> func) {
   if (!this->isTerminating_) {
     CFRunLoopRef runLoop = CFRunLoopGetCurrent();
@@ -188,20 +255,30 @@ static void PostToRuntimeLoop(Runtime* runtime, std::function<void()> fn, bool a
     this->heapLimitIsolate_ = nullptr;
   }
 
+  // Everything needed below is read first: publishing isDisposed_ is the last
+  // permitted touch of `this`. From that store on, a parent that is tearing
+  // down may delete this wrapper concurrently, and ~Runtime deletes it on this
+  // thread when the parent already handed ownership over.
+  Isolate* mainIsolate = this->mainIsolate_;
+  std::weak_ptr<EventLoop> mainLoop = this->mainLoop_;
+  std::shared_ptr<std::atomic<WorkerWrapper*>> selfRef = this->selfRef_;
+  int workerId = this->workerId_;
   this->isDisposed_ = true;
+
   Runtime* runtime = Runtime::GetCurrentRuntime();
   if (runtime != nullptr) {
     delete runtime;
   } else {
     // Runtime was never created (worker terminated before initialization).
     // The runtime destructor normally handles this cleanup, so do it here.
-    int workerId = this->workerId_;
     bool found;
     auto state = Caches::Workers->Get(workerId, found);
     if (found) {
       Caches::Workers->Remove(workerId);
     }
   }
+
+  PostThreadEndedNotification(mainIsolate, mainLoop, selfRef);
 }
 
 void WorkerWrapper::EnableMessageQueue() {
@@ -480,8 +557,8 @@ static void PostToRuntimeLoop(Runtime* runtime, std::function<void()> fn, bool a
 void WorkerWrapper::ForwardErrorPayloadToMain(const std::string& message, const std::string& source,
                                               const std::string& stackTrace, int lineNumber,
                                               bool async) {
-  auto runtime = static_cast<Runtime*>(mainIsolate_->GetData(Constants::RUNTIME_SLOT));
-  if (runtime == nullptr) {
+  std::shared_ptr<EventLoop> loop = mainLoop_.lock();
+  if (loop == nullptr) {
     return;
   }
   // The task runs later, on the parent's loop, and this wrapper may be gone by
@@ -492,8 +569,8 @@ static void PostToRuntimeLoop(Runtime* runtime, std::function<void()> fn, bool a
   // the Worker object is gone and there is nothing left to report to.
   Isolate* mainIsolate = mainIsolate_;
   std::shared_ptr<Persistent<Value>> poWorker = poWorker_;
-  PostToRuntimeLoop(
-      runtime,
+  PostToLoop(
+      loop,
       [mainIsolate, poWorker, message, source, stackTrace, lineNumber]() {
         v8::Locker locker(mainIsolate);
         Isolate::Scope isolate_scope(mainIsolate);

NativeScript/runtime/js/README.md

@@ -117,10 +117,12 @@ The two extra rules a lazy builtin lives by:
   are whatever user code left behind, so it should not reach for them at all.
 - The per-instance wrappers `defineEventHandler` creates live on the target's
   **own listener bag**, under a private symbol — never in a WeakMap keyed by
-  the target. An ObjectManager-registered object (a `Worker`) can be
-  resurrected by its finalizer while its thread is alive, and a resurrected
-  object's weak-collection entries are already gone, so a WeakMap would hand
-  the revived object a fresh, empty handler map.
+  the target. Own-instance state is Node's own design for handler attributes,
+  and it keeps the builtins independent of the patched collector's handling of
+  resurrected ephemeron keys (`kFinalizer` resurrection interacting with
+  WeakMaps has been a source of collector bugs, and the patch is re-ported on
+  every V8 upgrade — builtins not leaning on it means a re-port mistake breaks
+  app-level tests, not the event system itself).
 - No `import`/`export` — these are classic function bodies, not modules.
 - ESLint (`eslint.config.mjs` at the repo root, run by lint-staged) declares
   `exports`, `require`, `module`, `binding`, `primordials` and the reachable

NativeScript/runtime/js/events.js

@@ -44,12 +44,12 @@ function setListenerErrorReporter(fn) {
 
 // Event name -> handler-attribute wrapper (see defineEventHandler), stored on
 // the target's own listener bag under a symbol so it cannot collide with an
-// event type. Deliberately NOT a WeakMap keyed by the target: a Worker is an
-// ObjectManager-registered object whose finalizer resurrects it while its
-// thread is alive, and a resurrected object's weak-collection entries are
-// already gone. Each wrapper carries a `delta` that the listener count is
-// corrected by: the wrapper occupies one slot in the listener list from its
-// first assignment onwards, but a cleared handler is not a listener.
+// event type. Deliberately NOT a WeakMap keyed by the target: the wrappers
+// live with the target, as Node keeps them, and stay independent of how the
+// collector treats weak-collection entries of objects that native code keeps
+// alive. Each wrapper carries a `delta` that the listener count is corrected
+// by: the wrapper occupies one slot in the listener list from its first
+// assignment onwards, but a cleared handler is not a listener.
 var kHandlers = Symbol("handlers");
 
 function handlersOf(target) {