fix: stop over-releasing objects wrapped from a raw pointer

NativeScript/runtime/ArgConverter.h

@@ -58,6 +58,14 @@ class ArgConverter {
       v8::Local<v8::Context> context);
   static const Meta* FindMeta(Class klass,
                               const TypeEncoding* typeEncoding = nullptr);
+  // Looks up the JS wrapper cached for `target` in Caches::Instances, dropping
+  // the entry and reporting a miss when it no longer describes the object at
+  // that address. Every read of Instances that hands the wrapper straight back
+  // to JS must go through here: entries are keyed on the raw pointer, so an
+  // entry that outlived its object would otherwise alias whatever allocation
+  // recycled the address and give it a foreign prototype.
+  static std::shared_ptr<v8::Persistent<v8::Value>> FindCachedInstance(
+      v8::Isolate* isolate, const std::shared_ptr<Caches>& cache, id target);
   static const Meta* GetMeta(std::string name);
   static const ProtocolMeta* FindProtocolMeta(Protocol* protocol);
   static void MethodCallback(ffi_cif* cif, void* retValue, void** argValues,

NativeScript/runtime/ArgConverter.mm

@@ -559,12 +559,21 @@
   tns::Assert(klass != nullptr, isolate);
 
   id result = nil;
+  // A Caches::Instances entry owns exactly one reference to the object it maps
+  // (ObjectManager::DisposeValue gives it back, and ClassBuilder's swizzled
+  // retain/release read a retainCount of 1 as "only the map holds this"), so
+  // this function must hand the entry a +1 and no more. Tracks whether `result`
+  // already carries one: the alloc/init paths do, a pointer handed in from JS
+  // does not. Constructing from a pointer is non-consuming — interop.handleof
+  // hands out a retain-neutral address, and a +1 reaches JS as an Unmanaged to
+  // be claimed with takeRetainedValue — so that path takes its own reference.
+  bool resultIsOwned = false;
 
   if (info.Length() == 1) {
     BaseDataWrapper* wrapper = tns::GetValue(isolate, info[0]);
     if (wrapper != nullptr && wrapper->Type() == WrapperType::Pointer) {
       PointerWrapper* pointerWrapper = static_cast<PointerWrapper*>(wrapper);
-      result = CFBridgingRelease(pointerWrapper->Data());
+      result = (__bridge id)pointerWrapper->Data();
     }
   }
 
@@ -583,25 +592,34 @@
 
     V8VectorArgs vectorArgs(args);
     result = Interop::CallInitializer(context, initializer, result, klass, vectorArgs);
+    resultIsOwned = true;
   }
 
   if (result == nil) {
     result = [[klass alloc] init];
+    resultIsOwned = true;
   }
 
   auto cache = Caches::Get(isolate);
-  auto it = cache->Instances.find(result);
-  if (it != cache->Instances.end()) {
-    Local<Value> obj = it->second->Get(isolate);
-    info.GetReturnValue().Set(obj);
+  auto poInstance = ArgConverter::FindCachedInstance(isolate, cache, result);
+  if (poInstance != nullptr) {
+    // An initializer that answered with an already wrapped object (a singleton,
+    // a tagged pointer, a cached cluster instance) leaves us holding a +1 the
+    // existing entry has no use for.
+    if (resultIsOwned) {
+      [result release];
+    }
+    info.GetReturnValue().Set(poInstance->Get(isolate));
   } else {
     ObjCDataWrapper* wrapper = new ObjCDataWrapper(result);
     Local<Object> thiz = info.This();
     Local<Context> context = cache->GetContext();
     tns::SetValue(isolate, thiz, wrapper);
     std::shared_ptr<Persistent<Value>> poThiz = ObjectManager::Register(context, thiz);
     cache->Instances.emplace(result, poThiz);
-    // [result retain];
+    if (!resultIsOwned) {
+      [result retain];
+    }
   }
 }
 
@@ -935,6 +953,30 @@
   return receiver;
 }
 
+std::shared_ptr<Persistent<Value>> ArgConverter::FindCachedInstance(
+    Isolate* isolate, const std::shared_ptr<Caches>& cache, id target) {
+  auto it = cache->Instances.find(target);
+  if (it == cache->Instances.end()) {
+    return nullptr;
+  }
+
+  BaseDataWrapper* wrapper = tns::GetValue(isolate, it->second->Get(isolate));
+  if (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject) {
+    Class expected = static_cast<ObjCDataWrapper*>(wrapper)->Klass();
+    // A KVO-style isa swizzle replaces the class in place but keeps -class
+    // answering the original, so only an address that now belongs to a
+    // different object fails both comparisons. Dropping such an entry turns a
+    // wrapper that would otherwise be handed out with the wrong prototype into
+    // a plain cache miss, which rebuilds it correctly.
+    if (expected != nil && object_getClass(target) != expected && [target class] != expected) {
+      cache->Instances.erase(it);
+      return nullptr;
+    }
+  }
+
+  return it->second;
+}
+
 const Meta* ArgConverter::FindMeta(Class klass, const TypeEncoding* typeEncoding) {
   if (typeEncoding != nullptr &&
       typeEncoding->type == BinaryTypeEncodingType::InterfaceDeclarationReference) {

NativeScript/runtime/DataWrapper.h

@@ -1,6 +1,8 @@
 #ifndef DataWrapper_h
 #define DataWrapper_h
 
+#include <objc/runtime.h>
+
 #include <functional>
 #include <mutex>
 #include <thread>
@@ -365,17 +367,25 @@ class UnmanagedTypeWrapper : public BaseDataWrapper {
 class ObjCDataWrapper : public BaseDataWrapper {
  public:
   ObjCDataWrapper(id data, const TypeEncoding* typeEncoding = nullptr)
-      : data_(data), typeEncoding_(typeEncoding) {}
+      : data_(data),
+        typeEncoding_(typeEncoding),
+        klass_(object_getClass(data)) {}
 
   const WrapperType Type() { return WrapperType::ObjCObject; }
 
   id Data() { return this->data_; }
 
   const TypeEncoding* TypeEncoding() { return this->typeEncoding_; }
 
+  // The class Data() had when this wrapper was built. Data() alone cannot tell
+  // whether the wrapper still describes the object living at that address, so
+  // anything keyed on the raw pointer needs this to detect a recycled slot.
+  Class Klass() { return this->klass_; }
+
  private:
   id data_;
   const tns::TypeEncoding* typeEncoding_;
+  Class klass_;
 };
 
 class ObjCClassWrapper : public BaseDataWrapper {

NativeScript/runtime/Interop.mm

@@ -1244,9 +1244,9 @@ inline bool isBool() {
     }
 
     auto cache = Caches::Get(isolate);
-    auto it = cache->Instances.find(result);
-    if (it != cache->Instances.end()) {
-      return it->second->Get(isolate);
+    auto poInstance = ArgConverter::FindCachedInstance(isolate, cache, result);
+    if (poInstance != nullptr) {
+      return poInstance->Get(isolate);
     }
 
     // For NSProxy we will try to read the metadata from

NativeScript/runtime/ObjectManager.mm

@@ -204,7 +204,16 @@ void DisposeHandle(v8::Isolate* isolate,
       ObjCDataWrapper* objCObjectWrapper = static_cast<ObjCDataWrapper*>(wrapper);
       id target = objCObjectWrapper->Data();
       if (target != nil) {
-        cache->Instances.erase(target);
+        // Instances is keyed on the raw address, so an entry rebuilt for a
+        // later object living there must survive this wrapper going away —
+        // only the entry that still points back at this object is ours.
+        auto it = cache->Instances.find(target);
+        if (it != cache->Instances.end()) {
+          Local<Value> cached = it->second->Get(isolate);
+          if (cached.IsEmpty() || cached == value) {
+            cache->Instances.erase(it);
+          }
+        }
         [target release];
       }
       break;

TestRunner/app/tests/StaleWrapperCacheTests.js

@@ -0,0 +1,44 @@
+describe("Instance cache staleness", function () {
+    var ITERATIONS = 100;
+
+    // NSObject (isa only) and TNSBaseInterface (isa + two ints) both land in the
+    // 16-byte malloc bucket, so a TNSBaseInterface allocated after an NSObject is
+    // freed can be handed the very same address.
+    it("never hands out a wrapper built for an object that no longer lives at that address", function (done) {
+        for (var i = 0; i < ITERATIONS; i++) {
+            var original = NSObject.alloc().init();
+            var alias = new NSObject(interop.handleof(original));
+            if (i === 0) {
+                expect(alias).toBe(original);
+            }
+            original = null;
+            alias = null;
+        }
+
+        // An address can only be recycled once the runloop has drained its
+        // autorelease pool, so the reallocation half has to run in a later turn.
+        setTimeout(function () {
+            var wrongPrototype = 0;
+            var missingMethod = 0;
+            var instances = [];
+
+            for (var j = 0; j < ITERATIONS; j++) {
+                var instance = TNSBaseInterface.alloc().init();
+                instances.push(instance);
+
+                if (Object.getPrototypeOf(instance) !== TNSBaseInterface.prototype) {
+                    wrongPrototype++;
+                }
+                if (typeof instance.baseProtocolMethod2Optional !== "function") {
+                    missingMethod++;
+                }
+            }
+
+            expect(wrongPrototype).toBe(0, "instances built on a reused address got a foreign prototype");
+            expect(missingMethod).toBe(0, "instances built on a reused address lost their protocol methods");
+
+            instances = null;
+            done();
+        }, 0);
+    });
+});

TestRunner/app/tests/index.js

@@ -121,6 +121,7 @@ require("./Inheritance/ProtocolImplementationTests");
 require("./Inheritance/TypeScriptTests");
 //
 require("./MethodCallsTests");
+require("./StaleWrapperCacheTests");
 //import "./FunctionsTests";
 require("./VersionDiffTests");
 require("./ObjCConstructors");