fix(worker): isolate and wrapper lifetime fixes around startup and teardown

A throwing onclose run from the entry script read the worker isolate before it was published and crashed. Terminate() used the worker isolate with no synchronization against the worker thread deleting its runtime. EndWrapperLifetime kept using the wrapper after dispatching nsworkerended, whose listeners can shut the runtime down and delete it.

Author: adrian-niculescu

NativeScript/runtime/DataWrapper.h

@@ -553,12 +553,16 @@ class WorkerWrapper : public BaseDataWrapper {
   void Start(std::shared_ptr<v8::Persistent<v8::Value>> poWorker,
              std::function<v8::Isolate*()> func,
              std::optional<int> qualityOfService = std::nullopt);
-  void CallOnErrorHandlers(v8::TryCatch& tc);
+  // 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.
+  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
   // follows the same web order: the worker scope's `onerror` first, then — only
   // if that did not handle it — the parent's Worker error event.
-  void ReportEntryEvaluationRejection(v8::Local<v8::Context> context,
+  void ReportEntryEvaluationRejection(v8::Isolate* isolate,
+                                      v8::Local<v8::Context> context,
                                       v8::Local<v8::Value> reason);
   void PassUncaughtExceptionFromWorkerToMain(v8::Local<v8::Context> context,
                                              v8::TryCatch& tc,
@@ -629,7 +633,11 @@ class WorkerWrapper : public BaseDataWrapper {
 
  private:
   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_.
   v8::Isolate* workerIsolate_;
+  std::mutex workerIsolateMutex_;
   std::atomic<bool> isRunning_;
   std::atomic<bool> isClosing_;
   std::atomic<bool> isTerminating_;

NativeScript/runtime/Worker.mm

@@ -564,7 +564,7 @@ throw NativeScriptException(
                                       ? info[0]
                                       : Local<Value>(v8::Exception::Error(tns::ToV8String(
                                             iso, "Worker entry module evaluation rejected")));
-            w->ReportEntryEvaluationRejection(ctx, reason);
+            w->ReportEntryEvaluationRejection(iso, ctx, reason);
           };
           Local<v8::Function> onFulfilled;
           Local<v8::Function> onRejected;
@@ -798,7 +798,7 @@ throw NativeScriptException(
     TryCatch tc(isolate);
     success = onCloseFunc->Call(context, v8::Undefined(isolate), 0, args).ToLocal(&result);
     if (!success && tc.HasCaught()) {
-      worker->CallOnErrorHandlers(tc);
+      worker->CallOnErrorHandlers(isolate, tc);
     }
   }
 }

NativeScript/runtime/WorkerWrapper.mm

@@ -120,17 +120,26 @@ static void PostToLoop(const std::shared_ptr<EventLoop>& loop, std::function<voi
 }
 
 void WorkerWrapper::EndWrapperLifetime() {
-  Local<Value> worker =
-      this->poWorker_ != nullptr ? this->poWorker_->Get(this->mainIsolate_) : Local<Value>();
+  // The dispatch below runs listeners, and a listener may shut the runtime
+  // down, whose teardown deletes this wrapper. Everything the dispatch needs is
+  // read first, and the liveness token says afterwards whether `this` is still
+  // there to unroot.
+  Isolate* isolate = this->mainIsolate_;
+  std::shared_ptr<std::atomic<WorkerWrapper*>> selfRef = this->selfRef_;
+  Local<Value> worker = this->poWorker_ != nullptr ? this->poWorker_->Get(isolate) : Local<Value>();
   if (!worker.IsEmpty() && worker->IsObject()) {
-    TryCatch tc(this->mainIsolate_);
-    Worker::EmitEnded(this->mainIsolate_, worker.As<Object>());
+    TryCatch tc(isolate);
+    Worker::EmitEnded(isolate, worker.As<Object>());
     if (tc.HasCaught()) {
       Local<Value> error = tc.Exception();
-      Log(@"%s", tns::ToString(this->mainIsolate_, error).c_str());
-      this->mainIsolate_->ThrowException(error);
+      Log(@"%s", tns::ToString(isolate, error).c_str());
+      isolate->ThrowException(error);
     }
   }
+  if (selfRef->load(std::memory_order_acquire) == nullptr) {
+    // Deleted during the dispatch; that teardown released the Worker object.
+    return;
+  }
   this->UnrootWorkerObject();
 }
 
@@ -181,7 +190,7 @@ static void PostToLoop(const std::shared_ptr<EventLoop>& loop, std::function<voi
     this->onMessage_(this->workerIsolate_, globalTarget, message);
 
     if (tc.HasCaught()) {
-      this->CallOnErrorHandlers(tc);
+      this->CallOnErrorHandlers(this->workerIsolate_, tc);
     }
   }
 
@@ -232,7 +241,11 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptr<Even
         },
         this);
 
-    this->workerIsolate_ = func();
+    Isolate* workerIsolate = func();
+    {
+      std::lock_guard<std::mutex> lock(this->workerIsolateMutex_);
+      this->workerIsolate_ = workerIsolate;
+    }
 
     this->DrainPendingTasks();
 
@@ -242,6 +255,14 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptr<Even
     }
   }
 
+  // Withdrawn before the runtime and its isolate go away below. Terminate()
+  // uses the isolate under this mutex, so a terminate that already read it has
+  // finished with it by the time this returns, and a later one finds null.
+  {
+    std::lock_guard<std::mutex> lock(this->workerIsolateMutex_);
+    this->workerIsolate_ = nullptr;
+  }
+
   // The inspector must be gone before the Runtime (and with it the isolate)
   // is deleted below.
   this->DestroyInspector();
@@ -292,6 +313,9 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptr<Even
   // set terminating to true atomically
   bool wasTerminating = this->isTerminating_.exchange(true);
   if (!wasTerminating) {
+    // Held across the use, not just the read: the worker thread withdraws the
+    // isolate under the same mutex before deleting its runtime.
+    std::unique_lock<std::mutex> isolateLock(this->workerIsolateMutex_);
     if (this->workerIsolate_ != nullptr) {
       // Flagged before the request so a pump that is between iterations sees
       // it on its next check, rather than only once V8 has some JS to
@@ -307,6 +331,7 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptr<Even
       }
       this->workerIsolate_->TerminateExecution();
     }
+    isolateLock.unlock();
     {
       // A worker paused at a breakpoint sits in the inspector's nested pause
       // loop, not in the CFRunLoop — kick it loose so TerminateExecution and
@@ -414,11 +439,10 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptr<Even
   delete client;
 }
 
-void WorkerWrapper::CallOnErrorHandlers(TryCatch& tc) {
+void WorkerWrapper::CallOnErrorHandlers(Isolate* isolate, TryCatch& tc) {
   if (this->isTerminating_) {
     return;
   }
-  Isolate* isolate = this->workerIsolate_;
   Local<Context> context = Caches::Get(isolate)->GetContext();
   Local<Object> global = context->Global();
 
@@ -447,11 +471,11 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptr<Even
   this->PassUncaughtExceptionFromWorkerToMain(context, tc);
 }
 
-void WorkerWrapper::ReportEntryEvaluationRejection(Local<Context> context, Local<Value> reason) {
+void WorkerWrapper::ReportEntryEvaluationRejection(Isolate* isolate, Local<Context> context,
+                                                   Local<Value> reason) {
   if (this->isTerminating_) {
     return;
   }
-  Isolate* isolate = this->workerIsolate_;
   Local<Object> global = context->Global();
 
   Local<Value> onErrorVal;

TestRunner/app/tests/MessagingTests.js

@@ -252,6 +252,16 @@ describe("Messaging runtime edges", function () {
                 }
             };
         });
+
+        it("reports an error onclose threw while the entry script was still running", function (done) {
+            var worker = new Worker("./messaging/throwingOncloseWorker.js");
+            worker.onerror = function (event) {
+                event.preventDefault();
+                expect(event.message).toContain("boom from onclose");
+                worker.terminate();
+                done();
+            };
+        });
     });
 
     describe("AbortSignal handler attribute accounting", function () {

TestRunner/app/tests/messaging/throwingOncloseWorker.js

@@ -0,0 +1,6 @@
+// Closes from inside the entry script, so onclose runs before the entry has
+// finished evaluating.
+onclose = function () {
+    throw new Error("boom from onclose");
+};
+close();