fix(worker): decide who deletes a WorkerWrapper with one atomic state

NativeScript/runtime/DataWrapper.h

@@ -616,25 +616,45 @@ class WorkerWrapper : public BaseDataWrapper {
 
   const WrapperType Type();
   const int Id();
-  const inline bool isDisposed() { return isDisposed_; }
   const bool IsRunning();
   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_; }
+
+  // Deletion is decided by who still holds the wrapper. The parent's Worker
+  // object holds it from construction; Start() adds the worker thread. Each
+  // side lets go exactly once, with a compare-and-swap out of Holders::Both,
+  // so exactly one of them finds itself the last holder and deletes.
+  //
+  // Parent's thread, during the final disposal of the parent isolate. Returns
+  // true when the worker thread is already done with the wrapper (or never
+  // had it), which leaves the delete to the caller; false when the worker
+  // thread still uses it and deletes it when it ends.
+  bool ReleaseFromParent();
+  // Parent's thread, from the Worker object's finalizer. True when the worker
+  // thread is done with the wrapper (or never had it), so the caller may
+  // delete; nothing changes when false.
+  bool HeldByParentOnly() const;
+  // Worker thread, as its last touch of the wrapper and after it removed the
+  // wrapper's Caches::Workers entry. Deletes the wrapper when the parent has
+  // already let go; otherwise the parent may delete it from here on.
+  void ReleaseFromWorkerThread();
 
  private:
+  enum class Holders : uint8_t { Parent, Both, WorkerThread };
+
   v8::Isolate* mainIsolate_;
   v8::Isolate* workerIsolate_;
   std::atomic<bool> isRunning_;
   std::atomic<bool> isClosing_;
   std::atomic<bool> isTerminating_;
+  // The worker thread has started tearing down. Worker thread only; who
+  // deletes the wrapper is holders_, not this.
   std::atomic<bool> isDisposed_;
-  std::atomic<bool> isWeak_;
+  std::atomic<Holders> holders_;
   // False until the entry script has finished evaluating (EnableMessageQueue);
   // DrainPendingTasks leaves the queue untouched while disabled.
   std::atomic<bool> messagesEnabled_;
@@ -674,8 +694,10 @@ class WorkerWrapper : public BaseDataWrapper {
   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.
+  // parent deletes the wrapper with that isolate locked, which is what the task
+  // takes before reading this. The worker thread only deletes it after the
+  // parent's final disposal, which follows the shutdown of the loop such a
+  // task would run on.
   std::shared_ptr<std::atomic<WorkerWrapper*>> selfRef_;
 
   void BackgroundLooper(std::function<v8::Isolate*()> func);

NativeScript/runtime/ObjectManager.mm

@@ -303,20 +303,21 @@ void DisposeHandle(v8::Isolate* isolate,
     }
     case WrapperType::Worker: {
       WorkerWrapper* worker = static_cast<WorkerWrapper*>(wrapper);
-      if (!worker->isDisposed()) {
-        // 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();
-        }
+      // Final disposal lets go of the wrapper for good, and only deletes it
+      // when the worker thread is done with it; otherwise that thread deletes
+      // it when it ends.
+      //
+      // A weak callback deletes the wrapper only when the worker thread is
+      // done with it, and changes nothing otherwise. A running worker's Worker
+      // object is rooted (WorkerWrapper::RootWorkerObject), so a weak callback
+      // should not reach a live worker at all. The 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.
+      bool lastHolder = isFinalDisposal ? worker->ReleaseFromParent() : worker->HeldByParentOnly();
+      if (!lastHolder) {
         return false;
       }
       break;

NativeScript/runtime/Runtime.mm

@@ -292,14 +292,9 @@ void DisposeIsolateWhenPossible(Isolate* isolate) {
     messaging::CloseAllPorts(isolate_);
 
     if (IsRuntimeWorker()) {
-      std::shared_ptr<Caches::WorkerState> workerState = Caches::Workers->Get(this->workerId_);
-      WorkerWrapper* currentWorker =
-          workerState == nullptr ? nullptr : static_cast<WorkerWrapper*>(workerState->UserData());
+      // Only the registry entry: the wrapper outlives this runtime, and the
+      // worker thread lets go of it once this destructor has returned.
       Caches::Workers->Remove(this->workerId_);
-      // if the parent isolate is dead then deleting the wrapper is our responsibility
-      if (currentWorker != nullptr && currentWorker->IsWeak()) {
-        delete currentWorker;
-      }
     }
     Caches::Remove(this->isolate_);
 

NativeScript/runtime/WorkerWrapper.mm

@@ -52,7 +52,7 @@ static void PostToLoop(const std::shared_ptr<EventLoop>& loop, std::function<voi
       isClosing_(false),
       isTerminating_(false),
       isDisposed_(false),
-      isWeak_(false),
+      holders_(Holders::Parent),
       messagesEnabled_(false),
       onMessage_(onMessage),
       mainLoop_(Runtime::GetRuntime(mainIsolate)->GetEventLoop()),
@@ -84,6 +84,8 @@ static void PostToLoop(const std::shared_ptr<EventLoop>& loop, std::function<voi
   // entry script clears this flag from its own thread, and a store made after
   // queueing could land on top of that.
   this->isRunning_ = true;
+  // Also before queueing: the operation's last act is to let go of the wrapper.
+  this->holders_.store(Holders::Both, std::memory_order_release);
 
   NSBlockOperation* op = [NSBlockOperation blockOperationWithBlock:^{
     this->BackgroundLooper(func);
@@ -246,41 +248,62 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptr<Even
   // is deleted below.
   this->DestroyInspector();
 
-  // The callback closes over this wrapper, which ~Runtime may delete below,
-  // while V8 keeps the registration until the isolate is disposed - and
-  // disposal can be deferred past that point.
+  // The callback closes over this wrapper, which may be deleted at the end of
+  // this function, while V8 keeps the registration until the isolate is
+  // disposed - and disposal can be deferred past that point.
   if (this->heapLimitIsolate_ != nullptr) {
     v8::Locker locker(this->heapLimitIsolate_);
     this->heapLimitIsolate_->RemoveNearHeapLimitCallback(WorkerWrapper::OnNearHeapLimit, 0);
     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.
+  // Read before ReleaseFromWorkerThread below, after which `this` may be gone.
   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) {
+    // Removes this worker's Caches::Workers entry.
     delete runtime;
   } else {
-    // Runtime was never created (worker terminated before initialization).
-    // The runtime destructor normally handles this cleanup, so do it here.
-    bool found;
-    auto state = Caches::Workers->Get(workerId, found);
-    if (found) {
-      Caches::Workers->Remove(workerId);
-    }
+    // Runtime was never created (worker terminated before initialization), so
+    // the entry its destructor removes is removed here.
+    Caches::Workers->Remove(this->workerId_);
   }
 
+  // The registry entry is gone, so no other runtime's teardown can reach the
+  // wrapper through it any more, and nothing on this thread needs it again.
+  this->ReleaseFromWorkerThread();
+
   PostThreadEndedNotification(mainIsolate, mainLoop, selfRef);
 }
 
+bool WorkerWrapper::ReleaseFromParent() {
+  Holders expected = Holders::Both;
+  if (this->holders_.compare_exchange_strong(expected, Holders::WorkerThread,
+                                             std::memory_order_acq_rel)) {
+    return false;
+  }
+  // A failed exchange leaves the value found in `expected`. Anything but
+  // Parent means the parent let go earlier and the worker thread still has it.
+  return expected == Holders::Parent;
+}
+
+bool WorkerWrapper::HeldByParentOnly() const {
+  return this->holders_.load(std::memory_order_acquire) == Holders::Parent;
+}
+
+void WorkerWrapper::ReleaseFromWorkerThread() {
+  Holders expected = Holders::Both;
+  if (!this->holders_.compare_exchange_strong(expected, Holders::Parent,
+                                              std::memory_order_acq_rel) &&
+      expected == Holders::WorkerThread) {
+    delete this;
+  }
+}
+
 void WorkerWrapper::EnableMessageQueue() {
   this->messagesEnabled_.store(true, std::memory_order_release);
   this->queue_.Signal();

TestRunner/app/tests/WorkerLifetimeTests.js

@@ -256,3 +256,85 @@ describe("Worker teardown with a transferred port in flight", function () {
         };
     });
 });
+
+// A worker that ends with a child still running terminates the child and then
+// disposes the child's Worker object, while the child's own thread is tearing
+// down: both threads are at the end of the child's native wrapper at once.
+describe("Worker teardown with a running child worker", function () {
+    const ROUNDS = 24;
+    let originalTimeout;
+    // The worker of the round in progress, and whether the spec is over: a
+    // spec that failed or timed out must not leave a worker running or start
+    // another round from a late event.
+    let active = null;
+    let finished = false;
+    beforeEach(function () {
+        originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL;
+        jasmine.DEFAULT_TIMEOUT_INTERVAL = 60000;
+        finished = false;
+    });
+    afterEach(function () {
+        jasmine.DEFAULT_TIMEOUT_INTERVAL = originalTimeout;
+        finished = true;
+        if (active !== null) {
+            active.terminate();
+            active = null;
+        }
+    });
+
+    it("ends the parent whether it is terminated or closes itself", function (done) {
+        function finish() {
+            if (!finished) {
+                finished = true;
+                done();
+            }
+        }
+        (function round(index) {
+            if (index === ROUNDS) {
+                finish();
+                return;
+            }
+            let childUp = false;
+            const worker = new Worker("./workerLifetimeNestedParent.js");
+            active = worker;
+            worker.onerror = function (event) {
+                if (finished) {
+                    return true;
+                }
+                expect("worker error: " + event.message).toBeNull();
+                finish();
+                return true;
+            };
+            worker.onmessage = function (event) {
+                if (finished) {
+                    return;
+                }
+                // Anything else is the fixture reporting that its child failed.
+                expect(event.data).toBe("child up");
+                if (event.data !== "child up") {
+                    finish();
+                    return;
+                }
+                childUp = true;
+                if (index % 2 === 0) {
+                    worker.terminate();
+                } else {
+                    worker.postMessage("close");
+                }
+            };
+            worker.addEventListener("nsworkerended", function () {
+                if (finished) {
+                    return;
+                }
+                active = null;
+                // An end before the child was up did not exercise the teardown.
+                expect(childUp).toBe(true);
+                if (!childUp) {
+                    finish();
+                    return;
+                }
+                round(index + 1);
+            });
+        })(0);
+    });
+});

TestRunner/app/tests/workerLifetimeNestedParent.js

@@ -0,0 +1,17 @@
+// Reports once its child worker is running, so the test can end this worker
+// while the child is alive; "close" ends it from the inside instead.
+var child = new Worker("./eventLoopEchoWorker.js");
+child.onmessage = function () {
+    postMessage("child up");
+};
+child.onerror = function (event) {
+    postMessage("child error: " + event.message);
+    return true;
+};
+child.postMessage("ping");
+
+onmessage = function (event) {
+    if (event.data === "close") {
+        close();
+    }
+};