fix(runtime): interop memory-safety fixes from the worker memory-corruption hunt

NativeScript/runtime/ArrayAdapter.mm

@@ -11,16 +11,27 @@
 @implementation ArrayAdapter {
   IsolateWrapper* wrapper_;
   std::shared_ptr<Persistent<Value>> object_;
-  // we're responsible for this wrapper
+  // The wrapper this adapter attached to the JS object, or nullptr when the
+  // field was already taken. The adapter owns the claim exclusively --
+  // retirement paths leave adapter claims attached -- so -dealloc frees it in
+  // both isolate states; the field compare below guards the isolate-alive
+  // path against a slot someone else overwrote.
   ObjCDataWrapper* dataWrapper_;
 }
 
 - (instancetype)initWithJSObject:(Local<Object>)jsObject isolate:(Isolate*)isolate {
   if (self) {
     self->wrapper_ = new IsolateWrapper(isolate);
     self->object_ = std::make_shared<Persistent<Value>>(isolate, jsObject);
-    self->wrapper_->GetCache()->Instances.emplace(self, self->object_);
-    tns::SetValue(isolate, jsObject, (self->dataWrapper_ = new ObjCDataWrapper(self)));
+    self->wrapper_->GetCache()->Instances[self] = self->object_;
+    // A JS object's internal field holds at most one wrapper, owned by whoever
+    // attached it first. An adapter that finds the field taken stays detached
+    // and never writes or clears it; it still reads the object through object_.
+    if (tns::GetValue(isolate, jsObject) == nullptr) {
+      self->dataWrapper_ = new ObjCDataWrapper(self);
+      self->dataWrapper_->MarkAdapterClaim();
+      tns::SetValue(isolate, jsObject, self->dataWrapper_);
+    }
   }
 
   return self;
@@ -107,23 +118,30 @@ - (void)dealloc {
     Isolate::Scope isolate_scope(isolate);
     HandleScope handle_scope(isolate);
     wrapper_->GetCache()->Instances.erase(self);
-    Local<Value> value = self->object_->Get(isolate);
-    BaseDataWrapper* wrapper = tns::GetValue(isolate, value);
-    if (wrapper != nullptr) {
-      tns::DeleteValue(isolate, value);
-      // ensure we don't delete the same wrapper twice
-      // this is just needed as a failsafe in case some other wrapper is assigned to this object
-      if (wrapper == dataWrapper_) {
-        dataWrapper_ = nullptr;
+    // Detach and free only a wrapper that is still the one we attached: a
+    // finalizer or __releaseNativeCounterpart can have retired it already, and
+    // whatever else sits in the field belongs to another owner. Once the
+    // isolate is gone the field can no longer be read, so the claim is dropped
+    // rather than freed blind.
+    if (dataWrapper_ != nullptr) {
+      Local<Value> value = self->object_->Get(isolate);
+      if (tns::GetValue(isolate, value) == dataWrapper_) {
+        tns::DeleteValue(isolate, value);
+        delete dataWrapper_;
       }
-      delete wrapper;
+      dataWrapper_ = nullptr;
     }
     self->object_->Reset();
-  }
-  delete wrapper_;
-  if (dataWrapper_ != nullptr) {
+  } else if (dataWrapper_ != nullptr) {
+    // The isolate is gone, and with it the JS object and every reader of the
+    // claim; no other path deletes one (__releaseNativeCounterpart leaves
+    // adapter claims attached), so the owner frees it here — adapters
+    // released after a worker isolate's teardown otherwise leak one wrapper
+    // each.
     delete dataWrapper_;
+    dataWrapper_ = nullptr;
   }
+  delete wrapper_;
   self->object_ = nullptr;
   [super dealloc];
 }

NativeScript/runtime/DataWrapper.h

@@ -375,6 +375,13 @@ class ObjCDataWrapper : public BaseDataWrapper {
 
   id Data() { return this->data_; }
 
+  // True for the claim a collection adapter attaches to the plain JS object it
+  // was built from. The adapter owns that claim exclusively -- it must stay
+  // deletable from the adapter's -dealloc even after isolate teardown -- so no
+  // other retirement path may free it.
+  bool IsAdapterClaim() { return this->adapterClaim_; }
+  void MarkAdapterClaim() { this->adapterClaim_ = true; }
+
   const TypeEncoding* TypeEncoding() { return this->typeEncoding_; }
 
   // The class Data() had when this wrapper was built. Data() alone cannot tell
@@ -383,6 +390,7 @@ class ObjCDataWrapper : public BaseDataWrapper {
   Class Klass() { return this->klass_; }
 
  private:
+  bool adapterClaim_ = false;
   id data_;
   const tns::TypeEncoding* typeEncoding_;
   Class klass_;

NativeScript/runtime/DictionaryAdapter.mm

@@ -14,23 +14,27 @@ @interface DictionaryAdapterMapKeysEnumerator : NSEnumerator
 
 - (instancetype)initWithMap:(std::shared_ptr<Persistent<Value>>)map
                     isolate:(Isolate*)isolate
-                      cache:(std::shared_ptr<Caches>)cache;
+                      owner:(id)owner;
 
 @end
 
 @implementation DictionaryAdapterMapKeysEnumerator {
   IsolateWrapper* wrapper_;
   uint32_t index_;
   std::shared_ptr<Persistent<Value>> map_;
+  // The adapter owns the persistent this enumerator reads and resets it in
+  // -dealloc, so an enumeration keeps its adapter alive.
+  id owner_;
 }
 
 - (instancetype)initWithMap:(std::shared_ptr<Persistent<Value>>)map
                     isolate:(Isolate*)isolate
-                      cache:(std::shared_ptr<Caches>)cache {
+                      owner:(id)owner {
   if (self) {
     self->wrapper_ = new IsolateWrapper(isolate);
     self->index_ = 0;
     self->map_ = map;
+    self->owner_ = [owner retain];
   }
 
   return self;
@@ -76,6 +80,8 @@ - (id)nextObject {
 - (void)dealloc {
   self->map_ = nil;
   delete self->wrapper_;
+  [self->owner_ release];
+  self->owner_ = nil;
 
   [super dealloc];
 }
@@ -86,7 +92,7 @@ @interface DictionaryAdapterObjectKeysEnumerator : NSEnumerator
 
 - (instancetype)initWithProperties:(std::shared_ptr<Persistent<Value>>)dictionary
                            isolate:(Isolate*)isolate
-                             cache:(std::shared_ptr<Caches>)cache;
+                             owner:(id)owner;
 - (Local<v8::Array>)getProperties;
 
 @end
@@ -95,15 +101,19 @@ @implementation DictionaryAdapterObjectKeysEnumerator {
   IsolateWrapper* wrapper_;
   std::shared_ptr<Persistent<Value>> dictionary_;
   NSUInteger index_;
+  // The adapter owns the persistent this enumerator reads and resets it in
+  // -dealloc, so an enumeration keeps its adapter alive.
+  id owner_;
 }
 
 - (instancetype)initWithProperties:(std::shared_ptr<Persistent<Value>>)dictionary
                            isolate:(Isolate*)isolate
-                             cache:(std::shared_ptr<Caches>)cache {
+                             owner:(id)owner {
   if (self) {
     self->wrapper_ = new IsolateWrapper(isolate);
     self->dictionary_ = dictionary;
     self->index_ = 0;
+    self->owner_ = [owner retain];
   }
 
   return self;
@@ -199,6 +209,8 @@ - (NSArray*)allObjects {
 - (void)dealloc {
   self->dictionary_ = nil;
   delete self->wrapper_;
+  [self->owner_ release];
+  self->owner_ = nil;
 
   [super dealloc];
 }
@@ -208,15 +220,27 @@ - (void)dealloc {
 @implementation DictionaryAdapter {
   IsolateWrapper* wrapper_;
   std::shared_ptr<Persistent<Value>> object_;
+  // The wrapper this adapter attached to the JS object, or nullptr when the
+  // field was already taken. The adapter owns the claim exclusively --
+  // retirement paths leave adapter claims attached -- so -dealloc frees it in
+  // both isolate states; the field compare below guards the isolate-alive
+  // path against a slot someone else overwrote.
   ObjCDataWrapper* dataWrapper_;
 }
 
 - (instancetype)initWithJSObject:(Local<Object>)jsObject isolate:(Isolate*)isolate {
   if (self) {
     self->wrapper_ = new IsolateWrapper(isolate);
     self->object_ = std::make_shared<Persistent<Value>>(isolate, jsObject);
-    self->wrapper_->GetCache()->Instances.emplace(self, self->object_);
-    tns::SetValue(isolate, jsObject, (self->dataWrapper_ = new ObjCDataWrapper(self)));
+    self->wrapper_->GetCache()->Instances[self] = self->object_;
+    // A JS object's internal field holds at most one wrapper, owned by whoever
+    // attached it first. An adapter that finds the field taken stays detached
+    // and never writes or clears it; it still reads the object through object_.
+    if (tns::GetValue(isolate, jsObject) == nullptr) {
+      self->dataWrapper_ = new ObjCDataWrapper(self);
+      self->dataWrapper_->MarkAdapterClaim();
+      tns::SetValue(isolate, jsObject, self->dataWrapper_);
+    }
   }
 
   return self;
@@ -321,16 +345,14 @@ - (NSEnumerator*)keyEnumerator {
   Local<Value> obj = self->object_->Get(isolate);
 
   if (obj->IsMap()) {
-    return
-        [[[DictionaryAdapterMapKeysEnumerator alloc] initWithMap:self->object_
-                                                         isolate:isolate
-                                                           cache:wrapper_->GetCache()] autorelease];
+    return [[[DictionaryAdapterMapKeysEnumerator alloc] initWithMap:self->object_
+                                                            isolate:isolate
+                                                              owner:self] autorelease];
   }
 
   return [[[DictionaryAdapterObjectKeysEnumerator alloc] initWithProperties:self->object_
                                                                     isolate:isolate
-                                                                      cache:wrapper_->GetCache()]
-      autorelease];
+                                                                      owner:self] autorelease];
 }
 
 - (void)dealloc {
@@ -340,18 +362,31 @@ - (void)dealloc {
     Isolate::Scope isolate_scope(isolate);
     HandleScope handle_scope(isolate);
     wrapper_->GetCache()->Instances.erase(self);
-    Local<Value> value = self->object_->Get(isolate);
-    BaseDataWrapper* wrapper = tns::GetValue(isolate, value);
-    if (wrapper != nullptr) {
-      if (wrapper == dataWrapper_) {
-        dataWrapper_ = nullptr;
+    // Detach and free only a wrapper that is still the one we attached: a
+    // finalizer or __releaseNativeCounterpart can have retired it already, and
+    // whatever else sits in the field belongs to another owner. Once the
+    // isolate is gone the field can no longer be read, so the claim is dropped
+    // rather than freed blind.
+    if (dataWrapper_ != nullptr) {
+      Local<Value> value = self->object_->Get(isolate);
+      if (tns::GetValue(isolate, value) == dataWrapper_) {
+        tns::DeleteValue(isolate, value);
+        delete dataWrapper_;
       }
-      tns::DeleteValue(isolate, value);
-      delete wrapper;
+      dataWrapper_ = nullptr;
     }
-  }
-  if (dataWrapper_ != nullptr) {
+    // Persistent<Value> does not reset in its destructor; the enumerators
+    // vended by -keyEnumerator hold this adapter alive, so nothing can be
+    // reading the handle by the time this runs.
+    self->object_->Reset();
+  } else if (dataWrapper_ != nullptr) {
+    // The isolate is gone, and with it the JS object and every reader of the
+    // claim; no other path deletes one (__releaseNativeCounterpart leaves
+    // adapter claims attached), so the owner frees it here — adapters
+    // released after a worker isolate's teardown otherwise leak one wrapper
+    // each.
     delete dataWrapper_;
+    dataWrapper_ = nullptr;
   }
   self->object_ = nullptr;
   delete self->wrapper_;

NativeScript/runtime/Interop.h

@@ -213,6 +213,11 @@ class Interop {
     JSBlockDescriptor* descriptor;
     void* userData;
     ffi_closure* ffiClosure;
+    // The wrapper caching this block on the JS function it was built from. It
+    // is owned here rather than through that function: the cache slot lives in
+    // a V8 heap that can be torn down (a worker isolate) while the block is
+    // still referenced by native code.
+    BlockWrapper* blockWrapper;
 
     static JSBlockDescriptor kJSBlockDescriptor;
   } JSBlock;

NativeScript/runtime/Interop.mm

@@ -38,6 +38,7 @@
         [](JSBlock* block) {
           if (block->descriptor == &JSBlock::kJSBlockDescriptor) {
             MethodCallbackWrapper* wrapper = static_cast<MethodCallbackWrapper*>(block->userData);
+            BlockWrapper* blockWrapper = block->blockWrapper;
             // 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,
@@ -50,16 +51,22 @@
               HandleScope handle_scope(isolate);
               Local<Value> callback = wrapper->callback_->Get(isolate);
               if (!callback.IsEmpty() && callback->IsObject()) {
-                BlockWrapper* blockWrapper =
-                    static_cast<BlockWrapper*>(tns::GetValue(isolate, callback));
-                tns::DeleteValue(isolate, callback);
-                delete blockWrapper;
+                // The callback's slot is the cache's owner, so only a wrapper
+                // still sitting in it is ours to free.
+                if (tns::GetValue(isolate, callback) == blockWrapper) {
+                  tns::DeleteValue(isolate, callback);
+                } else {
+                  blockWrapper = nullptr;
+                }
               }
               // 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.
+            delete blockWrapper;
             delete wrapper;
             ffi_closure_free(block->ffiClosure);
             block->~JSBlock();
@@ -109,6 +116,7 @@
       .descriptor = &JSBlock::kJSBlockDescriptor,
       .userData = userData,
       .ffiClosure = result.second,
+      .blockWrapper = nullptr,
   };
 
   object_setClass((__bridge id)blockPointer, objc_getClass("__NSMallocBlock__"));
@@ -539,6 +547,7 @@ inline bool isBool() {
                                       userData);
 
       BlockWrapper* wrapper = new BlockWrapper((void*)blockPtr, blockTypeEncoding, false);
+      reinterpret_cast<JSBlock*>((void*)blockPtr)->blockWrapper = wrapper;
       tns::SetValue(isolate, arg.As<v8::Function>(), wrapper);
     }
 
@@ -1675,7 +1684,11 @@ inline bool isBool() {
   void* errorRef = nullptr;
   if (methodCall.provideErrorOutParameter_) {
     void* dest = call.ArgumentBuffer(argsCount);
-    errorRef = malloc(ffi_type_pointer.size);
+    // Zero-initialized: a callee writes *error only on failure, so the
+    // success-path read below must find nil. Garbage here is read through a
+    // __strong pointer -- ARC retains and releases it -- so a stale non-null
+    // value over-releases whatever lives at that address now.
+    errorRef = calloc(1, ffi_type_pointer.size);
     Interop::SetValue(dest, errorRef);
   }
 

NativeScript/runtime/Metadata.mm

@@ -106,6 +106,11 @@ static UInt8 getSystemVersion() {
       @try {
         id instance = [klass alloc];
         std::lock_guard<UnfairMutex> lock(sampleInstancesMutex);
+        // A losing emplace (a +initialize re-entry or another thread populated
+        // the entry first) LEAKS `instance`, knowingly: it was never init'd, so
+        // releasing it would run -dealloc against zero-filled ivars of an
+        // arbitrary class on an arbitrary thread.
+        // https://github.com/NativeScript/ios/issues/459
         sampleInstance = sampleInstances.emplace(klass, instance).first->second;
       } @catch (id err) {
         return false;