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

* fix(runtime): Worker wrappers are strong roots while their thread runs

Replaces the finalizer-resurrection lifetime with reachability: the
wrapper's persistent goes strong once the thread starts and is released
only by the thread-exit notification, posted from the worker's teardown
to the parent's event loop — terminate() initiates the wind-down but
never drops the root early, so no GC can condemn a wrapper whose thread
is still draining. ObjectManager's refuse-and-re-weaken branch stays as
a defensive fallback but is unreachable for workers.

The motivation is a reproduced heap corruption: the patched collector's
kFinalizer resurrection handles ephemeron keys in the atomic pause but
not under concurrent marking — a resurrected WeakMap key whose values
are reachable only through the entry leaves a dangling value slot that
crashes ConcurrentMarkingVisitor::RecordSlot on a later cycle. Strong
lifetime takes Worker off that path entirely; the collector bug is
tracked separately for the other resurrectable wrapper types.

The thread-exit notification also dispatches the internal
nsworkerended event on the Worker object, so node:worker_threads'
Worker shim now emits 'exit' exactly once for self-close as well as
terminate().

Suite: 1663/0 incl. new WorkerLifetimeTests (WeakMap-key repro that
crashed before this change, collectability after terminate and
self-close, delivery to an unreferenced live worker).

* docs(runtime): the listener-bag rule is Node parity plus patch independence, not a live crash

The wrapper-keyed-WeakMap corruption was a collector bug fixed in the
v8-14.9.207.39-6 prebuilts; the rule stays because own-instance state is
Node's design for handler attributes and keeps the builtins off the
resurrection/ephemeron interplay the kFinalizer patch must re-cover on
every V8 upgrade.

* fix(runtime): reach the parent through its event loop, never its isolate, from the worker thread

Worker-thread posts to the parent read the parent isolate's runtime slot and
then the runtime's loop. The parent's destructor terminates its children
without joining them, clears that slot and disposes the isolate, so a child
ending while a worker-parent was torn down could read a freed isolate or a
runtime mid-destruction. The wrapper now captures a weak_ptr to the parent's
loop on the parent's thread at construction; a loop that has shut down drops
the post and an expired pointer means the parent is gone. BackgroundLooper
also reads everything it needs before publishing isDisposed_, which is what
allows a tearing-down parent to delete the wrapper concurrently.

* test(runtime): a terminated worker whose dropped message sentinels a port it owns must end

* fix(runtime): settle terminate() from the thread-ended notification, after exit

The shim emitted exit and resolved terminate() off a microtask, before the
thread was down and before messages and errors the worker had already
queued on the parent's loop had run. Both now follow the runtime's
end-of-worker notification, so nothing the worker sent can arrive after
exit. The code stays 0 for every end, as the cross-runtime suite pins.

* docs(runtime): drop the resurrection rationale the strong Worker root made stale, and name the real patch gate

Author: edusperoni

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