diff --git a/NativeScript/runtime/ArgConverter.h b/NativeScript/runtime/ArgConverter.h index 9fa21ee4d..b8bcfe5ac 100644 --- a/NativeScript/runtime/ArgConverter.h +++ b/NativeScript/runtime/ArgConverter.h @@ -40,20 +40,23 @@ class ArgConverter { v8::Local receiver, V8Args& args, const MethodMeta* meta, bool isMethodCallback); - // skipGCRegistration: for ObjC wrappers the object gets a strong handle and - // no finalizer. static v8::Local ConvertArgument( v8::Local context, BaseDataWrapper* wrapper, - bool skipGCRegistration = false, const std::vector& additionalProtocols = std::vector()); static v8::Local CreateJsWrapper( v8::Local context, BaseDataWrapper* wrapper, - v8::Local receiver, bool skipGCRegistration = false, + v8::Local receiver, const std::vector& additionalProtocols = std::vector()); static std::shared_ptr> CreateEmptyObject( - v8::Local context, bool skipGCRegistration = false); + v8::Local context); + // The JS `this` for native code calling a JS-implemented method on an + // object JS has no wrapper for yet. For an object already deallocating it + // is a borrowed wrapper, stored in `borrowed`, that the caller must detach + // and delete once the call returns. + static v8::Local WrapNativeCallbackSelf( + v8::Local context, id self, ObjCDataWrapper** borrowed); static const Meta* FindMeta(Class klass, const TypeEncoding* typeEncoding = nullptr); // Looks up the JS wrapper cached for `target` in Caches::Instances, dropping @@ -102,8 +105,7 @@ class ArgConverter { static v8::Local NewEmptyInstance( v8::Local context, v8::Persistent* ctorFunc); static std::shared_ptr> CreateEmptyInstance( - v8::Local context, v8::Persistent* ctorFunc, - bool skipGCRegistration = false); + v8::Local context, v8::Persistent* ctorFunc); static void FindMethodOverloads(Class klass, std::string methodName, MemberType type, std::vector& overloads); diff --git a/NativeScript/runtime/ArgConverter.mm b/NativeScript/runtime/ArgConverter.mm index d5464db87..f83403f87 100644 --- a/NativeScript/runtime/ArgConverter.mm +++ b/NativeScript/runtime/ArgConverter.mm @@ -2,6 +2,7 @@ #include #include #include +#include "ClassBuilder.h" #include "DictionaryAdapter.h" #include "ExternalMemory.h" #include "Helpers.h" @@ -17,6 +18,83 @@ namespace tns { +namespace { + +// -allowsWeakReference answers NO once deallocation has begun, and for the +// rare classes that refuse weak references altogether. Runs before `obj` has +// a cached wrapper, so nothing here may be a message a JS class implements: +// it would re-enter MethodCallback for the same object. +bool IsDeallocating(id obj) { + static SEL allowsWeakReference = sel_registerName("allowsWeakReference"); + Class nativeClass = ClassBuilder::FindNativeClass(object_getClass(obj)); + // class_getMethodImplementation answers the forwarding IMP for a selector + // the class lacks. + if (!class_respondsToSelector(nativeClass, allowsWeakReference)) { + return false; + } + IMP imp = class_getMethodImplementation(nativeClass, allowsWeakReference); + 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) { + const char* name = sel_getName(selector); + while (*name == '_') { + name++; + } + return strncmp(name, "init", 4) == 0 && (name[4] < 'a' || name[4] > '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) { + BinaryTypeEncodingType type = typeEncoding->type; + if (type != BinaryTypeEncodingType::IdEncoding && + type != BinaryTypeEncodingType::InterfaceDeclarationReference && + type != BinaryTypeEncodingType::InstanceTypeEncoding) { + return; + } + + id returned = *static_cast(retValue); + if (returned == nil || returned == *static_cast(argValues[0]) || + !IsInitFamily(*static_cast(argValues[1]))) { + return; + } + + // 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]; + } +} + +// 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. +struct BorrowedSelfScope { + explicit BorrowedSelfScope(Isolate* isolate) : isolate(isolate) {} + ~BorrowedSelfScope() { + if (wrapper != nullptr && tns::GetValue(isolate, receiver) == wrapper) { + tns::SetValue(isolate, receiver, nullptr); + delete wrapper; + } + } + Isolate* isolate; + Local receiver; + ObjCDataWrapper* wrapper = nullptr; +}; + +} // namespace + void ArgConverter::Init(Local context) { Isolate* isolate = v8::Isolate::GetCurrent(); auto cache = Caches::Get(isolate); @@ -119,15 +197,13 @@ } Local ArgConverter::ConvertArgument(Local context, BaseDataWrapper* wrapper, - bool skipGCRegistration, const std::vector& additionalProtocols) { Isolate* isolate = v8::Isolate::GetCurrent(); if (wrapper == nullptr) { return Null(isolate); } - Local result = - CreateJsWrapper(context, wrapper, Local(), skipGCRegistration, additionalProtocols); + Local result = CreateJsWrapper(context, wrapper, Local(), additionalProtocols); return result; } @@ -352,15 +428,15 @@ } Local thiz = context->Global(); + BorrowedSelfScope borrowedSelf(isolate); if (data->initialParamIndex_ > 1) { id self_ = *static_cast(argValues[0]); auto it = cache->Instances.find(self_); if (it != cache->Instances.end()) { thiz = it->second->Get(data->isolateWrapper_.Isolate()).As(); } else { - ObjCDataWrapper* wrapper = new ObjCDataWrapper(self_); - thiz = ArgConverter::CreateJsWrapper(context, wrapper, Local(), true).As(); - tns::DeleteWrapperIfUnused(isolate, thiz, wrapper); + thiz = ArgConverter::WrapNativeCallbackSelf(context, self_, &borrowedSelf.wrapper); + borrowedSelf.receiver = thiz; } } @@ -437,6 +513,9 @@ 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); + } } } else { memset(retValue, 0, cif->rtype->size); @@ -845,7 +924,7 @@ } Local ArgConverter::CreateJsWrapper(Local context, BaseDataWrapper* wrapper, - Local receiver, bool skipGCRegistration, + Local receiver, const std::vector& additionalProtocols) { Isolate* isolate = v8::Isolate::GetCurrent(); @@ -861,7 +940,7 @@ ObjCAllocDataWrapper* allocDataWrapper = static_cast(wrapper); Class klass = allocDataWrapper->Klass(); - std::shared_ptr> poValue = CreateEmptyObject(context, false); + std::shared_ptr> poValue = CreateEmptyObject(context); receiver = poValue->Get(isolate).As(); const Meta* meta = FindMeta(klass); @@ -904,7 +983,7 @@ if (it != cache->Instances.end()) { receiver = it->second->Get(isolate).As(); } else { - std::shared_ptr> poValue = CreateEmptyObject(context, skipGCRegistration); + std::shared_ptr> poValue = CreateEmptyObject(context); receiver = poValue->Get(isolate).As(); tns::SetValue(isolate, receiver, wrapper); cache->Instances.emplace(target, poValue); @@ -1069,12 +1148,42 @@ return protocolMeta; } -std::shared_ptr> ArgConverter::CreateEmptyObject(Local context, - bool skipGCRegistration) { +Local ArgConverter::WrapNativeCallbackSelf(Local context, id self, + ObjCDataWrapper** borrowed) { + Isolate* isolate = v8::Isolate::GetCurrent(); + auto cache = Caches::Get(isolate); + ObjCDataWrapper* wrapper = new ObjCDataWrapper(self); + + if (IsDeallocating(self)) { + // Called from -dealloc: a retain cannot keep the object alive, so the + // wrapper is neither cached nor owning, and the caller detaches it. + wrapper->MarkBorrowed(); + Local receiver = NewEmptyInstance(context, cache->EmptyObjCtorFunc.get()); + CreateJsWrapper(context, wrapper, receiver); + *borrowed = wrapper; + return receiver; + } + + Local result = CreateJsWrapper(context, wrapper, Local()).As(); + if (tns::DeleteWrapperIfUnused(isolate, result, wrapper)) { + return result; + } + // Native code usually still holds the object (it is mid-initialization or + // owned by whoever called in), and its JS state must outlive this call: the + // handle stays strong until the Instances entry is the object's only owner. + auto it = cache->Instances.find(self); + if (it != cache->Instances.end() && it->second->IsWeak()) { + ObjectManager::ManageOwnership(isolate, wrapper, + it->second->ClearWeak()); + } + return result; +} + +std::shared_ptr> ArgConverter::CreateEmptyObject(Local context) { Isolate* isolate = v8::Isolate::GetCurrent(); Persistent* ctorFunc = Caches::Get(isolate)->EmptyObjCtorFunc.get(); tns::Assert(ctorFunc != nullptr, isolate); - return ArgConverter::CreateEmptyInstance(context, ctorFunc, skipGCRegistration); + return ArgConverter::CreateEmptyInstance(context, ctorFunc); } Local ArgConverter::NewEmptyInstance(Local context, @@ -1090,18 +1199,9 @@ } std::shared_ptr> ArgConverter::CreateEmptyInstance( - Local context, Persistent* ctorFunc, bool skipGCRegistration) { - Isolate* isolate = v8::Isolate::GetCurrent(); + Local context, Persistent* ctorFunc) { Local result = NewEmptyInstance(context, ctorFunc); - - std::shared_ptr> poValue; - if (!skipGCRegistration) { - poValue = ObjectManager::Register(context, result); - } else { - poValue = std::make_shared>(isolate, result); - } - - return poValue; + return ObjectManager::Register(context, result); } Local ArgConverter::CreateEmptyInstanceFunction(Local context) { diff --git a/NativeScript/runtime/ClassBuilder.h b/NativeScript/runtime/ClassBuilder.h index 8ab4a9f6a..d6a353279 100644 --- a/NativeScript/runtime/ClassBuilder.h +++ b/NativeScript/runtime/ClassBuilder.h @@ -37,6 +37,10 @@ class ClassBuilder { v8::Local context); static std::string GetTypeEncoding(const TypeEncoding* typeEncoding, int argsCount); + // `klass` or its nearest superclass not defined from JS. + static Class FindNativeClass(Class klass); + // The implementation of `method` from FindNativeClass(klass). + static IMP FindNotOverridenMethod(Class klass, SEL method); private: static std::atomic classNameCounter_; @@ -72,7 +76,6 @@ class ClassBuilder { static std::string GetTypeEncoding(const TypeEncoding* typeEncoding); static BinaryTypeEncodingType GetTypeEncodingType(v8::Isolate* isolate, v8::Local value); - static IMP FindNotOverridenMethod(Class klass, SEL method); struct CacheItem { public: diff --git a/NativeScript/runtime/ClassBuilder.mm b/NativeScript/runtime/ClassBuilder.mm index ce792f4aa..3baac7706 100644 --- a/NativeScript/runtime/ClassBuilder.mm +++ b/NativeScript/runtime/ClassBuilder.mm @@ -332,10 +332,16 @@ void ScopeClassNameToIsolate(std::string& name, int isolateId) { BaseDataWrapper* wrapper = tns::GetValue(isolate, value); if (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject) { ObjCDataWrapper* objcWrapper = static_cast(wrapper); - objcWrapper->GcProtect(); + if (objcWrapper->OwnershipState() != nullptr) { + ObjectManager::ReconcileOwnership(isolate, objcWrapper); + } else { + objcWrapper->GcProtect(); + } } } }; + // After the retain: reconciling reads the new retain count. + id result = retain(self, @selector(retain)); if (CFRunLoopGetCurrent() != runtimeLoop) { // bare entry: the closure does its own Locker ceremony, exactly // like the performed block it replaces @@ -343,6 +349,7 @@ void ScopeClassNameToIsolate(std::string& name, int isolateId) { } else { gcProtect(); } + return result; } return retain(self, @selector(retain)); @@ -371,34 +378,28 @@ void ScopeClassNameToIsolate(std::string& name, int isolateId) { BaseDataWrapper* wrapper = tns::GetValue(isolate, value); if (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject) { ObjCDataWrapper* objcWrapper = static_cast(wrapper); - objcWrapper->GcUnprotect(); + if (objcWrapper->OwnershipState() != nullptr) { + ObjectManager::ReconcileOwnership(isolate, objcWrapper); + } else { + objcWrapper->GcUnprotect(); + } } } } }; auto runtime = Runtime::GetRuntime(isolate); auto runtimeLoop = runtime->RuntimeLoop(); + // After the release: reconciling reads the new retain count, and + // the Instances entry still holds the object. + release(self, @selector(release)); if (CFRunLoopGetCurrent() != runtimeLoop) { // bare entry: the closure does its own Locker ceremony, exactly // like the performed block it replaces runtime->GetEventLoop()->PostInternalBare(gcUnprotect); } else { - auto innerCache = isolateWrapper.GetCache(); - auto it = innerCache->Instances.find(self); - if (it != innerCache->Instances.end()) { - v8::Locker locker(isolate); - Isolate::Scope isolate_scope(isolate); - HandleScope handle_scope(isolate); - if (it->second != nullptr) { - Local value = it->second->Get(isolate); - BaseDataWrapper* wrapper = tns::GetValue(isolate, value); - if (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject) { - ObjCDataWrapper* objcWrapper = static_cast(wrapper); - objcWrapper->GcUnprotect(); - } - } - } + gcUnprotect(); } + return; } release(self, @selector(release)); @@ -1016,12 +1017,16 @@ void ScopeClassNameToIsolate(std::string& name, int isolateId) { info.GetReturnValue().Set(superValue); } -IMP ClassBuilder::FindNotOverridenMethod(Class klass, SEL method) { +Class ClassBuilder::FindNativeClass(Class klass) { while (class_conformsToProtocol(klass, @protocol(TNSDerivedClass))) { klass = class_getSuperclass(klass); } - return class_getMethodImplementation(klass, method); + return klass; +} + +IMP ClassBuilder::FindNotOverridenMethod(Class klass, SEL method) { + return class_getMethodImplementation(FindNativeClass(klass), method); } std::atomic ClassBuilder::classNameCounter_{0}; diff --git a/NativeScript/runtime/DataWrapper.h b/NativeScript/runtime/DataWrapper.h index a98655a54..7b2b2c591 100644 --- a/NativeScript/runtime/DataWrapper.h +++ b/NativeScript/runtime/DataWrapper.h @@ -416,8 +416,22 @@ class ObjCDataWrapper : public BaseDataWrapper { // anything keyed on the raw pointer needs this to detect a recycled slot. Class Klass() { return this->klass_; } + // Set for a wrapper whose handle ObjectManager::ReconcileOwnership keeps + // strong while native code also retains the object, and weak once the + // Instances entry is its only owner. The handle's finalizer state, needed to + // re-arm it after a strong period. + ObjectWeakCallbackState* OwnershipState() { return this->ownershipState_; } + void SetOwnershipState(ObjectWeakCallbackState* state) { this->ownershipState_ = state; } + + // A wrapper that holds no reference to its object (one made for an object + // already deallocating), so nothing may release on its behalf. + bool IsBorrowed() { return this->borrowed_; } + void MarkBorrowed() { this->borrowed_ = true; } + private: bool adapterClaim_ = false; + bool borrowed_ = false; + ObjectWeakCallbackState* ownershipState_ = nullptr; id data_; const tns::TypeEncoding* typeEncoding_; Class klass_; diff --git a/NativeScript/runtime/Interop.h b/NativeScript/runtime/Interop.h index 085bb416f..9dcfed691 100644 --- a/NativeScript/runtime/Interop.h +++ b/NativeScript/runtime/Interop.h @@ -107,7 +107,8 @@ class Interop { BaseCall* call, bool marshalToPrimitive, v8::Local parentStruct = v8::Local(), bool isStructMember = false, bool ownsReturnedObject = false, - bool returnsUnmanaged = false, bool isInitializer = false); + bool returnsUnmanaged = false, bool isInitializer = false, + bool releaseCachedOwnedReturn = false); static void SetStructPropertyValue(v8::Local context, StructWrapper* wrapper, const StructField& field, diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index 196714558..5cbc8f408 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -1029,7 +1029,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 isInitializer, bool releaseCachedOwnedReturn) { Isolate* isolate = v8::Isolate::GetCurrent(); if (returnsUnmanaged) { @@ -1363,7 +1363,16 @@ static bool SnapshotArrayLike(Isolate* isolate, Local object, LocalGet(isolate); + 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) { + [result release]; + } + return cached; } // For NSProxy we will try to read the metadata from @@ -1373,8 +1382,7 @@ static bool SnapshotArrayLike(Isolate* isolate, Local object, Local additionalProtocols = Interop::GetAdditionalProtocols(typeEncoding); - Local jsResult = - ArgConverter::ConvertArgument(context, wrapper, false, additionalProtocols); + Local jsResult = ArgConverter::ConvertArgument(context, wrapper, additionalProtocols); if (ownsReturnedObject || isInitializer) { [result release]; @@ -1795,7 +1803,9 @@ throw NativeScriptException( SEL selector = methodCall.selector_; if (isInstanceMethod) { SEL swizzledMethodSelector = Interop::GetSwizzledMethodSelector(selector); - if ([methodCall.target_ respondsToSelector:swizzledMethodSelector]) { + // Not -respondsToSelector:, which a JS class may implement by calling + // its base implementation through this very path. + if (class_respondsToSelector(object_getClass(methodCall.target_), swizzledMethodSelector)) { selector = swizzledMethodSelector; } @@ -1867,8 +1877,8 @@ throw NativeScriptException( } ObjCDataWrapper* wrapper = new ObjCDataWrapper((id)e); - Local nativeWrapper = - ArgConverter::CreateJsWrapper(context, wrapper, Local(), true); + Local nativeWrapper = ArgConverter::CreateJsWrapper(context, wrapper, Local()); + tns::DeleteWrapperIfUnused(isolate, nativeWrapper, wrapper); jsErrObj->Set(context, tns::ToV8String(isolate, "nativeException"), nativeWrapper) .FromMaybe(false); @@ -1911,8 +1921,8 @@ throw NativeScriptException( // Wrap the native NSError instance into a JS object and attach as nativeException ObjCDataWrapper* wrapper = new ObjCDataWrapper(error); - Local nativeWrapper = - ArgConverter::CreateJsWrapper(context, wrapper, Local(), true); + Local nativeWrapper = ArgConverter::CreateJsWrapper(context, wrapper, Local()); + tns::DeleteWrapperIfUnused(isolate, nativeWrapper, wrapper); jsErrObj->Set(context, tns::ToV8String(isolate, "nativeException"), nativeWrapper) .FromMaybe(false); @@ -1927,10 +1937,10 @@ throw NativeScriptException( } } - Local result = - Interop::GetResult(methodCall.context_, methodCall.typeEncoding_, &call, marshalToPrimitive, - Local(), false, methodCall.ownsReturnedObject_, - methodCall.returnsUnmanaged_, methodCall.isInitializer_); + Local result = Interop::GetResult( + methodCall.context_, methodCall.typeEncoding_, &call, marshalToPrimitive, Local(), + false, methodCall.ownsReturnedObject_, methodCall.returnsUnmanaged_, + methodCall.isInitializer_, !isInstanceMethod); // 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/NativeScriptException.h b/NativeScript/runtime/NativeScriptException.h index f11b77de2..9928fcdb2 100644 --- a/NativeScript/runtime/NativeScriptException.h +++ b/NativeScript/runtime/NativeScriptException.h @@ -91,6 +91,7 @@ class NativeScriptException { private: v8::Persistent* javascriptException_; + v8::Isolate* isolate_ = nullptr; std::string name_; std::string message_; std::string stackTrace_; diff --git a/NativeScript/runtime/NativeScriptException.mm b/NativeScript/runtime/NativeScriptException.mm index 0a3fe5b30..ee29e0ac1 100644 --- a/NativeScript/runtime/NativeScriptException.mm +++ b/NativeScript/runtime/NativeScriptException.mm @@ -96,6 +96,7 @@ static void ConsiderStackCandidate(PendingErrorDisplay& state, v8::Isolate* isol } Local error = tc.Exception(); this->javascriptException_ = new Persistent(isolate, tc.Exception()); + this->isolate_ = isolate; this->message_ = GetErrorMessage(isolate, error, message); this->stackTrace_ = tns::GetSmartStackTrace(isolate, &tc, error); this->fullMessage_ = GetFullMessage(isolate, tc, this->message_); @@ -112,6 +113,7 @@ static void ConsiderStackCandidate(PendingErrorDisplay& state, v8::Isolate* isol ->Set(context, ToV8String(isolate, "name"), ToV8String(isolate, this->name_)) .FromMaybe(false); this->javascriptException_ = new Persistent(isolate, error); + this->isolate_ = isolate; this->message_ = GetErrorMessage(isolate, error, message); this->stackTrace_ = GetErrorStackTrace(isolate, Exception::GetStackTrace(error)); this->fullMessage_ = @@ -120,11 +122,28 @@ static void ConsiderStackCandidate(PendingErrorDisplay& state, v8::Isolate* isol NativeScriptException::NativeScriptException(Isolate* isolate, Local jsError, const std::string& message) { this->javascriptException_ = new Persistent(isolate, jsError); + this->isolate_ = isolate; this->message_ = message; this->name_ = "NativeScriptException"; } -NativeScriptException::~NativeScriptException() { delete this->javascriptException_; } +NativeScriptException::~NativeScriptException() { + if (this->javascriptException_ == nullptr) { + return; + } + // Deleting a Persistent does not release its V8 handle. The exception can + // be caught after the Locker that ran the throwing code is gone (event + // loop tasks), so the handle is released under one. + if (Runtime::IsAlive(this->isolate_)) { + if (v8::Locker::IsLocked(this->isolate_)) { + this->javascriptException_->Reset(); + } else { + v8::Locker locker(this->isolate_); + this->javascriptException_->Reset(); + } + } + delete this->javascriptException_; +} void NativeScriptException::OnUncaughtError(Local message, Local error) { @try { diff --git a/NativeScript/runtime/ObjectManager.h b/NativeScript/runtime/ObjectManager.h index 67691ff32..145ba894e 100644 --- a/NativeScript/runtime/ObjectManager.h +++ b/NativeScript/runtime/ObjectManager.h @@ -6,6 +6,7 @@ namespace tns { class ObjectManager; +class ObjCDataWrapper; // Parameter of the kFinalizer weak callback armed on target_. // @@ -53,6 +54,15 @@ class ObjectManager { // Disposes every handle from Register() that is still alive. Replaces the // old Isolate::VisitHandlesWithClassIds walk, which V8 removed. static void DisposeAllRegistered(v8::Isolate* isolate); + // Takes over the handle `state` belongs to (the wrapper's Instances entry): + // from now on it is strong while something besides that entry retains the + // object and weak otherwise, re-checked whenever ReconcileOwnership runs and + // when a major collection starts. + static void ManageOwnership(v8::Isolate* isolate, ObjCDataWrapper* wrapper, + ObjectWeakCallbackState* state); + // Requires the isolate locked and entered, on any thread. + static void ReconcileOwnership(v8::Isolate* isolate, + ObjCDataWrapper* wrapper); private: static void ReleaseNativeCounterpartCallback( diff --git a/NativeScript/runtime/ObjectManager.mm b/NativeScript/runtime/ObjectManager.mm index 904ab2333..793612cd3 100644 --- a/NativeScript/runtime/ObjectManager.mm +++ b/NativeScript/runtime/ObjectManager.mm @@ -2,7 +2,9 @@ #include #include #include +#include #include "Caches.h" +#include "ClassBuilder.h" #include "Constants.h" #include "DataWrapper.h" #include "FFICall.h" @@ -15,9 +17,63 @@ static Class NSTimerClass = objc_getClass("NSTimer"); +namespace { + +// Wrappers whose handle ObjectManager::ManageOwnership took over, keyed by the +// finalizer state their handle carries. +struct OwnershipTracking { + std::unordered_map managed; +}; + +void ForgetOwnership(Isolate* isolate, ObjectWeakCallbackState* state) { + if (OwnershipTracking* tracking = Caches::StateFor(isolate)) { + tracking->managed.erase(state); + } +} + +void ApplyOwnership(ObjectWeakCallbackState* state, bool nativelyOwned) { + bool weak = state->target_->IsWeak(); + if (nativelyOwned && weak) { + state->target_->ClearWeak(); + } else if (!nativelyOwned && !weak) { + state->target_->SetWeak(state, ObjectManager::FinalizerCallback, WeakCallbackType::kFinalizer); + } +} + +// The GC prologue may not enter JS, so a JS-implemented -retainCount is +// skipped. Timers read as unowned: the retain count of an uninitialized NSTimer +// (NSTimer.alloc()) can hang. +bool NativelyOwnedWithoutJS(id obj) { + for (Class c = object_getClass(obj); c != nil; c = class_getSuperclass(c)) { + if (c == NSTimerClass) { + return false; + } + } + IMP imp = ClassBuilder::FindNotOverridenMethod(object_getClass(obj), @selector(retainCount)); + return ((NSUInteger (*)(id, SEL))imp)(obj, @selector(retainCount)) > 1; +} + +void ReconcileOwnershipBeforeGC(Isolate* isolate, GCType, GCCallbackFlags) { + OwnershipTracking* tracking = Caches::StateFor(isolate); + if (tracking == nullptr) { + return; + } + for (auto& [state, wrapper] : tracking->managed) { + if (state->disposing_ || state->target_ == nullptr || state->target_->IsEmpty()) { + continue; + } + ApplyOwnership(state, NativelyOwnedWithoutJS(wrapper->Data())); + } +} + +} // namespace + void ObjectManager::Init(Isolate* isolate, Local globalTemplate) { globalTemplate->Set(tns::ToV8String(isolate, "__releaseNativeCounterpart"), FunctionTemplate::New(isolate, ReleaseNativeCounterpartCallback)); + isolate->AddGCPrologueCallback( + ReconcileOwnershipBeforeGC, + static_cast(kGCTypeMarkSweepCompact | kGCTypeIncrementalMarking)); } namespace { @@ -139,6 +195,29 @@ void DisposeHandle(v8::Isolate* isolate, DisposeHandle(isolate, cache->FunctionReferenceCtorFunc); } +void ObjectManager::ManageOwnership(Isolate* isolate, ObjCDataWrapper* wrapper, + ObjectWeakCallbackState* state) { + OwnershipTracking* tracking = Caches::StateFor(isolate); + if (tracking == nullptr || state == nullptr) { + return; + } + wrapper->SetOwnershipState(state); + tracking->managed[state] = wrapper; + ObjectManager::ReconcileOwnership(isolate, wrapper); +} + +void ObjectManager::ReconcileOwnership(Isolate* isolate, ObjCDataWrapper* wrapper) { + ObjectWeakCallbackState* state = wrapper->OwnershipState(); + if (state == nullptr || state->disposing_ || state->target_ == nullptr || + state->target_->IsEmpty()) { + return; + } + // The handle's strength carries the ownership; a GcProtect from the retain + // swizzle would keep a weak handle from ever being disposed. + wrapper->GcUnprotect(); + ApplyOwnership(state, NativelyOwnedWithoutJS(wrapper->Data())); +} + void ObjectManager::FinalizerCallback(const WeakCallbackInfo& data) { ObjectWeakCallbackState* state = data.GetParameter(); Isolate* isolate = data.GetIsolate(); @@ -167,12 +246,23 @@ void DisposeHandle(v8::Isolate* isolate, if (disposed || state->target_->IsEmpty()) { state->target_->Reset(); UnlinkRegistered(state); + ForgetOwnership(isolate, state); delete state; return; } state->target_->ClearWeak(); state->target_->SetWeak(state, FinalizerCallback, WeakCallbackType::kFinalizer); + + // Refused because native code took the object back: keep the handle strong + // rather than resurrecting it at every collection. + BaseDataWrapper* wrapper = tns::GetValue(isolate, value); + if (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject) { + ObjCDataWrapper* objcWrapper = static_cast(wrapper); + if (objcWrapper->OwnershipState() == state) { + ApplyOwnership(state, NativelyOwnedWithoutJS(objcWrapper->Data())); + } + } } bool ObjectManager::DisposeValue(Isolate* isolate, Local value, bool isFinalDisposal) { @@ -200,6 +290,16 @@ void DisposeHandle(v8::Isolate* isolate, return false; } + // A handle ReconcileOwnership keeps is weak while the Instances entry looked + // like the only owner; a retain from another thread can still be on its way + // to making it strong again. + if (!isFinalDisposal && wrapper->Type() == WrapperType::ObjCObject) { + ObjCDataWrapper* objcWrapper = static_cast(wrapper); + if (objcWrapper->OwnershipState() != nullptr && NativelyOwnedWithoutJS(objcWrapper->Data())) { + return false; + } + } + std::shared_ptr cache = Caches::Get(isolate); switch (wrapper->Type()) { case WrapperType::Struct: @@ -350,11 +450,33 @@ void DisposeHandle(v8::Isolate* isolate, } ObjCDataWrapper* objcWrapper = static_cast(wrapper); + if (objcWrapper->IsBorrowed()) { + if (tns::GetValue(isolate, value) == wrapper) { + tns::SetValue(isolate, value.As(), nullptr); + delete wrapper; + } + return; + } + id data = objcWrapper->Data(); if (data != nil) { std::shared_ptr cache = Caches::Get(isolate); auto it = cache->Instances.find(data); - if (it != cache->Instances.end()) { + if (ObjectWeakCallbackState* managed = objcWrapper->OwnershipState()) { + // The handle ReconcileOwnership manages is this wrapper's own, whatever + // Instances maps the object to now. + if (managed->disposing_) { + return; + } + if (it != cache->Instances.end() && it->second == managed->target_) { + cache->Instances.erase(it); + } + managed->target_->Reset(); + UnlinkRegistered(managed); + ForgetOwnership(isolate, managed); + delete managed; + objcWrapper->SetOwnershipState(nullptr); + } else if (it != cache->Instances.end()) { std::shared_ptr> handle = it->second; ObjectWeakCallbackState* state = handle->IsWeak() ? handle->ClearWeak() : nullptr; @@ -372,6 +494,7 @@ void DisposeHandle(v8::Isolate* isolate, // parameter. handle->Reset(); UnlinkRegistered(state); + ForgetOwnership(isolate, state); delete state; } } diff --git a/TestFixtures/Marshalling/TNSAllocLog.h b/TestFixtures/Marshalling/TNSAllocLog.h index 78c263782..ef80f9a93 100644 --- a/TestFixtures/Marshalling/TNSAllocLog.h +++ b/TestFixtures/Marshalling/TNSAllocLog.h @@ -12,4 +12,11 @@ @end +// -init answers a new TNSAllocLog in place of the receiver. +@interface TNSSubstitutingInit : NSObject + +- (instancetype)init; + +@end + #endif /* TNSAllocLog_h */ diff --git a/TestFixtures/Marshalling/TNSAllocLog.m b/TestFixtures/Marshalling/TNSAllocLog.m index 448da23a4..e04bb917d 100644 --- a/TestFixtures/Marshalling/TNSAllocLog.m +++ b/TestFixtures/Marshalling/TNSAllocLog.m @@ -21,3 +21,11 @@ - (void)dealloc { } @end + +@implementation TNSSubstitutingInit + +- (instancetype)init { + return (id)[[TNSAllocLog alloc] init]; +} + +@end diff --git a/TestRunner/app/tests/NativeCallbackSelfTests.js b/TestRunner/app/tests/NativeCallbackSelfTests.js new file mode 100644 index 000000000..049ae5e21 --- /dev/null +++ b/TestRunner/app/tests/NativeCallbackSelfTests.js @@ -0,0 +1,411 @@ +// Wrappers created for objects that native code hands to JS-implemented +// methods, and the native objects attached to errors: kept while native code +// owns them, released exactly once afterwards. +describe("Native callback self wrappers", function () { + function scrub(n) { + return n > 0 ? scrub(n - 1) + n : 0; + } + + // Collect, then give autorelease pools and posted retain/release work a + // turn of the run loop. + function settle(rounds, callback) { + scrub(300); + __collect(); + __collect(); + setTimeout(function () { + NSObject.new(); + if (rounds > 1) { + settle(rounds - 1, callback); + } else { + callback(); + } + }, 10); + } + + function deallocs() { + return (TNSGetOutput().match(/TNSAllocLog dealloc/g) || []).length; + } + + var initCalls = 0; + var JSInit = (function (_super) { + __extends(JSInit, _super); + function JSInit() { + return (_super !== null && _super.apply(this, arguments)) || this; + } + JSInit.prototype.init = function () { + var self = _super.prototype.init.call(this); + initCalls++; + self.initialized = true; + return self; + }; + return JSInit; + })(TNSAllocLog); + + // Objects earlier specs dropped are collected first, so the dealloc log + // only counts this spec's objects. + beforeEach(function (done) { + settle(2, function () { + TNSClearOutput(); + initCalls = 0; + done(); + }); + }); + + // +new runs the JS-implemented init from native code, before JS has a + // wrapper for the object. + it("keeps the JS state of a natively owned object whose JS initializer created its wrapper, then frees it once", function (done) { + var owner = NSMutableArray.new(); + (function () { + var obj = JSInit.new(); + obj.state = { i: 1 }; + 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(1); + obj = null; + expect(deallocs()).toBe(0); + owner.removeAllObjects(); + settle(3, function () { + expect(deallocs()).toBe(1); + done(); + }); + }); + }); + + it("frees an object JS alone holds after +new ran its JS initializer", function (done) { + var obj = JSInit.new(); + obj.state = { i: 2 }; + settle(2, function () { + expect(obj.state.i).toBe(2); + expect(deallocs()).toBe(0); + obj = null; + settle(3, function () { + expect(deallocs()).toBe(1); + done(); + }); + }); + }); + + it("keeps the JS state of cells UIKit creates from a registered class until their superview lets them go", function (done) { + var cellInits = 0; + var Cell = UITableViewCell.extend({ + initWithStyleReuseIdentifier: function (style, reuseIdentifier) { + var cell = UITableViewCell.prototype.initWithStyleReuseIdentifier.call(this, style, reuseIdentifier); + cellInits++; + cell.fromInitializer = true; + return cell; + } + }, { name: "NativeCallbackSelfCell" }); + var watch = NSHashTable.weakObjectsHashTable(); + var host = UIView.new(); + (function () { + var table = UITableView.new(); + table.registerClassForCellReuseIdentifier(Cell.class(), "cell"); + for (var i = 0; i < 4; i++) { + var cell = table.dequeueReusableCellWithIdentifier("cell"); + cell.owner = { row: i }; + host.addSubview(cell); + watch.addObject(cell); + } + })(); + settle(3, function () { + expect(cellInits).toBe(4); + var subviews = host.subviews; + expect(subviews.count).toBe(4); + for (var i = 0; i < subviews.count; i++) { + var cell = subviews.objectAtIndex(i); + expect(cell.fromInitializer).toBe(true); + expect(cell.owner && cell.owner.row).toBe(i); + } + subviews = null; + cell = null; + (function () { + var views = host.subviews; + for (var j = views.count - 1; j >= 0; j--) { + views.objectAtIndex(j).removeFromSuperview(); + } + })(); + settle(4, function () { + expect(host.subviews.count).toBe(0); + expect(watch.allObjects.count).toBe(0); + done(); + }); + }); + }); + + it("frees a managed wrapper's object once when __releaseNativeCounterpart runs on it", function (done) { + var owner = NSMutableArray.new(); + var natively = JSInit.new(); + owner.addObject(natively); + var jsOnly = JSInit.new(); + settle(2, function () { + __releaseNativeCounterpart(natively); + __releaseNativeCounterpart(jsOnly); + natively = null; + jsOnly = null; + settle(3, function () { + expect(deallocs()).toBe(1); + owner.removeAllObjects(); + settle(3, function () { + expect(deallocs()).toBe(2); + done(); + }); + }); + }); + }); + + it("does not release an instance method's owned return that is its receiver", function (done) { + var obj = TNSAllocLog.alloc().init(); + var same = obj.awakeAfterUsingCoder(null); + expect(same).toBe(obj); + settle(3, function () { + expect(deallocs()).toBe(0); + expect(obj.hash > 0).toBe(true); + done(); + }); + }); + + it("does not release what a JS copyWithZone hands to a native -copy", function (done) { + var target = TNSAllocLog.alloc().init(); + var Copyable = TNSAllocLog.extend({ + copyWithZone: function (zone) { + return target; + } + }, { name: "NativeCallbackSelfCopyable", protocols: [NSCopying] }); + var original = Copyable.alloc().init(); + var copy = original.copy(); + expect(copy).toBe(target); + copy = null; + settle(3, function () { + expect(deallocs()).toBe(0); + expect(target.hash > 0).toBe(true); + expect(original.hash > 0).toBe(true); + done(); + }); + }); + + // The JS init runs from native code on an object JS has no wrapper for + // yet; wrapping it must not send it a message its JS class implements. + function expectRespondsToSelectorOverrideWorks(Responder, counter) { + var creators = [ + function () { + return Responder.alloc().init(); + }, + function () { + return Responder.new(); + } + ]; + creators.forEach(function (create) { + counter.inits = 0; + counter.responds = 0; + counter.voidCalls = 0; + var obj = create(); + expect(obj instanceof Responder).toBe(true); + expect(counter.inits).toBe(1); + TNSTestNativeCallbacks.inheritanceVoidSelector(obj); + expect(counter.responds).toBeGreaterThan(0); + expect(counter.voidCalls).toBe(1); + }); + } + + it("wraps objects of an extend() class that implements respondsToSelector", function () { + var counter = {}; + var Responder = NSObject.extend({ + init: function () { + counter.inits++; + return NSObject.prototype.init.call(this); + }, + respondsToSelector: function (selector) { + counter.responds++; + return NSObject.prototype.respondsToSelector.call(this, selector); + }, + voidSelector: function () { + counter.voidCalls++; + } + }, { + name: "NativeCallbackSelfResponder", + exposedMethods: { + voidSelector: { returns: interop.types.void } + } + }); + expectRespondsToSelectorOverrideWorks(Responder, counter); + }); + + it("wraps objects of an __extends class that implements respondsToSelector", function () { + var counter = {}; + var Responder = (function (_super) { + __extends(Responder, _super); + function Responder() { + return (_super !== null && _super.apply(this, arguments)) || this; + } + Responder.prototype.init = function () { + counter.inits++; + return _super.prototype.init.call(this); + }; + Responder.prototype.respondsToSelector = function (selector) { + counter.responds++; + return _super.prototype.respondsToSelector.call(this, selector); + }; + Responder.prototype.voidSelector = function () { + counter.voidCalls++; + }; + Responder.ObjCExposedMethods = { + voidSelector: { returns: interop.types.void } + }; + return Responder; + })(NSObject); + expectRespondsToSelectorOverrideWorks(Responder, counter); + }); + + // Native callers own what an init-family method returns, including an + // object a JS initializer answers in place of its receiver. + function expectInitializerResultOwnedByCaller(create, done) { + var owner = NSMutableArray.new(); + (function () { + var obj = create(); + expect(obj instanceof TNSAllocLog).toBe(true); + owner.addObject(obj); + })(); + settle(3, function () { + expect(initCalls).toBe(1); + expect(deallocs()).toBe(0); + var obj = owner.objectAtIndex(0); + expect(obj instanceof TNSAllocLog).toBe(true); + expect(obj.hash > 0).toBe(true); + obj = null; + owner.removeAllObjects(); + settle(3, function () { + expect(deallocs()).toBe(1); + done(); + }); + }); + } + + var initResult = null; + var InitReturnsOther = NSObject.extend({ + init: function () { + initCalls++; + return initResult; + } + }, { name: "NativeCallbackSelfInitReturnsOther" }); + + function createThroughInitReturningOther(construct) { + initResult = TNSAllocLog.new(); + var obj = construct(); + expect(obj).toBe(initResult); + initResult = null; + return obj; + } + + it("keeps a wrapped object a JS initializer returns instead of its receiver (+new)", function (done) { + expectInitializerResultOwnedByCaller(function () { + return createThroughInitReturningOther(function () { + return InitReturnsOther.new(); + }); + }, done); + }); + + it("keeps a wrapped object a JS initializer returns instead of its receiver (new)", function (done) { + expectInitializerResultOwnedByCaller(function () { + return createThroughInitReturningOther(function () { + return new InitReturnsOther(); + }); + }, done); + }); + + var InitReturnsSubstitute = TNSSubstitutingInit.extend({ + init: function () { + initCalls++; + return TNSSubstitutingInit.prototype.init.call(this); + } + }, { name: "NativeCallbackSelfInitReturnsSubstitute" }); + + it("keeps the substitute a super initializer returned through a JS initializer (+new)", function (done) { + expectInitializerResultOwnedByCaller(function () { + return InitReturnsSubstitute.new(); + }, done); + }); + + it("keeps the substitute a super initializer returned through a JS initializer (new)", function (done) { + expectInitializerResultOwnedByCaller(function () { + return new InitReturnsSubstitute(); + }, done); + }); + + it("reports a receiver kept from a JS method run by -dealloc as released", function (done) { + var saved = null; + var Keeper = TNSApi.extend({ + methodCalledInDealloc: function () { + saved = this; + } + }, { name: "NativeCallbackSelfDeallocKeeper" }); + (function () { + var holder = NSMutableArray.alloc().init(); + holder.addObject(Keeper.alloc().init()); + })(); + settle(4, function () { + expect(saved).not.toBeNull(); + var value; + var error = null; + try { + value = saved.hash; + } catch (e) { + error = e; + } + if (error !== null) { + expect(error.message).toContain("disposed native object"); + } else { + expect(value).toBeUndefined(); + } + saved = null; + done(); + }); + }); + + it("keeps an error's NSError while the error lives and frees it afterwards", function (done) { + var watch = NSHashTable.weakObjectsHashTable(); + var error = null; + try { + NSFileManager.defaultManager.contentsOfDirectoryAtPathError("/nonexistent-native-callback-self"); + } catch (e) { + error = e; + } + expect(error).not.toBeNull(); + expect(error.nativeException).toBeDefined(); + watch.addObject(error.nativeException); + settle(3, function () { + expect(watch.allObjects.count).toBe(1); + expect(error.nativeException.code).toBe(error.code); + error = null; + settle(3, function () { + expect(watch.allObjects.count).toBe(0); + done(); + }); + }); + }); + + it("keeps an error's NSException while the error lives and frees it afterwards", function (done) { + var watch = NSHashTable.weakObjectsHashTable(); + var error = null; + try { + NSArray.array().objectAtIndex(3); + } catch (e) { + error = e; + } + expect(error).not.toBeNull(); + expect(error.nativeException).toBeDefined(); + watch.addObject(error.nativeException); + settle(3, function () { + expect(watch.allObjects.count).toBe(1); + expect(error.nativeException.name).toBe("NSRangeException"); + error = null; + settle(3, function () { + expect(watch.allObjects.count).toBe(0); + done(); + }); + }); + }); +}); diff --git a/TestRunner/app/tests/index.js b/TestRunner/app/tests/index.js index 9c9ce44f4..96de9c100 100644 --- a/TestRunner/app/tests/index.js +++ b/TestRunner/app/tests/index.js @@ -134,6 +134,7 @@ require("./NsWorkerThreadsTests"); require("./NsUrlTests"); require("./NsInteropTests"); require("./GCFinalizerTests"); +require("./NativeCallbackSelfTests"); require("./BlockCacheRaceTests"); require("./WorkerConcurrentStartupTests"); require("./WorkerOptionsTests");