diff --git a/NativeScript/runtime/Caches.h b/NativeScript/runtime/Caches.h index 01d0b642..78499a78 100644 --- a/NativeScript/runtime/Caches.h +++ b/NativeScript/runtime/Caches.h @@ -169,6 +169,9 @@ class Caches { robin_hood::unordered_map>> Instances; + // Classes built by __extends, whose swizzled retain/release drive + // GcProtect. Only their instances can be kept alive while retained. + robin_hood::unordered_set RetainTrackedClasses; // Root struct objects by (backing buffer, struct type). Child views created // for nested-struct fields are never entered here. robin_hood::unordered_map, diff --git a/NativeScript/runtime/ClassBuilder.mm b/NativeScript/runtime/ClassBuilder.mm index 5b91128b..798590fb 100644 --- a/NativeScript/runtime/ClassBuilder.mm +++ b/NativeScript/runtime/ClassBuilder.mm @@ -332,7 +332,7 @@ 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(); + ObjectManager::SetGcProtected(it->second, objcWrapper, true); } } }; @@ -371,7 +371,7 @@ 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(); + ObjectManager::SetGcProtected(it->second, objcWrapper, false); } } } @@ -394,7 +394,7 @@ 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(); + ObjectManager::SetGcProtected(it->second, objcWrapper, false); } } } @@ -404,6 +404,7 @@ void ScopeClassNameToIsolate(std::string& name, int isolateId) { release(self, @selector(release)); }); class_addMethod(extendedClass, @selector(release), newRelease, "v@:"); + cache->RetainTrackedClasses.insert(extendedClass); info.GetReturnValue().SetUndefined(); }).ToLocalChecked(); diff --git a/NativeScript/runtime/DataWrapper.h b/NativeScript/runtime/DataWrapper.h index 4059e2e6..e29b906b 100644 --- a/NativeScript/runtime/DataWrapper.h +++ b/NativeScript/runtime/DataWrapper.h @@ -400,8 +400,22 @@ class ObjCDataWrapper : public BaseDataWrapper { // anything keyed on the raw pointer needs this to detect a recycled slot. Class Klass() { return this->klass_; } + // Set by interop.keepAliveWhileRetained. While such an instance is + // GcProtected its registered handle is strong rather than a refused + // finalizer, so V8 traces what it references; + // ObjectManager::SyncKeepAliveRoot parks the weak callback's parameter here + // until the handle is weak again. + bool KeepsAliveWhileRetained() { return this->keepAliveWhileRetained_; } + void KeepAliveWhileRetained() { this->keepAliveWhileRetained_ = true; } + ObjectWeakCallbackState* RootedState() { return this->rootedState_; } + void SetRootedState(ObjectWeakCallbackState* state) { + this->rootedState_ = state; + } + private: bool adapterClaim_ = false; + bool keepAliveWhileRetained_ = false; + ObjectWeakCallbackState* rootedState_ = nullptr; id data_; const tns::TypeEncoding* typeEncoding_; Class klass_; diff --git a/NativeScript/runtime/Interop.h b/NativeScript/runtime/Interop.h index b35cb5a0..6ec4d36a 100644 --- a/NativeScript/runtime/Interop.h +++ b/NativeScript/runtime/Interop.h @@ -143,6 +143,8 @@ class Interop { v8::Local interop); static void RegisterHandleOfFunction(v8::Local context, v8::Local interop); + static void RegisterKeepAliveWhileRetainedFunction( + v8::Local context, v8::Local interop); static void RegisterAllocFunction(v8::Local context, v8::Local interop); static void RegisterFreeFunction(v8::Local context, diff --git a/NativeScript/runtime/InteropTypes.mm b/NativeScript/runtime/InteropTypes.mm index a149d055..bc593d16 100644 --- a/NativeScript/runtime/InteropTypes.mm +++ b/NativeScript/runtime/InteropTypes.mm @@ -28,6 +28,7 @@ RegisterBufferFromDataFunction(context, interop); RegisterStringFromCString(context, interop); RegisterHandleOfFunction(context, interop); + RegisterKeepAliveWhileRetainedFunction(context, interop); RegisterAllocFunction(context, interop); RegisterFreeFunction(context, interop); RegisterAdoptFunction(context, interop); @@ -303,6 +304,69 @@ new PrimitiveDataWrapper(sizeof(void*), tns::Assert(success, isolate); } +// interop.keepAliveWhileRetained(obj): while native code retains `obj` (an +// instance of a class built by __extends), its JS object is held strongly +// instead of only being refused finalization, so everything it references +// stays alive too. The cost is the one that refusal exists to avoid: a cycle +// running back from that native retain to `obj` through JS is never collected. +void Interop::RegisterKeepAliveWhileRetainedFunction(Local context, + Local interop) { + Local func; + bool success = + v8::Function::New(context, [](const FunctionCallbackInfo& info) { + Isolate* isolate = info.GetIsolate(); + auto throwTypeError = [isolate](const char* message) { + isolate->ThrowException(Exception::TypeError(tns::ToV8String(isolate, message))); + }; + + if (info.Length() < 1 || !info[0]->IsObject()) { + throwTypeError("interop.keepAliveWhileRetained expects a native object."); + return; + } + Local arg = info[0].As(); + BaseDataWrapper* wrapper = tns::GetValue(isolate, arg); + if (wrapper == nullptr || wrapper->Type() != WrapperType::ObjCObject) { + throwTypeError("interop.keepAliveWhileRetained expects a native object."); + return; + } + ObjCDataWrapper* objcWrapper = static_cast(wrapper); + id target = objcWrapper->Data(); + if (target == nil) { + throwTypeError("interop.keepAliveWhileRetained expects a native object."); + return; + } + + auto cache = Caches::Get(isolate); + bool tracked = false; + for (Class klass = object_getClass(target); klass != nil; + klass = class_getSuperclass(klass)) { + if (cache->RetainTrackedClasses.contains(klass)) { + tracked = true; + break; + } + } + if (!tracked) { + throwTypeError("interop.keepAliveWhileRetained expects an instance of a class built by " + "__extends: only those observe native retains."); + return; + } + + objcWrapper->KeepAliveWhileRetained(); + auto it = cache->Instances.find(target); + if (it != cache->Instances.end() && it->second != nullptr && !it->second->IsEmpty() && + it->second->Get(isolate) == arg) { + ObjectManager::SyncKeepAliveRoot(it->second, objcWrapper); + } + }).ToLocal(&func); + + Isolate* isolate = v8::Isolate::GetCurrent(); + tns::Assert(success, isolate); + + success = interop->Set(context, tns::ToV8String(isolate, "keepAliveWhileRetained"), func) + .FromMaybe(false); + tns::Assert(success, isolate); +} + void Interop::RegisterAllocFunction(Local context, Local interop) { Local func; bool success = v8::Function::New(context, [](const FunctionCallbackInfo& info) { diff --git a/NativeScript/runtime/ObjectManager.h b/NativeScript/runtime/ObjectManager.h index 67691ff3..77a5d45a 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_. // @@ -50,6 +51,17 @@ class ObjectManager { const v8::WeakCallbackInfo& data); static bool DisposeValue(v8::Isolate* isolate, v8::Local value, bool isFinalDisposal = false); + // The only way a wrapper's GcProtect flag may change: it also keeps an + // interop.keepAliveWhileRetained handle strong exactly while protected. + static void SetGcProtected( + const std::shared_ptr>& handle, + ObjCDataWrapper* wrapper, bool isProtected); + // Makes `handle` strong when `wrapper` keeps alive while retained and is + // protected, and weak again otherwise. `handle` must be the wrapper's entry + // in Caches::Instances. + static void SyncKeepAliveRoot( + const std::shared_ptr>& handle, + ObjCDataWrapper* wrapper); // Disposes every handle from Register() that is still alive. Replaces the // old Isolate::VisitHandlesWithClassIds walk, which V8 removed. static void DisposeAllRegistered(v8::Isolate* isolate); diff --git a/NativeScript/runtime/ObjectManager.mm b/NativeScript/runtime/ObjectManager.mm index 18a2c6cd..b65ad371 100644 --- a/NativeScript/runtime/ObjectManager.mm +++ b/NativeScript/runtime/ObjectManager.mm @@ -339,6 +339,49 @@ void DisposeHandle(v8::Isolate* isolate, return true; } +void ObjectManager::SetGcProtected(const std::shared_ptr>& handle, + ObjCDataWrapper* wrapper, bool isProtected) { + if (isProtected) { + wrapper->GcProtect(); + } else { + wrapper->GcUnprotect(); + } + SyncKeepAliveRoot(handle, wrapper); +} + +void ObjectManager::SyncKeepAliveRoot(const std::shared_ptr>& handle, + ObjCDataWrapper* wrapper) { + bool wantsRoot = wrapper->KeepsAliveWhileRetained() && wrapper->IsGcProtected(); + bool isRooted = wrapper->RootedState() != nullptr; + if (wantsRoot == isRooted || handle == nullptr || handle->IsEmpty()) { + return; + } + + if (wantsRoot) { + // A handle that is not weak was never registered with a finalizer, so + // there is no weakness to suspend. + if (!handle->IsWeak()) { + return; + } + ObjectWeakCallbackState* state = handle->ClearWeak(); + if (state == nullptr) { + return; + } + if (state->disposing_) { + // Reached from inside this handle's own finalizer frame, which owns the + // state: leave the weakness in place for it. + handle->SetWeak(state, FinalizerCallback, WeakCallbackType::kFinalizer); + return; + } + wrapper->SetRootedState(state); + return; + } + + ObjectWeakCallbackState* state = wrapper->RootedState(); + wrapper->SetRootedState(nullptr); + handle->SetWeak(state, FinalizerCallback, WeakCallbackType::kFinalizer); +} + void ObjectManager::ReleaseNativeCounterpartCallback(const FunctionCallbackInfo& info) { Isolate* isolate = info.GetIsolate(); @@ -375,8 +418,17 @@ void DisposeHandle(v8::Isolate* isolate, auto it = cache->Instances.find(data); if (it != cache->Instances.end()) { std::shared_ptr> handle = it->second; - ObjectWeakCallbackState* state = - handle->IsWeak() ? handle->ClearWeak() : nullptr; + // A handle kept alive while retained is strong, its state parked on the + // wrapper; retiring it without taking that state back would leave the + // handle rooted forever. + ObjectWeakCallbackState* state = nullptr; + ObjectWeakCallbackState* rooted = objcWrapper->RootedState(); + if (rooted != nullptr && rooted->target_ == handle) { + objcWrapper->SetRootedState(nullptr); + state = rooted; + } else if (handle->IsWeak()) { + state = handle->ClearWeak(); + } if (state != nullptr && state->disposing_) { // Reached from a -dealloc running inside this handle's own finalizer: // that frame already released the native counterpart and owns the diff --git a/TestRunner/app/tests/KeepAliveWhileRetainedTests.js b/TestRunner/app/tests/KeepAliveWhileRetainedTests.js new file mode 100644 index 00000000..7b9f4ca4 --- /dev/null +++ b/TestRunner/app/tests/KeepAliveWhileRetainedTests.js @@ -0,0 +1,131 @@ +describe("interop.keepAliveWhileRetained", function () { + // Only the TypeScript __extends path installs the retain/release swizzles + // that observe native retains. + function makeExtendedClass() { + return (function (_super) { + __extends(KeepAliveParent, _super); + function KeepAliveParent() { + return (_super !== null && _super.apply(this, arguments)) || this; + } + return KeepAliveParent; + })(NSObject); + } + + // Overwrites the stack region that held the creation locals so + // conservative stack scanning cannot keep dead wrappers alive. + function scrubStack() { + return (function scrub(n) { + return n > 0 ? scrub(n - 1) + n : 0; + })(300); + } + + it("keeps what a natively retained instance references alive", function () { + var Parent = makeExtendedClass(); + var N = 16; + var holder = NSMutableArray.alloc().init(); + + (function create() { + for (var i = 0; i < N; i++) { + var parent = Parent.alloc().init(); + // Before and after the native retain: both orders must root. + if (i % 2 === 0) { + interop.keepAliveWhileRetained(parent); + holder.addObject(parent); + } else { + holder.addObject(parent); + interop.keepAliveWhileRetained(parent); + } + // Owned only by JS, reachable only through the parent. + parent.child = NSMutableArray.alloc().init(); + parent.child.addObject(i); + parent.__mine = true; + } + })(); + + scrubStack(); + __collect(); + __collect(); + + for (var i = 0; i < N; i++) { + var parent = holder.objectAtIndex(i); + expect(parent.__mine).toBe(true); + expect(parent.child.count).toBe(1); + expect(parent.child.objectAtIndex(0)).toBe(i); + } + }); + + it("lets the instance be collected once native lets go", function (done) { + var Parent = makeExtendedClass(); + var holder = NSMutableArray.alloc().init(); + var ref; + + (function create() { + var parent = Parent.alloc().init(); + interop.keepAliveWhileRetained(parent); + holder.addObject(parent); + ref = new WeakRef(parent); + })(); + + scrubStack(); + __collect(); + setTimeout(function () { + __collect(); + // A boolean, not the object: Jasmine must not hold a reference. + expect(ref.deref() !== undefined).toBe(true); + + holder.removeAllObjects(); + scrubStack(); + setTimeout(function () { + __collect(); + setTimeout(function () { + __collect(); + expect(ref.deref()).toBeUndefined(); + done(); + }, 0); + }, 0); + }, 0); + }); + + it("throws a TypeError for objects whose retains are not observed", function () { + expect(function () { + interop.keepAliveWhileRetained(NSObject.alloc().init()); + }).toThrowError(TypeError); + + var PlainExtended = NSObject.extend({}, { name: "KeepAlivePlainExtended" }); + expect(function () { + interop.keepAliveWhileRetained(PlainExtended.alloc().init()); + }).toThrowError(TypeError); + + expect(function () { + interop.keepAliveWhileRetained({}); + }).toThrowError(TypeError); + expect(function () { + interop.keepAliveWhileRetained(); + }).toThrowError(TypeError); + }); + + // Retiring a registration whose handle is strong has to take its parked + // weak-callback state back; otherwise the handle stays a root forever. + it("frees the handle when an instance kept alive is retired by __releaseNativeCounterpart", function (done) { + var Parent = makeExtendedClass(); + var holder = NSMutableArray.alloc().init(); + var ref; + + (function create() { + var parent = Parent.alloc().init(); + interop.keepAliveWhileRetained(parent); + holder.addObject(parent); + ref = new WeakRef(parent); + __releaseNativeCounterpart(parent); + })(); + + scrubStack(); + __collect(); + setTimeout(function () { + __collect(); + expect(ref.deref()).toBeUndefined(); + holder.removeAllObjects(); + done(); + }, 0); + }); +}); diff --git a/TestRunner/app/tests/index.js b/TestRunner/app/tests/index.js index 4a10ad7e..2891ac29 100644 --- a/TestRunner/app/tests/index.js +++ b/TestRunner/app/tests/index.js @@ -132,6 +132,7 @@ require("./NsRuntimeTests"); require("./NsWorkerThreadsTests"); require("./NsUrlTests"); require("./GCFinalizerTests"); +require("./KeepAliveWhileRetainedTests"); require("./BlockCacheRaceTests"); require("./WorkerConcurrentStartupTests"); require("./WorkerOptionsTests"); diff --git a/docs/knowledge/v8-resurrecting-finalizers.md b/docs/knowledge/v8-resurrecting-finalizers.md index c7326287..91a88351 100644 --- a/docs/knowledge/v8-resurrecting-finalizers.md +++ b/docs/knowledge/v8-resurrecting-finalizers.md @@ -195,6 +195,15 @@ both struct property interceptors, the two `InteropTypes.mm` argument readers, a get/set, toString). Regression spec: *"throws instead of crashing when touching a released object through a resurrected parent"* in `GCFinalizerTests.js`. +An instance can opt out of the hazard with `interop.keepAliveWhileRetained(obj)`: while it is +`GcProtected` its registered handle is made strong (`ObjectManager::SyncKeepAliveRoot`, the weak +callback's parameter parked on the `ObjCDataWrapper`) instead of relying on the refused +finalizer, so it is never queued and its closure is traced like any other root. That is exactly +the conversion the previous paragraphs rule out as a default, so it is per instance and the +caller accepts the cost: a cycle from the native retain back to the instance through JS leaks. +It suits delegates and providers that a native framework retains and nothing they reference +retains back. Only `__extends` instances accept it, since only they observe native retains. + Two facts worth knowing when reasoning about which objects can husk: - `Pointer`/`interop.alloc` objects never husk at runtime — `Caches::PointerInstances`