fix(runtime): release owned instance-method returns that hit a cached wrapper

NativeScript/runtime/ArgConverter.h

@@ -13,23 +13,34 @@ namespace tns {
 
 class ArgConverter;
 
+// What native callers of a JS-implemented method own of the object it answers.
+enum class NativeReturnOwnership : uint8_t {
+  Unowned,
+  // A +1; the receiver stays the caller's.
+  Owned,
+  // A +1 in place of the receiver, whose reference the caller passed in.
+  ReplacesReceiver,
+};
+
 struct MethodCallbackWrapper {
  public:
-  MethodCallbackWrapper(v8::Isolate* isolate,
-                        std::shared_ptr<v8::Persistent<v8::Value>> callback,
-                        const uint8_t initialParamIndex,
-                        const uint8_t paramsCount,
-                        const TypeEncoding* typeEncoding)
+  MethodCallbackWrapper(
+      v8::Isolate* isolate, std::shared_ptr<v8::Persistent<v8::Value>> callback,
+      const uint8_t initialParamIndex, const uint8_t paramsCount,
+      const TypeEncoding* typeEncoding,
+      NativeReturnOwnership returnOwnership = NativeReturnOwnership::Unowned)
       : isolateWrapper_(isolate),
         callback_(callback),
         initialParamIndex_(initialParamIndex),
         paramsCount_(paramsCount),
-        typeEncoding_(typeEncoding) {}
+        typeEncoding_(typeEncoding),
+        returnOwnership_(returnOwnership) {}
   IsolateWrapper isolateWrapper_;
   std::shared_ptr<v8::Persistent<v8::Value>> callback_;
   const uint8_t initialParamIndex_;
   const uint8_t paramsCount_;
   const TypeEncoding* typeEncoding_;
+  const NativeReturnOwnership returnOwnership_;
 };
 
 class ArgConverter {
@@ -71,6 +82,21 @@ class ArgConverter {
   static const ProtocolMeta* FindProtocolMeta(Protocol* protocol);
   static void MethodCallback(ffi_cif* cif, void* retValue, void** argValues,
                              void* userData);
+  // For a JS implementation of the instance method `selector`; `meta` is null
+  // for methods the metadata does not describe, which are classified by
+  // their selector's method family.
+  static NativeReturnOwnership ReturnOwnership(SEL selector,
+                                               const MethodMeta* meta);
+  // Gives the native caller of a JS implementation the reference it owns of
+  // the object SetValue wrote into retValue. `consumedArgument`, an argument
+  // the method took over, already carries that reference when it is the
+  // result.
+  static void RetainOwnedResult(v8::Isolate* isolate,
+                                NativeReturnOwnership ownership,
+                                void** argValues, void* retValue,
+                                const TypeEncoding* typeEncoding,
+                                v8::Local<v8::Value> value,
+                                id consumedArgument = nil);
   // returnSize is the ABI size of the return slot (cif->rtype->size); an
   // empty, null or undefined value zeroes all of it.
   static void SetValue(v8::Local<v8::Context> context, void* retValue,

NativeScript/runtime/ArgConverter.mm

@@ -36,45 +36,43 @@ bool IsDeallocating(id obj) {
   return !((BOOL (*)(id, SEL))imp)(obj, allowsWeakReference);
 }
 
-// Clang's init method family: optional leading underscores, then "init", then
-// the end of the selector's first piece or anything but a lowercase letter.
-bool IsInitFamily(SEL selector) {
+// Clang's method family rule: optional leading underscores, then the family
+// name, then the end of the selector's first piece or anything but a
+// lowercase letter.
+bool InMethodFamily(SEL selector, const char* family) {
   const char* name = sel_getName(selector);
   while (*name == '_') {
     name++;
   }
-  return strncmp(name, "init", 4) == 0 && (name[4] < 'a' || name[4] > 'z');
+  size_t length = strlen(family);
+  return strncmp(name, family, length) == 0 && (name[length] < 'a' || name[length] > 'z');
 }
 
-// The native caller of an init-family method owns its result. SetValue hands
-// a wrapped object or a string over at +0: for the receiver that is the
-// reference the caller passed in, any other object needs its own. The
-// receiver is left alone: when a super initializer substituted it, that
-// initializer already released it, and nothing here can tell that case apart.
-void RetainInitializerResult(Isolate* isolate, void** argValues, void* retValue,
-                             const TypeEncoding* typeEncoding, Local<Value> value) {
+bool IsObjectEncoding(const TypeEncoding* typeEncoding) {
   BinaryTypeEncodingType type = typeEncoding->type;
-  if (type != BinaryTypeEncodingType::IdEncoding &&
-      type != BinaryTypeEncodingType::InterfaceDeclarationReference &&
-      type != BinaryTypeEncodingType::InstanceTypeEncoding) {
-    return;
-  }
+  return type == BinaryTypeEncodingType::IdEncoding ||
+         type == BinaryTypeEncodingType::InterfaceDeclarationReference ||
+         type == BinaryTypeEncodingType::InstanceTypeEncoding;
+}
 
-  id returned = *static_cast<id*>(retValue);
-  if (returned == nil || returned == *static_cast<id*>(argValues[0]) ||
-      !IsInitFamily(*static_cast<SEL*>(argValues[1]))) {
-    return;
+// The argument a JS-implemented method takes over from its native caller, or
+// nil. Reads only the native call, so it is usable without the isolate.
+id ConsumedArgument(const MethodCallbackWrapper* data, void** argValues) {
+  if (data->initialParamIndex_ != 2) {
+    return nil;
   }
-
-  // A plain JS object becomes a DictionaryAdapter SetValue already returns
-  // at +1.
-  value = tns::UnwrapProxy(value);
-  BaseDataWrapper* wrapper = value->IsObject() ? tns::GetValue(isolate, value) : nullptr;
-  bool atPlusZero =
-      value->IsString() || (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject);
-  if (atPlusZero) {
-    [returned retain];
+  SEL selector = *static_cast<SEL*>(argValues[1]);
+  for (int i = 0; i < data->paramsCount_; i++) {
+    if (!Interop::ConsumesArgument(selector, i)) {
+      continue;
+    }
+    const TypeEncoding* typeEncoding = data->typeEncoding_;
+    for (int j = 0; j <= i; j++) {
+      typeEncoding = typeEncoding->next();
+    }
+    return IsObjectEncoding(typeEncoding) ? *static_cast<id*>(argValues[i + 2]) : nil;
   }
+  return nil;
 }
 
 // The borrowed wrapper WrapNativeCallbackSelf made for an object in -dealloc
@@ -192,7 +190,7 @@ explicit BorrowedSelfScope(Isolate* isolate) : isolate(isolate) {}
     target = [allocKlass alloc];
   }
 
-  ObjCMethodCall methodCall(context, meta, target, klass, args, callSuper);
+  ObjCMethodCall methodCall(context, meta, target, klass, args, callSuper, allocKlass != nil);
   return Interop::CallFunction(methodCall);
 }
 
@@ -364,6 +362,8 @@ explicit BorrowedSelfScope(Isolate* isolate) : isolate(isolate) {}
 
 void ArgConverter::MethodCallback(ffi_cif* cif, void* retValue, void** argValues, void* userData) {
   MethodCallbackWrapper* data = static_cast<MethodCallbackWrapper*>(userData);
+  // Owned by this call, which gives it up on every path unless it answers it.
+  id consumedArgument = ConsumedArgument(data, argValues);
 
   Isolate* isolate = data->isolateWrapper_.Isolate();
 
@@ -380,6 +380,7 @@ explicit BorrowedSelfScope(Isolate* isolate) : isolate(isolate) {}
     IsolatePin pin = data->isolateWrapper_.Pin();
     if (!pin || !data->isolateWrapper_.IsValid()) {
       memset(retValue, 0, cif->rtype->size);
+      [consumedArgument release];
       return;
     }
 
@@ -391,6 +392,7 @@ explicit BorrowedSelfScope(Isolate* isolate) : isolate(isolate) {}
     // what makes this second look reliable.
     if (!data->isolateWrapper_.IsValid()) {
       memset(retValue, 0, cif->rtype->size);
+      [consumedArgument release];
       return;
     }
     Isolate::Scope isolate_scope(isolate);
@@ -513,20 +515,71 @@ explicit BorrowedSelfScope(Isolate* isolate) : isolate(isolate) {}
         memset(retValue, 0, cif->rtype->size);
       } else {
         ArgConverter::SetValue(context, retValue, cif->rtype->size, result, data->typeEncoding_);
-        if (data->initialParamIndex_ == 2) {
-          RetainInitializerResult(isolate, argValues, retValue, data->typeEncoding_, result);
-        }
+        ArgConverter::RetainOwnedResult(isolate, data->returnOwnership_, argValues, retValue,
+                                        data->typeEncoding_, result, consumedArgument);
       }
     } else {
       memset(retValue, 0, cif->rtype->size);
     }
+
+    // A wrapper JS made for the consumed argument holds a reference of its own.
+    if (consumedArgument != nil && !(IsObjectEncoding(data->typeEncoding_) &&
+                                     *static_cast<id*>(retValue) == consumedArgument)) {
+      [consumedArgument release];
+    }
   }  // all V8 scopes destruct here
 
   if (pendingThrow != nil) {
     @throw pendingThrow;
   }
 }
 
+// SetValue hands a wrapped object or a string over at +0, so an owned result
+// gets a reference of its own, except a receiver answered in place of itself:
+// that is the reference the caller passed in. The receiver is never released
+// when another object is answered instead: a super initializer that
+// substituted it already did, and nothing here can tell that case apart.
+void ArgConverter::RetainOwnedResult(Isolate* isolate, NativeReturnOwnership ownership,
+                                     void** argValues, void* retValue,
+                                     const TypeEncoding* typeEncoding, Local<Value> value,
+                                     id consumedArgument) {
+  if (ownership == NativeReturnOwnership::Unowned || !IsObjectEncoding(typeEncoding)) {
+    return;
+  }
+
+  id returned = *static_cast<id*>(retValue);
+  if (returned == nil || returned == consumedArgument ||
+      (ownership == NativeReturnOwnership::ReplacesReceiver &&
+       returned == *static_cast<id*>(argValues[0]))) {
+    return;
+  }
+
+  // A plain JS object becomes a DictionaryAdapter SetValue already returns
+  // at +1. SetValue writes a string only into an id or interface slot; for
+  // instancetype it leaves the slot unwritten.
+  value = tns::UnwrapProxy(value);
+  BaseDataWrapper* wrapper = value->IsObject() ? tns::GetValue(isolate, value) : nullptr;
+  bool atPlusZero =
+      (value->IsString() && typeEncoding->type != BinaryTypeEncodingType::InstanceTypeEncoding) ||
+      (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject);
+  if (atPlusZero) {
+    [returned retain];
+  }
+}
+
+NativeReturnOwnership ArgConverter::ReturnOwnership(SEL selector, const MethodMeta* meta) {
+  bool isInitializer = meta != nullptr ? meta->isInitializer() : InMethodFamily(selector, "init");
+  if (Interop::ConsumesReceiver(selector, isInitializer)) {
+    return NativeReturnOwnership::ReplacesReceiver;
+  }
+
+  bool owned = meta != nullptr
+                   ? meta->ownsReturnedCocoaObject()
+                   : InMethodFamily(selector, "copy") || InMethodFamily(selector, "mutableCopy") ||
+                         InMethodFamily(selector, "new") || Interop::ReturnsRetained(selector);
+  return owned ? NativeReturnOwnership::Owned : NativeReturnOwnership::Unowned;
+}
+
 void ArgConverter::SetValue(Local<Context> context, void* retValue, size_t returnSize,
                             Local<Value> value, const TypeEncoding* typeEncoding) {
   if (typeEncoding->type == BinaryTypeEncodingType::VoidEncoding) {
@@ -706,7 +759,7 @@ explicit BorrowedSelfScope(Isolate* isolate) : isolate(isolate) {}
     // 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) {
+    if (Interop::ReleasesCachedOwnedReturn(resultIsOwned, OwnedReturnReceiver::AllocatedForCall)) {
       [result release];
     }
     info.GetReturnValue().Set(poInstance->Get(isolate));

NativeScript/runtime/ClassBuilder.mm

@@ -690,7 +690,8 @@ void ScopeClassNameToIsolate(std::string& name, int isolateId) {
       std::shared_ptr<Persistent<Value>> poCallback =
           std::make_shared<Persistent<Value>>(isolate, method);
       MethodCallbackWrapper* userData =
-          new MethodCallbackWrapper(isolate, poCallback, 2, argsCount, typeEncoding);
+          new MethodCallbackWrapper(isolate, poCallback, 2, argsCount, typeEncoding,
+                                    ArgConverter::ReturnOwnership(selector, nullptr));
       IMP methodBody =
           Interop::CreateMethod(2, argsCount, typeEncoding, ArgConverter::MethodCallback, userData);
       std::string typeInfo = GetTypeEncoding(typeEncoding, argsCount);
@@ -793,9 +794,10 @@ void ScopeClassNameToIsolate(std::string& name, int isolateId) {
           std::make_shared<Persistent<Value>>(isolate, method);
       const TypeEncoding* typeEncoding = methodMeta->encodings()->first();
       uint8_t argsCount = methodMeta->encodings()->count - 1;
-      MethodCallbackWrapper* userData =
-          new MethodCallbackWrapper(isolate, poCallback, 2, argsCount, typeEncoding);
       SEL selector = methodMeta->selector();
+      MethodCallbackWrapper* userData =
+          new MethodCallbackWrapper(isolate, poCallback, 2, argsCount, typeEncoding,
+                                    ArgConverter::ReturnOwnership(selector, methodMeta));
       IMP methodBody =
           Interop::CreateMethod(2, argsCount, typeEncoding, ArgConverter::MethodCallback, userData);
       std::string typeInfo = GetTypeEncoding(typeEncoding, argsCount);
@@ -931,8 +933,13 @@ void ScopeClassNameToIsolate(std::string& name, int isolateId) {
             // Default getter result on a JS throw: zeroed return buffer.
             memset(retValue, 0, cif->rtype->size);
           } else {
-            const TypeEncoding* typeEncoding = context->meta_->getter()->encodings()->first();
+            const MethodMeta* getterMeta = context->meta_->getter();
+            const TypeEncoding* typeEncoding = getterMeta->encodings()->first();
             ArgConverter::SetValue(v8Context, retValue, cif->rtype->size, res, typeEncoding);
+            ArgConverter::RetainOwnedResult(
+                isolate,
+                ArgConverter::ReturnOwnership(*static_cast<SEL*>(argValues[1]), getterMeta),
+                argValues, retValue, typeEncoding, res);
           }
         }
         if (pendingThrow != nil) {

NativeScript/runtime/Interop.h

@@ -15,6 +15,21 @@ namespace tns {
 typedef void (*FFIMethodCallback)(ffi_cif* cif, void* retValue,
                                   void** argValues, void* userData);
 
+// The receiver of a call that returns an object, as far as releasing an owned
+// result that already has a wrapper is concerned.
+enum class OwnedReturnReceiver : uint8_t {
+  // A class method or a C function.
+  None,
+  // An instance method that leaves its receiver to the caller.
+  Kept,
+  // An initializer or NS_REPLACES_RECEIVER method sent to a receiver that
+  // existed before the call.
+  Consumed,
+  // An initializer or NS_REPLACES_RECEIVER method sent to a receiver the
+  // runtime allocated for this call.
+  AllocatedForCall,
+};
+
 struct MethodCall {
   MethodCall(v8::Local<v8::Context> context, bool isPrimitiveFunction,
              void* functionPointer, const TypeEncoding* typeEncoding,
@@ -50,6 +65,7 @@ struct MethodCall {
   bool ownsReturnedObject_;
   bool returnsUnmanaged_;
   bool isInitializer_;
+  bool receiverAllocatedForCall_ = false;
 };
 
 struct CMethodCall : MethodCall {
@@ -63,15 +79,18 @@ struct CMethodCall : MethodCall {
 
 struct ObjCMethodCall : public MethodCall {
   ObjCMethodCall(v8::Local<v8::Context> context, const MethodMeta* meta,
-                 id target, Class clazz, V8Args& args, bool callSuper)
+                 id target, Class clazz, V8Args& args, bool callSuper,
+                 bool receiverAllocatedForCall = false)
       : MethodCall(context, false,
                    callSuper ? (void*)objc_msgSendSuper : (void*)objc_msgSend,
                    meta->encodings()->first(), args, target, clazz,
                    meta->selector(), callSuper, meta->type(),
                    meta->hasErrorOutParameter() &&
                        args.Length() < meta->encodings()->count - 1,
                    meta->ownsReturnedCocoaObject(), false,
-                   meta->isInitializer()) {}
+                   meta->isInitializer()) {
+    receiverAllocatedForCall_ = receiverAllocatedForCall;
+  }
 };
 
 class Interop {
@@ -96,19 +115,37 @@ class Interop {
                             Class clazz, V8Args& args);
   static v8::Local<v8::Value> CallFunction(ObjCMethodCall& methodCall);
   static v8::Local<v8::Value> CallFunction(CMethodCall& methodCall);
+  // Whether a method answers in place of its receiver, consuming the caller's
+  // reference to it: the init family, and NS_REPLACES_RECEIVER methods, which
+  // the metadata does not record.
+  static bool ConsumesReceiver(SEL selector, bool isInitializer);
+  // Whether a method takes over the caller's reference to its argument at
+  // `index`, counted from the first after the selector: NS_RELEASES_ARGUMENT
+  // parameters, which the metadata does not record.
+  static bool ConsumesArgument(SEL selector, int index);
+  // Whether a method outside the copy, mutableCopy and new families answers
+  // an object its caller owns (NS_RETURNS_RETAINED), for implementations the
+  // metadata does not describe.
+  static bool ReturnsRetained(SEL selector);
+  // Whether an owned object result that already has a wrapper carries a +1
+  // the caller must drop, the Instances entry holding its own reference.
+  static bool ReleasesCachedOwnedReturn(bool ownsReturnedObject,
+                                        OwnedReturnReceiver receiver);
   static v8::Local<v8::Value> GetResultByType(
       v8::Local<v8::Context> context, BaseDataWrapper* typeWrapper,
       BaseCall* call,
       v8::Local<v8::Object> parentStruct = v8::Local<v8::Object>());
   // A non-empty `parentStruct` makes a struct-typed result a view into the
-  // parent's buffer (`call` points at the field) instead of a copy.
+  // parent's buffer (`call` points at the field) instead of a copy. The
+  // default `ownedReturnReceiver` never releases an owned result found already
+  // wrapped.
   static v8::Local<v8::Value> GetResult(
       v8::Local<v8::Context> context, const TypeEncoding* typeEncoding,
       BaseCall* call, bool marshalToPrimitive,
       v8::Local<v8::Object> parentStruct = v8::Local<v8::Object>(),
       bool isStructMember = false, bool ownsReturnedObject = false,
       bool returnsUnmanaged = false, bool isInitializer = false,
-      bool releaseCachedOwnedReturn = false);
+      OwnedReturnReceiver ownedReturnReceiver = OwnedReturnReceiver::Consumed);
   static void SetStructPropertyValue(v8::Local<v8::Context> context,
                                      StructWrapper* wrapper,
                                      const StructField& field,