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

NativeScript/runtime/DataWrapper.h

@@ -19,6 +19,7 @@ class WorkerInspectorClient;
 namespace tns {
 
 class PrimitiveDataWrapper;
+struct ObjectWeakCallbackState;
 
 enum class WrapperType {
   Base = 1 << 0,
@@ -576,6 +577,20 @@ class WorkerWrapper : public BaseDataWrapper {
   void Close();
   void Terminate();
 
+  // 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, since terminate() and the thread-exit
+  // notification can both reach it.
+  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();
   const inline bool isDisposed() { return isDisposed_; }
@@ -609,6 +624,15 @@ class WorkerWrapper : public BaseDataWrapper {
   // thread) and DestroyInspector() (worker thread) agree on liveness.
   v8_inspector::WorkerInspectorClient* inspector_ = nullptr;
   std::mutex inspectorMutex_;
+  // 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

@@ -278,7 +278,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

@@ -18,11 +18,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
@@ -80,10 +81,16 @@
             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) {
@@ -351,6 +358,10 @@ throw NativeScriptException(
     });
 
     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();
 
     std::shared_ptr<Caches::WorkerState> state =
         std::make_shared<Caches::WorkerState>(isolate, poWorker, worker);
@@ -512,6 +523,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());
@@ -549,6 +570,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

@@ -3,6 +3,7 @@
 #include "Constants.h"
 #include "DataWrapper.h"
 #include "Helpers.h"
+#include "ObjectManager.h"
 #include "Runtime.h"
 #include "RuntimeConfig.h"
 #include "Worker.h"
@@ -55,7 +56,10 @@ static void PostToRuntimeLoop(Runtime* runtime, std::function<void()> fn, bool a
       isDisposed_(false),
       isWeak_(false),
       messagesEnabled_(false),
-      onMessage_(onMessage) {}
+      onMessage_(onMessage),
+      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; }
 
@@ -91,6 +95,44 @@ static void PostToRuntimeLoop(Runtime* runtime, std::function<void()> fn, bool a
   this->isRunning_ = true;
 }
 
+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
@@ -151,6 +193,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::shared_ptr<std::atomic<WorkerWrapper*>> selfRef) {
+  auto runtime = static_cast<Runtime*>(mainIsolate->GetData(Constants::RUNTIME_SLOT));
+  if (runtime == nullptr) {
+    return;
+  }
+  PostToRuntimeLoop(
+      runtime,
+      [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();
@@ -177,6 +246,13 @@ static void PostToRuntimeLoop(Runtime* runtime, std::function<void()> fn, bool a
   this->DestroyInspector();
 
   this->isDisposed_ = true;
+
+  // Read before the Runtime goes: its destructor deletes this wrapper when the
+  // parent isolate already tore down and handed ownership over, so nothing
+  // below may touch `this`.
+  Isolate* mainIsolate = this->mainIsolate_;
+  std::shared_ptr<std::atomic<WorkerWrapper*>> selfRef = this->selfRef_;
+
   Runtime* runtime = Runtime::GetCurrentRuntime();
   if (runtime != nullptr) {
     delete runtime;
@@ -190,6 +266,8 @@ static void PostToRuntimeLoop(Runtime* runtime, std::function<void()> fn, bool a
       Caches::Workers->Remove(workerId);
     }
   }
+
+  PostThreadEndedNotification(mainIsolate, selfRef);
 }
 
 void WorkerWrapper::EnableMessageQueue() {

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/node-worker-threads.js

@@ -175,11 +175,31 @@ class Worker extends WorkerEmitter {
     worker.onerror = function (error) {
       self.emit("error", error);
     };
+    // The runtime's end-of-worker event, which a worker's own close() reaches
+    // as much as a terminate() does — so 'exit' is not the terminate()-only
+    // signal it used to be.
+    FunctionPrototypeCall(
+      addEventListener,
+      worker,
+      "nsworkerended",
+      function () {
+        self.#reportExit();
+      }
+    );
     soon(function () {
       self.emit("online", undefined);
     });
   }
 
+  // Both ends of a worker report through here, and Node emits 'exit' once.
+  #reportExit() {
+    if (this.#exited) {
+      return;
+    }
+    this.#exited = true;
+    this.emit("exit", 0);
+  }
+
   postMessage(value, transfer) {
     this.#worker.postMessage(value, transfer);
   }
@@ -188,10 +208,7 @@ class Worker extends WorkerEmitter {
     this.#worker.terminate();
     const self = this;
     return PromisePrototypeThen(PromiseResolve(), function () {
-      if (!self.#exited) {
-        self.#exited = true;
-        self.emit("exit", 0);
-      }
+      self.#reportExit();
       return 0;
     });
   }

NativeScript/runtime/js/worker-events.js

@@ -12,6 +12,7 @@
 const { ObjectDefineProperty, ObjectSetPrototypeOf } = primordials;
 
 const {
+  Event,
   EventTarget,
   defineEventHandler,
   dispatchEventRethrowing,
@@ -73,6 +74,15 @@ function emitError(message, filename, lineno, stackTrace) {
   return event.defaultPrevented;
 }
 
+// The parent-side end-of-worker callout, invoked by native with the Worker
+// object as `this` once the worker's thread has finished — its own close() as
+// much as a terminate(). `nsworkerended` is 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'.
+function emitEnded() {
+  dispatchEventRethrowing(this, new Event("nsworkerended"));
+}
+
 ObjectSetPrototypeOf(g.Worker.prototype, EventTarget.prototype);
 defineEventHandler(g.Worker.prototype, "message");
 defineEventHandler(g.Worker.prototype, "messageerror");
@@ -99,4 +109,4 @@ for (const name of ["onmessage", "onmessageerror"]) {
   });
 }
 
-module.exports = { emitMessage, emitError };
+module.exports = { emitMessage, emitError, emitEnded };