fix(worker): terminate() reaches a worker still inside its entry script

The worker's isolate was published to its wrapper only after the startup
function returned, i.e. after the entry script had finished evaluating, so
Terminate() found nothing to interrupt for a worker still in its entry: a
synchronous loop there ran forever, and a parked top-level await ended
only when the loader's settle deadline expired. Node and the web both
interrupt the running entry.

The isolate is now published right after the worker runtime is
initialized, before the entry runs, and the startup function honors a
terminate() that landed earlier by checking the flag before any app code.
A termination that lands inside the entry unwinds RunModule, which no
longer rethrows or reports on a terminating isolate, and the thread goes
straight to its existing teardown path. The module runner and the
exception constructor name a caught termination for what it is instead of
formatting the TryCatch, and the settle pump checks for one after its loop
too, so a termination is never reported as a timeout or as an entry
rejection.

Also documents the real resourceLimits and ios.priority Worker options in
docs/worker-threads.md, whose node:worker_threads table claimed the runtime
imposed no limits.

Suite 1735/0, also under AddressSanitizer.

Fixes #445

Author: edusperoni

NativeScript/runtime/DataWrapper.h

@@ -550,12 +550,20 @@ class WorkerWrapper : public BaseDataWrapper {
   void CreateInspector(v8::Isolate* isolate, const std::string& scriptPath);
   void DestroyInspector();
 
+  // `func` runs on the worker thread: it creates the worker's runtime, hands
+  // the isolate to PublishIsolate once the runtime is initialized, and runs
+  // the entry script.
   void Start(std::shared_ptr<v8::Persistent<v8::Value>> poWorker,
-             std::function<v8::Isolate*()> func,
+             std::function<void()> func,
              std::optional<int> qualityOfService = std::nullopt);
+  // Worker thread, once the runtime is initialized and before the entry script
+  // runs. From here Terminate() reaches V8: it interrupts whatever JS the
+  // worker runs, the entry script included, and flags the module pumps so a
+  // parked graph stops waiting. A terminate() that landed earlier is honored by
+  // the caller checking IsTerminating() right after this, before any app code.
+  void PublishIsolate(v8::Isolate* isolate);
   // Both reporters take the isolate from their caller, which is running on
-  // it: they are reachable while the entry script is still evaluating, before
-  // workerIsolate_ is published.
+  // it: they are reachable while the entry script is still evaluating.
   void CallOnErrorHandlers(v8::Isolate* isolate, v8::TryCatch& tc);
   // Reports a rejected entry-evaluation promise. A rejection carries a reason
   // rather than a TryCatch, so it cannot go through CallOnErrorHandlers, but it
@@ -599,11 +607,6 @@ class WorkerWrapper : public BaseDataWrapper {
   // used to name the cap in the message forwarded to the parent.
   void WatchHeapLimit(v8::Isolate* isolate, const std::string& scriptPath,
                       std::optional<size_t> maxOldGenerationSizeBytes);
-  // Whether the heap cap was hit. The worker startup path checks this to stop
-  // before running anything else in an isolate V8 is terminating.
-  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
@@ -622,6 +625,9 @@ class WorkerWrapper : public BaseDataWrapper {
   const int Id();
   const bool IsRunning();
   const bool IsClosing();
+  // Set by Terminate() from any thread, by the near-heap-limit callback, and by
+  // the worker thread itself once a close() takes effect. One-way.
+  const bool IsTerminating();
   const int WorkerId();
   const inline v8::Isolate* GetMainIsolate() { return mainIsolate_; }
   // The only route from the worker thread to the parent: see mainLoop_.
@@ -650,9 +656,13 @@ class WorkerWrapper : public BaseDataWrapper {
   enum class Holders : uint8_t { Parent, Both, WorkerThread };
 
   v8::Isolate* mainIsolate_;
-  // Written by the worker thread only: published once the worker's startup
-  // function returns, withdrawn before the worker's runtime is deleted. Any
-  // other thread reads and uses it under workerIsolateMutex_.
+  // Written by the worker thread only: published by PublishIsolate once the
+  // worker's runtime is initialized and before its entry script runs,
+  // withdrawn before the runtime is deleted. Any other thread reads and uses
+  // it under workerIsolateMutex_. Null while the runtime is being set up, so a
+  // Terminate() in that window cannot interrupt the builtins Runtime::Init
+  // evaluates; the startup function checks the terminating flag right after
+  // publishing instead.
   v8::Isolate* workerIsolate_;
   std::mutex workerIsolateMutex_;
   std::atomic<bool> isRunning_;
@@ -682,10 +692,11 @@ class WorkerWrapper : public BaseDataWrapper {
   // thread) and DestroyInspector() (worker thread) agree on liveness.
   v8_inspector::WorkerInspectorClient* inspector_ = nullptr;
   std::mutex inspectorMutex_;
-  // The worker isolate as seen from the heap-limit callback. Separate from
-  // workerIsolate_, which BackgroundLooper only publishes once the entry script
-  // has finished evaluating — the point at which a heap cap is most likely to
-  // be hit is inside that entry.
+  // The isolate the near-heap-limit callback is armed on. Worker thread only.
+  // Kept apart from workerIsolate_ because the callback is armed before the
+  // isolate is published and can fire during Runtime::Init, and because
+  // removing a callback V8 never had registered is fatal: non-null here means
+  // exactly "armed, remove at teardown".
   v8::Isolate* heapLimitIsolate_ = nullptr;
   std::string heapLimitMessage_;
   std::string heapLimitSource_;
@@ -707,7 +718,7 @@ class WorkerWrapper : public BaseDataWrapper {
   // task would run on.
   std::shared_ptr<std::atomic<WorkerWrapper*>> selfRef_;
 
-  void BackgroundLooper(std::function<v8::Isolate*()> func);
+  void BackgroundLooper(std::function<void()> func);
   void DrainPendingTasks();
   void ForwardErrorPayloadToMain(const std::string& message,
                                  const std::string& source,

NativeScript/runtime/ModuleInternal.mm

@@ -293,6 +293,11 @@ throw NativeScriptException(
   success = requireFunc->Call(context, globalObject, 1, args).ToLocal(&result);
 
   if (!success || tc.HasCaught()) {
+    // A termination is caught like an exception but carries no error value;
+    // naming it as the failure keeps it from being reported as one.
+    if (tc.HasTerminated()) {
+      throw NativeScriptException("Module evaluation interrupted by isolate termination: " + path);
+    }
     if (RuntimeConfig.IsDebug) {
       Log(@"***** JavaScript exception occurred *****");
       Log(@"Error in require() call:");
@@ -882,6 +887,10 @@ throw NativeScriptException(isolate,
         moduleFunc->Call(context, thiz, sizeof(requireArgs) / sizeof(Local<Value>), requireArgs)
             .ToLocal(&result);
     if (!success || tc.HasCaught()) {
+      if (tc.HasTerminated()) {
+        throw NativeScriptException("Module evaluation interrupted by isolate termination: " +
+                                    modulePath);
+      }
       throw NativeScriptException(isolate, tc, "Error calling module function");
     }
   }
@@ -1277,6 +1286,13 @@ throw NativeScriptException(
   Local<Value> result;
   if (!module->Evaluate(context).ToLocal(&result)) {
     RemoveModuleFromRegistry(isolate, canonicalPath);
+    // Same rule as the pump below: a termination outranks any failure detail,
+    // and reading the TryCatch as an error would run JS on the dying isolate.
+    if (tcEval.HasTerminated() || isolate->IsExecutionTerminating()) {
+      LogEsmPhase(canonicalPath, "evaluate", "terminated");
+      throw NativeScriptException("Module evaluation interrupted by isolate termination: " +
+                                  canonicalPath);
+    }
     const char* classification = "unknown";
     if (tcEval.HasCaught()) {
       Local<Message> msg = tcEval.Message();
@@ -1364,32 +1380,38 @@ throw NativeScriptException("ES module " + canonicalPath +
   NSDate* deadline = [NSDate dateWithTimeIntervalSinceNow:options.deadlineSeconds];
   bool settled = false;
 
+  // Termination outranks a settled result and a timeout alike. Handing back a
+  // namespace would send the caller on to run more JS — enabling a queue,
+  // draining messages — on an isolate V8 has already been told to stop, and
+  // reporting a timeout would name a reason that is not the real one.
+  //
+  // Three signals are consulted: V8 only reports a termination it has already
+  // materialized, which needs JS to run, and a graph parked on a promise
+  // nothing settles never gives it any; one that materialized inside a
+  // checkpoint lands in promiseTc. Message-only exception: building a V8
+  // error on a terminating isolate is not allowed.
+  auto throwIfTerminating = [&]() {
+    if (!promiseTc.HasTerminated() && !isolate->IsExecutionTerminating() &&
+        (runtime == nullptr || !runtime->IsTerminationRequested())) {
+      return;
+    }
+    LogEsmPhase(canonicalPath, "evaluate", "terminated");
+    // Probed, not consumed: only a still-pending promise leaves a
+    // half-evaluated module in the registry. One that already settled is
+    // complete, and evicting it would throw away a good entry for no reason
+    // — the result simply goes unused.
+    if (promise->State() == Promise::kPending) {
+      RemoveModuleFromRegistry(isolate, canonicalPath);
+    }
+    throw NativeScriptException("Module evaluation interrupted by isolate termination: " +
+                                canonicalPath);
+  };
+
   // State is checked before the first pump: a synchronous graph's
   // evaluation promise is already settled when Evaluate() returns, so it
   // exits here without paying for a runloop slice.
   while (!promiseTc.HasCaught()) {
-    // Termination outranks a settled result. Handing back a namespace here
-    // would send the caller on to run more JS — enabling a queue, draining
-    // messages — on an isolate V8 has already been told to stop, so a
-    // termination seen at the loop head always throws, settled or not.
-    //
-    // Both signals are consulted: V8 only reports a termination it has already
-    // materialized, which needs JS to run, and a graph parked on a promise
-    // nothing settles never gives it any. Message-only exception: building a
-    // V8 error on a terminating isolate is not allowed.
-    if (isolate->IsExecutionTerminating() ||
-        (runtime != nullptr && runtime->IsTerminationRequested())) {
-      LogEsmPhase(canonicalPath, "evaluate", "terminated");
-      // Probed, not consumed: only a still-pending promise leaves a
-      // half-evaluated module in the registry. One that already settled is
-      // complete, and evicting it would throw away a good entry for no reason
-      // — the result simply goes unused.
-      if (promise->State() == Promise::kPending) {
-        RemoveModuleFromRegistry(isolate, canonicalPath);
-      }
-      throw NativeScriptException("Module evaluation interrupted by isolate termination: " +
-                                  canonicalPath);
-    }
+    throwIfTerminating();
 
     Promise::PromiseState state = promise->State();
     if (state != Promise::kPending) {
@@ -1411,6 +1433,11 @@ throw NativeScriptException("Module evaluation interrupted by isolate terminatio
     }
   }
 
+  // The loop leaves through its condition when a pump materializes the
+  // termination, and through the deadline when the request arrived during the
+  // final slice.
+  throwIfTerminating();
+
   if (!settled && promise->State() == Promise::kPending) {
     LogEsmPhase(canonicalPath, "evaluate", "promise-timeout");
     if (options.timeoutBehavior == ModuleEvaluationOptions::TimeoutBehavior::kThrow) {

NativeScript/runtime/NativeScriptException.mm

@@ -85,6 +85,15 @@ static void ConsiderStackCandidate(PendingErrorDisplay& state, v8::Isolate* isol
 
 NativeScriptException::NativeScriptException(Isolate* isolate, TryCatch& tc,
                                              const std::string& message) {
+  // A caught termination has no exception value, message or stack to read:
+  // V8 hands back a sentinel, and formatting it would run JS on an isolate
+  // that must not run any.
+  if (tc.HasTerminated()) {
+    this->javascriptException_ = nullptr;
+    this->message_ = message;
+    this->name_ = "NativeScriptException";
+    return;
+  }
   Local<Value> error = tc.Exception();
   this->javascriptException_ = new Persistent<Value>(isolate, tc.Exception());
   this->message_ = GetErrorMessage(isolate, error, message);
@@ -333,7 +342,7 @@ static void ScheduleDeferredThrow(Isolate* isolate, NSException* e) {
   if (error->IsObject()) {
     auto errObject = error.As<Object>();
     auto fullMessageString = tns::ToV8String(isolate, "fullMessage");
-    if (errObject->HasOwnProperty(context, fullMessageString).ToChecked()) {
+    if (errObject->HasOwnProperty(context, fullMessageString).FromMaybe(false)) {
       // check if we have a "fullMessage" on the error, and log that instead - since it includes
       // more info about the exception.
       v8::Local<v8::Value> fullMessage_;
@@ -765,7 +774,7 @@ static bool GiveWorkerOnErrorAChance(Isolate* isolate, Local<Context> context, L
   std::string errMessage;
   bool hasFullErrorMessage = false;
   auto v8FullMessage = tns::ToV8String(isolate, "fullMessage");
-  if (error->IsObject() && error.As<Object>()->Has(context, v8FullMessage).ToChecked()) {
+  if (error->IsObject() && error.As<Object>()->Has(context, v8FullMessage).FromMaybe(false)) {
     hasFullErrorMessage = true;
     Local<Value> errMsgVal;
     bool success = error.As<Object>()->Get(context, v8FullMessage).ToLocal(&errMsgVal);

NativeScript/runtime/Worker.mm

@@ -433,7 +433,7 @@ throw NativeScriptException(
     // vocabulary updates).
     tns::LoaderVocabulary inheritedVocabulary = tns::CaptureLoaderVocabulary(isolate);
 
-    std::function<Isolate*()> func([worker, workerPath, inheritedVocabulary, resourceLimits]() {
+    std::function<void()> func([worker, workerPath, inheritedVocabulary, resourceLimits]() {
       // Name the looper thread after its entry script so a crash report
       // identifies which worker died instead of an anonymous NSOperationQueue
       // thread. Darwin caps thread names at 63 bytes; keep the basename only.
@@ -477,6 +477,14 @@ throw NativeScriptException(
       // the worker's scripts are visible to the debugger from the start.
       worker->CreateInspector(isolate, resolvedPath);
 
+      // From here terminate() interrupts this isolate. A terminate() that
+      // landed before now had nothing to interrupt, so it is honored here,
+      // before any app code runs: the thread goes straight to teardown.
+      worker->PublishIsolate(isolate);
+      if (worker->IsTerminating()) {
+        return;
+      }
+
       TryCatch tc(isolate);
 
       // If the script can be determined missing up-front, report it through
@@ -491,7 +499,7 @@ throw NativeScriptException(
           worker->PassUncaughtExceptionFromWorkerToMain(
               "Worker script does not exist: " + resolvedPath, resolvedPath, "", 1, true);
           worker->Terminate();
-          return isolate;
+          return;
         }
       }
 
@@ -500,16 +508,22 @@ throw NativeScriptException(
       } catch (NativeScriptException& ex) {
         // Re-arm the failure as the pending V8 exception (the original JS
         // error when one was captured) so the tc.HasCaught() path below
-        // routes it to worker.onerror with full detail.
-        Isolate::Scope isolate_scope(isolate);
-        HandleScope handle_scope(isolate);
-        ex.ReThrowToV8(isolate);
+        // routes it to worker.onerror with full detail. Not on an isolate
+        // that is terminating: the failure then is the termination itself,
+        // and throwing on such an isolate is not allowed.
+        if (!worker->IsTerminating()) {
+          Isolate::Scope isolate_scope(isolate);
+          HandleScope handle_scope(isolate);
+          ex.ReThrowToV8(isolate);
+        }
       }
 
-      // The near-heap-limit callback has already reported to the parent and
-      // asked V8 to terminate this isolate; everything below would run JS on it.
-      if (worker->HeapLimitExceeded()) {
-        return isolate;
+      // The entry was cut short — by terminate(), or by the near-heap-limit
+      // callback, which has already reported to the parent and asked V8 to
+      // terminate this isolate. Everything below would run JS on it, and a
+      // terminated worker reports no error.
+      if (worker->IsTerminating()) {
+        return;
       }
 
       // WHATWG parity: enable the implicit port's message queue once the
@@ -590,8 +604,6 @@ throw NativeScriptException(
         worker->PassUncaughtExceptionFromWorkerToMain(context, tc, true);
         worker->Terminate();
       }
-
-      return isolate;
     });
 
     // The registry entry has to exist before the worker can run: the worker