fix(worker): keep a termination from being re-armed as an error on its way out

A termination that interrupts a CommonJS entry unwinds through the native
require callback, whose catch re-threw the message-only failure onto the
isolate as an ordinary Error: V8 then reported an exception rather than a
termination to RunModule's TryCatch, which built a full error report before
the worker boundary dropped it. ReThrowToV8 now leaves a terminating isolate
alone, since the pending termination is the failure, and RunModule consults
the same three termination signals as the settle pump.

The pumped graph walk ahead of an entry's compile bails on termination but
reports nothing, so LoadESModule went on to the synchronous HTTP loader, which
can block on the network. Both walks are now followed by a termination check.

Also refreshes the drain comment that described the pre-publication isolate
window, widens the startup-sweep spec's spread so its rounds reach the settle
pump, and corrects the resourceLimits RangeError wording.

Author: edusperoni

NativeScript/runtime/ModuleInternal.mm

@@ -258,7 +258,10 @@ static bool IsHttpModulePath(const std::string& path) {
       moduleNamespace =
           ModuleInternal::LoadESModule(isolate, path, BootEntryEvaluationOptions(isHttpModule));
     } catch (const NativeScriptException& ex) {
-      if (RuntimeConfig.IsDebug) {
+      Runtime* runtime = Runtime::GetRuntime(isolate);
+      bool terminating = isolate->IsExecutionTerminating() ||
+                         (runtime != nullptr && runtime->IsTerminationRequested());
+      if (RuntimeConfig.IsDebug && !terminating) {
         Log(@"***** JavaScript exception occurred *****");
         Log(@"Error loading ES module: %s", path.c_str());
         Log(@"Exception: %s", ex.getMessage().c_str());
@@ -294,8 +297,12 @@ throw NativeScriptException(
 
   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()) {
+    // naming it as the failure keeps it from being reported as one. All three
+    // signals, as in the settle pump: a native frame between here and the
+    // interrupted JS may have swallowed the sentinel on its way out.
+    Runtime* runtime = Runtime::GetRuntime(isolate);
+    if (tc.HasTerminated() || isolate->IsExecutionTerminating() ||
+        (runtime != nullptr && runtime->IsTerminationRequested())) {
       throw NativeScriptException("Module evaluation interrupted by isolate termination: " + path);
     }
     if (RuntimeConfig.IsDebug) {
@@ -1461,6 +1468,21 @@ throw NativeScriptException("Module evaluation interrupted by isolate terminatio
   return MaybeLocal<Promise>();
 }
 
+// A pumped graph walk bails on a termination request but reports nothing;
+// what follows it would compile, fetch synchronously or evaluate on an
+// isolate that must not run anything more.
+static void ThrowIfLoadInterruptedByTermination(Isolate* isolate,
+                                                const std::string& canonicalPath) {
+  Runtime* runtime = Runtime::GetRuntime(isolate);
+  if (!isolate->IsExecutionTerminating() &&
+      (runtime == nullptr || !runtime->IsTerminationRequested())) {
+    return;
+  }
+  LogEsmPhase(canonicalPath, "load", "terminated");
+  throw NativeScriptException("Module evaluation interrupted by isolate termination: " +
+                              canonicalPath);
+}
+
 Local<Value> ModuleInternal::LoadESModule(Isolate* isolate, const std::string& path,
                                           const ModuleEvaluationOptions& options) {
   bool isHttpModule = IsHttpModulePath(path);
@@ -1540,6 +1562,7 @@ throw NativeScriptException("Module evaluation interrupted by isolate terminatio
     // registry hit and instantiation resolves as pure lookup. On timeout or
     // partial coverage the legacy synchronous path still owns correctness.
     RunModuleGraphLoadPumped(isolate, context, requestPath, kModuleEvaluateDeadlineSeconds);
+    ThrowIfLoadInterruptedByTermination(isolate, canonicalPath);
     MaybeLocal<Module> maybeMod = LoadHttpModuleForUrl(isolate, context, requestPath);
     if (!maybeMod.ToLocal(&module)) {
       logPhase("compile", "fail", "http-loader");
@@ -1563,6 +1586,7 @@ throw NativeScriptException("Module evaluation interrupted by isolate terminatio
     // async fetch, so it legitimately waits here and then evaluates
     // synchronously.
     RunModuleGraphLoadPumped(isolate, context, canonicalPath, kModuleEvaluateDeadlineSeconds);
+    ThrowIfLoadInterruptedByTermination(isolate, canonicalPath);
     auto walkedIt = registry.find(canonicalPath);
     if (walkedIt != registry.end()) {
       Local<Module> walked = walkedIt->second.Get(isolate);

NativeScript/runtime/NativeScriptException.mm

@@ -714,6 +714,15 @@ static bool GiveWorkerOnErrorAChance(Isolate* isolate, Local<Context> context, L
 }
 
 void NativeScriptException::ReThrowToV8(Isolate* isolate) {
+  // An isolate that is terminating keeps its termination pending until the JS
+  // frames below unwind; throwing on it would replace that with an ordinary
+  // error, and building the error allocates on a heap that may have just hit
+  // its cap. The failure being re-armed here is the termination itself.
+  Runtime* runtime = Runtime::GetRuntime(isolate);
+  if (isolate->IsExecutionTerminating() ||
+      (runtime != nullptr && runtime->IsTerminationRequested())) {
+    return;
+  }
   @try {
     // The Isolate::Scope here is necessary because the Exception::Error method internally relies on
     // the Isolate::GetCurrent method which might return null if we do not use the proper scope

NativeScript/runtime/WorkerWrapper.mm

@@ -154,12 +154,14 @@ static void PostToLoop(const std::shared_ptr<EventLoop>& loop, std::function<voi
 
 void WorkerWrapper::DrainPendingTasks() {
   // The drain source is armed (and can be signaled by a main-thread
-  // PostMessage) BEFORE `workerIsolate_` is assigned in BackgroundLooper, and
-  // worker creation spins this runloop inside that window: the entry's module
-  // graph load pumps it while waiting on fetches, which fires this source with
-  // a null isolate (crash in v8::Locker::Initialize). Bail until the isolate
-  // exists — the messages stay queued and the explicit DrainPendingTasks()
-  // call right after isolate creation delivers them.
+  // PostMessage) before the worker's runtime exists, and the entry's module
+  // graph load pumps this runloop while waiting on fetches. A fire that
+  // arrives before the isolate is published has nothing to lock (a null
+  // isolate crashes v8::Locker::Initialize); one that arrives during the entry
+  // locks and then leaves at the messagesEnabled_ gate below, since the queue
+  // is only enabled once the entry has finished evaluating. Either way the
+  // messages stay queued for the explicit DrainPendingTasks() call after the
+  // startup function returns.
   if (this->workerIsolate_ == nullptr) {
     return;
   }

TestRunner/app/tests/WorkerTerminateTests.js

@@ -15,8 +15,7 @@ describe("Worker terminate during entry evaluation", function () {
     });
 
     // Resolves once the worker's thread has ended; rejects when it has not
-    // within `limitMs`, which is the old behaviour for an entry that never
-    // returns.
+    // within `limitMs`.
     function waitForEnd(worker, limitMs) {
         return new Promise(function (resolve, reject) {
             var timer = setTimeout(function () {
@@ -68,9 +67,11 @@ describe("Worker terminate during entry evaluation", function () {
         };
     });
 
-    // Round i terminates i milliseconds after construction, which lands
-    // anywhere from before the thread has started, through runtime setup, to
-    // inside the entry's settle pump.
+    // Round i terminates 25·i milliseconds after construction. Runtime setup
+    // takes a few hundred milliseconds on a simulator and the local entry's
+    // settle pump lasts one second after it, so the rounds land anywhere from
+    // before the thread has started, through runtime setup, to inside the
+    // pump.
     it("ends a worker terminated at any point of its startup", function (done) {
         var ROUNDS = 16;
         (function round(i) {
@@ -84,7 +85,7 @@ describe("Worker terminate during entry evaluation", function () {
             if (i === 0) {
                 worker.terminate();
             } else {
-                setTimeout(function () { worker.terminate(); }, i);
+                setTimeout(function () { worker.terminate(); }, i * 25);
             }
             ended.then(function () { round(i + 1); }, settle(done));
         })(0);

docs/worker-threads.md

@@ -307,8 +307,9 @@ new Worker("./w.js", {
 
 Node's `stackSizeMb` and `codeRangeSizeMb` are ignored like any other unknown
 key. A `resourceLimits` that is not an object, or a key that is not a number,
-throws a `TypeError`; a non-finite, non-positive or oversized value throws a
-`RangeError`, as does a fractional or out-of-range `jsDispatchTableSizeMb`.
+throws a `TypeError`; a non-finite value, one worth less than a byte, or one
+too large to hold in bytes throws a `RangeError`, as does a fractional or
+out-of-range `jsDispatchTableSizeMb`.
 `resourceLimits: null` is the same as omitting it.
 
 Exhausting a worker's heap does not take the process down. Every worker