diff --git a/NativeScript/runtime/DisposerPHV.h b/NativeScript/runtime/DisposerPHV.h deleted file mode 100644 index fd05f0c7..00000000 --- a/NativeScript/runtime/DisposerPHV.h +++ /dev/null @@ -1,30 +0,0 @@ -// -// DisposerPHV.hpp -// NativeScript -// -// Created by Eduardo Speroni on 2/25/23. -// Copyright © 2023 Progress. All rights reserved. -// - -#ifndef DisposerPHV_h -#define DisposerPHV_h -#include "v8.h" - -namespace tns { - -class DisposerPHV : public v8::PersistentHandleVisitor { -public: - - v8::Isolate* isolate_; - - DisposerPHV(v8::Isolate* isolate) : isolate_(isolate) {} - virtual ~DisposerPHV() {} - - virtual void VisitPersistentHandle(v8::Persistent* value, uint16_t class_id); -}; - - -} - - -#endif /* DisposerPHV_h */ diff --git a/NativeScript/runtime/DisposerPHV.mm b/NativeScript/runtime/DisposerPHV.mm deleted file mode 100644 index b9b998fe..00000000 --- a/NativeScript/runtime/DisposerPHV.mm +++ /dev/null @@ -1,44 +0,0 @@ -// -// DisposerPHV.cpp -// NativeScript -// -// Created by Eduardo Speroni on 2/25/23. -// Copyright © 2023 Progress. All rights reserved. -// - -#include "DisposerPHV.h" -#include "Constants.h" -#include "Helpers.h" -#include "ObjectManager.h" - -using namespace tns; - -void DisposerPHV::VisitPersistentHandle( - v8::Persistent* value, - uint16_t class_id) { - - // delete persistent handles on isolate disposal. - switch (class_id) { - case Constants::ClassTypes::DataWrapper: { - v8::HandleScope scope(isolate_); - // use ObjectManager anyway, as it handles a bigger variety of wrappers - ObjectManager::DisposeValue(isolate_, value->Get(isolate_), true); - break; - } - case Constants::ClassTypes::ObjectManagedValue: { - v8::HandleScope scope(isolate_); - ObjectManager::DisposeValue(isolate_, value->Get(isolate_), true); - if (value->IsWeak()) { - ObjectWeakCallbackState* state = value->ClearWeak(); - state->target_->Reset(); - delete state; - }; - break; - } - default: - break; - } - if ( class_id== Constants::ClassTypes::DataWrapper ) { - - } -} diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index 8a6544b2..d56a974d 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -38,6 +38,11 @@ [](JSBlock* block) { if (block->descriptor == &JSBlock::kJSBlockDescriptor) { MethodCallbackWrapper* wrapper = static_cast(block->userData); + // Runs on whatever thread drops the last native reference. That is + // safe inline: callback_ is a strong, unregistered persistent, so + // resetting it never touches the finalizer drain's bookkeeping, + // and a foreign-thread Locker into the block's own isolate is + // legitimate now that extended class names are worker-scoped. if (wrapper->isolateWrapper_.IsValid()) { Isolate* isolate = wrapper->isolateWrapper_.Isolate(); v8::Locker locker(isolate); @@ -48,9 +53,12 @@ BlockWrapper* blockWrapper = static_cast(tns::GetValue(isolate, callback)); tns::DeleteValue(isolate, callback); - wrapper->callback_->Reset(); delete blockWrapper; } + // Unconditional: an already-detached callback still owns its + // node, and dropping the persistent without a reset would leave + // that node rooted forever. + wrapper->callback_->Reset(); } delete wrapper; ffi_closure_free(block->ffiClosure); diff --git a/NativeScript/runtime/NSDataAdapter.mm b/NativeScript/runtime/NSDataAdapter.mm index a0a49af8..5176fe60 100644 --- a/NativeScript/runtime/NSDataAdapter.mm +++ b/NativeScript/runtime/NSDataAdapter.mm @@ -104,7 +104,6 @@ - (void)dealloc { delete dataWrapper_; } - self->object_->Reset(); delete self->wrapper_; self->object_ = nullptr; [super dealloc]; diff --git a/NativeScript/runtime/ObjectManager.h b/NativeScript/runtime/ObjectManager.h index 46420adc..67691ff3 100644 --- a/NativeScript/runtime/ObjectManager.h +++ b/NativeScript/runtime/ObjectManager.h @@ -7,6 +7,16 @@ namespace tns { class ObjectManager; +// Parameter of the kFinalizer weak callback armed on target_. +// +// Ownership: created by ObjectManager::Register and deleted by exactly two +// sites -- ObjectManager::FinalizerCallback's disposed branch and +// DisposeAllRegistered. Retiring a registration from anywhere else (the +// __releaseNativeCounterpart builtin is the only one) must Reset target_ +// first: resetting frees the V8 node, which clears its pending-finalizer bit +// and guarantees no further callback, so the state is unreachable afterwards +// and safe to unlink and delete. Dropping the weakness without resetting +// leaves the node rooted forever with parameter() pointing at the freed state. struct ObjectWeakCallbackState { ObjectWeakCallbackState(std::shared_ptr> target) : target_(target) {} @@ -21,6 +31,13 @@ struct ObjectWeakCallbackState { ObjectWeakCallbackState** head_ = nullptr; ObjectWeakCallbackState* prev_ = nullptr; ObjectWeakCallbackState* next_ = nullptr; + + // Set while one of the two owning sites is disposing this handle's value. + // Disposal releases the native counterpart, whose -dealloc can re-enter JS + // and reach __releaseNativeCounterpart for this very handle; retiring it + // there would free the state under the frame that owns it. Retirement + // observes the flag and leaves the handle to that frame. + bool disposing_ = false; }; class ObjectManager { diff --git a/NativeScript/runtime/ObjectManager.mm b/NativeScript/runtime/ObjectManager.mm index 5a0cb2f4..c683ed9a 100644 --- a/NativeScript/runtime/ObjectManager.mm +++ b/NativeScript/runtime/ObjectManager.mm @@ -104,9 +104,15 @@ void DisposeHandle(v8::Isolate* isolate, Isolate::Scope isolateScope(isolate); HandleScope scope(isolate); - // Detach the whole list first so disposal can't walk into freed entries. + // 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 + // walk still owns. ObjectWeakCallbackState* state = cache->ObjectManagedValues; cache->ObjectManagedValues = nullptr; + for (ObjectWeakCallbackState* claimed = state; claimed != nullptr; claimed = claimed->next_) { + claimed->disposing_ = true; + } while (state != nullptr) { ObjectWeakCallbackState* next = state->next_; @@ -136,17 +142,37 @@ void DisposeHandle(v8::Isolate* isolate, void ObjectManager::FinalizerCallback(const WeakCallbackInfo& data) { ObjectWeakCallbackState* state = data.GetParameter(); Isolate* isolate = data.GetIsolate(); + + if (state->disposing_) { + // Another frame owns this state's teardown — the DisposeAllRegistered + // walk, whose pre-claimed entries a nested collection can still condemn. + // Re-arm to satisfy the finalizer contract on this node and leave the + // clear/reset/delete to the owner. + state->target_->ClearWeak(); + state->target_->SetWeak(state, FinalizerCallback, WeakCallbackType::kFinalizer); + return; + } + + state->disposing_ = true; Local value = state->target_->Get(isolate); bool disposed = ObjectManager::DisposeValue(isolate, value); - - if (disposed) { - UnlinkRegistered(state); + state->disposing_ = false; + + // 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 + // there is nothing left to reset or re-arm -- ClearWeak/SetWeak would write + // through a dead slot -- and the registration must be retired even when + // disposal was refused. + if (disposed || state->target_->IsEmpty()) { state->target_->Reset(); + UnlinkRegistered(state); delete state; - } else { - state->target_->ClearWeak(); - state->target_->SetWeak(state, FinalizerCallback, WeakCallbackType::kFinalizer); + return; } + + state->target_->ClearWeak(); + state->target_->SetWeak(state, FinalizerCallback, WeakCallbackType::kFinalizer); } bool ObjectManager::DisposeValue(Isolate* isolate, Local value, bool isFinalDisposal) { @@ -332,12 +358,25 @@ void DisposeHandle(v8::Isolate* isolate, std::shared_ptr cache = Caches::Get(isolate); auto it = cache->Instances.find(data); if (it != cache->Instances.end()) { - ObjectWeakCallbackState* state = it->second->ClearWeak(); + std::shared_ptr> handle = it->second; + ObjectWeakCallbackState* state = + handle->IsWeak() ? handle->ClearWeak() : nullptr; + 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 + // state, so restore the weakness and let it finish. + handle->SetWeak(state, FinalizerCallback, WeakCallbackType::kFinalizer); + return; + } + cache->Instances.erase(it); if (state != nullptr) { + // Reset before deleting the state: it frees the node, which clears the + // pending-finalizer bit and guarantees no callback can reach the freed + // parameter. + handle->Reset(); UnlinkRegistered(state); delete state; } - cache->Instances.erase(it); } // Release the runtime's strong reference (taken when the object was first diff --git a/TestRunner/app/tests/GCFinalizerTests.js b/TestRunner/app/tests/GCFinalizerTests.js index 80e58616..0482a9c0 100644 --- a/TestRunner/app/tests/GCFinalizerTests.js +++ b/TestRunner/app/tests/GCFinalizerTests.js @@ -141,4 +141,164 @@ describe("GC finalizer callbacks", function () { done(); }, 0); }); + + // 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); + } + + // Retiring a registration has to reset the persistent, not just drop its + // weakness: a handle left non-weak is a strong root, so the object it + // names can never be collected again. + it("frees the handle of an object retired by __releaseNativeCounterpart", function (done) { + var ref; + (function () { + var obj = TNSObjCTypes.alloc().init(); + ref = new WeakRef(obj); + __releaseNativeCounterpart(obj); + })(); + + scrubStack(); + __collect(); + setTimeout(function () { + __collect(); + expect(ref.deref()).toBeUndefined(); + done(); + }, 0); + }); + + // Retirement reached from a -dealloc that a finalizer drove: it frees the + // victim's state while the drain is mid-iteration over the handle table. + it("retires another registration from a -dealloc reached by the drain", function () { + var victims = []; + var retired = 0; + + var DeallocRetire = TNSApi.extend({ + methodCalledInDealloc: function () { + var victim = victims.pop(); + if (victim !== undefined) { + __releaseNativeCounterpart(victim); + retired++; + } + } + }, { name: "TNSApiDeallocRetire" }); + + // Kept alive by JS for the whole spec, so each retirement hits a live + // registration rather than one the same GC already queued. + var keepAlive = NSMutableArray.alloc().init(); + for (var i = 0; i < 8; i++) { + var victim = TNSObjCTypes.alloc().init(); + keepAlive.addObject(victim); + victims.push(victim); + } + + (function () { + var holder = NSMutableArray.alloc().init(); + for (var j = 0; j < 8; j++) { + holder.addObject(DeallocRetire.alloc().init()); + } + })(); + + scrubStack(); + __collect(); + __collect(); + + expect(retired).toBeGreaterThan(0); + expect(keepAlive.count).toBe(8); + }); + + // Collection adapters reset their own persistent and delete wrappers from + // -dealloc; a graph of them dies in one drain, so those resets land while + // the finalizer that released the graph is still on the stack. + it("survives adapter deallocs cascading out of a finalizer", function () { + var rounds = 24; + + (function () { + for (var i = 0; i < rounds; i++) { + var holder = NSMutableArray.alloc().init(); + holder.addObject([1, 2, 3]); + holder.addObject({ a: 1, b: 2 }); + holder.addObject(new Uint8Array(8)); + holder.addObject(NSMutableArray.arrayWithArray([[i], { i: i }])); + } + })(); + + scrubStack(); + __collect(); + __collect(); + + // A live adapter still answers after the sweep. + var survivor = NSMutableArray.arrayWithArray([1, 2, 3]); + expect(survivor.count).toBe(3); + expect(survivor.objectAtIndex(1)).toBe(2); + }); + + // A natively held block's last release can land inside the finalizer + // drain, where the JSBlock dispose helper must not touch handles itself. + it("tears down a natively held block released by a finalizer", function (done) { + var ref; + (function () { + var callback = function () { + TNSLog("retained block called"); + }; + ref = new WeakRef(callback); + + var owner = TNSObjCTypes.alloc().init(); + owner.methodRetainingBlock(callback); + owner.methodCallRetainingBlock(); + + var holder = NSMutableArray.alloc().init(); + holder.addObject(owner); + })(); + TNSClearOutput(); + + scrubStack(); + // The deferred teardown runs on a later event-loop pass, so the + // collectability check polls rather than assuming one tick suffices. + var attempts = 20; + (function pollCollected() { + __collect(); + if (ref.deref() === undefined) { + done(); + return; + } + if (--attempts === 0) { + expect(ref.deref()).toBeUndefined(); + done(); + return; + } + setTimeout(pollCollected); + })(); + }); + + // Releasing the block detaches the function's wrapper, so a later + // marshal of the same function must build a fresh block rather than + // reaching for the dead one. + it("re-marshals a function whose block was already released", function (done) { + var callback = function () { + TNSLog("re-marshalled block called"); + }; + + var first = TNSObjCTypes.alloc().init(); + first.methodRetainingBlock(callback); + first.methodReleaseRetainingBlock(); + first = null; + + // The block's remaining reference is the autoreleased one taken when + // it was marshalled; it goes away with the pool at the end of the turn. + setTimeout(function () { + __collect(); + var second = TNSObjCTypes.alloc().init(); + second.methodRetainingBlock(callback); + TNSClearOutput(); + second.methodCallRetainingBlock(); + + expect(TNSGetOutput()).toBe("re-marshalled block called"); + TNSClearOutput(); + done(); + }, 0); + }); }); diff --git a/TestRunner/app/tests/Marshalling/ObjCTypesTests.js b/TestRunner/app/tests/Marshalling/ObjCTypesTests.js index d1b4ca83..8f4fa3a4 100644 --- a/TestRunner/app/tests/Marshalling/ObjCTypesTests.js +++ b/TestRunner/app/tests/Marshalling/ObjCTypesTests.js @@ -101,12 +101,22 @@ describe(module.id, function () { expect(!!functionRef.deref()).toBe(true); verifyBlockCall(); instance.methodReleaseRetainingBlock(); - gc(); - setTimeout(() => { + // The JS side of the block is torn down on the event loop after the + // native release, so collectability lands a few ticks later. + var attempts = 20; + (function pollCollected() { gc(); - expect(!!functionRef.deref()).toBe(false); - done(); - }) + if (functionRef.deref() === undefined) { + done(); + return; + } + if (--attempts === 0) { + expect(!!functionRef.deref()).toBe(false); + done(); + return; + } + setTimeout(pollCollected); + })(); }); }); diff --git a/docs/knowledge/v8-14-migration.md b/docs/knowledge/v8-14-migration.md index 65519837..64b87ace 100644 --- a/docs/knowledge/v8-14-migration.md +++ b/docs/knowledge/v8-14-migration.md @@ -246,10 +246,10 @@ where `Holder() == This()` and nothing inherits it. ## Outstanding -Nothing blocking. One follow-up: +Nothing blocking. -- `DisposerPHV.{h,mm}` is now dead code -- `Isolate::VisitHandlesWithClassIds` no longer exists, - so the visitor can never be driven. Its logic moved to `ObjectManager::DisposeAllRegistered()`. +- `DisposerPHV.{h,mm}` were deleted: `Isolate::VisitHandlesWithClassIds` no longer exists, so the + visitor could never be driven. Its logic lives in `ObjectManager::DisposeAllRegistered()`. ### Behavioural parity traps in the accessor rewrite diff --git a/v8ios.xcodeproj/project.pbxproj b/v8ios.xcodeproj/project.pbxproj index 1afa9b63..f4674418 100644 --- a/v8ios.xcodeproj/project.pbxproj +++ b/v8ios.xcodeproj/project.pbxproj @@ -36,8 +36,6 @@ 3C78BA5D2A0D600100C20A88 /* ModuleBinding.hpp in Headers */ = {isa = PBXBuildFile; fileRef = 3C78BA5B2A0D600100C20A88 /* ModuleBinding.hpp */; }; 3CA6E53529A78C6000D30F8B /* IsolateWrapper.h in Headers */ = {isa = PBXBuildFile; fileRef = 3CA6E53429A78C6000D30F8B /* IsolateWrapper.h */; }; 3CBFF7442971C1C200C5DE36 /* ArcMacro.h in Headers */ = {isa = PBXBuildFile; fileRef = 3CBFF7432971C1C200C5DE36 /* ArcMacro.h */; }; - 3CD1D9C129AA2C14004C1C21 /* DisposerPHV.mm in Sources */ = {isa = PBXBuildFile; fileRef = 3CD1D9BF29AA2C14004C1C21 /* DisposerPHV.mm */; }; - 3CD1D9C229AA2C14004C1C21 /* DisposerPHV.h in Headers */ = {isa = PBXBuildFile; fileRef = 3CD1D9C029AA2C14004C1C21 /* DisposerPHV.h */; }; 3CEA20DC2A7DA8320009BE8F /* IsolateWrapper.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 3CEA20DB2A7DA8320009BE8F /* IsolateWrapper.cpp */; }; 6573B9CD291FE29F00B0ED7C /* V8Runtime.h in Headers */ = {isa = PBXBuildFile; fileRef = 6573B9C2291FE29F00B0ED7C /* V8Runtime.h */; }; 6573B9CE291FE29F00B0ED7C /* JSIV8ValueConverter.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6573B9C3291FE29F00B0ED7C /* JSIV8ValueConverter.cpp */; }; @@ -489,8 +487,6 @@ 3C78BA5B2A0D600100C20A88 /* ModuleBinding.hpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.h; path = ModuleBinding.hpp; sourceTree = ""; }; 3CA6E53429A78C6000D30F8B /* IsolateWrapper.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = IsolateWrapper.h; sourceTree = ""; }; 3CBFF7432971C1C200C5DE36 /* ArcMacro.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = ArcMacro.h; sourceTree = ""; }; - 3CD1D9BF29AA2C14004C1C21 /* DisposerPHV.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = DisposerPHV.mm; sourceTree = ""; }; - 3CD1D9C029AA2C14004C1C21 /* DisposerPHV.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = DisposerPHV.h; sourceTree = ""; }; 3CEA20DB2A7DA8320009BE8F /* IsolateWrapper.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = IsolateWrapper.cpp; sourceTree = ""; }; 3CEF9CCC28F896B70056BA45 /* SpinLock.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = SpinLock.h; sourceTree = ""; }; 6573B9C2291FE29F00B0ED7C /* V8Runtime.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = V8Runtime.h; sourceTree = ""; }; @@ -1543,8 +1539,6 @@ 4A5C201A2E2B000100000004 /* RuntimeBuiltins.h */, 4A5C201A2E2B000100000003 /* RuntimeBuiltins.cpp */, 4A5C201A2E2B000100000005 /* js */, - 3CD1D9BF29AA2C14004C1C21 /* DisposerPHV.mm */, - 3CD1D9C029AA2C14004C1C21 /* DisposerPHV.h */, C22C092122CA3F370080D176 /* Worker.h */, C22C092022CA3F370080D176 /* Worker.mm */, C23E8F7422CDE88D0078FD4C /* WorkerWrapper.mm */, @@ -1701,7 +1695,6 @@ C247C16922F82842001D2CA2 /* v8-tracing.h in Headers */, 91B25A0B29DAC83D00E3CE04 /* ns-v8-tracing-agent-impl.h in Headers */, 6573B9E9291FE2A700B0ED7C /* threadsafe.h in Headers */, - 3CD1D9C229AA2C14004C1C21 /* DisposerPHV.h in Headers */, C22536B7241A318900192740 /* ffitarget.h in Headers */, C2DDEBAB229EAC8300345BFE /* WeakRef.h in Headers */, 3C78BA5D2A0D600100C20A88 /* ModuleBinding.hpp in Headers */, @@ -2326,7 +2319,6 @@ C2F4D0CD2334B1BC0008A2EB /* RuntimeConfig.cpp in Sources */, C23E8F7622CDE88D0078FD4C /* WorkerWrapper.mm in Sources */, C266567B22AA630F00EE15CC /* NSDataAdapter.mm in Sources */, - 3CD1D9C129AA2C14004C1C21 /* DisposerPHV.mm in Sources */, 91B25A0A29DAC83D00E3CE04 /* ns-v8-tracing-agent-impl.mm in Sources */, C2DDEBA6229EAC8300345BFE /* Helpers.mm in Sources */, C26656B322B3768C00EE15CC /* InteropTypes.mm in Sources */,