fix(runtime): free a JS block's wrapper only once its isolate's teardown is done

NativeScript/runtime/ArgConverter.mm

@@ -293,18 +293,22 @@
 
   Isolate* isolate = data->isolateWrapper_.Isolate();
 
-  if (!data->isolateWrapper_.IsValid()) {
-    memset(retValue, 0, cif->rtype->size);
-    return;
-  }
-
   // Declared before all V8 scopes: an ObjC exception must never unwind through a
   // live V8 scope (Locker/HandleScope/Context::Scope). A branded escape caught
   // below is captured here and @thrown only after the inner block closes every
   // V8 scope.
   NSException* __strong pendingThrow = nil;
 
   {
+    // Outlives the Locker: a runtime torn down while this thread waits for the
+    // lock cannot dispose the isolate before the wait is over. Free when this
+    // thread has already entered the isolate.
+    IsolatePin pin = data->isolateWrapper_.Pin();
+    if (!pin || !data->isolateWrapper_.IsValid()) {
+      memset(retValue, 0, cif->rtype->size);
+      return;
+    }
+
     v8::Locker locker(isolate);
     // Checked again with the isolate locked: a runtime being destroyed holds
     // this lock while it removes its caches, so the check above can pass and

NativeScript/runtime/Caches.cpp

@@ -12,7 +12,8 @@ namespace tns {
 Caches::Caches(Isolate* isolate, const int& isolateId)
     : PromiseRejections(std::make_unique<PromiseRejectionTracker>(isolate)),
       isolate_(isolate),
-      isolateId_(isolateId) {}
+      isolateId_(isolateId),
+      gateId_(isolateId) {}
 
 Caches::~Caches() {
   // Subsystem state may hold v8 handles and reference the core caches below;

NativeScript/runtime/Caches.h

@@ -117,6 +117,11 @@ class Caches {
 
   inline int getIsolateId() { return isolateId_; }
 
+  // The id assigned at Init; unlike getIsolateId() it survives
+  // InvalidateIsolate, so it still names this isolate's gate during teardown.
+  // -1 for the stand-in Get() returns once the isolate's Caches is gone.
+  inline int getGateId() const { return gateId_; }
+
   inline void InvalidateIsolate() { isolateId_ = -1; }
 
   inline bool IsValid() { return isolateId_ != -1; }
@@ -317,6 +322,7 @@ class Caches {
   v8::Isolate* isolate_;
   std::shared_ptr<v8::Persistent<v8::Context>> context_;
   int isolateId_;
+  const int gateId_;
 };
 
 }  // namespace tns

NativeScript/runtime/Interop.mm

@@ -44,30 +44,36 @@
             // resetting it never touches the finalizer drain's bookkeeping,
             // and a foreign-thread Locker into the block's own isolate is
             // legitimate now that extended class names are worker-scoped.
-            if (wrapper->isolateWrapper_.IsValid()) {
-              Isolate* isolate = wrapper->isolateWrapper_.Isolate();
-              v8::Locker locker(isolate);
-              // Re-checked under the lock: ~Runtime holds it while it tears the
-              // isolate's caches down, so the check above can predate that.
-              if (wrapper->isolateWrapper_.IsValid()) {
-                Isolate::Scope isolate_scope(isolate);
-                HandleScope handle_scope(isolate);
-                Local<Value> callback = wrapper->callback_->Get(isolate);
-                if (!callback.IsEmpty() && callback->IsObject()) {
-                  // The slot may hold another wrapper by now; only our own is
-                  // cleared from it.
-                  if (tns::GetValue(isolate, callback) == blockWrapper) {
-                    tns::DeleteValue(isolate, callback);
+            //
+            // Validity is not the test here: the slot stays reachable after
+            // the isolate is invalidated, by JS still running under the Locker
+            // and by the teardown's own walk of registered objects. Until the
+            // gate closes, the wrapper is only freed once the slot is cleared.
+            {
+              IsolatePin pin = wrapper->isolateWrapper_.Pin();
+              if (pin) {
+                Isolate* isolate = wrapper->isolateWrapper_.Isolate();
+                v8::Locker locker(isolate);
+                if (!wrapper->isolateWrapper_.IsTornDown()) {
+                  Isolate::Scope isolate_scope(isolate);
+                  HandleScope handle_scope(isolate);
+                  Local<Value> callback = wrapper->callback_->Get(isolate);
+                  if (!callback.IsEmpty() && callback->IsObject()) {
+                    // The slot may hold another wrapper by now; only our own is
+                    // cleared from it.
+                    if (tns::GetValue(isolate, callback) == blockWrapper) {
+                      tns::DeleteValue(isolate, callback);
+                    }
                   }
+                  // Unconditional: an already-detached callback still owns its
+                  // node, and dropping the persistent without a reset would leave
+                  // that node rooted forever.
+                  wrapper->callback_->Reset();
                 }
-                // Unconditional: an already-detached callback still owns its
-                // node, and dropping the persistent without a reset would leave
-                // that node rooted forever.
-                wrapper->callback_->Reset();
               }
             }
-            // Outside the isolate guard: once the isolate is gone the cache
-            // slot is unreachable and nothing else can free the wrapper.
+            // Outside the gate: once the teardown is done the cache slot is
+            // unreachable and nothing else can free the wrapper.
             delete blockWrapper;
             delete wrapper;
             ffi_closure_free(block->ffiClosure);
@@ -570,8 +576,9 @@ inline bool isBool() {
         blockPtr = CFAutorelease(Block_copy(wrapper->Block()));
       } else if (TryRetainJSBlock(static_cast<JSBlock*>(wrapper->Block()))) {
         // Reading the block is safe even when its last release raced ahead:
-        // while the isolate is valid, dispose clears this slot under the
-        // Locker this thread holds, before libclosure frees the block.
+        // until the isolate's teardown closes its gate, dispose clears this
+        // slot under the Locker this thread holds, before libclosure frees
+        // the block.
         blockPtr = CFAutorelease(wrapper->Block());
       }
     }

NativeScript/runtime/IsolateWrapper.cpp

@@ -7,12 +7,95 @@
 //
 
 #include "IsolateWrapper.h"
+
+#include <cstdint>
+#include <mutex>
+
 #include "Runtime.h"
+#include "UnfairLock.h"
+#include "robin_hood.h"
 
 namespace tns {
 
 bool IsolateWrapper::IsValid() const {
-    return Runtime::IsAlive(isolate_) && isolate_->GetData(tns::Constants::CACHES_ISOLATE_SLOT) != nullptr && GetCache()->getIsolateId() == isolateId_;
+  if (!Runtime::IsAlive(isolate_) ||
+      isolate_->GetData(tns::Constants::CACHES_ISOLATE_SLOT) == nullptr) {
+    return false;
+  }
+  std::shared_ptr<Caches> cache = GetCache();
+  return cache->IsValid() && cache->getGateId() == gateId_;
+}
+
+namespace IsolateGates {
+
+namespace {
+
+struct Gate {
+  uint32_t pins = 0;
+  bool closed = false;
+};
+
+// Trivially destructible, so exit() leaves it usable.
+static UnfairMutex gatesMutex;
+
+// Never destroyed: exit() runs static destructors while other threads may
+// still be pinning.
+robin_hood::unordered_map<int, Gate>& Gates() {
+  static auto* gates = new robin_hood::unordered_map<int, Gate>();
+  return *gates;
+}
+
+}  // namespace
+
+void Open(int id) {
+  std::lock_guard<UnfairMutex> lock(gatesMutex);
+  Gates()[id] = Gate();
+}
+
+bool TryPin(int id) {
+  std::lock_guard<UnfairMutex> lock(gatesMutex);
+  auto it = Gates().find(id);
+  if (it == Gates().end() || it->second.closed) {
+    return false;
+  }
+  it->second.pins++;
+  return true;
+}
+
+void Unpin(int id) {
+  std::lock_guard<UnfairMutex> lock(gatesMutex);
+  auto it = Gates().find(id);
+  if (it != Gates().end()) {
+    it->second.pins--;
+  }
+}
+
+void Close(int id) {
+  std::lock_guard<UnfairMutex> lock(gatesMutex);
+  auto it = Gates().find(id);
+  if (it != Gates().end()) {
+    it->second.closed = true;
+  }
+}
+
+bool IsClosed(int id) {
+  std::lock_guard<UnfairMutex> lock(gatesMutex);
+  auto it = Gates().find(id);
+  return it == Gates().end() || it->second.closed;
+}
+
+bool RetireIfUnpinned(int id) {
+  std::lock_guard<UnfairMutex> lock(gatesMutex);
+  auto it = Gates().find(id);
+  if (it == Gates().end()) {
+    return true;
+  }
+  if (it->second.pins > 0) {
+    return false;
+  }
+  Gates().erase(it);
+  return true;
 }
 
+}  // namespace IsolateGates
 }

NativeScript/runtime/IsolateWrapper.h

@@ -9,36 +9,86 @@
 #ifndef IsolateWrapper_h
 #define IsolateWrapper_h
 
-#include "v8.h"
+#include <type_traits>
+
 #include "Caches.h"
 #include "Constants.h"
+#include "v8.h"
 
 namespace tns {
 
+// One gate per isolate, keyed by Caches::getGateId() (ids are never reused).
+// A thread that has not entered the isolate pins its gate before waiting for
+// or holding the isolate's Locker. A pin taken before Close() defers
+// Isolate::Dispose until it is released. Once the teardown has closed the
+// gate, no wrapper can be reached from JS any more and pins are refused. A
+// missing entry reads as closed.
+namespace IsolateGates {
+void Open(int id);
+bool TryPin(int id);
+void Unpin(int id);
+// Called by the teardown while it holds the isolate's Locker, after the last
+// walk that reads wrappers through JS objects.
+void Close(int id);
+bool IsClosed(int id);
+// Drops the entry unless a pin is held; the isolate may be disposed only once
+// this returns true.
+bool RetireIfUnpinned(int id);
+}  // namespace IsolateGates
+
+// Owns a pin for its scope. Declare it before the Locker it guards, so the
+// Locker is released first. A thread that has entered the isolate takes no
+// pin: an entered isolate stays in use, which already defers its disposal.
+class IsolatePin {
+ public:
+  IsolatePin(int id, bool entered)
+      : id_(id),
+        pinned_(!entered && IsolateGates::TryPin(id)),
+        usable_(entered || pinned_) {}
+  ~IsolatePin() {
+    if (pinned_) {
+      IsolateGates::Unpin(id_);
+    }
+  }
+  IsolatePin(const IsolatePin&) = delete;
+  IsolatePin& operator=(const IsolatePin&) = delete;
+  explicit operator bool() const { return usable_; }
+
+ private:
+  int id_;
+  bool pinned_;
+  bool usable_;
+};
+
+// Kept trivially copyable: ObjC blocks that live as long as the process (the
+// extended classes' synthesized methods) capture it by value.
 class IsolateWrapper {
 public:
     bool IsValid() const;
     inline std::shared_ptr<tns::Caches> GetCache() const {
         return tns::Caches::Get(isolate_);
     }
-    inline v8::Isolate* Isolate() {
-        return isolate_;
-    }
-    inline int IsolateId() {
-        return isolateId_;
-    }
+    inline v8::Isolate* Isolate() { return isolate_; }
     inline IsolateWrapper(v8::Isolate* isolate) {
         isolate_ = isolate;
-        isolateId_ = tns::Caches::Get(isolate_)->getIsolateId();
+        gateId_ = tns::Caches::Get(isolate_)->getGateId();
+    }
+    // Hold the returned pin across any wait for the isolate's Locker from a
+    // thread that is not already inside the isolate; an empty pin means the
+    // isolate may already be disposed and must not be touched at all.
+    inline IsolatePin Pin() const {
+      return IsolatePin(gateId_, v8::Isolate::TryGetCurrent() == isolate_);
     }
-
-private:
+    // True once the teardown has finished with every wrapper reachable from
+    // JS. Read it while pinned.
+    inline bool IsTornDown() const { return IsolateGates::IsClosed(gateId_); }
+
+   private:
     v8::Isolate* isolate_;
-    int isolateId_;
-
-
+    int gateId_;
 };
 
+static_assert(std::is_trivially_copyable_v<IsolateWrapper>);
 }
 
 #endif /* IsolateWrapper_h */

NativeScript/runtime/Runtime.mm

@@ -193,15 +193,18 @@ static void InitializeImportMetaObject(Local<Context> context, Local<Module> mod
 // CFNotificationCenterRemoveObserver(CFNotificationCenterGetLocalCenter(), this,
 // kCFTimeZoneSystemTimeZoneDidChangeNotification, NULL);
 
-void DisposeIsolateWhenPossible(Isolate* isolate) {
-  // Disposal is deferred only while the isolate is still entered, which happens
+void DisposeIsolateWhenPossible(Isolate* isolate, int gateId) {
+  // Disposal is deferred while the isolate is still entered, which happens
   // when the runtime is deleted by code running inside its own isolate: an
-  // embedder calling shutdownRuntime from a JS callback. exit() never deletes a
-  // runtime, so a process that is dying leaves its isolates to the OS.
-  if (isolate->IsInUse()) {
+  // embedder calling shutdownRuntime from a JS callback. It is also deferred
+  // while another thread holds a pin to wait for or use the isolate's Locker.
+  // Never block here instead: the pin holder may be waiting for a Locker this
+  // thread still holds further up the stack. exit() never deletes a runtime,
+  // so a process that is dying leaves its isolates to the OS.
+  if (isolate->IsInUse() || !IsolateGates::RetireIfUnpinned(gateId)) {
     dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(10.0 * NSEC_PER_MSEC)),
                    dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_LOW, 0), ^{
-                     DisposeIsolateWhenPossible(isolate);
+                     DisposeIsolateWhenPossible(isolate, gateId);
                    });
   } else {
     isolate->Dispose();
@@ -234,10 +237,11 @@ void DisposeIsolateWhenPossible(Isolate* isolate) {
   NativeScriptException::OnIsolateTeardown(isolate_);
 
   auto currentIsolate = this->isolate_;
+  int gateId = Caches::Get(this->isolate_)->getGateId();
   {
-    // make sure we remove the isolate from the list of active isolates first
-    // this will make sure isAlive(isolate) will return false and prevent locking of the v8 isolate
-    // after it terminates execution
+    // First, so IsAlive turns false: native callbacks arriving from here on
+    // bail before taking the Locker. A block's dispose still pins and locks
+    // until the gate closes below.
     SpinLock lock(isolatesMutex_);
     Runtime::isolates_.erase(
         std::remove(Runtime::isolates_.begin(), Runtime::isolates_.end(), this->isolate_),
@@ -299,9 +303,14 @@ void DisposeIsolateWhenPossible(Isolate* isolate) {
     Caches::Remove(this->isolate_);
 
     this->isolate_->SetData(Constants::RUNTIME_SLOT, nullptr);
+
+    // Last, and under the Locker: from here on nothing reads a wrapper through
+    // a JS object, so threads that never pinned may free theirs without the
+    // lock, and threads already pinned see the gate closed once they get it.
+    IsolateGates::Close(gateId);
   }
 
-  DisposeIsolateWhenPossible(this->isolate_);
+  DisposeIsolateWhenPossible(this->isolate_, gateId);
 
   // Matched erase: only removes the registry entry while it still maps to
   // this runtime's loop, so a worker isolate that reuses this pointer after
@@ -384,6 +393,8 @@ void DisposeIsolateWhenPossible(Isolate* isolate) {
 void Runtime::Init(Isolate* isolate, bool isWorker) {
   std::shared_ptr<Caches> cache =
       Caches::Init(isolate, nextIsolateId.fetch_add(1, std::memory_order_relaxed));
+  // Before anything can build an IsolateWrapper for this isolate.
+  IsolateGates::Open(cache->getGateId());
   cache->isWorker = isWorker;
   cache->ObjectCtorInitializer = MetadataBuilder::GetOrCreateConstructorFunctionTemplate;
   cache->StructPrototypeInitializer = MetadataBuilder::GetOrCreateStructPrototype;