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");