From 0057fd8b15e6cfe0ca3c2c5307683bce2cdaf739 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 26 Aug 2026 23:00:59 -0300 Subject: [PATCH 1/2] fix(runtime): finalizer-safe ownership for registered handles and deferred JSBlock teardown An in-flight kFinalizer callback could have its state freed or its node reset underneath it, breaking the contract that a finalizer either resets its handle or re-arms it and corrupting the drain's bookkeeping (the production V8_Fatal CHECKs on worker isolates). ObjectWeakCallbackState now has exactly two deleting sites -- FinalizerCallback's disposed branch and DisposeAllRegistered -- and every other retirement resets the persistent first, which frees the node, clears the pending bit and guarantees no further callback. A disposing flag makes a reentrant retirement (a -dealloc reached from DisposeValue calling __releaseNativeCounterpart) defer to the frame that owns the state. FinalizerCallback re-checks handle emptiness after DisposeValue: an adapter dealloc can reset the very persistent being finalized, and ClearWeak on an empty handle writes through a dead slot, so a handle emptied underneath its callback is retired, never re-armed. __releaseNativeCounterpart gains its missing Reset -- it retired registrations by deleting the state while leaving the node rooted forever with parameter() dangling at freed memory. The JSBlock dispose helper no longer does V8 work inline: the last native release can land on any thread, including inside the finalizer drain via dealloc cascades, so handle teardown posts to the owning isolate's event loop (a refused post means the isolate is gone and only native memory remains). The block pointer is cleared synchronously, and marshalling builds a fresh block for a wrapper whose JSBlock already died. Removes DisposerPHV (dead since VisitHandlesWithClassIds went away) and an unlocked, guardless Reset in NSDataAdapter's dealloc. Suite green; new GCFinalizerTests specs cover the retired-handle collectability contract, dealloc-cascade reentrancy, and the natively-held-block production shape. Reverting only the added Reset crashes the runtime outright. --- NativeScript/runtime/ArgConverter.h | 4 + NativeScript/runtime/DataWrapper.h | 5 + NativeScript/runtime/DisposerPHV.h | 30 ----- NativeScript/runtime/DisposerPHV.mm | 44 ------- NativeScript/runtime/Interop.mm | 69 ++++++++--- NativeScript/runtime/NSDataAdapter.mm | 1 - NativeScript/runtime/ObjectManager.h | 17 +++ NativeScript/runtime/ObjectManager.mm | 47 +++++-- TestRunner/app/tests/GCFinalizerTests.js | 150 +++++++++++++++++++++++ docs/knowledge/v8-14-migration.md | 6 +- v8ios.xcodeproj/project.pbxproj | 8 -- 11 files changed, 269 insertions(+), 112 deletions(-) delete mode 100644 NativeScript/runtime/DisposerPHV.h delete mode 100644 NativeScript/runtime/DisposerPHV.mm diff --git a/NativeScript/runtime/ArgConverter.h b/NativeScript/runtime/ArgConverter.h index 5e918109..c3730116 100644 --- a/NativeScript/runtime/ArgConverter.h +++ b/NativeScript/runtime/ArgConverter.h @@ -30,6 +30,10 @@ struct MethodCallbackWrapper { const uint8_t initialParamIndex_; const uint8_t paramsCount_; const TypeEncoding* typeEncoding_; + // Set only for JSBlocks: the wrapper attached to callback_, owned by the + // block's dispose helper. Reaching it without V8 lets the helper neuter the + // block pointer on whatever thread the last release lands on. + BlockWrapper* blockWrapper_ = nullptr; }; class ArgConverter { diff --git a/NativeScript/runtime/DataWrapper.h b/NativeScript/runtime/DataWrapper.h index a72d37db..c29df8ab 100644 --- a/NativeScript/runtime/DataWrapper.h +++ b/NativeScript/runtime/DataWrapper.h @@ -459,6 +459,11 @@ class BlockWrapper : public BaseDataWrapper { void* Block() { return this->block_; } + // A JSBlock's dispose helper frees the block while this wrapper is still + // attached to its JS function; clearing the pointer there keeps a later + // marshalling of that function from copying dead block memory. + void ClearBlock() { this->block_ = nullptr; } + const TypeEncoding* Encodings() { return this->typeEncoding_; } bool OwnsBlock() { return this->ownsBlock_; } 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..7fa50ed9 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -30,6 +30,45 @@ static_cast(kUint64AllBitsSet << 53) + 1; // -9007199254740991 (-(2^53-1)) static constexpr int64_t kMaxSafeInteger = -kMinSafeInteger; // 9007199254740991 (2^53-1) +namespace { + +// Tears down the JS side of a JSBlock whose last native reference just went +// away. The release lands on whatever thread the owning native code runs on, +// and can land inside ObjectManager's finalizer drain (a -dealloc cascade +// started by DisposeValue), where mutating global handles corrupts the drain's +// bookkeeping. So the handles are only ever touched from the owning runtime's +// home thread, through its event loop. A refused post means the loop is gone +// with its isolate, which took the handles with it -- only native memory is +// left to free. +void DisposeBlockCallback(MethodCallbackWrapper* wrapper) { + BlockWrapper* blockWrapper = wrapper->blockWrapper_; + Runtime* runtime = wrapper->isolateWrapper_.IsValid() + ? Runtime::GetRuntime(wrapper->isolateWrapper_.Isolate()) + : nullptr; + std::shared_ptr eventLoop = runtime != nullptr ? runtime->GetEventLoop() : nullptr; + + bool posted = eventLoop != nullptr && eventLoop->PostInternal([wrapper, blockWrapper]() { + Isolate* isolate = wrapper->isolateWrapper_.Isolate(); + Local callback = wrapper->callback_->Get(isolate); + // The function may have been handed to native again in the meantime, which + // attaches a fresh wrapper; only detach the one this block owns. + if (!callback.IsEmpty() && callback->IsObject() && + tns::GetValue(isolate, callback) == blockWrapper) { + tns::DeleteValue(isolate, callback); + } + wrapper->callback_->Reset(); + delete blockWrapper; + delete wrapper; + }); + + if (!posted) { + delete blockWrapper; + delete wrapper; + } +} + +} // namespace + Interop::JSBlock::JSBlockDescriptor Interop::JSBlock::kJSBlockDescriptor = { .reserved = 0, .size = sizeof(JSBlock), @@ -38,21 +77,12 @@ [](JSBlock* block) { if (block->descriptor == &JSBlock::kJSBlockDescriptor) { MethodCallbackWrapper* wrapper = static_cast(block->userData); - if (wrapper->isolateWrapper_.IsValid()) { - Isolate* isolate = wrapper->isolateWrapper_.Isolate(); - v8::Locker locker(isolate); - Isolate::Scope isolate_scope(isolate); - HandleScope handle_scope(isolate); - Local callback = wrapper->callback_->Get(isolate); - if (!callback.IsEmpty() && callback->IsObject()) { - BlockWrapper* blockWrapper = - static_cast(tns::GetValue(isolate, callback)); - tns::DeleteValue(isolate, callback); - wrapper->callback_->Reset(); - delete blockWrapper; - } + // The block memory dies with this call, so the wrapper that still + // names it must stop naming it now, before the deferred teardown. + if (wrapper->blockWrapper_ != nullptr) { + wrapper->blockWrapper_->ClearBlock(); } - delete wrapper; + DisposeBlockCallback(wrapper); ffi_closure_free(block->ffiClosure); block->~JSBlock(); } @@ -515,13 +545,17 @@ inline bool isBool() { CFTypeRef blockPtr = nullptr; BaseDataWrapper* baseWrapper = tns::GetValue(isolate, arg); - if (baseWrapper != nullptr && baseWrapper->Type() == WrapperType::Block) { - BlockWrapper* wrapper = static_cast(baseWrapper); + BlockWrapper* liveWrapper = baseWrapper != nullptr && baseWrapper->Type() == WrapperType::Block + ? static_cast(baseWrapper) + : nullptr; + // A cleared block means the wrapper outlived its JSBlock and is waiting for + // the deferred teardown; this call needs a fresh one. + if (liveWrapper != nullptr && liveWrapper->Block() != nullptr) { // The callee takes the block at +0 and copies it if it needs to keep it, // so the copy that keeps it alive across the call must be balanced: the // JSBlock dispose helper owns the ffi closure and the callback wrapper // and only runs once the last reference goes away. - blockPtr = CFAutorelease(Block_copy(wrapper->Block())); + blockPtr = CFAutorelease(Block_copy(liveWrapper->Block())); } else { std::shared_ptr> poCallback = std::make_shared>(isolate, arg); @@ -531,6 +565,7 @@ inline bool isBool() { userData); BlockWrapper* wrapper = new BlockWrapper((void*)blockPtr, blockTypeEncoding, false); + userData->blockWrapper_ = wrapper; tns::SetValue(isolate, arg.As(), wrapper); } 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..708c0d53 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,27 @@ void DisposeHandle(v8::Isolate* isolate, void ObjectManager::FinalizerCallback(const WeakCallbackInfo& data) { ObjectWeakCallbackState* state = data.GetParameter(); Isolate* isolate = data.GetIsolate(); + + 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 +348,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..8559aa11 100644 --- a/TestRunner/app/tests/GCFinalizerTests.js +++ b/TestRunner/app/tests/GCFinalizerTests.js @@ -141,4 +141,154 @@ 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(); + __collect(); + setTimeout(function () { + __collect(); + expect(ref.deref()).toBeUndefined(); + done(); + }, 0); + }); + + // The wrapper attached to the JS function outlives its block until the + // deferred teardown runs, so a re-marshal in that window must build a new + // block instead of copying freed memory. + 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/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 */, From 2ef5e509874dc6b4ec95cf2a22976155c162a124 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 26 Aug 2026 23:20:00 -0300 Subject: [PATCH 2/2] fix(runtime): keep JSBlock dispose inline and guard the teardown walk's claim The dispose helper goes back to inline teardown under the isolate Locker: 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. The deferred posting -- and the cleared-block re-marshal machinery it required -- is removed; the unconditional callback_ Reset stays, since an already-detached callback still owns its node. FinalizerCallback now honors the disposing claim on entry: a nested collection during DisposeAllRegistered's walk can condemn a pre-claimed state, and disposing it there would free memory the walk still holds. The callback re-arms its node -- satisfying the finalizer contract -- and leaves clear, reset and delete to the owner. Block-collectability specs poll instead of assuming a single tick suffices; drain interleaving makes one tick a coin flip either way. --- NativeScript/runtime/ArgConverter.h | 4 - NativeScript/runtime/DataWrapper.h | 5 -- NativeScript/runtime/Interop.mm | 77 ++++++------------- NativeScript/runtime/ObjectManager.mm | 10 +++ TestRunner/app/tests/GCFinalizerTests.js | 26 +++++-- .../app/tests/Marshalling/ObjCTypesTests.js | 20 +++-- 6 files changed, 68 insertions(+), 74 deletions(-) diff --git a/NativeScript/runtime/ArgConverter.h b/NativeScript/runtime/ArgConverter.h index c3730116..5e918109 100644 --- a/NativeScript/runtime/ArgConverter.h +++ b/NativeScript/runtime/ArgConverter.h @@ -30,10 +30,6 @@ struct MethodCallbackWrapper { const uint8_t initialParamIndex_; const uint8_t paramsCount_; const TypeEncoding* typeEncoding_; - // Set only for JSBlocks: the wrapper attached to callback_, owned by the - // block's dispose helper. Reaching it without V8 lets the helper neuter the - // block pointer on whatever thread the last release lands on. - BlockWrapper* blockWrapper_ = nullptr; }; class ArgConverter { diff --git a/NativeScript/runtime/DataWrapper.h b/NativeScript/runtime/DataWrapper.h index c29df8ab..a72d37db 100644 --- a/NativeScript/runtime/DataWrapper.h +++ b/NativeScript/runtime/DataWrapper.h @@ -459,11 +459,6 @@ class BlockWrapper : public BaseDataWrapper { void* Block() { return this->block_; } - // A JSBlock's dispose helper frees the block while this wrapper is still - // attached to its JS function; clearing the pointer there keeps a later - // marshalling of that function from copying dead block memory. - void ClearBlock() { this->block_ = nullptr; } - const TypeEncoding* Encodings() { return this->typeEncoding_; } bool OwnsBlock() { return this->ownsBlock_; } diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index 7fa50ed9..d56a974d 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -30,45 +30,6 @@ static_cast(kUint64AllBitsSet << 53) + 1; // -9007199254740991 (-(2^53-1)) static constexpr int64_t kMaxSafeInteger = -kMinSafeInteger; // 9007199254740991 (2^53-1) -namespace { - -// Tears down the JS side of a JSBlock whose last native reference just went -// away. The release lands on whatever thread the owning native code runs on, -// and can land inside ObjectManager's finalizer drain (a -dealloc cascade -// started by DisposeValue), where mutating global handles corrupts the drain's -// bookkeeping. So the handles are only ever touched from the owning runtime's -// home thread, through its event loop. A refused post means the loop is gone -// with its isolate, which took the handles with it -- only native memory is -// left to free. -void DisposeBlockCallback(MethodCallbackWrapper* wrapper) { - BlockWrapper* blockWrapper = wrapper->blockWrapper_; - Runtime* runtime = wrapper->isolateWrapper_.IsValid() - ? Runtime::GetRuntime(wrapper->isolateWrapper_.Isolate()) - : nullptr; - std::shared_ptr eventLoop = runtime != nullptr ? runtime->GetEventLoop() : nullptr; - - bool posted = eventLoop != nullptr && eventLoop->PostInternal([wrapper, blockWrapper]() { - Isolate* isolate = wrapper->isolateWrapper_.Isolate(); - Local callback = wrapper->callback_->Get(isolate); - // The function may have been handed to native again in the meantime, which - // attaches a fresh wrapper; only detach the one this block owns. - if (!callback.IsEmpty() && callback->IsObject() && - tns::GetValue(isolate, callback) == blockWrapper) { - tns::DeleteValue(isolate, callback); - } - wrapper->callback_->Reset(); - delete blockWrapper; - delete wrapper; - }); - - if (!posted) { - delete blockWrapper; - delete wrapper; - } -} - -} // namespace - Interop::JSBlock::JSBlockDescriptor Interop::JSBlock::kJSBlockDescriptor = { .reserved = 0, .size = sizeof(JSBlock), @@ -77,12 +38,29 @@ void DisposeBlockCallback(MethodCallbackWrapper* wrapper) { [](JSBlock* block) { if (block->descriptor == &JSBlock::kJSBlockDescriptor) { MethodCallbackWrapper* wrapper = static_cast(block->userData); - // The block memory dies with this call, so the wrapper that still - // names it must stop naming it now, before the deferred teardown. - if (wrapper->blockWrapper_ != nullptr) { - wrapper->blockWrapper_->ClearBlock(); + // 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); + Isolate::Scope isolate_scope(isolate); + HandleScope handle_scope(isolate); + Local callback = wrapper->callback_->Get(isolate); + if (!callback.IsEmpty() && callback->IsObject()) { + BlockWrapper* blockWrapper = + static_cast(tns::GetValue(isolate, callback)); + tns::DeleteValue(isolate, callback); + 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(); } - DisposeBlockCallback(wrapper); + delete wrapper; ffi_closure_free(block->ffiClosure); block->~JSBlock(); } @@ -545,17 +523,13 @@ inline bool isBool() { CFTypeRef blockPtr = nullptr; BaseDataWrapper* baseWrapper = tns::GetValue(isolate, arg); - BlockWrapper* liveWrapper = baseWrapper != nullptr && baseWrapper->Type() == WrapperType::Block - ? static_cast(baseWrapper) - : nullptr; - // A cleared block means the wrapper outlived its JSBlock and is waiting for - // the deferred teardown; this call needs a fresh one. - if (liveWrapper != nullptr && liveWrapper->Block() != nullptr) { + if (baseWrapper != nullptr && baseWrapper->Type() == WrapperType::Block) { + BlockWrapper* wrapper = static_cast(baseWrapper); // The callee takes the block at +0 and copies it if it needs to keep it, // so the copy that keeps it alive across the call must be balanced: the // JSBlock dispose helper owns the ffi closure and the callback wrapper // and only runs once the last reference goes away. - blockPtr = CFAutorelease(Block_copy(liveWrapper->Block())); + blockPtr = CFAutorelease(Block_copy(wrapper->Block())); } else { std::shared_ptr> poCallback = std::make_shared>(isolate, arg); @@ -565,7 +539,6 @@ inline bool isBool() { userData); BlockWrapper* wrapper = new BlockWrapper((void*)blockPtr, blockTypeEncoding, false); - userData->blockWrapper_ = wrapper; tns::SetValue(isolate, arg.As(), wrapper); } diff --git a/NativeScript/runtime/ObjectManager.mm b/NativeScript/runtime/ObjectManager.mm index 708c0d53..c683ed9a 100644 --- a/NativeScript/runtime/ObjectManager.mm +++ b/NativeScript/runtime/ObjectManager.mm @@ -143,6 +143,16 @@ void DisposeHandle(v8::Isolate* isolate, 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); diff --git a/TestRunner/app/tests/GCFinalizerTests.js b/TestRunner/app/tests/GCFinalizerTests.js index 8559aa11..0482a9c0 100644 --- a/TestRunner/app/tests/GCFinalizerTests.js +++ b/TestRunner/app/tests/GCFinalizerTests.js @@ -256,17 +256,27 @@ describe("GC finalizer callbacks", function () { TNSClearOutput(); scrubStack(); - __collect(); - setTimeout(function () { + // 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(); - expect(ref.deref()).toBeUndefined(); - done(); - }, 0); + if (ref.deref() === undefined) { + done(); + return; + } + if (--attempts === 0) { + expect(ref.deref()).toBeUndefined(); + done(); + return; + } + setTimeout(pollCollected); + })(); }); - // The wrapper attached to the JS function outlives its block until the - // deferred teardown runs, so a re-marshal in that window must build a new - // block instead of copying freed memory. + // 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"); 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); + })(); }); });