From 100a9b40a98adbe677f0bc6d7c56478bba3c8bb6 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Sat, 14 Nov 2026 16:23:42 -0300 Subject: [PATCH 1/4] fix(runtime): release owned instance-method returns that hit a cached wrapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An owned result (copy, mutableCopy and new families, ns_returns_retained, initializers) whose object already had a JS wrapper kept its +1 when the method was an instance method: the Instances entry holds its own reference, so the object leaked one retain per call. `immutable.copy()` and `X.alloc().init…()` on a class whose initializer wraps the object (a JS initializer, or a native one calling a JS-implemented method) both hit it. The release was limited to class methods and C functions because two kinds of owned instance returns carried no net +1: methods that consume their receiver and answer it, and JS-implemented methods, which handed results to native callers at +0. The second was a bug of its own: a native caller of a JS copyWithZone:, mutableCopyWithZone: or new-family method released an object it did not own, freeing it under a live wrapper. - MethodCallback hands an owned result to native callers at +1: always for the copy/mutableCopy/new families and metadata-flagged owned returns (the receiver included), and for initializers and -awakeAfterUsingCoder: when the result is not the receiver. The classification comes from the method's metadata, or from the selector's method family for methods the metadata does not describe, and is stored on the MethodCallbackWrapper. - Interop::ReleasesCachedOwnedReturn is the one cache-hit rule, used by GetResult and ConstructObject: release unless the call consumed a receiver that existed before it (re-init, super.init…() inside a JS initializer, NS_REPLACES_RECEIVER on a live object). An initializer on a receiver allocated for the call releases whatever it answers; Invoke marks such calls on ObjCMethodCall. - Interop::ConsumesReceiver treats the init family and -awakeAfterUsingCoder: (the SDK's only NS_REPLACES_RECEIVER method) as receiver-consuming; the metadata does not record ns_consumes_self. Fixes #566 --- NativeScript/runtime/ArgConverter.h | 28 ++- NativeScript/runtime/ArgConverter.mm | 50 +++-- NativeScript/runtime/ClassBuilder.mm | 8 +- NativeScript/runtime/Interop.h | 37 +++- NativeScript/runtime/Interop.mm | 41 +++- NativeScript/runtime/MetadataBuilder.mm | 3 +- TestFixtures/Marshalling/TNSAllocLog.h | 14 ++ TestFixtures/Marshalling/TNSAllocLog.m | 29 +++ .../app/tests/NativeCallbackSelfTests.js | 209 ++++++++++++++++++ 9 files changed, 377 insertions(+), 42 deletions(-) diff --git a/NativeScript/runtime/ArgConverter.h b/NativeScript/runtime/ArgConverter.h index b8bcfe5a..7a889ba3 100644 --- a/NativeScript/runtime/ArgConverter.h +++ b/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> callback, - const uint8_t initialParamIndex, - const uint8_t paramsCount, - const TypeEncoding* typeEncoding) + MethodCallbackWrapper( + v8::Isolate* isolate, std::shared_ptr> 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> callback_; const uint8_t initialParamIndex_; const uint8_t paramsCount_; const TypeEncoding* typeEncoding_; + const NativeReturnOwnership returnOwnership_; }; class ArgConverter { @@ -71,6 +82,11 @@ 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); // 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 context, void* retValue, diff --git a/NativeScript/runtime/ArgConverter.mm b/NativeScript/runtime/ArgConverter.mm index f83403f8..78cae0ec 100644 --- a/NativeScript/runtime/ArgConverter.mm +++ b/NativeScript/runtime/ArgConverter.mm @@ -36,23 +36,25 @@ 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) { +// 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 RetainOwnedResult(Isolate* isolate, NativeReturnOwnership ownership, void** argValues, + void* retValue, const TypeEncoding* typeEncoding, Local value) { BinaryTypeEncodingType type = typeEncoding->type; if (type != BinaryTypeEncodingType::IdEncoding && type != BinaryTypeEncodingType::InterfaceDeclarationReference && @@ -61,8 +63,8 @@ void RetainInitializerResult(Isolate* isolate, void** argValues, void* retValue, } id returned = *static_cast(retValue); - if (returned == nil || returned == *static_cast(argValues[0]) || - !IsInitFamily(*static_cast(argValues[1]))) { + if (returned == nil || (ownership == NativeReturnOwnership::ReplacesReceiver && + returned == *static_cast(argValues[0]))) { return; } @@ -192,7 +194,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); } @@ -513,8 +515,9 @@ 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); + if (data->returnOwnership_ != NativeReturnOwnership::Unowned) { + RetainOwnedResult(isolate, data->returnOwnership_, argValues, retValue, + data->typeEncoding_, result); } } } else { @@ -527,6 +530,19 @@ explicit BorrowedSelfScope(Isolate* isolate) : isolate(isolate) {} } } +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"); + return owned ? NativeReturnOwnership::Owned : NativeReturnOwnership::Unowned; +} + void ArgConverter::SetValue(Local context, void* retValue, size_t returnSize, Local value, const TypeEncoding* typeEncoding) { if (typeEncoding->type == BinaryTypeEncodingType::VoidEncoding) { @@ -706,7 +722,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)); diff --git a/NativeScript/runtime/ClassBuilder.mm b/NativeScript/runtime/ClassBuilder.mm index 3baac770..40289ae9 100644 --- a/NativeScript/runtime/ClassBuilder.mm +++ b/NativeScript/runtime/ClassBuilder.mm @@ -690,7 +690,8 @@ void ScopeClassNameToIsolate(std::string& name, int isolateId) { std::shared_ptr> poCallback = std::make_shared>(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>(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); diff --git a/NativeScript/runtime/Interop.h b/NativeScript/runtime/Interop.h index 9dcfed69..8bfd7d57 100644 --- a/NativeScript/runtime/Interop.h +++ b/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 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,7 +79,8 @@ struct CMethodCall : MethodCall { struct ObjCMethodCall : public MethodCall { ObjCMethodCall(v8::Local 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, @@ -71,7 +88,9 @@ struct ObjCMethodCall : public MethodCall { meta->hasErrorOutParameter() && args.Length() < meta->encodings()->count - 1, meta->ownsReturnedCocoaObject(), false, - meta->isInitializer()) {} + meta->isInitializer()) { + receiverAllocatedForCall_ = receiverAllocatedForCall; + } }; class Interop { @@ -96,19 +115,29 @@ class Interop { Class clazz, V8Args& args); static v8::Local CallFunction(ObjCMethodCall& methodCall); static v8::Local 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 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 GetResultByType( v8::Local context, BaseDataWrapper* typeWrapper, BaseCall* call, v8::Local parentStruct = v8::Local()); // 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 GetResult( v8::Local context, const TypeEncoding* typeEncoding, BaseCall* call, bool marshalToPrimitive, v8::Local parentStruct = v8::Local(), bool isStructMember = false, bool ownsReturnedObject = false, bool returnsUnmanaged = false, bool isInitializer = false, - bool releaseCachedOwnedReturn = false); + OwnedReturnReceiver ownedReturnReceiver = OwnedReturnReceiver::Consumed); static void SetStructPropertyValue(v8::Local context, StructWrapper* wrapper, const StructField& field, diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index 5cbc8f40..8bc057b6 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -168,6 +168,19 @@ return Interop::CallFunctionInternal(methodCall); } +bool Interop::ConsumesReceiver(SEL selector, bool isInitializer) { + return isInitializer || selector == @selector(awakeAfterUsingCoder:); +} + +// Balanced only while every owned result other than a consumed receiver +// reaches here at +1, JS-implemented methods included: MethodCallback retains +// what a JS implementation answers at +0, and for initializers anything but +// the receiver. AllocatedForCall does not check that the result is the +// receiver, so narrowing that retain makes this over-release. +bool Interop::ReleasesCachedOwnedReturn(bool ownsReturnedObject, OwnedReturnReceiver receiver) { + return ownsReturnedObject && receiver != OwnedReturnReceiver::Consumed; +} + id Interop::CallInitializer(Local context, const MethodMeta* methodMeta, id target, Class clazz, V8Args& args) { const TypeEncoding* typeEncoding = methodMeta->encodings()->first(); @@ -1029,7 +1042,7 @@ static bool SnapshotArrayLike(Isolate* isolate, Local object, Local Interop::GetResult(Local context, const TypeEncoding* typeEncoding, BaseCall* call, bool marshalToPrimitive, Local parentStruct, bool isStructMember, bool ownsReturnedObject, bool returnsUnmanaged, - bool isInitializer, bool releaseCachedOwnedReturn) { + bool isInitializer, OwnedReturnReceiver ownedReturnReceiver) { Isolate* isolate = v8::Isolate::GetCurrent(); if (returnsUnmanaged) { @@ -1364,12 +1377,7 @@ static bool SnapshotArrayLike(Isolate* isolate, Local object, Local cached = poInstance->Get(isolate); - // The Instances entry holds its own reference, so a returned +1 is - // surplus. Only class methods and C functions qualify: an instance - // method can hand back its receiver without a net retain (initializers, - // NS_REPLACES_RECEIVER) or come from a JS implementation, which returns - // at +0. - if (ownsReturnedObject && releaseCachedOwnedReturn) { + if (ReleasesCachedOwnedReturn(ownsReturnedObject || isInitializer, ownedReturnReceiver)) { [result release]; } return cached; @@ -1937,10 +1945,21 @@ throw NativeScriptException( } } - Local result = Interop::GetResult( - methodCall.context_, methodCall.typeEncoding_, &call, marshalToPrimitive, Local(), - false, methodCall.ownsReturnedObject_, methodCall.returnsUnmanaged_, - methodCall.isInitializer_, !isInstanceMethod); + OwnedReturnReceiver receiver = OwnedReturnReceiver::None; + if (isInstanceMethod) { + if (!ConsumesReceiver(methodCall.selector_, methodCall.isInitializer_)) { + receiver = OwnedReturnReceiver::Kept; + } else if (methodCall.receiverAllocatedForCall_) { + receiver = OwnedReturnReceiver::AllocatedForCall; + } else { + receiver = OwnedReturnReceiver::Consumed; + } + } + + Local result = + Interop::GetResult(methodCall.context_, methodCall.typeEncoding_, &call, marshalToPrimitive, + Local(), false, methodCall.ownsReturnedObject_, + methodCall.returnsUnmanaged_, methodCall.isInitializer_, receiver); // Owned returns, initializers and class factory methods hand out objects JS // most likely holds the last reference to. Instance getters do not: their diff --git a/NativeScript/runtime/MetadataBuilder.mm b/NativeScript/runtime/MetadataBuilder.mm index eca54cce..c2add51e 100644 --- a/NativeScript/runtime/MetadataBuilder.mm +++ b/NativeScript/runtime/MetadataBuilder.mm @@ -1397,7 +1397,8 @@ CMethodCall methodCall(context, item->userData_, typeEncoding, args, std::shared_ptr> poCallback = std::make_shared>(isolate, value); MethodCallbackWrapper* userData = - new MethodCallbackWrapper(isolate, poCallback, 2, argsCount, typeEncoding); + new MethodCallbackWrapper(isolate, poCallback, 2, argsCount, typeEncoding, + ArgConverter::ReturnOwnership(methodMeta->selector(), methodMeta)); IMP methodBody = Interop::CreateMethod(2, argsCount, typeEncoding, ArgConverter::MethodCallback, userData); IMP nativeImp = diff --git a/TestFixtures/Marshalling/TNSAllocLog.h b/TestFixtures/Marshalling/TNSAllocLog.h index ef80f9a9..a384c36d 100644 --- a/TestFixtures/Marshalling/TNSAllocLog.h +++ b/TestFixtures/Marshalling/TNSAllocLog.h @@ -19,4 +19,18 @@ @end +// Native callers that own what an owned-return message answers and drop it +// straight away. +@interface TNSOwnedReturns : TNSAllocLog + +// Answers a new TNSAllocLog. +- (id)newChild; + ++ (void)copyAndDrop:(id)object; ++ (void)mutableCopyAndDrop:(id)object; ++ (void)newChildAndDrop:(TNSOwnedReturns*)object; ++ (void)awakeAndDrop:(id)object; + +@end + #endif /* TNSAllocLog_h */ diff --git a/TestFixtures/Marshalling/TNSAllocLog.m b/TestFixtures/Marshalling/TNSAllocLog.m index e04bb917..c70b76bd 100644 --- a/TestFixtures/Marshalling/TNSAllocLog.m +++ b/TestFixtures/Marshalling/TNSAllocLog.m @@ -29,3 +29,32 @@ - (instancetype)init { } @end + +@implementation TNSOwnedReturns + +- (id)newChild { + return [[TNSAllocLog alloc] init]; +} + ++ (void)copyAndDrop:(id)object { + id result = [object copy]; + (void)result; +} + ++ (void)mutableCopyAndDrop:(id)object { + id result = [object mutableCopy]; + (void)result; +} + ++ (void)newChildAndDrop:(TNSOwnedReturns*)object { + id result = [object newChild]; + (void)result; +} + ++ (void)awakeAndDrop:(id)object { + NSCoder* coder = nil; + id result = [object awakeAfterUsingCoder:coder]; + (void)result; +} + +@end diff --git a/TestRunner/app/tests/NativeCallbackSelfTests.js b/TestRunner/app/tests/NativeCallbackSelfTests.js index 049ae5e2..9822939d 100644 --- a/TestRunner/app/tests/NativeCallbackSelfTests.js +++ b/TestRunner/app/tests/NativeCallbackSelfTests.js @@ -408,4 +408,213 @@ describe("Native callback self wrappers", function () { }); }); }); + + it("leaves the retain count of an immutable object whose -copy answers itself unchanged", function (done) { + var array = NSArray.arrayWithArray([1, 2, 3]); + settle(2, function () { + var before = array.retainCount(); + var same = 0; + for (var i = 0; i < 50; i++) { + if (array.copy() === array) { + same++; + } + } + expect(same).toBe(50); + settle(2, function () { + expect(array.retainCount()).toBe(before); + done(); + }); + }); + }); + + it("frees an object JS alone holds after alloc().init() ran its JS initializer", function (done) { + var obj = JSInit.alloc().init(); + obj.state = { i: 3 }; + settle(2, function () { + expect(initCalls).toBe(1); + expect(obj.state.i).toBe(3); + expect(deallocs()).toBe(0); + obj = null; + settle(3, function () { + expect(deallocs()).toBe(1); + done(); + }); + }); + }); + + it("keeps the JS state of a natively owned object whose JS initializer ran from alloc().init(), then frees it once", function (done) { + var owner = NSMutableArray.new(); + (function () { + var obj = JSInit.alloc().init(); + obj.state = { i: 4 }; + owner.addObject(obj); + })(); + settle(3, function () { + var obj = owner.objectAtIndex(0); + expect(initCalls).toBe(1); + expect(obj.initialized).toBe(true); + expect(obj.state && obj.state.i).toBe(4); + obj = null; + expect(deallocs()).toBe(0); + owner.removeAllObjects(); + settle(3, function () { + expect(deallocs()).toBe(1); + done(); + }); + }); + }); + + it("keeps a wrapped object a JS initializer returns instead of its receiver (alloc().init())", function (done) { + expectInitializerResultOwnedByCaller(function () { + return createThroughInitReturningOther(function () { + return InitReturnsOther.alloc().init(); + }); + }, done); + }); + + it("keeps the substitute a super initializer returned through a JS initializer (alloc().init())", function (done) { + expectInitializerResultOwnedByCaller(function () { + return InitReturnsSubstitute.alloc().init(); + }, done); + }); + + it("does not release a wrapped receiver that is initialized again", function (done) { + var plain = TNSAllocLog.new(); + var jsInit = JSInit.new(); + expect(plain.init()).toBe(plain); + expect(TNSAllocLog.prototype.init.call(plain)).toBe(plain); + expect(TNSAllocLog.prototype.init.apply(plain, [])).toBe(plain); + expect(jsInit.init()).toBe(jsInit); + expect(JSInit.prototype.init.call(jsInit)).toBe(jsInit); + expect(JSInit.prototype.init.apply(jsInit, [])).toBe(jsInit); + expect(TNSAllocLog.prototype.init.call(jsInit)).toBe(jsInit); + settle(3, function () { + var freed = deallocs(); + expect(freed).toBe(0); + if (freed === 0) { + expect(plain.hash > 0).toBe(true); + expect(jsInit.initialized).toBe(true); + } + plain = null; + jsInit = null; + settle(3, function () { + expect(deallocs()).toBe(2); + done(); + }); + }); + }); + + // Native code that owns what an owned-return method answers drops it. The + // object a JS implementation answered must outlive that, and be freed + // exactly once after JS lets go. TNSOwnedReturns and TNSAllocLog log their + // deallocation; `freedAfterwards` counts those among receiver and result. + function expectKeptThroughNativeDrop(state, drop, freedAfterwards, done) { + drop(state.receiver); + settle(3, function () { + var freed = deallocs(); + expect(freed).toBe(0); + if (freed === 0) { + expect(state.receiver.hash > 0).toBe(true); + expect(state.result.hash > 0).toBe(true); + } + state.receiver = null; + state.result = null; + settle(3, function () { + expect(deallocs()).toBe(freedAfterwards); + done(); + }); + }); + } + + function ownedReturnState(base, name, method, protocols, answersReceiver) { + var state = {}; + var implementation = {}; + implementation[method] = function () { + return answersReceiver ? this : state.result; + }; + var Klass = base.extend(implementation, { name: name, protocols: protocols }); + state.receiver = Klass.new(); + state.result = answersReceiver ? state.receiver : TNSAllocLog.new(); + return state; + } + + it("hands a native -copy the receiver a JS copyWithZone answers at +1", function (done) { + var state = ownedReturnState(TNSOwnedReturns, "NativeCallbackSelfCopySelf", "copyWithZone", [NSCopying], true); + expectKeptThroughNativeDrop(state, function (receiver) { + TNSOwnedReturns.copyAndDrop(receiver); + }, 1, done); + }); + + it("hands a native -copy the other object a JS copyWithZone answers at +1", function (done) { + var state = ownedReturnState(TNSOwnedReturns, "NativeCallbackSelfCopyOther", "copyWithZone", [NSCopying], false); + expectKeptThroughNativeDrop(state, function (receiver) { + TNSOwnedReturns.copyAndDrop(receiver); + }, 2, done); + }); + + it("hands a native -mutableCopy the object a JS mutableCopyWithZone answers at +1", function (done) { + var state = ownedReturnState(TNSOwnedReturns, "NativeCallbackSelfMutableCopy", "mutableCopyWithZone", [NSMutableCopying], false); + expectKeptThroughNativeDrop(state, function (receiver) { + TNSOwnedReturns.mutableCopyAndDrop(receiver); + }, 2, done); + }); + + it("hands a native caller the object a JS new-family method answers at +1", function (done) { + var state = ownedReturnState(TNSOwnedReturns, "NativeCallbackSelfNewOther", "newChild", [], false); + expectKeptThroughNativeDrop(state, function (receiver) { + TNSOwnedReturns.newChildAndDrop(receiver); + }, 2, done); + }); + + it("hands a native caller the receiver a JS new-family method answers at +1", function (done) { + var state = ownedReturnState(TNSOwnedReturns, "NativeCallbackSelfNewSelf", "newChild", [], true); + expectKeptThroughNativeDrop(state, function (receiver) { + TNSOwnedReturns.newChildAndDrop(receiver); + }, 1, done); + }); + + it("hands a native caller back the receiver a JS awakeAfterUsingCoder answers", function (done) { + var state = ownedReturnState(TNSOwnedReturns, "NativeCallbackSelfAwakeSelf", "awakeAfterUsingCoder", [], true); + expectKeptThroughNativeDrop(state, function (receiver) { + TNSOwnedReturns.awakeAndDrop(receiver); + }, 1, done); + }); + + // The receiver is not a TNSAllocLog: its reference, consumed by the call, + // is not given back when the JS method answers another object. + it("hands a native caller the other object a JS awakeAfterUsingCoder answers at +1", function (done) { + var state = ownedReturnState(NSObject, "NativeCallbackSelfAwakeOther", "awakeAfterUsingCoder", [], false); + expectKeptThroughNativeDrop(state, function (receiver) { + TNSOwnedReturns.awakeAndDrop(receiver); + }, 1, done); + }); + + function expectBalancedJsCopies(answersReceiver, name, done) { + var state = ownedReturnState(TNSOwnedReturns, name, "copyWithZone", [NSCopying], answersReceiver); + for (var i = 0; i < 20; i++) { + expect(state.receiver.copy()).toBe(state.result); + } + settle(3, function () { + var freed = deallocs(); + expect(freed).toBe(0); + if (freed === 0) { + expect(state.receiver.hash > 0).toBe(true); + expect(state.result.hash > 0).toBe(true); + } + state.receiver = null; + state.result = null; + settle(3, function () { + expect(deallocs()).toBe(answersReceiver ? 1 : 2); + done(); + }); + }); + } + + it("balances copy() called from JS on a JS copyWithZone that answers its receiver", function (done) { + expectBalancedJsCopies(true, "NativeCallbackSelfJsCopySelf", done); + }); + + it("balances copy() called from JS on a JS copyWithZone that answers another object", function (done) { + expectBalancedJsCopies(false, "NativeCallbackSelfJsCopyOther", done); + }); }); From ee9fd8a9bf570eb57d0dfd6ef4f78bde86b436d5 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Sat, 14 Nov 2026 16:26:10 -0300 Subject: [PATCH 2/4] fix(runtime): do not read an unwritten return slot when retaining an owned result SetValue writes a JS string only into an id or interface return slot and leaves an instancetype slot untouched, so the retain for owned results must not treat a string answered for an instancetype return as an object it can retain. --- NativeScript/runtime/ArgConverter.mm | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/NativeScript/runtime/ArgConverter.mm b/NativeScript/runtime/ArgConverter.mm index 78cae0ec..fab27bfe 100644 --- a/NativeScript/runtime/ArgConverter.mm +++ b/NativeScript/runtime/ArgConverter.mm @@ -69,11 +69,13 @@ void RetainOwnedResult(Isolate* isolate, NativeReturnOwnership ownership, void** } // A plain JS object becomes a DictionaryAdapter SetValue already returns - // at +1. + // 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() || (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject); + (value->IsString() && type != BinaryTypeEncodingType::InstanceTypeEncoding) || + (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject); if (atPlusZero) { [returned retain]; } From fccb35aa7cf006d0fddf97402587a81bedee8e04 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Sat, 14 Nov 2026 19:45:44 -0300 Subject: [PATCH 3/4] fix(runtime): balance consumed arguments and JS property getters with owned returns -[NSKeyedUnarchiverDelegate unarchiver:didDecodeObject:] receives its object argument at +1 (NS_RELEASES_ARGUMENT) and returns at +1. A JS implementation that answered that same object was balanced only by accident before owned results were retained, and now leaked one reference per decoded object; one that answered another object, or nil, never released the argument at all. The metadata does not record consumed parameters, so Interop::ConsumesArgument names this one: MethodCallback neither retains nor releases an answered consumed argument and releases it otherwise, and a call from JS to a native implementation hands the callee the reference it consumes, which the cached-owned-return release then balances. Getters that JS implements, through extend() or by redefining a native class's property, returned with SetValue alone, so native callers of a copy/new-family getter got +0 where they own +1 and over-released the result. They now take the same RetainOwnedResult path as methods, classified by ArgConverter::ReturnOwnership from the getter's metadata. Specs cover the unarchiver delegate (unchanged, substituted and nil answers against a delegate-less control), a native delegate called from JS, JS new-family getters read natively and from JS, methods without metadata classified by name, JS replacements of a native class's owned-return method and getter, and a native -init that runs a JS method on its receiver. --- NativeScript/runtime/ArgConverter.h | 10 + NativeScript/runtime/ArgConverter.mm | 86 +++--- NativeScript/runtime/ClassBuilder.mm | 7 +- NativeScript/runtime/Interop.h | 4 + NativeScript/runtime/Interop.mm | 12 + NativeScript/runtime/MetadataBuilder.mm | 6 +- TestFixtures/Marshalling/TNSAllocLog.h | 39 ++- TestFixtures/Marshalling/TNSAllocLog.m | 63 ++++- .../app/tests/NativeCallbackSelfTests.js | 245 +++++++++++++++++- 9 files changed, 431 insertions(+), 41 deletions(-) diff --git a/NativeScript/runtime/ArgConverter.h b/NativeScript/runtime/ArgConverter.h index 7a889ba3..b01ea5f3 100644 --- a/NativeScript/runtime/ArgConverter.h +++ b/NativeScript/runtime/ArgConverter.h @@ -87,6 +87,16 @@ class ArgConverter { // 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 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 context, void* retValue, diff --git a/NativeScript/runtime/ArgConverter.mm b/NativeScript/runtime/ArgConverter.mm index fab27bfe..0f7b98a2 100644 --- a/NativeScript/runtime/ArgConverter.mm +++ b/NativeScript/runtime/ArgConverter.mm @@ -48,37 +48,11 @@ bool InMethodFamily(SEL selector, const char* family) { return strncmp(name, family, length) == 0 && (name[length] < 'a' || name[length] > 'z'); } -// 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 RetainOwnedResult(Isolate* isolate, NativeReturnOwnership ownership, void** argValues, - void* retValue, const TypeEncoding* typeEncoding, Local value) { +bool IsObjectEncoding(const TypeEncoding* typeEncoding) { BinaryTypeEncodingType type = typeEncoding->type; - if (type != BinaryTypeEncodingType::IdEncoding && - type != BinaryTypeEncodingType::InterfaceDeclarationReference && - type != BinaryTypeEncodingType::InstanceTypeEncoding) { - return; - } - - id returned = *static_cast(retValue); - if (returned == nil || (ownership == NativeReturnOwnership::ReplacesReceiver && - returned == *static_cast(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() && type != BinaryTypeEncodingType::InstanceTypeEncoding) || - (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject); - if (atPlusZero) { - [returned retain]; - } + return type == BinaryTypeEncodingType::IdEncoding || + type == BinaryTypeEncodingType::InterfaceDeclarationReference || + type == BinaryTypeEncodingType::InstanceTypeEncoding; } // The borrowed wrapper WrapNativeCallbackSelf made for an object in -dealloc @@ -409,6 +383,8 @@ explicit BorrowedSelfScope(Isolate* isolate) : isolate(isolate) {} std::vector> v8Args; v8Args.reserve(data->paramsCount_); + SEL selector = data->initialParamIndex_ == 2 ? *static_cast(argValues[1]) : nullptr; + id consumedArgument = nil; const TypeEncoding* typeEncoding = data->typeEncoding_; for (int i = 0; i < data->paramsCount_; i++) { typeEncoding = typeEncoding->next(); @@ -419,6 +395,10 @@ explicit BorrowedSelfScope(Isolate* isolate) : isolate(isolate) {} } int argIndex = i + data->initialParamIndex_; + if (selector != nullptr && Interop::ConsumesArgument(selector, i) && + IsObjectEncoding(typeEncoding)) { + consumedArgument = *static_cast(argValues[argIndex]); + } uint8_t* argBuffer = (uint8_t*)argValues[argIndex]; BaseCall call(argBuffer); @@ -517,14 +497,19 @@ 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->returnOwnership_ != NativeReturnOwnership::Unowned) { - RetainOwnedResult(isolate, data->returnOwnership_, argValues, retValue, - data->typeEncoding_, result); - } + ArgConverter::RetainOwnedResult(isolate, data->returnOwnership_, argValues, retValue, + data->typeEncoding_, result, consumedArgument); } } else { memset(retValue, 0, cif->rtype->size); } + + // The call owns the argument it consumed and hands that reference on only + // by answering it. A wrapper JS made for the argument holds its own. + if (consumedArgument != nil && !(IsObjectEncoding(data->typeEncoding_) && + *static_cast(retValue) == consumedArgument)) { + [consumedArgument release]; + } } // all V8 scopes destruct here if (pendingThrow != nil) { @@ -532,6 +517,39 @@ explicit BorrowedSelfScope(Isolate* isolate) : isolate(isolate) {} } } +// 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, + id consumedArgument) { + if (ownership == NativeReturnOwnership::Unowned || !IsObjectEncoding(typeEncoding)) { + return; + } + + id returned = *static_cast(retValue); + if (returned == nil || returned == consumedArgument || + (ownership == NativeReturnOwnership::ReplacesReceiver && + returned == *static_cast(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)) { diff --git a/NativeScript/runtime/ClassBuilder.mm b/NativeScript/runtime/ClassBuilder.mm index 40289ae9..507879f0 100644 --- a/NativeScript/runtime/ClassBuilder.mm +++ b/NativeScript/runtime/ClassBuilder.mm @@ -933,8 +933,13 @@ new MethodCallbackWrapper(isolate, poCallback, 2, argsCount, typeEncoding, // 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(argValues[1]), getterMeta), + argValues, retValue, typeEncoding, res); } } if (pendingThrow != nil) { diff --git a/NativeScript/runtime/Interop.h b/NativeScript/runtime/Interop.h index 8bfd7d57..e40a4d3e 100644 --- a/NativeScript/runtime/Interop.h +++ b/NativeScript/runtime/Interop.h @@ -119,6 +119,10 @@ class Interop { // 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 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, diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index 8bc057b6..68d6fbd0 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -172,6 +172,10 @@ return isInitializer || selector == @selector(awakeAfterUsingCoder:); } +bool Interop::ConsumesArgument(SEL selector, int index) { + return index == 1 && selector == @selector(unarchiver:didDecodeObject:); +} + // Balanced only while every owned result other than a consumed receiver // reaches here at +1, JS-implemented methods included: MethodCallback retains // what a JS implementation answers at +0, and for initializers anything but @@ -1838,6 +1842,14 @@ throw NativeScriptException( Interop::SetFFIParams(methodCall.context_, methodCall.typeEncoding_, &call, argsCount, initialParameterIndex, methodCall.args_); + if (initialParameterIndex > 1) { + for (int i = initialParameterIndex; i < argsCount; i++) { + if (ConsumesArgument(methodCall.selector_, i - initialParameterIndex)) { + [*static_cast(call.ArgumentBuffer(i)) retain]; + } + } + } + void* errorRef = nullptr; if (methodCall.provideErrorOutParameter_) { void* dest = call.ArgumentBuffer(argsCount); diff --git a/NativeScript/runtime/MetadataBuilder.mm b/NativeScript/runtime/MetadataBuilder.mm index c2add51e..8ceedd9e 100644 --- a/NativeScript/runtime/MetadataBuilder.mm +++ b/NativeScript/runtime/MetadataBuilder.mm @@ -1478,8 +1478,12 @@ new MethodCallbackWrapper(isolate, poCallback, 2, argsCount, typeEncoding, } 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(argValues[1]), getterMeta), + argValues, retValue, typeEncoding, res); } } if (pendingThrow != nil) { diff --git a/TestFixtures/Marshalling/TNSAllocLog.h b/TestFixtures/Marshalling/TNSAllocLog.h index a384c36d..71f43603 100644 --- a/TestFixtures/Marshalling/TNSAllocLog.h +++ b/TestFixtures/Marshalling/TNSAllocLog.h @@ -26,10 +26,47 @@ // Answers a new TNSAllocLog. - (id)newChild; +// Its getter answers a new TNSAllocLog, owned by the caller. +@property(nonatomic, readonly) id newItem; + + (void)copyAndDrop:(id)object; + (void)mutableCopyAndDrop:(id)object; -+ (void)newChildAndDrop:(TNSOwnedReturns*)object; ++ (void)newChildAndDrop:(id)object; + (void)awakeAndDrop:(id)object; ++ (void)newItemAndDrop:(id)object; +// Answers `newItem` of `object` without passing ownership on. ++ (id)readNewItemOf:(id)object; + +@end + +// Declares the owned-return members of TNSOwnedReturns itself, so JS can +// replace them on this class alone: metadata leaves out members a subclass +// redeclares. +@interface TNSSwizzledOwnedReturns : TNSAllocLog + +- (id)newChild; + +@property(nonatomic, readonly) id newItem; + +@end + +// A TNSAllocLog that survives a keyed archive. +@interface TNSCodingAllocLog : TNSAllocLog + +@end + +// Implements NSKeyedUnarchiverDelegate: answers the object it is given, or a +// new TNSAllocLog once `substitutes` is set. +@interface TNSDecodeDelegate : NSObject + +@property(nonatomic) BOOL substitutes; + +@end + +// -init sends -initHook to the receiver before returning it. +@interface TNSInitCallsHook : TNSAllocLog + +- (void)initHook; @end diff --git a/TestFixtures/Marshalling/TNSAllocLog.m b/TestFixtures/Marshalling/TNSAllocLog.m index c70b76bd..1eedb09b 100644 --- a/TestFixtures/Marshalling/TNSAllocLog.m +++ b/TestFixtures/Marshalling/TNSAllocLog.m @@ -46,7 +46,7 @@ + (void)mutableCopyAndDrop:(id)object { (void)result; } -+ (void)newChildAndDrop:(TNSOwnedReturns*)object { ++ (void)newChildAndDrop:(id)object { id result = [object newChild]; (void)result; } @@ -57,4 +57,65 @@ + (void)awakeAndDrop:(id)object { (void)result; } +- (id)newItem { + return [[TNSAllocLog alloc] init]; +} + ++ (void)newItemAndDrop:(id)object { + id result = [object newItem]; + (void)result; +} + ++ (id)readNewItemOf:(id)object { + return [object newItem]; +} + +@end + +@implementation TNSSwizzledOwnedReturns + +- (id)newChild { + return [[TNSAllocLog alloc] init]; +} + +- (id)newItem { + return [[TNSAllocLog alloc] init]; +} + +@end + +@implementation TNSCodingAllocLog + ++ (BOOL)supportsSecureCoding { + return YES; +} + +- (instancetype)initWithCoder:(NSCoder*)coder { + return [super init]; +} + +- (void)encodeWithCoder:(NSCoder*)coder { +} + +@end + +@implementation TNSDecodeDelegate + +- (id)unarchiver:(NSKeyedUnarchiver*)unarchiver didDecodeObject:(id)object { + return self.substitutes ? [[TNSAllocLog alloc] init] : object; +} + +@end + +@implementation TNSInitCallsHook + +- (instancetype)init { + self = [super init]; + [self initHook]; + return self; +} + +- (void)initHook { +} + @end diff --git a/TestRunner/app/tests/NativeCallbackSelfTests.js b/TestRunner/app/tests/NativeCallbackSelfTests.js index 9822939d..7f82bfea 100644 --- a/TestRunner/app/tests/NativeCallbackSelfTests.js +++ b/TestRunner/app/tests/NativeCallbackSelfTests.js @@ -526,13 +526,22 @@ describe("Native callback self wrappers", function () { }); } - function ownedReturnState(base, name, method, protocols, answersReceiver) { + function ownedReturnState(base, name, method, protocols, answersReceiver, extra) { var state = {}; var implementation = {}; - implementation[method] = function () { + var answer = function () { return answersReceiver ? this : state.result; }; - var Klass = base.extend(implementation, { name: name, protocols: protocols }); + // `extra.getter` implements `method` as a property getter; + // `extra.exposedMethods` declares it to a class that has no metadata + // for it. + extra = extra || {}; + if (extra.getter) { + Object.defineProperty(implementation, method, { get: answer, enumerable: true, configurable: true }); + } else { + implementation[method] = answer; + } + var Klass = base.extend(implementation, { name: name, protocols: protocols, exposedMethods: extra.exposedMethods }); state.receiver = Klass.new(); state.result = answersReceiver ? state.receiver : TNSAllocLog.new(); return state; @@ -617,4 +626,234 @@ describe("Native callback self wrappers", function () { it("balances copy() called from JS on a JS copyWithZone that answers another object", function (done) { expectBalancedJsCopies(false, "NativeCallbackSelfJsCopyOther", done); }); + + it("hands a native caller the object a JS new-family method without metadata answers at +1", function (done) { + var state = ownedReturnState(TNSAllocLog, "NativeCallbackSelfExposedNew", "newChild", [], false, { + exposedMethods: { newChild: { returns: NSObject, params: [] } } + }); + expectKeptThroughNativeDrop(state, function (receiver) { + TNSOwnedReturns.newChildAndDrop(receiver); + }, 2, done); + }); + + function ownedGetterState(answersReceiver, name) { + return ownedReturnState(TNSOwnedReturns, name, "newItem", [], answersReceiver, { getter: true }); + } + + it("hands a native caller the other object a JS new-family getter answers at +1", function (done) { + var state = ownedGetterState(false, "NativeCallbackSelfGetterOther"); + expectKeptThroughNativeDrop(state, function (receiver) { + TNSOwnedReturns.newItemAndDrop(receiver); + }, 2, done); + }); + + it("hands a native caller the receiver a JS new-family getter answers at +1", function (done) { + var state = ownedGetterState(true, "NativeCallbackSelfGetterSelf"); + expectKeptThroughNativeDrop(state, function (receiver) { + TNSOwnedReturns.newItemAndDrop(receiver); + }, 1, done); + }); + + function expectBalancedGetterReads(answersReceiver, name, done) { + var state = ownedGetterState(answersReceiver, name); + for (var i = 0; i < 20; i++) { + expect(state.receiver.newItem).toBe(state.result); + expect(TNSOwnedReturns.readNewItemOf(state.receiver)).toBe(state.result); + } + expectKeptThroughNativeDrop(state, function () {}, answersReceiver ? 1 : 2, done); + } + + it("balances reads of a JS new-family getter that answers another object", function (done) { + expectBalancedGetterReads(false, "NativeCallbackSelfGetterReadsOther", done); + }); + + it("balances reads of a JS new-family getter that answers its receiver", function (done) { + expectBalancedGetterReads(true, "NativeCallbackSelfGetterReadsSelf", done); + }); + + // The replacements stay installed for the rest of the run, on a class no + // other spec uses, and answer what `swizzled.answer` gives them. + var swizzled = { installed: false, answer: null }; + + function swizzledState(answersReceiver) { + if (!swizzled.installed) { + swizzled.installed = true; + TNSSwizzledOwnedReturns.prototype.newChild = function () { + return swizzled.answer(this); + }; + Object.defineProperty(TNSSwizzledOwnedReturns.prototype, "newItem", { + get: function () { + return swizzled.answer(this); + }, + configurable: true + }); + } + var state = { receiver: TNSSwizzledOwnedReturns.new() }; + state.result = answersReceiver ? state.receiver : TNSAllocLog.new(); + swizzled.answer = function (self) { + return answersReceiver ? self : state.result; + }; + return state; + } + + it("hands a native caller the other object a JS replacement of a new-family method answers at +1", function (done) { + var state = swizzledState(false); + expectKeptThroughNativeDrop(state, function (receiver) { + TNSOwnedReturns.newChildAndDrop(receiver); + }, 2, done); + }); + + it("hands a native caller the receiver a JS replacement of a new-family method answers at +1", function (done) { + var state = swizzledState(true); + expectKeptThroughNativeDrop(state, function (receiver) { + TNSOwnedReturns.newChildAndDrop(receiver); + }, 1, done); + }); + + it("hands a native caller the other object a JS replacement of a new-family getter answers at +1", function (done) { + var state = swizzledState(false); + expectKeptThroughNativeDrop(state, function (receiver) { + TNSOwnedReturns.newItemAndDrop(receiver); + expect(TNSOwnedReturns.readNewItemOf(receiver)).toBe(state.result); + }, 2, done); + }); + + it("frees once an object whose native -init ran a JS method on it (alloc().init())", function (done) { + var hooks = 0; + var Hooked = TNSInitCallsHook.extend({ + initHook: function () { + hooks++; + this.hooked = true; + } + }, { name: "NativeCallbackSelfInitHook" }); + var obj = Hooked.alloc().init(); + expect(hooks).toBe(1); + expect(obj.hooked).toBe(true); + settle(3, function () { + expect(deallocs()).toBe(0); + expect(obj.hash > 0).toBe(true); + obj = null; + settle(3, function () { + expect(deallocs()).toBe(1); + done(); + }); + }); + }); + + // -unarchiver:didDecodeObject: takes over the object it is given + // (NS_RELEASES_ARGUMENT) and answers an owned one. + var decodeAnswer = null; + var decodeCalls = 0; + var DecodeDelegate = NSObject.extend({ + unarchiverDidDecodeObject: function (unarchiver, object) { + decodeCalls++; + return decodeAnswer(object); + } + }, { name: "NativeCallbackSelfDecodeDelegate", protocols: [NSKeyedUnarchiverDelegate] }); + + // Decodes an array of two TNSCodingAllocLogs through a delegate answering + // `answer(object)`, or through none when `answer` is null. `freedAfterDecode` + // TNSAllocLogs are gone once the unarchiver is, `freedAtEnd` once JS + // drops the decoded root too. + function expectBalancedDecode(answer, freedAfterDecode, freedAtEnd, checkRoot, done) { + var data = (function () { + var root = NSMutableArray.new(); + root.addObject(TNSCodingAllocLog.new()); + root.addObject(TNSCodingAllocLog.new()); + return NSKeyedArchiver.archivedDataWithRootObjectRequiringSecureCodingError(root, false); + })(); + settle(3, function () { + expect(deallocs()).toBe(2); + TNSClearOutput(); + var state = {}; + (function () { + var delegate = answer ? DecodeDelegate.new() : null; + decodeAnswer = answer; + decodeCalls = 0; + var unarchiver = NSKeyedUnarchiver.alloc().initForReadingFromDataError(data); + unarchiver.requiresSecureCoding = false; + unarchiver.delegate = delegate; + state.root = unarchiver.decodeObjectForKey(NSKeyedArchiveRootObjectKey); + unarchiver.finishDecoding(); + unarchiver.delegate = null; + decodeAnswer = null; + })(); + expect(decodeCalls).toBe(answer ? 3 : 0); + settle(3, function () { + expect(deallocs()).toBe(freedAfterDecode); + checkRoot(state.root); + state.root = null; + settle(3, function () { + expect(deallocs()).toBe(freedAtEnd); + done(); + }); + }); + }); + } + + function expectDecodedRoot(elementClass) { + return function (root) { + expect(root instanceof NSArray).toBe(true); + expect(root.count).toBe(2); + for (var i = 0; i < root.count; i++) { + var element = root.objectAtIndex(i); + expect(element instanceof elementClass).toBe(true); + expect(element.hash > 0).toBe(true); + } + }; + } + + it("decodes without an unarchiver delegate", function (done) { + expectBalancedDecode(null, 0, 2, expectDecodedRoot(TNSCodingAllocLog), done); + }); + + it("hands an unarchiver back the objects a JS delegate answers unchanged", function (done) { + expectBalancedDecode(function (object) { + return object; + }, 0, 2, expectDecodedRoot(TNSCodingAllocLog), done); + }); + + it("releases the objects a JS unarchiver delegate replaces", function (done) { + expectBalancedDecode(function (object) { + return object instanceof TNSCodingAllocLog ? TNSAllocLog.new() : object; + }, 2, 4, function (root) { + expectDecodedRoot(TNSAllocLog)(root); + expect(root.objectAtIndex(0) instanceof TNSCodingAllocLog).toBe(false); + }, done); + }); + + // The unarchiver keeps an object its delegate answers nil for. + it("releases the objects a JS unarchiver delegate answers nil for", function (done) { + expectBalancedDecode(function (object) { + return null; + }, 0, 2, expectDecodedRoot(TNSCodingAllocLog), done); + }); + + it("gives a native unarchiver delegate called from JS the reference it takes over", function (done) { + var delegate = TNSDecodeDelegate.new(); + var object = TNSAllocLog.new(); + var i; + for (i = 0; i < 20; i++) { + expect(delegate.unarchiverDidDecodeObject(null, object)).toBe(object); + } + delegate.substitutes = true; + for (i = 0; i < 20; i++) { + var other = delegate.unarchiverDidDecodeObject(null, object); + expect(other instanceof TNSAllocLog).toBe(true); + expect(other).not.toBe(object); + } + other = null; + settle(3, function () { + var freed = deallocs(); + expect(freed).toBe(20); + if (freed === 20) { + expect(object.hash > 0).toBe(true); + } + object = null; + settle(3, function () { + expect(deallocs()).toBe(21); + done(); + }); + }); + }); }); From 4598259575ac1756c9ecad7e452e1acaa76b2fa6 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Sat, 14 Nov 2026 20:38:29 -0300 Subject: [PATCH 4/4] fix(runtime): own unarchiver delegate results without metadata and release consumed arguments on rejected calls A JS class that implements -unarchiver:didDecodeObject: through exposedMethods, without declaring NSKeyedUnarchiverDelegate, has no metadata, and the selector-family classification called it unowned. A substituted object then went back to the unarchiver at +0 and was freed while the decoded graph still held it. Interop::ReturnsRetained records the selector's retained result next to its consumed argument, and the no-metadata branch of ArgConverter::ReturnOwnership consults it. MethodCallback's two isolate-validity guards answered nil without releasing the argument the method consumes, leaking it for every call rejected during teardown. The consumed argument is now found once, before the guards, from the native call alone, and both guards release it. --- NativeScript/runtime/ArgConverter.mm | 35 +++++++--- NativeScript/runtime/Interop.h | 4 ++ NativeScript/runtime/Interop.mm | 4 ++ .../app/tests/NativeCallbackSelfTests.js | 70 ++++++++++++------- 4 files changed, 79 insertions(+), 34 deletions(-) diff --git a/NativeScript/runtime/ArgConverter.mm b/NativeScript/runtime/ArgConverter.mm index 0f7b98a2..d4307091 100644 --- a/NativeScript/runtime/ArgConverter.mm +++ b/NativeScript/runtime/ArgConverter.mm @@ -55,6 +55,26 @@ bool IsObjectEncoding(const TypeEncoding* typeEncoding) { type == BinaryTypeEncodingType::InstanceTypeEncoding; } +// 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; + } + SEL selector = *static_cast(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(argValues[i + 2]) : nil; + } + return nil; +} + // The borrowed wrapper WrapNativeCallbackSelf made for an object in -dealloc // is detached once the native call returns: the object is gone afterwards, // so a receiver JS kept must read as released. @@ -342,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(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(); @@ -358,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; } @@ -369,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); @@ -383,8 +407,6 @@ explicit BorrowedSelfScope(Isolate* isolate) : isolate(isolate) {} std::vector> v8Args; v8Args.reserve(data->paramsCount_); - SEL selector = data->initialParamIndex_ == 2 ? *static_cast(argValues[1]) : nullptr; - id consumedArgument = nil; const TypeEncoding* typeEncoding = data->typeEncoding_; for (int i = 0; i < data->paramsCount_; i++) { typeEncoding = typeEncoding->next(); @@ -395,10 +417,6 @@ explicit BorrowedSelfScope(Isolate* isolate) : isolate(isolate) {} } int argIndex = i + data->initialParamIndex_; - if (selector != nullptr && Interop::ConsumesArgument(selector, i) && - IsObjectEncoding(typeEncoding)) { - consumedArgument = *static_cast(argValues[argIndex]); - } uint8_t* argBuffer = (uint8_t*)argValues[argIndex]; BaseCall call(argBuffer); @@ -504,8 +522,7 @@ explicit BorrowedSelfScope(Isolate* isolate) : isolate(isolate) {} memset(retValue, 0, cif->rtype->size); } - // The call owns the argument it consumed and hands that reference on only - // by answering it. A wrapper JS made for the argument holds its own. + // A wrapper JS made for the consumed argument holds a reference of its own. if (consumedArgument != nil && !(IsObjectEncoding(data->typeEncoding_) && *static_cast(retValue) == consumedArgument)) { [consumedArgument release]; @@ -559,7 +576,7 @@ explicit BorrowedSelfScope(Isolate* isolate) : isolate(isolate) {} bool owned = meta != nullptr ? meta->ownsReturnedCocoaObject() : InMethodFamily(selector, "copy") || InMethodFamily(selector, "mutableCopy") || - InMethodFamily(selector, "new"); + InMethodFamily(selector, "new") || Interop::ReturnsRetained(selector); return owned ? NativeReturnOwnership::Owned : NativeReturnOwnership::Unowned; } diff --git a/NativeScript/runtime/Interop.h b/NativeScript/runtime/Interop.h index e40a4d3e..de332854 100644 --- a/NativeScript/runtime/Interop.h +++ b/NativeScript/runtime/Interop.h @@ -123,6 +123,10 @@ class Interop { // `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, diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index 68d6fbd0..ae7be610 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -172,10 +172,14 @@ return isInitializer || selector == @selector(awakeAfterUsingCoder:); } +// -[NSKeyedUnarchiverDelegate unarchiver:didDecodeObject:] is the only SDK +// method with an NS_RELEASES_ARGUMENT parameter; it also returns retained. bool Interop::ConsumesArgument(SEL selector, int index) { return index == 1 && selector == @selector(unarchiver:didDecodeObject:); } +bool Interop::ReturnsRetained(SEL selector) { return ConsumesArgument(selector, 1); } + // Balanced only while every owned result other than a consumed receiver // reaches here at +1, JS-implemented methods included: MethodCallback retains // what a JS implementation answers at +0, and for initializers anything but diff --git a/TestRunner/app/tests/NativeCallbackSelfTests.js b/TestRunner/app/tests/NativeCallbackSelfTests.js index 7f82bfea..180485ea 100644 --- a/TestRunner/app/tests/NativeCallbackSelfTests.js +++ b/TestRunner/app/tests/NativeCallbackSelfTests.js @@ -751,11 +751,24 @@ describe("Native callback self wrappers", function () { } }, { name: "NativeCallbackSelfDecodeDelegate", protocols: [NSKeyedUnarchiverDelegate] }); - // Decodes an array of two TNSCodingAllocLogs through a delegate answering - // `answer(object)`, or through none when `answer` is null. `freedAfterDecode` - // TNSAllocLogs are gone once the unarchiver is, `freedAtEnd` once JS - // drops the decoded root too. - function expectBalancedDecode(answer, freedAfterDecode, freedAtEnd, checkRoot, done) { + // The same method with no metadata to describe it. + var ExposedDecodeDelegate = NSObject.extend({ + "unarchiver:didDecodeObject:": function (unarchiver, object) { + decodeCalls++; + return decodeAnswer(object); + } + }, { + name: "NativeCallbackSelfExposedDecodeDelegate", + exposedMethods: { + "unarchiver:didDecodeObject:": { returns: NSObject, params: [NSObject, NSObject] } + } + }); + + // Decodes an array of two TNSCodingAllocLogs through a `Delegate` + // answering `answer(object)`, or through none when `answer` is null. + // `freedAfterDecode` TNSAllocLogs are gone once the unarchiver is, + // `freedAtEnd` once JS drops the decoded root too. + function expectBalancedDecode(Delegate, answer, freedAfterDecode, freedAtEnd, checkRoot, done) { var data = (function () { var root = NSMutableArray.new(); root.addObject(TNSCodingAllocLog.new()); @@ -767,7 +780,7 @@ describe("Native callback self wrappers", function () { TNSClearOutput(); var state = {}; (function () { - var delegate = answer ? DecodeDelegate.new() : null; + var delegate = answer ? Delegate.new() : null; decodeAnswer = answer; decodeCalls = 0; var unarchiver = NSKeyedUnarchiver.alloc().initForReadingFromDataError(data); @@ -804,29 +817,36 @@ describe("Native callback self wrappers", function () { } it("decodes without an unarchiver delegate", function (done) { - expectBalancedDecode(null, 0, 2, expectDecodedRoot(TNSCodingAllocLog), done); + expectBalancedDecode(null, null, 0, 2, expectDecodedRoot(TNSCodingAllocLog), done); }); - it("hands an unarchiver back the objects a JS delegate answers unchanged", function (done) { - expectBalancedDecode(function (object) { - return object; - }, 0, 2, expectDecodedRoot(TNSCodingAllocLog), done); - }); + [ + { Delegate: DecodeDelegate, suffix: "" }, + { Delegate: ExposedDecodeDelegate, suffix: " (exposedMethods, no protocol)" } + ].forEach(function (variant) { + var Delegate = variant.Delegate; - it("releases the objects a JS unarchiver delegate replaces", function (done) { - expectBalancedDecode(function (object) { - return object instanceof TNSCodingAllocLog ? TNSAllocLog.new() : object; - }, 2, 4, function (root) { - expectDecodedRoot(TNSAllocLog)(root); - expect(root.objectAtIndex(0) instanceof TNSCodingAllocLog).toBe(false); - }, done); - }); + it("hands an unarchiver back the objects a JS delegate answers unchanged" + variant.suffix, function (done) { + expectBalancedDecode(Delegate, function (object) { + return object; + }, 0, 2, expectDecodedRoot(TNSCodingAllocLog), done); + }); - // The unarchiver keeps an object its delegate answers nil for. - it("releases the objects a JS unarchiver delegate answers nil for", function (done) { - expectBalancedDecode(function (object) { - return null; - }, 0, 2, expectDecodedRoot(TNSCodingAllocLog), done); + it("releases the objects a JS unarchiver delegate replaces" + variant.suffix, function (done) { + expectBalancedDecode(Delegate, function (object) { + return object instanceof TNSCodingAllocLog ? TNSAllocLog.new() : object; + }, 2, 4, function (root) { + expectDecodedRoot(TNSAllocLog)(root); + expect(root.objectAtIndex(0) instanceof TNSCodingAllocLog).toBe(false); + }, done); + }); + + // The unarchiver keeps an object its delegate answers nil for. + it("releases the objects a JS unarchiver delegate answers nil for" + variant.suffix, function (done) { + expectBalancedDecode(Delegate, function (object) { + return null; + }, 0, 2, expectDecodedRoot(TNSCodingAllocLog), done); + }); }); it("gives a native unarchiver delegate called from JS the reference it takes over", function (done) {