From fb25dc346bbbfd66c7c4ada6b1b162af837559f2 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Mon, 10 Aug 2026 23:45:33 -0300 Subject: [PATCH] fix: stop over-releasing objects wrapped from a raw pointer `new SomeClass(interop.handleof(obj))` reached ArgConverter::ConstructObject, which converted the pointer with CFBridgingRelease. The runtime is built without ARC, so that expands to a bare `[(id)ptr autorelease]` and consumes a reference JS never owned: interop.handleof is retain-neutral, and genuine +1 handoffs travel through Unmanaged/takeRetainedValue or the ownsReturnedCocoaObject metadata flag instead. The path also stopped taking a reference of its own when 1129d15f commented out the blanket `[result retain]` to fix a leak on the alloc/init paths. The object therefore died at the next autorelease pool drain while its Caches::Instances entry -- keyed on the raw address and erased only by V8 GC finalization -- lived on. A later allocation landing in the same malloc size class inherited that entry, and the blind cache hits in Interop::GetResult and ConstructObject handed the old wrapper back verbatim, so the new object came back to JS with a foreign prototype. That is the one-shot MethodCallsTests flakiness (Base_InstanceBaseProtocolMethod2Optional "is not a function", Derived_DerivedProtocolProperty1 setter never running) seen after Marshalling/ReferenceTests runs CastPointerToNSObject; V8 later finalizing the stale entry released a freed address on top of it. Pointer construction is now non-consuming and takes its own reference, so an Instances entry always owns one reference to the object it maps and the address cannot be recycled underneath it. Cache reads go through ArgConverter::FindCachedInstance, which drops an entry whose wrapper no longer describes the object at that address, turning any future stale entry into a rebuilt wrapper rather than a wrong one. ObjectManager only erases the entry that still points back at the wrapper being disposed, so a rebuilt entry survives its predecessor. --- NativeScript/runtime/ArgConverter.h | 8 +++ NativeScript/runtime/ArgConverter.mm | 54 ++++++++++++++++--- NativeScript/runtime/DataWrapper.h | 12 ++++- NativeScript/runtime/Interop.mm | 6 +-- NativeScript/runtime/ObjectManager.mm | 11 +++- .../app/tests/StaleWrapperCacheTests.js | 44 +++++++++++++++ TestRunner/app/tests/index.js | 1 + 7 files changed, 125 insertions(+), 11 deletions(-) create mode 100644 TestRunner/app/tests/StaleWrapperCacheTests.js diff --git a/NativeScript/runtime/ArgConverter.h b/NativeScript/runtime/ArgConverter.h index c74ed2ed..62bc7bb4 100644 --- a/NativeScript/runtime/ArgConverter.h +++ b/NativeScript/runtime/ArgConverter.h @@ -58,6 +58,14 @@ class ArgConverter { v8::Local context); static const Meta* FindMeta(Class klass, const TypeEncoding* typeEncoding = nullptr); + // Looks up the JS wrapper cached for `target` in Caches::Instances, dropping + // the entry and reporting a miss when it no longer describes the object at + // that address. Every read of Instances that hands the wrapper straight back + // to JS must go through here: entries are keyed on the raw pointer, so an + // entry that outlived its object would otherwise alias whatever allocation + // recycled the address and give it a foreign prototype. + static std::shared_ptr> FindCachedInstance( + v8::Isolate* isolate, const std::shared_ptr& cache, id target); static const Meta* GetMeta(std::string name); static const ProtocolMeta* FindProtocolMeta(Protocol* protocol); static void MethodCallback(ffi_cif* cif, void* retValue, void** argValues, diff --git a/NativeScript/runtime/ArgConverter.mm b/NativeScript/runtime/ArgConverter.mm index c1d8ed9e..0615fef9 100644 --- a/NativeScript/runtime/ArgConverter.mm +++ b/NativeScript/runtime/ArgConverter.mm @@ -559,12 +559,21 @@ tns::Assert(klass != nullptr, isolate); id result = nil; + // A Caches::Instances entry owns exactly one reference to the object it maps + // (ObjectManager::DisposeValue gives it back, and ClassBuilder's swizzled + // retain/release read a retainCount of 1 as "only the map holds this"), so + // this function must hand the entry a +1 and no more. Tracks whether `result` + // already carries one: the alloc/init paths do, a pointer handed in from JS + // does not. Constructing from a pointer is non-consuming — interop.handleof + // hands out a retain-neutral address, and a +1 reaches JS as an Unmanaged to + // be claimed with takeRetainedValue — so that path takes its own reference. + bool resultIsOwned = false; if (info.Length() == 1) { BaseDataWrapper* wrapper = tns::GetValue(isolate, info[0]); if (wrapper != nullptr && wrapper->Type() == WrapperType::Pointer) { PointerWrapper* pointerWrapper = static_cast(wrapper); - result = CFBridgingRelease(pointerWrapper->Data()); + result = (__bridge id)pointerWrapper->Data(); } } @@ -583,17 +592,24 @@ V8VectorArgs vectorArgs(args); result = Interop::CallInitializer(context, initializer, result, klass, vectorArgs); + resultIsOwned = true; } if (result == nil) { result = [[klass alloc] init]; + resultIsOwned = true; } auto cache = Caches::Get(isolate); - auto it = cache->Instances.find(result); - if (it != cache->Instances.end()) { - Local obj = it->second->Get(isolate); - info.GetReturnValue().Set(obj); + auto poInstance = ArgConverter::FindCachedInstance(isolate, cache, result); + if (poInstance != nullptr) { + // An initializer that answered with an already wrapped object (a singleton, + // a tagged pointer, a cached cluster instance) leaves us holding a +1 the + // existing entry has no use for. + if (resultIsOwned) { + [result release]; + } + info.GetReturnValue().Set(poInstance->Get(isolate)); } else { ObjCDataWrapper* wrapper = new ObjCDataWrapper(result); Local thiz = info.This(); @@ -601,7 +617,9 @@ tns::SetValue(isolate, thiz, wrapper); std::shared_ptr> poThiz = ObjectManager::Register(context, thiz); cache->Instances.emplace(result, poThiz); - // [result retain]; + if (!resultIsOwned) { + [result retain]; + } } } @@ -935,6 +953,30 @@ return receiver; } +std::shared_ptr> ArgConverter::FindCachedInstance( + Isolate* isolate, const std::shared_ptr& cache, id target) { + auto it = cache->Instances.find(target); + if (it == cache->Instances.end()) { + return nullptr; + } + + BaseDataWrapper* wrapper = tns::GetValue(isolate, it->second->Get(isolate)); + if (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject) { + Class expected = static_cast(wrapper)->Klass(); + // A KVO-style isa swizzle replaces the class in place but keeps -class + // answering the original, so only an address that now belongs to a + // different object fails both comparisons. Dropping such an entry turns a + // wrapper that would otherwise be handed out with the wrong prototype into + // a plain cache miss, which rebuilds it correctly. + if (expected != nil && object_getClass(target) != expected && [target class] != expected) { + cache->Instances.erase(it); + return nullptr; + } + } + + return it->second; +} + const Meta* ArgConverter::FindMeta(Class klass, const TypeEncoding* typeEncoding) { if (typeEncoding != nullptr && typeEncoding->type == BinaryTypeEncodingType::InterfaceDeclarationReference) { diff --git a/NativeScript/runtime/DataWrapper.h b/NativeScript/runtime/DataWrapper.h index 25cfd37b..55820c70 100644 --- a/NativeScript/runtime/DataWrapper.h +++ b/NativeScript/runtime/DataWrapper.h @@ -1,6 +1,8 @@ #ifndef DataWrapper_h #define DataWrapper_h +#include + #include #include #include @@ -365,7 +367,9 @@ class UnmanagedTypeWrapper : public BaseDataWrapper { class ObjCDataWrapper : public BaseDataWrapper { public: ObjCDataWrapper(id data, const TypeEncoding* typeEncoding = nullptr) - : data_(data), typeEncoding_(typeEncoding) {} + : data_(data), + typeEncoding_(typeEncoding), + klass_(object_getClass(data)) {} const WrapperType Type() { return WrapperType::ObjCObject; } @@ -373,9 +377,15 @@ class ObjCDataWrapper : public BaseDataWrapper { const TypeEncoding* TypeEncoding() { return this->typeEncoding_; } + // The class Data() had when this wrapper was built. Data() alone cannot tell + // whether the wrapper still describes the object living at that address, so + // anything keyed on the raw pointer needs this to detect a recycled slot. + Class Klass() { return this->klass_; } + private: id data_; const tns::TypeEncoding* typeEncoding_; + Class klass_; }; class ObjCClassWrapper : public BaseDataWrapper { diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index b2a6380c..fb29bbf2 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -1244,9 +1244,9 @@ inline bool isBool() { } auto cache = Caches::Get(isolate); - auto it = cache->Instances.find(result); - if (it != cache->Instances.end()) { - return it->second->Get(isolate); + auto poInstance = ArgConverter::FindCachedInstance(isolate, cache, result); + if (poInstance != nullptr) { + return poInstance->Get(isolate); } // For NSProxy we will try to read the metadata from diff --git a/NativeScript/runtime/ObjectManager.mm b/NativeScript/runtime/ObjectManager.mm index 3f94da78..5a0cb2f4 100644 --- a/NativeScript/runtime/ObjectManager.mm +++ b/NativeScript/runtime/ObjectManager.mm @@ -204,7 +204,16 @@ void DisposeHandle(v8::Isolate* isolate, ObjCDataWrapper* objCObjectWrapper = static_cast(wrapper); id target = objCObjectWrapper->Data(); if (target != nil) { - cache->Instances.erase(target); + // Instances is keyed on the raw address, so an entry rebuilt for a + // later object living there must survive this wrapper going away — + // only the entry that still points back at this object is ours. + auto it = cache->Instances.find(target); + if (it != cache->Instances.end()) { + Local cached = it->second->Get(isolate); + if (cached.IsEmpty() || cached == value) { + cache->Instances.erase(it); + } + } [target release]; } break; diff --git a/TestRunner/app/tests/StaleWrapperCacheTests.js b/TestRunner/app/tests/StaleWrapperCacheTests.js new file mode 100644 index 00000000..7259182f --- /dev/null +++ b/TestRunner/app/tests/StaleWrapperCacheTests.js @@ -0,0 +1,44 @@ +describe("Instance cache staleness", function () { + var ITERATIONS = 100; + + // NSObject (isa only) and TNSBaseInterface (isa + two ints) both land in the + // 16-byte malloc bucket, so a TNSBaseInterface allocated after an NSObject is + // freed can be handed the very same address. + it("never hands out a wrapper built for an object that no longer lives at that address", function (done) { + for (var i = 0; i < ITERATIONS; i++) { + var original = NSObject.alloc().init(); + var alias = new NSObject(interop.handleof(original)); + if (i === 0) { + expect(alias).toBe(original); + } + original = null; + alias = null; + } + + // An address can only be recycled once the runloop has drained its + // autorelease pool, so the reallocation half has to run in a later turn. + setTimeout(function () { + var wrongPrototype = 0; + var missingMethod = 0; + var instances = []; + + for (var j = 0; j < ITERATIONS; j++) { + var instance = TNSBaseInterface.alloc().init(); + instances.push(instance); + + if (Object.getPrototypeOf(instance) !== TNSBaseInterface.prototype) { + wrongPrototype++; + } + if (typeof instance.baseProtocolMethod2Optional !== "function") { + missingMethod++; + } + } + + expect(wrongPrototype).toBe(0, "instances built on a reused address got a foreign prototype"); + expect(missingMethod).toBe(0, "instances built on a reused address lost their protocol methods"); + + instances = null; + done(); + }, 0); + }); +}); diff --git a/TestRunner/app/tests/index.js b/TestRunner/app/tests/index.js index 7fdd3869..46a1040c 100644 --- a/TestRunner/app/tests/index.js +++ b/TestRunner/app/tests/index.js @@ -121,6 +121,7 @@ require("./Inheritance/ProtocolImplementationTests"); require("./Inheritance/TypeScriptTests"); // require("./MethodCallsTests"); +require("./StaleWrapperCacheTests"); //import "./FunctionsTests"; require("./VersionDiffTests"); require("./ObjCConstructors");