fix(runtime): finalizer-safe handle ownership and deferred JSBlock teardown

NativeScript/runtime/Interop.mm

@@ -38,6 +38,11 @@
         [](JSBlock* block) {
           if (block->descriptor == &JSBlock::kJSBlockDescriptor) {
             MethodCallbackWrapper* wrapper = static_cast<MethodCallbackWrapper*>(block->userData);
+            // Runs on whatever thread drops the last native reference. That is
+            // safe inline: callback_ is a strong, unregistered persistent, so
+            // 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);
@@ -48,9 +53,12 @@
                 BlockWrapper* blockWrapper =
                     static_cast<BlockWrapper*>(tns::GetValue(isolate, callback));
                 tns::DeleteValue(isolate, callback);
-                wrapper->callback_->Reset();
                 delete blockWrapper;
               }
+              // 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();
             }
             delete wrapper;
             ffi_closure_free(block->ffiClosure);

NativeScript/runtime/NSDataAdapter.mm

@@ -104,7 +104,6 @@ - (void)dealloc {
     delete dataWrapper_;
   }
 
-  self->object_->Reset();
   delete self->wrapper_;
   self->object_ = nullptr;
   [super dealloc];

NativeScript/runtime/ObjectManager.h

@@ -7,6 +7,16 @@ namespace tns {
 
 class ObjectManager;
 
+// Parameter of the kFinalizer weak callback armed on target_.
+//
+// Ownership: created by ObjectManager::Register and deleted by exactly two
+// sites -- ObjectManager::FinalizerCallback's disposed branch and
+// DisposeAllRegistered. Retiring a registration from anywhere else (the
+// __releaseNativeCounterpart builtin is the only one) must Reset target_
+// first: resetting frees the V8 node, which clears its pending-finalizer bit
+// and guarantees no further callback, so the state is unreachable afterwards
+// and safe to unlink and delete. Dropping the weakness without resetting
+// leaves the node rooted forever with parameter() pointing at the freed state.
 struct ObjectWeakCallbackState {
   ObjectWeakCallbackState(std::shared_ptr<v8::Persistent<v8::Value>> target)
       : target_(target) {}
@@ -21,6 +31,13 @@ struct ObjectWeakCallbackState {
   ObjectWeakCallbackState** head_ = nullptr;
   ObjectWeakCallbackState* prev_ = nullptr;
   ObjectWeakCallbackState* next_ = nullptr;
+
+  // Set while one of the two owning sites is disposing this handle's value.
+  // Disposal releases the native counterpart, whose -dealloc can re-enter JS
+  // and reach __releaseNativeCounterpart for this very handle; retiring it
+  // there would free the state under the frame that owns it. Retirement
+  // observes the flag and leaves the handle to that frame.
+  bool disposing_ = false;
 };
 
 class ObjectManager {

NativeScript/runtime/ObjectManager.mm

@@ -104,9 +104,15 @@ void DisposeHandle(v8::Isolate* isolate,
   Isolate::Scope isolateScope(isolate);
   HandleScope scope(isolate);
 
-  // Detach the whole list first so disposal can't walk into freed entries.
+  // Detach the whole list first so disposal can't walk into freed entries, and
+  // claim every state up front: disposing one entry can re-enter JS from
+  // -dealloc and reach __releaseNativeCounterpart for any other entry this
+  // walk still owns.
   ObjectWeakCallbackState* state = cache->ObjectManagedValues;
   cache->ObjectManagedValues = nullptr;
+  for (ObjectWeakCallbackState* claimed = state; claimed != nullptr; claimed = claimed->next_) {
+    claimed->disposing_ = true;
+  }
 
   while (state != nullptr) {
     ObjectWeakCallbackState* next = state->next_;
@@ -136,17 +142,37 @@ void DisposeHandle(v8::Isolate* isolate,
 void ObjectManager::FinalizerCallback(const WeakCallbackInfo<ObjectWeakCallbackState>& data) {
   ObjectWeakCallbackState* state = data.GetParameter();
   Isolate* isolate = data.GetIsolate();
+
+  if (state->disposing_) {
+    // Another frame owns this state's teardown — the DisposeAllRegistered
+    // walk, whose pre-claimed entries a nested collection can still condemn.
+    // Re-arm to satisfy the finalizer contract on this node and leave the
+    // clear/reset/delete to the owner.
+    state->target_->ClearWeak<void>();
+    state->target_->SetWeak(state, FinalizerCallback, WeakCallbackType::kFinalizer);
+    return;
+  }
+
+  state->disposing_ = true;
   Local<Value> value = state->target_->Get(isolate);
   bool disposed = ObjectManager::DisposeValue(isolate, value);
-
-  if (disposed) {
-    UnlinkRegistered(state);
+  state->disposing_ = false;
+
+  // Disposal releases the native counterpart, and a -dealloc reached that way
+  // can reset this very handle (the collection adapters reset the persistent
+  // they were built from). An empty handle means the node is already freed, so
+  // there is nothing left to reset or re-arm -- ClearWeak/SetWeak would write
+  // through a dead slot -- and the registration must be retired even when
+  // disposal was refused.
+  if (disposed || state->target_->IsEmpty()) {
     state->target_->Reset();
+    UnlinkRegistered(state);
     delete state;
-  } else {
-    state->target_->ClearWeak<void>();
-    state->target_->SetWeak(state, FinalizerCallback, WeakCallbackType::kFinalizer);
+    return;
   }
+
+  state->target_->ClearWeak<void>();
+  state->target_->SetWeak(state, FinalizerCallback, WeakCallbackType::kFinalizer);
 }
 
 bool ObjectManager::DisposeValue(Isolate* isolate, Local<Value> value, bool isFinalDisposal) {
@@ -332,12 +358,25 @@ void DisposeHandle(v8::Isolate* isolate,
     std::shared_ptr<Caches> cache = Caches::Get(isolate);
     auto it = cache->Instances.find(data);
     if (it != cache->Instances.end()) {
-      ObjectWeakCallbackState* state = it->second->ClearWeak<ObjectWeakCallbackState>();
+      std::shared_ptr<Persistent<Value>> handle = it->second;
+      ObjectWeakCallbackState* state =
+          handle->IsWeak() ? handle->ClearWeak<ObjectWeakCallbackState>() : nullptr;
+      if (state != nullptr && state->disposing_) {
+        // Reached from a -dealloc running inside this handle's own finalizer:
+        // that frame already released the native counterpart and owns the
+        // state, so restore the weakness and let it finish.
+        handle->SetWeak(state, FinalizerCallback, WeakCallbackType::kFinalizer);
+        return;
+      }
+      cache->Instances.erase(it);
       if (state != nullptr) {
+        // Reset before deleting the state: it frees the node, which clears the
+        // pending-finalizer bit and guarantees no callback can reach the freed
+        // parameter.
+        handle->Reset();
         UnlinkRegistered(state);
         delete state;
       }
-      cache->Instances.erase(it);
     }
 
     // Release the runtime's strong reference (taken when the object was first

TestRunner/app/tests/GCFinalizerTests.js

@@ -141,4 +141,164 @@ describe("GC finalizer callbacks", function () {
             done();
         }, 0);
     });
+
+    // Overwrites the stack region that held the creation locals so
+    // conservative stack scanning cannot keep dead wrappers alive.
+    function scrubStack() {
+        return (function scrub(n) {
+            return n > 0 ? scrub(n - 1) + n : 0;
+        })(300);
+    }
+
+    // Retiring a registration has to reset the persistent, not just drop its
+    // weakness: a handle left non-weak is a strong root, so the object it
+    // names can never be collected again.
+    it("frees the handle of an object retired by __releaseNativeCounterpart", function (done) {
+        var ref;
+        (function () {
+            var obj = TNSObjCTypes.alloc().init();
+            ref = new WeakRef(obj);
+            __releaseNativeCounterpart(obj);
+        })();
+
+        scrubStack();
+        __collect();
+        setTimeout(function () {
+            __collect();
+            expect(ref.deref()).toBeUndefined();
+            done();
+        }, 0);
+    });
+
+    // Retirement reached from a -dealloc that a finalizer drove: it frees the
+    // victim's state while the drain is mid-iteration over the handle table.
+    it("retires another registration from a -dealloc reached by the drain", function () {
+        var victims = [];
+        var retired = 0;
+
+        var DeallocRetire = TNSApi.extend({
+            methodCalledInDealloc: function () {
+                var victim = victims.pop();
+                if (victim !== undefined) {
+                    __releaseNativeCounterpart(victim);
+                    retired++;
+                }
+            }
+        }, { name: "TNSApiDeallocRetire" });
+
+        // Kept alive by JS for the whole spec, so each retirement hits a live
+        // registration rather than one the same GC already queued.
+        var keepAlive = NSMutableArray.alloc().init();
+        for (var i = 0; i < 8; i++) {
+            var victim = TNSObjCTypes.alloc().init();
+            keepAlive.addObject(victim);
+            victims.push(victim);
+        }
+
+        (function () {
+            var holder = NSMutableArray.alloc().init();
+            for (var j = 0; j < 8; j++) {
+                holder.addObject(DeallocRetire.alloc().init());
+            }
+        })();
+
+        scrubStack();
+        __collect();
+        __collect();
+
+        expect(retired).toBeGreaterThan(0);
+        expect(keepAlive.count).toBe(8);
+    });
+
+    // Collection adapters reset their own persistent and delete wrappers from
+    // -dealloc; a graph of them dies in one drain, so those resets land while
+    // the finalizer that released the graph is still on the stack.
+    it("survives adapter deallocs cascading out of a finalizer", function () {
+        var rounds = 24;
+
+        (function () {
+            for (var i = 0; i < rounds; i++) {
+                var holder = NSMutableArray.alloc().init();
+                holder.addObject([1, 2, 3]);
+                holder.addObject({ a: 1, b: 2 });
+                holder.addObject(new Uint8Array(8));
+                holder.addObject(NSMutableArray.arrayWithArray([[i], { i: i }]));
+            }
+        })();
+
+        scrubStack();
+        __collect();
+        __collect();
+
+        // A live adapter still answers after the sweep.
+        var survivor = NSMutableArray.arrayWithArray([1, 2, 3]);
+        expect(survivor.count).toBe(3);
+        expect(survivor.objectAtIndex(1)).toBe(2);
+    });
+
+    // A natively held block's last release can land inside the finalizer
+    // drain, where the JSBlock dispose helper must not touch handles itself.
+    it("tears down a natively held block released by a finalizer", function (done) {
+        var ref;
+        (function () {
+            var callback = function () {
+                TNSLog("retained block called");
+            };
+            ref = new WeakRef(callback);
+
+            var owner = TNSObjCTypes.alloc().init();
+            owner.methodRetainingBlock(callback);
+            owner.methodCallRetainingBlock();
+
+            var holder = NSMutableArray.alloc().init();
+            holder.addObject(owner);
+        })();
+        TNSClearOutput();
+
+        scrubStack();
+        // The deferred teardown runs on a later event-loop pass, so the
+        // collectability check polls rather than assuming one tick suffices.
+        var attempts = 20;
+        (function pollCollected() {
+            __collect();
+            if (ref.deref() === undefined) {
+                done();
+                return;
+            }
+            if (--attempts === 0) {
+                expect(ref.deref()).toBeUndefined();
+                done();
+                return;
+            }
+            setTimeout(pollCollected);
+        })();
+    });
+
+    // Releasing the block detaches the function's wrapper, so a later
+    // marshal of the same function must build a fresh block rather than
+    // reaching for the dead one.
+    it("re-marshals a function whose block was already released", function (done) {
+        var callback = function () {
+            TNSLog("re-marshalled block called");
+        };
+
+        var first = TNSObjCTypes.alloc().init();
+        first.methodRetainingBlock(callback);
+        first.methodReleaseRetainingBlock();
+        first = null;
+
+        // The block's remaining reference is the autoreleased one taken when
+        // it was marshalled; it goes away with the pool at the end of the turn.
+        setTimeout(function () {
+            __collect();
+            var second = TNSObjCTypes.alloc().init();
+            second.methodRetainingBlock(callback);
+            TNSClearOutput();
+            second.methodCallRetainingBlock();
+
+            expect(TNSGetOutput()).toBe("re-marshalled block called");
+            TNSClearOutput();
+            done();
+        }, 0);
+    });
 });