From c17fc9b293d116d6d18a0fd53ff75042aa094ec5 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Sat, 10 Oct 2026 16:10:58 -0300 Subject: [PATCH 1/2] feat(runtime): report calls on released ObjC objects and name what released them * An ObjC method or property call whose receiver lost its native half (a husk the finalizer drain leaves behind, see the "Known hazard" in docs/knowledge/v8-resurrecting-finalizers.md) now follows releasedObjectPolicy like every other released-object touch: it is skipped and reported, or it throws a ReferenceError. It used to throw a debug-only error blaming HMR, and to assert in release builds. * In debug builds the drain names the hazard when it creates one. FinalizerCallback tags each ObjC husk with its native class, and when it keeps a natively retained object it records that object's own enumerable data properties one level deep; whichever of the pair finalizes second tags the husk with "Owner.property" and logs a warning. GetValueOrReport puts both tags in its message, so the error says what was released and through which property. * The per-collection bookkeeping lives in Caches, is reset by a GC prologue and is released in DisposeAllRegistered while the isolate is still alive. --- NativeScript/runtime/ArgConverter.mm | 24 ++-- NativeScript/runtime/Caches.h | 9 ++ NativeScript/runtime/Helpers.h | 7 + NativeScript/runtime/Helpers.mm | 34 ++++- NativeScript/runtime/ObjectManager.mm | 138 +++++++++++++++++++ TestRunner/app/tests/GCFinalizerTests.js | 80 +++++++++++ docs/knowledge/v8-resurrecting-finalizers.md | 13 +- docs/ns-builtin-modules.md | 16 ++- 8 files changed, 297 insertions(+), 24 deletions(-) diff --git a/NativeScript/runtime/ArgConverter.mm b/NativeScript/runtime/ArgConverter.mm index 80263ab9..991d6e71 100644 --- a/NativeScript/runtime/ArgConverter.mm +++ b/NativeScript/runtime/ArgConverter.mm @@ -37,21 +37,15 @@ BaseDataWrapper* wrapper = tns::GetValue(isolate, receiver); if (wrapper == nullptr) { - // During fast view churn like HMR in development, JS objects can outlive their - // native wrappers briefly. In Debug, throw a catchable JS error instead of crashing. - // In Release, assert so crash reporting can capture unexpected cases. - if (RuntimeConfig.IsDebug) { - const char* selectorStr = meta ? meta->selectorAsString() : ""; - const char* jsNameStr = meta ? meta->jsName() : ""; - const char* classNameStr = klass ? class_getName(klass) : ""; - std::string errMsg = std::string("Cannot call method '") + jsNameStr + - "' on a disposed native object (class: " + classNameStr + - ", selector: " + selectorStr + - "). This can happen during HMR or fast view churn."; - throw NativeScriptException(isolate, errMsg); - } else { - tns::Assert(false, isolate); - } + // A JS object can outlive its native half: a husk the finalizer drain + // left behind (docs/knowledge/v8-resurrecting-finalizers.md, "Known + // hazard"), or one briefly orphaned by fast view churn like HMR. Like + // every other touch of a released object, the call follows + // releasedObjectPolicy: it is skipped and reported, or it throws. + std::string operation = std::string("method '") + (meta ? meta->jsName() : "") + + "' (" + (meta ? meta->selectorAsString() : "") + ")"; + tns::GetValueOrReport(isolate, receiver, operation.c_str()); + return Local(); } if (wrapper->Type() == WrapperType::ObjCAllocObject) { diff --git a/NativeScript/runtime/Caches.h b/NativeScript/runtime/Caches.h index 01d0b642..a3800868 100644 --- a/NativeScript/runtime/Caches.h +++ b/NativeScript/runtime/Caches.h @@ -143,6 +143,15 @@ class Caches { // wrappers. ObjectWeakCallbackState* ObjectManagedValues = nullptr; + // Debug builds only, for naming what a finalizer drain released through a + // natively retained object it kept (see ObjectManager::FinalizerCallback). + // The drain can run the child's finalizer before or after the parent's, so + // each side records what the other needs. A GC prologue clears both, so they + // only ever describe the collection in progress. + std::vector> ReleasedThisGC; + std::vector, std::string>> + KeptObjectProperties; + robin_hood::unordered_map>> Prototypes; diff --git a/NativeScript/runtime/Helpers.h b/NativeScript/runtime/Helpers.h index 5b48ade5..60b2dcd4 100644 --- a/NativeScript/runtime/Helpers.h +++ b/NativeScript/runtime/Helpers.h @@ -284,6 +284,13 @@ void SetReleasedObjectPolicy(ReleasedObjectPolicy policy); // field unchecked must go through this. BaseDataWrapper* GetValueOrReport(v8::Isolate* isolate, const v8::Local& val, const char* operation); + +// Private keys a debug-build finalizer drain sets on a husk so the +// GetValueOrReport message can say what was released and through what: +// the native class name, and "Owner.property" when the drain released it +// through a natively retained object it kept (ObjectManager::FinalizerCallback). +constexpr const char* kReleasedClassKey = "ns:releasedClass"; +constexpr const char* kReleasedThroughKey = "ns:releasedThrough"; void DeleteValue(v8::Isolate* isolate, const v8::Local& val); bool DeleteWrapperIfUnused(v8::Isolate* isolate, const v8::Local& obj, BaseDataWrapper* value); diff --git a/NativeScript/runtime/Helpers.mm b/NativeScript/runtime/Helpers.mm index ef35ddaa..84da3aef 100644 --- a/NativeScript/runtime/Helpers.mm +++ b/NativeScript/runtime/Helpers.mm @@ -471,9 +471,39 @@ void WriteDebugLine(tns::LogCategory category, const char* message) { return wrapper; } + std::string releasedClass; + std::string releasedThrough; + if (val->IsObject()) { + Local context = isolate->GetCurrentContext(); + Local obj = val.As(); + Local tag; + if (obj->GetPrivate(context, + Private::ForApi(isolate, tns::ToV8String(isolate, kReleasedClassKey))) + .ToLocal(&tag) && + tag->IsString()) { + releasedClass = tns::ToString(isolate, tag); + } + if (obj->GetPrivate(context, + Private::ForApi(isolate, tns::ToV8String(isolate, kReleasedThroughKey))) + .ToLocal(&tag) && + tag->IsString()) { + releasedThrough = tns::ToString(isolate, tag); + } + } + std::string message = - std::string("The native object has been released or is not a valid native object (") + - operation + ")"; + releasedClass.empty() + ? std::string("The native object has been released or is not a valid native object (") + + operation + ")" + : std::string("The native ") + releasedClass + " object has been released (" + operation + + ")"; + if (!releasedThrough.empty()) { + message += ". It was released by the garbage collector while still referenced from '" + + releasedThrough + "': '" + releasedThrough.substr(0, releasedThrough.rfind('.')) + + "' is kept alive only because native code retains it, which does not keep the " + "objects its JS properties reference alive. Keep the owner or the property's " + "value reachable from a JS root"; + } if (GetReleasedObjectPolicy() == ReleasedObjectPolicy::kThrow) { isolate->ThrowException(v8::Exception::ReferenceError(tns::ToV8String(isolate, message))); diff --git a/NativeScript/runtime/ObjectManager.mm b/NativeScript/runtime/ObjectManager.mm index 18a2c6cd..c9ecb872 100644 --- a/NativeScript/runtime/ObjectManager.mm +++ b/NativeScript/runtime/ObjectManager.mm @@ -7,6 +7,7 @@ #include "DataWrapper.h" #include "FFICall.h" #include "Helpers.h" +#include "RuntimeConfig.h" using namespace v8; using namespace std; @@ -15,13 +16,129 @@ static Class NSTimerClass = objc_getClass("NSTimer"); +namespace { + +void ResetReleaseTracking(Isolate* isolate, GCType type, GCCallbackFlags flags) { + std::shared_ptr cache = Caches::Get(isolate); + if (cache == nullptr) { + return; + } + cache->ReleasedThisGC.clear(); + cache->KeptObjectProperties.clear(); +} + +} // namespace + void ObjectManager::Init(Isolate* isolate, Local globalTemplate) { globalTemplate->Set(tns::ToV8String(isolate, "__releaseNativeCounterpart"), FunctionTemplate::New(isolate, ReleaseNativeCounterpartCallback)); + if (RuntimeConfig.IsDebug) { + isolate->AddGCPrologueCallback(ResetReleaseTracking); + } } namespace { +// Names the class of an ObjC object this finalizer is about to release, or +// returns an empty string when disposal will not release one (protected, +// already detached, or not an ObjC wrapper). Read before disposal: it deletes +// the wrapper. +std::string ReleasableClassName(Isolate* isolate, Local value) { + BaseDataWrapper* wrapper = tns::GetValue(isolate, value); + if (wrapper == nullptr || wrapper->Type() != WrapperType::ObjCObject || + wrapper->IsGcProtected()) { + return std::string(); + } + id target = static_cast(wrapper)->Data(); + return target != nil ? std::string(object_getClassName(target)) : std::string(); +} + +void SetTag(Isolate* isolate, Local obj, const char* key, const std::string& value) { + Local context; + if (!obj->GetCreationContext(isolate).ToLocal(&context)) { + return; + } + obj->SetPrivate(context, Private::ForApi(isolate, tns::ToV8String(isolate, key)), + tns::ToV8String(isolate, value)) + .FromMaybe(false); +} + +void MarkReleasedThrough(Isolate* isolate, Local released, const std::string& path) { + SetTag(isolate, released, tns::kReleasedThroughKey, path); + Log(@"Warning: the native object referenced by '%s' was released by the garbage collector. " + @"'%s' is kept alive only because native code retains it, which does not keep the objects " + @"its JS properties reference alive. Keep the owner or the property's value reachable " + @"from a JS root.", + path.c_str(), path.substr(0, path.rfind('.')).c_str()); +} + +void TrackReleased(Isolate* isolate, Local released, const std::string& className) { + SetTag(isolate, released, tns::kReleasedClassKey, className); + std::shared_ptr cache = Caches::Get(isolate); + for (auto& entry : cache->KeptObjectProperties) { + if (entry.first.Get(isolate) == released) { + MarkReleasedThrough(isolate, released, entry.second); + return; + } + } + cache->ReleasedThisGC.emplace_back(isolate, released); +} + +// A natively retained object this finalizer kept: its own data properties are +// the objects the same drain may release through it. One level deep, which is +// the delegate-holds-a-native-object shape. +void TrackKept(Isolate* isolate, Local kept) { + BaseDataWrapper* wrapper = tns::GetValue(isolate, kept); + if (wrapper == nullptr || wrapper->Type() != WrapperType::ObjCObject || + !wrapper->IsGcProtected()) { + return; + } + id target = static_cast(wrapper)->Data(); + Local context; + Local names; + if (target == nil || !kept->GetCreationContext(isolate).ToLocal(&context) || + !kept->GetOwnPropertyNames(context, PropertyFilter::ONLY_ENUMERABLE).ToLocal(&names)) { + return; + } + + std::string owner = object_getClassName(target); + std::shared_ptr cache = Caches::Get(isolate); + for (uint32_t i = 0; i < names->Length(); i++) { + Local name; + if (!names->Get(context, i).ToLocal(&name) || !name->IsString()) { + continue; + } + // Accessors would run JS from inside the drain. + if (kept->HasRealNamedCallbackProperty(context, name.As()).FromMaybe(true)) { + continue; + } + Local value; + if (!kept->GetRealNamedProperty(context, name.As()).ToLocal(&value) || + !value->IsObject()) { + continue; + } + Local child = value.As(); + std::string path = owner + "." + tns::ToString(isolate, name); + + bool releasedAlready = false; + for (auto& released : cache->ReleasedThisGC) { + if (released.Get(isolate) == child) { + releasedAlready = true; + break; + } + } + if (releasedAlready) { + MarkReleasedThrough(isolate, child, path); + } else if (!ReleasableClassName(isolate, child).empty()) { + cache->KeptObjectProperties.emplace_back(v8::Global(isolate, child), path); + } + } +} + +} // namespace + +namespace { + void LinkRegistered(v8::Isolate* isolate, ObjectWeakCallbackState* state) { std::shared_ptr cache = Caches::Get(isolate); if (cache == nullptr) { @@ -104,6 +221,11 @@ void DisposeHandle(v8::Isolate* isolate, Isolate::Scope isolateScope(isolate); HandleScope scope(isolate); + // Globals reset in their destructor, and the Caches holding them can outlive + // the isolate; let them go while it is still alive. + cache->ReleasedThisGC.clear(); + cache->KeptObjectProperties.clear(); + // Detach the whole list first so disposal can't walk into freed entries, and // claim every state up front: disposing one entry can re-enter JS from // -dealloc and reach __releaseNativeCounterpart for any other entry this @@ -155,9 +277,25 @@ void DisposeHandle(v8::Isolate* isolate, state->disposing_ = true; Local value = state->target_->Get(isolate); + std::string releasedClass; + if (RuntimeConfig.IsDebug) { + releasedClass = ReleasableClassName(isolate, value); + } bool disposed = ObjectManager::DisposeValue(isolate, value); state->disposing_ = false; + // Debug-only naming of the "Known hazard" in + // docs/knowledge/v8-resurrecting-finalizers.md: a kept, natively retained + // object can come back holding a property whose native object this drain + // released. The husk then explains itself (GetValueOrReport). + if (RuntimeConfig.IsDebug && !value.IsEmpty() && value->IsObject()) { + if (disposed && !releasedClass.empty()) { + TrackReleased(isolate, value.As(), releasedClass); + } else if (!disposed) { + TrackKept(isolate, value.As()); + } + } + // Disposal releases the native counterpart, and a -dealloc reached that way // can reset this very handle (the collection adapters reset the persistent // they were built from). An empty handle means the node is already freed, so diff --git a/TestRunner/app/tests/GCFinalizerTests.js b/TestRunner/app/tests/GCFinalizerTests.js index d51ea954..3b7fed46 100644 --- a/TestRunner/app/tests/GCFinalizerTests.js +++ b/TestRunner/app/tests/GCFinalizerTests.js @@ -142,6 +142,86 @@ describe("GC finalizer callbacks", function () { }, 0); }); + // The same hazard with an ObjC object in the property: the kept parent + // comes back holding a husk of a native object only JS owned. Calls on it + // follow releasedObjectPolicy like the struct touches above, and the + // debug-build drain names what was released and through which property. + it("names the property an ObjC object was released through when its natively retained owner was kept", function (done) { + var OwnerOfChild = (function (_super) { + __extends(OwnerOfChild, _super); + function OwnerOfChild() { + return (_super !== null && _super.apply(this, arguments)) || this; + } + return OwnerOfChild; + })(NSObject); + + var N = 16; + var holder = NSMutableArray.alloc().init(); + + (function create() { + for (var i = 0; i < N; i++) { + var owner = OwnerOfChild.alloc().init(); + owner.child = NSMutableArray.alloc().init(); // owned only by JS + owner.__mine = true; + holder.addObject(owner); // native-only retain → GcProtect + } + })(); + + (function scrub(n) { + return n > 0 ? scrub(n - 1) + n : 0; + })(300); + __collect(); + __collect(); + + var runtime = require("ns:runtime"); + runtime.setConfig("releasedObjectPolicy", "throw"); + var husks = []; + var thrown = []; + for (var i = 0; i < N; i++) { + var kept = holder.objectAtIndex(i); + if (kept.__mine !== true) { + continue; + } + try { + kept.child.count; + } catch (e) { + husks.push(kept.child); + thrown.push(e); + } + } + runtime.setConfig("releasedObjectPolicy", "report"); + + // Zero husks is a legal (if unlucky) outcome, as in the struct spec. + TNSLog("objc husk spec: " + husks.length + "/" + N + " husks formed"); + if (husks.length === 0) { + done(); + return; + } + + expect(thrown[0] instanceof ReferenceError).toBe(true); + expect(thrown[0].message).toMatch(/The native \S+ object has been released \(method 'count'/); + expect(thrown[0].message).toMatch(/while still referenced from '\S+\.child'/); + + var events = []; + var onReleased = function (e) { + events.push(e); + }; + global.addEventListener("releasednativeaccess", onReleased); + + // Report policy: the call is skipped. + expect(husks[0].count).toBeUndefined(); + expect(husks[0].addObject(NSObject.new())).toBeUndefined(); + + setTimeout(function () { + global.removeEventListener("releasednativeaccess", onReleased); + // Deduplicated per object: the second call does not report. + expect(events.length).toBe(1); + expect(events[0].operation).toMatch(/^method 'count'/); + expect(String(events[0].error)).toMatch(/\.child'/); + done(); + }, 0); + }); + // Overwrites the stack region that held the creation locals so // conservative stack scanning cannot keep dead wrappers alive. function scrubStack() { diff --git a/docs/knowledge/v8-resurrecting-finalizers.md b/docs/knowledge/v8-resurrecting-finalizers.md index c7326287..d03f13dc 100644 --- a/docs/knowledge/v8-resurrecting-finalizers.md +++ b/docs/knowledge/v8-resurrecting-finalizers.md @@ -192,8 +192,17 @@ neutered field goes through it — `Pointer.cpp`'s methods, `ExtVector.cpp` toSt `Interop::SetStructPropertyValue` (both struct cases), `MetadataBuilder::GetStructData` and both struct property interceptors, the two `InteropTypes.mm` argument readers, and the `Reference.cpp` accessors (`SetValueCallback`, `GetWrappedPointer`, `GetDataPair` → indexed -get/set, toString). Regression spec: *"throws instead of crashing when touching a released -object through a resurrected parent"* in `GCFinalizerTests.js`. +get/set, toString), and ObjC method and property calls on a husk receiver +(`ArgConverter::Invoke`). Regression specs: *"reports or throws (never crashes) when touching a +released object through a resurrected parent"* and *"names the property an ObjC object was +released through when its natively retained owner was kept"* in `GCFinalizerTests.js`. + +In debug builds the drain also names the hazard when it creates one: `FinalizerCallback` tags +each ObjC husk with its class, and when it keeps a protected object it records that object's +own enumerable data properties, one level deep. Whichever of the pair finalizes second matches +the other (`Caches::ReleasedThisGC` / `Caches::KeptObjectProperties`, cleared by a GC +prologue), tags the husk with `Owner.property` and logs a warning; `GetValueOrReport` puts both +tags in its message. Two facts worth knowing when reasoning about which objects can husk: diff --git a/docs/ns-builtin-modules.md b/docs/ns-builtin-modules.md index e3b3ff30..33b15a98 100644 --- a/docs/ns-builtin-modules.md +++ b/docs/ns-builtin-modules.md @@ -148,16 +148,22 @@ already been released (a state a resurrected object can expose — see the iOS runtime's `docs/knowledge/v8-resurrecting-finalizers.md`): - `"report"` (default): the operation no-ops — reads produce `undefined`, - writes are skipped, `toString` yields a ``-style - placeholder — and a cancelable `releasednativeaccess` event fires on - `globalThis` (`event.error` carries a `ReferenceError` whose stack names the - touch site; `event.operation` names the API surface, e.g. - `"struct field assignment"`). Reports are deduplicated per released object. + writes and ObjC method calls are skipped (a call returns `undefined`), + `toString` yields a ``-style placeholder — and a + cancelable `releasednativeaccess` event fires on `globalThis` (`event.error` + carries a `ReferenceError` whose stack names the touch site; + `event.operation` names the API surface, e.g. `"struct field assignment"` or + `"method 'count' (count)"`). Reports are deduplicated per released object. In debug builds the event's default action is a `console.warn`; a listener that handles the report suppresses it with `preventDefault()`. - `"throw"`: the touch throws a catchable `ReferenceError` synchronously, and no event fires. +In debug builds the error message also names the released object's native +class and, when the collector released it through a property of a natively +retained object it kept alive, that property (e.g. `'MyDelegate.provider'`), +and the release itself logs a warning when it happens. + ```js const { setConfig } = require("ns:runtime"); From e8ed8efd116ca8198c84ba6663f666bd79b57977 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Sat, 10 Oct 2026 16:24:02 -0300 Subject: [PATCH 2/2] fix(runtime): always throw for calls on released ObjC objects, keep the drain tracking cheap * A method or property call on a released ObjC receiver now always throws a ReferenceError, whatever releasedObjectPolicy says. Skipping it under "report" broke the caller's contract (a completion handler that never runs) and, with no listener and no release-build console, left no trace at all; the thrown error reaches the app's error handlers in every build. The message comes from the new ReleasedObjectMessage, shared with GetValueOrReport. * FinalizerCallback runs exactly the previous DisposeValue call unless RuntimeConfig.IsDebug, and the debug tracking no longer tags or records every released object. Only a kept, natively retained object walks its own data properties: a property that is already a husk is labelled on the spot (class from its constructor), and a releasable one is recorded in Caches::KeptObjectProperties for its own finalizer to label. A drain that keeps nothing records nothing, and a husk is reported once. --- NativeScript/runtime/ArgConverter.mm | 19 +++--- NativeScript/runtime/Caches.h | 10 ++-- NativeScript/runtime/Helpers.h | 10 +++- NativeScript/runtime/Helpers.mm | 21 ++++--- NativeScript/runtime/ObjectManager.mm | 63 +++++++++++--------- TestRunner/app/tests/GCFinalizerTests.js | 24 ++++---- docs/knowledge/v8-resurrecting-finalizers.md | 19 +++--- docs/ns-builtin-modules.md | 16 +++-- 8 files changed, 104 insertions(+), 78 deletions(-) diff --git a/NativeScript/runtime/ArgConverter.mm b/NativeScript/runtime/ArgConverter.mm index 991d6e71..d7095d10 100644 --- a/NativeScript/runtime/ArgConverter.mm +++ b/NativeScript/runtime/ArgConverter.mm @@ -39,13 +39,18 @@ if (wrapper == nullptr) { // A JS object can outlive its native half: a husk the finalizer drain // left behind (docs/knowledge/v8-resurrecting-finalizers.md, "Known - // hazard"), or one briefly orphaned by fast view churn like HMR. Like - // every other touch of a released object, the call follows - // releasedObjectPolicy: it is skipped and reported, or it throws. - std::string operation = std::string("method '") + (meta ? meta->jsName() : "") + - "' (" + (meta ? meta->selectorAsString() : "") + ")"; - tns::GetValueOrReport(isolate, receiver, operation.c_str()); - return Local(); + // hazard"), or one briefly orphaned by fast view churn like HMR. Unlike + // the other released-object touches this always throws, whatever + // releasedObjectPolicy says: a skipped call breaks its caller's + // contract (a completion that never fires), and an exception reaches + // the app's error handlers in every build. + std::string message = tns::ReleasedObjectMessage( + isolate, receiver, + (std::string("method '") + (meta ? meta->jsName() : "") + "' (" + + (meta ? meta->selectorAsString() : "") + ")") + .c_str()); + throw NativeScriptException( + isolate, v8::Exception::ReferenceError(tns::ToV8String(isolate, message)), message); } if (wrapper->Type() == WrapperType::ObjCAllocObject) { diff --git a/NativeScript/runtime/Caches.h b/NativeScript/runtime/Caches.h index a3800868..feb1376c 100644 --- a/NativeScript/runtime/Caches.h +++ b/NativeScript/runtime/Caches.h @@ -143,12 +143,10 @@ class Caches { // wrappers. ObjectWeakCallbackState* ObjectManagedValues = nullptr; - // Debug builds only, for naming what a finalizer drain released through a - // natively retained object it kept (see ObjectManager::FinalizerCallback). - // The drain can run the child's finalizer before or after the parent's, so - // each side records what the other needs. A GC prologue clears both, so they - // only ever describe the collection in progress. - std::vector> ReleasedThisGC; + // Debug builds only: the ObjC objects a finalizer drain may release + // through a natively retained object it kept, as "Owner.property", so their + // own finalizer can name the husk (ObjectManager::FinalizerCallback). A GC + // prologue clears it, so it only describes the collection in progress. std::vector, std::string>> KeptObjectProperties; diff --git a/NativeScript/runtime/Helpers.h b/NativeScript/runtime/Helpers.h index 60b2dcd4..cd7145fc 100644 --- a/NativeScript/runtime/Helpers.h +++ b/NativeScript/runtime/Helpers.h @@ -285,8 +285,14 @@ void SetReleasedObjectPolicy(ReleasedObjectPolicy policy); BaseDataWrapper* GetValueOrReport(v8::Isolate* isolate, const v8::Local& val, const char* operation); -// Private keys a debug-build finalizer drain sets on a husk so the -// GetValueOrReport message can say what was released and through what: +// The message for a touch of a released object: names the native class and, +// when a debug-build drain recorded it, the "Owner.property" it was released +// through (see the keys below). +std::string ReleasedObjectMessage(v8::Isolate* isolate, const v8::Local& val, + const char* operation); + +// Private keys a debug-build finalizer drain sets on a husk so +// ReleasedObjectMessage can say what was released and through what: // the native class name, and "Owner.property" when the drain released it // through a natively retained object it kept (ObjectManager::FinalizerCallback). constexpr const char* kReleasedClassKey = "ns:releasedClass"; diff --git a/NativeScript/runtime/Helpers.mm b/NativeScript/runtime/Helpers.mm index 84da3aef..75fe7b75 100644 --- a/NativeScript/runtime/Helpers.mm +++ b/NativeScript/runtime/Helpers.mm @@ -464,13 +464,8 @@ void WriteDebugLine(tns::LogCategory category, const char* message) { va_end(ap); } -tns::BaseDataWrapper* tns::GetValueOrReport(Isolate* isolate, const Local& val, - const char* operation) { - BaseDataWrapper* wrapper = tns::GetValue(isolate, val); - if (wrapper != nullptr) { - return wrapper; - } - +std::string tns::ReleasedObjectMessage(Isolate* isolate, const Local& val, + const char* operation) { std::string releasedClass; std::string releasedThrough; if (val->IsObject()) { @@ -505,6 +500,18 @@ void WriteDebugLine(tns::LogCategory category, const char* message) { "value reachable from a JS root"; } + return message; +} + +tns::BaseDataWrapper* tns::GetValueOrReport(Isolate* isolate, const Local& val, + const char* operation) { + BaseDataWrapper* wrapper = tns::GetValue(isolate, val); + if (wrapper != nullptr) { + return wrapper; + } + + std::string message = tns::ReleasedObjectMessage(isolate, val, operation); + if (GetReleasedObjectPolicy() == ReleasedObjectPolicy::kThrow) { isolate->ThrowException(v8::Exception::ReferenceError(tns::ToV8String(isolate, message))); return nullptr; diff --git a/NativeScript/runtime/ObjectManager.mm b/NativeScript/runtime/ObjectManager.mm index c9ecb872..d22a0bc7 100644 --- a/NativeScript/runtime/ObjectManager.mm +++ b/NativeScript/runtime/ObjectManager.mm @@ -23,7 +23,6 @@ void ResetReleaseTracking(Isolate* isolate, GCType type, GCCallbackFlags flags) if (cache == nullptr) { return; } - cache->ReleasedThisGC.clear(); cache->KeptObjectProperties.clear(); } @@ -72,16 +71,16 @@ void MarkReleasedThrough(Isolate* isolate, Local released, const std::st path.c_str(), path.substr(0, path.rfind('.')).c_str()); } +// Child finalized after its kept owner: the owner's walk recorded it. void TrackReleased(Isolate* isolate, Local released, const std::string& className) { - SetTag(isolate, released, tns::kReleasedClassKey, className); std::shared_ptr cache = Caches::Get(isolate); for (auto& entry : cache->KeptObjectProperties) { if (entry.first.Get(isolate) == released) { + SetTag(isolate, released, tns::kReleasedClassKey, className); MarkReleasedThrough(isolate, released, entry.second); return; } } - cache->ReleasedThisGC.emplace_back(isolate, released); } // A natively retained object this finalizer kept: its own data properties are @@ -120,21 +119,43 @@ void TrackKept(Isolate* isolate, Local kept) { Local child = value.As(); std::string path = owner + "." + tns::ToString(isolate, name); - bool releasedAlready = false; - for (auto& released : cache->ReleasedThisGC) { - if (released.Get(isolate) == child) { - releasedAlready = true; - break; + // Child finalized before its kept owner: it is already a husk, a wrapper + // object whose internal field disposal neutered. A husk labelled by an + // earlier collection is not reported again. + if (child->InternalFieldCount() > 0 && tns::GetValue(isolate, child) == nullptr) { + Local throughKey = + Private::ForApi(isolate, tns::ToV8String(isolate, tns::kReleasedThroughKey)); + if (!child->HasPrivate(context, throughKey).FromMaybe(true)) { + SetTag(isolate, child, tns::kReleasedClassKey, + tns::ToString(isolate, child->GetConstructorName())); + MarkReleasedThrough(isolate, child, path); } - } - if (releasedAlready) { - MarkReleasedThrough(isolate, child, path); } else if (!ReleasableClassName(isolate, child).empty()) { cache->KeptObjectProperties.emplace_back(v8::Global(isolate, child), path); } } } +// DisposeValue plus the debug-only naming of the "Known hazard" in +// docs/knowledge/v8-resurrecting-finalizers.md: a kept, natively retained +// object can come back holding a property whose native object this drain +// released, and the husk then explains itself (ReleasedObjectMessage). +bool DisposeAndTrackRelease(Isolate* isolate, Local value) { + std::string releasedClass; + if (!Caches::Get(isolate)->KeptObjectProperties.empty()) { + releasedClass = ReleasableClassName(isolate, value); + } + bool disposed = ObjectManager::DisposeValue(isolate, value); + if (!value.IsEmpty() && value->IsObject()) { + if (disposed && !releasedClass.empty()) { + TrackReleased(isolate, value.As(), releasedClass); + } else if (!disposed) { + TrackKept(isolate, value.As()); + } + } + return disposed; +} + } // namespace namespace { @@ -223,7 +244,6 @@ void DisposeHandle(v8::Isolate* isolate, // Globals reset in their destructor, and the Caches holding them can outlive // the isolate; let them go while it is still alive. - cache->ReleasedThisGC.clear(); cache->KeptObjectProperties.clear(); // Detach the whole list first so disposal can't walk into freed entries, and @@ -277,25 +297,10 @@ void DisposeHandle(v8::Isolate* isolate, state->disposing_ = true; Local value = state->target_->Get(isolate); - std::string releasedClass; - if (RuntimeConfig.IsDebug) { - releasedClass = ReleasableClassName(isolate, value); - } - bool disposed = ObjectManager::DisposeValue(isolate, value); + bool disposed = RuntimeConfig.IsDebug ? DisposeAndTrackRelease(isolate, value) + : ObjectManager::DisposeValue(isolate, value); state->disposing_ = false; - // Debug-only naming of the "Known hazard" in - // docs/knowledge/v8-resurrecting-finalizers.md: a kept, natively retained - // object can come back holding a property whose native object this drain - // released. The husk then explains itself (GetValueOrReport). - if (RuntimeConfig.IsDebug && !value.IsEmpty() && value->IsObject()) { - if (disposed && !releasedClass.empty()) { - TrackReleased(isolate, value.As(), releasedClass); - } else if (!disposed) { - TrackKept(isolate, value.As()); - } - } - // Disposal releases the native counterpart, and a -dealloc reached that way // can reset this very handle (the collection adapters reset the persistent // they were built from). An empty handle means the node is already freed, so diff --git a/TestRunner/app/tests/GCFinalizerTests.js b/TestRunner/app/tests/GCFinalizerTests.js index 3b7fed46..95f2d51e 100644 --- a/TestRunner/app/tests/GCFinalizerTests.js +++ b/TestRunner/app/tests/GCFinalizerTests.js @@ -143,9 +143,10 @@ describe("GC finalizer callbacks", function () { }); // The same hazard with an ObjC object in the property: the kept parent - // comes back holding a husk of a native object only JS owned. Calls on it - // follow releasedObjectPolicy like the struct touches above, and the - // debug-build drain names what was released and through which property. + // comes back holding a husk of a native object only JS owned. Unlike the + // struct touches above, calls on it throw under either releasedObjectPolicy + // (a skipped call would break its caller's contract), and the debug-build + // drain names what was released and through which property. it("names the property an ObjC object was released through when its natively retained owner was kept", function (done) { var OwnerOfChild = (function (_super) { __extends(OwnerOfChild, _super); @@ -173,8 +174,6 @@ describe("GC finalizer callbacks", function () { __collect(); __collect(); - var runtime = require("ns:runtime"); - runtime.setConfig("releasedObjectPolicy", "throw"); var husks = []; var thrown = []; for (var i = 0; i < N; i++) { @@ -189,7 +188,6 @@ describe("GC finalizer callbacks", function () { thrown.push(e); } } - runtime.setConfig("releasedObjectPolicy", "report"); // Zero husks is a legal (if unlucky) outcome, as in the struct spec. TNSLog("objc husk spec: " + husks.length + "/" + N + " husks formed"); @@ -198,6 +196,8 @@ describe("GC finalizer callbacks", function () { return; } + // Under the default "report" policy too. + expect(require("ns:runtime").getConfig("releasedObjectPolicy")).toBe("report"); expect(thrown[0] instanceof ReferenceError).toBe(true); expect(thrown[0].message).toMatch(/The native \S+ object has been released \(method 'count'/); expect(thrown[0].message).toMatch(/while still referenced from '\S+\.child'/); @@ -208,16 +208,14 @@ describe("GC finalizer callbacks", function () { }; global.addEventListener("releasednativeaccess", onReleased); - // Report policy: the call is skipped. - expect(husks[0].count).toBeUndefined(); - expect(husks[0].addObject(NSObject.new())).toBeUndefined(); + expect(function () { + husks[0].addObject(NSObject.new()); + }).toThrowError(ReferenceError, /method 'addObject'/); setTimeout(function () { global.removeEventListener("releasednativeaccess", onReleased); - // Deduplicated per object: the second call does not report. - expect(events.length).toBe(1); - expect(events[0].operation).toMatch(/^method 'count'/); - expect(String(events[0].error)).toMatch(/\.child'/); + // A call throws instead of reporting. + expect(events.length).toBe(0); done(); }, 0); }); diff --git a/docs/knowledge/v8-resurrecting-finalizers.md b/docs/knowledge/v8-resurrecting-finalizers.md index d03f13dc..78a35dd0 100644 --- a/docs/knowledge/v8-resurrecting-finalizers.md +++ b/docs/knowledge/v8-resurrecting-finalizers.md @@ -192,17 +192,20 @@ neutered field goes through it — `Pointer.cpp`'s methods, `ExtVector.cpp` toSt `Interop::SetStructPropertyValue` (both struct cases), `MetadataBuilder::GetStructData` and both struct property interceptors, the two `InteropTypes.mm` argument readers, and the `Reference.cpp` accessors (`SetValueCallback`, `GetWrappedPointer`, `GetDataPair` → indexed -get/set, toString), and ObjC method and property calls on a husk receiver -(`ArgConverter::Invoke`). Regression specs: *"reports or throws (never crashes) when touching a +get/set, toString). ObjC method and property calls on a husk receiver (`ArgConverter::Invoke`) +always throw a `ReferenceError` instead, whatever the policy: a skipped call breaks its +caller's contract. Regression specs: *"reports or throws (never crashes) when touching a released object through a resurrected parent"* and *"names the property an ObjC object was released through when its natively retained owner was kept"* in `GCFinalizerTests.js`. -In debug builds the drain also names the hazard when it creates one: `FinalizerCallback` tags -each ObjC husk with its class, and when it keeps a protected object it records that object's -own enumerable data properties, one level deep. Whichever of the pair finalizes second matches -the other (`Caches::ReleasedThisGC` / `Caches::KeptObjectProperties`, cleared by a GC -prologue), tags the husk with `Owner.property` and logs a warning; `GetValueOrReport` puts both -tags in its message. +In debug builds the drain also names the hazard when it creates one. When `FinalizerCallback` +keeps a protected ObjC object it walks that object's own enumerable data properties, one level +deep. A property that is already a husk (the child finalized first) is tagged with its class and +`Owner.property` on the spot; a releasable ObjC object is recorded in +`Caches::KeptObjectProperties` (cleared by a GC prologue) so that its own finalizer, if it runs +later in the drain, tags it. Either way a warning is logged once per husk, and +`ReleasedObjectMessage` puts both tags in the message. A drain that keeps nothing records +nothing, so the per-release cost is one empty-vector check. Two facts worth knowing when reasoning about which objects can husk: diff --git a/docs/ns-builtin-modules.md b/docs/ns-builtin-modules.md index 33b15a98..fc67539e 100644 --- a/docs/ns-builtin-modules.md +++ b/docs/ns-builtin-modules.md @@ -148,17 +148,21 @@ already been released (a state a resurrected object can expose — see the iOS runtime's `docs/knowledge/v8-resurrecting-finalizers.md`): - `"report"` (default): the operation no-ops — reads produce `undefined`, - writes and ObjC method calls are skipped (a call returns `undefined`), - `toString` yields a ``-style placeholder — and a - cancelable `releasednativeaccess` event fires on `globalThis` (`event.error` - carries a `ReferenceError` whose stack names the touch site; - `event.operation` names the API surface, e.g. `"struct field assignment"` or - `"method 'count' (count)"`). Reports are deduplicated per released object. + writes are skipped, `toString` yields a ``-style + placeholder — and a cancelable `releasednativeaccess` event fires on + `globalThis` (`event.error` carries a `ReferenceError` whose stack names the + touch site; `event.operation` names the API surface, e.g. + `"struct field assignment"`). Reports are deduplicated per released object. In debug builds the event's default action is a `console.warn`; a listener that handles the report suppresses it with `preventDefault()`. - `"throw"`: the touch throws a catchable `ReferenceError` synchronously, and no event fires. +ObjC method and property calls on a released object are the exception: they +always throw a `ReferenceError`, whatever the policy says. Skipping a call would +break its caller's contract (a completion handler that never runs), and an +exception reaches the app's error handlers in every build. + In debug builds the error message also names the released object's native class and, when the collector released it through a property of a natively retained object it kept alive, that property (e.g. `'MyDelegate.provider'`),