diff --git a/NativeScript/runtime/ArgConverter.mm b/NativeScript/runtime/ArgConverter.mm index 80263ab9..d7095d10 100644 --- a/NativeScript/runtime/ArgConverter.mm +++ b/NativeScript/runtime/ArgConverter.mm @@ -37,21 +37,20 @@ 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. 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 01d0b642..feb1376c 100644 --- a/NativeScript/runtime/Caches.h +++ b/NativeScript/runtime/Caches.h @@ -143,6 +143,13 @@ class Caches { // wrappers. ObjectWeakCallbackState* ObjectManagedValues = nullptr; + // 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; + robin_hood::unordered_map>> Prototypes; diff --git a/NativeScript/runtime/Helpers.h b/NativeScript/runtime/Helpers.h index 5b48ade5..cd7145fc 100644 --- a/NativeScript/runtime/Helpers.h +++ b/NativeScript/runtime/Helpers.h @@ -284,6 +284,19 @@ void SetReleasedObjectPolicy(ReleasedObjectPolicy policy); // field unchecked must go through this. BaseDataWrapper* GetValueOrReport(v8::Isolate* isolate, const v8::Local& val, const char* operation); + +// 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"; +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..75fe7b75 100644 --- a/NativeScript/runtime/Helpers.mm +++ b/NativeScript/runtime/Helpers.mm @@ -464,6 +464,45 @@ void WriteDebugLine(tns::LogCategory category, const char* message) { va_end(ap); } +std::string tns::ReleasedObjectMessage(Isolate* isolate, const Local& val, + const char* operation) { + 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 = + 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"; + } + + return message; +} + tns::BaseDataWrapper* tns::GetValueOrReport(Isolate* isolate, const Local& val, const char* operation) { BaseDataWrapper* wrapper = tns::GetValue(isolate, val); @@ -471,9 +510,7 @@ void WriteDebugLine(tns::LogCategory category, const char* message) { return wrapper; } - std::string message = - std::string("The native object has been released or is not a valid native object (") + - operation + ")"; + std::string message = tns::ReleasedObjectMessage(isolate, val, operation); 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..d22a0bc7 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,150 @@ 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->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()); +} + +// Child finalized after its kept owner: the owner's walk recorded it. +void TrackReleased(Isolate* isolate, Local released, const std::string& 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; + } + } +} + +// 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); + + // 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); + } + } 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 { + void LinkRegistered(v8::Isolate* isolate, ObjectWeakCallbackState* state) { std::shared_ptr cache = Caches::Get(isolate); if (cache == nullptr) { @@ -104,6 +242,10 @@ 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->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,7 +297,8 @@ void DisposeHandle(v8::Isolate* isolate, state->disposing_ = true; Local value = state->target_->Get(isolate); - bool disposed = ObjectManager::DisposeValue(isolate, value); + bool disposed = RuntimeConfig.IsDebug ? DisposeAndTrackRelease(isolate, value) + : ObjectManager::DisposeValue(isolate, value); state->disposing_ = false; // Disposal releases the native counterpart, and a -dealloc reached that way diff --git a/TestRunner/app/tests/GCFinalizerTests.js b/TestRunner/app/tests/GCFinalizerTests.js index d51ea954..95f2d51e 100644 --- a/TestRunner/app/tests/GCFinalizerTests.js +++ b/TestRunner/app/tests/GCFinalizerTests.js @@ -142,6 +142,84 @@ 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. 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); + 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 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); + } + } + + // 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; + } + + // 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'/); + + var events = []; + var onReleased = function (e) { + events.push(e); + }; + global.addEventListener("releasednativeaccess", onReleased); + + expect(function () { + husks[0].addObject(NSObject.new()); + }).toThrowError(ReferenceError, /method 'addObject'/); + + setTimeout(function () { + global.removeEventListener("releasednativeaccess", onReleased); + // A call throws instead of reporting. + expect(events.length).toBe(0); + 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..78a35dd0 100644 --- a/docs/knowledge/v8-resurrecting-finalizers.md +++ b/docs/knowledge/v8-resurrecting-finalizers.md @@ -192,8 +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). Regression spec: *"throws instead of crashing when touching a released -object through a resurrected parent"* in `GCFinalizerTests.js`. +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. 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 e3b3ff30..fc67539e 100644 --- a/docs/ns-builtin-modules.md +++ b/docs/ns-builtin-modules.md @@ -158,6 +158,16 @@ runtime's `docs/knowledge/v8-resurrecting-finalizers.md`): - `"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'`), +and the release itself logs a warning when it happens. + ```js const { setConfig } = require("ns:runtime");