From 5372fa47ea614f6811364858ccc315691fedde85 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Thu, 27 Aug 2026 01:45:38 -0300 Subject: [PATCH 01/10] fix(runtime): single-owner adapter wrappers and reentrancy-safe disposal The collection adapters attached an ObjCDataWrapper to their JS object unconditionally, and DisposeValue's ObjCObject branch releases the adapter with the wrapper pointer cached in a local -- the adapter's -dealloc, running inside that release, freed the same wrapper the tail then deleted again. The double-free's recycled chunk corrupted live allocations (captured in the field as a registered persistent's slot word zeroed while its node stayed armed), surfacing as three distinct GC crash signatures on worker isolates within seconds of heavy collection marshalling. Ownership is now single and explicit: a JS object's internal field holds at most one wrapper and owns it; the first adapter to attach wins, a later one stays detached and never writes or clears the field; dataWrapper_ is a claim token for recognising our own wrapper, not an ownership handle. Every path that runs arbitrary code between reading the field and freeing it -- DisposeValue's tail and __releaseNativeCounterpart -- re-reads the field and frees only a wrapper still attached. DictionaryAdapter also gains the object_->Reset() the other adapters already had (its absence leaked the armed global-handle node), and its key enumerators retain the adapter -- NSEnumerator semantics -- so the reset cannot empty the persistent under a live enumeration. New GCFinalizerTests specs pin the ownership contract under the production workload mix (adapter marshalling interleaved with native TextDecoder/atob churn, finalizer-driven releases, a worker-isolate variant); they are tripwires -- the old double-free needs a guard-malloc/ASan lane to abort deterministically. Suite 1512/0. --- NativeScript/runtime/ArrayAdapter.mm | 37 +++-- NativeScript/runtime/DictionaryAdapter.mm | 68 ++++++--- NativeScript/runtime/NSDataAdapter.mm | 36 +++-- NativeScript/runtime/ObjectManager.mm | 19 ++- TestRunner/app/tests/GCFinalizerTests.js | 156 +++++++++++++++++++++ TestRunner/app/tests/adapterChurnWorker.js | 24 ++++ 6 files changed, 284 insertions(+), 56 deletions(-) create mode 100644 TestRunner/app/tests/adapterChurnWorker.js diff --git a/NativeScript/runtime/ArrayAdapter.mm b/NativeScript/runtime/ArrayAdapter.mm index ed7d10f3..904ace2c 100644 --- a/NativeScript/runtime/ArrayAdapter.mm +++ b/NativeScript/runtime/ArrayAdapter.mm @@ -11,7 +11,9 @@ @implementation ArrayAdapter { IsolateWrapper* wrapper_; std::shared_ptr> object_; - // we're responsible for this wrapper + // The wrapper this adapter attached to the JS object, or nullptr when the + // field was already taken. Ownership lives with the field, not with this + // pointer: it is only the claim used to recognise our own wrapper there. ObjCDataWrapper* dataWrapper_; } @@ -19,8 +21,14 @@ - (instancetype)initWithJSObject:(Local)jsObject isolate:(Isolate*)isola if (self) { self->wrapper_ = new IsolateWrapper(isolate); self->object_ = std::make_shared>(isolate, jsObject); - self->wrapper_->GetCache()->Instances.emplace(self, self->object_); - tns::SetValue(isolate, jsObject, (self->dataWrapper_ = new ObjCDataWrapper(self))); + self->wrapper_->GetCache()->Instances[self] = self->object_; + // A JS object's internal field holds at most one wrapper, owned by whoever + // attached it first. An adapter that finds the field taken stays detached + // and never writes or clears it; it still reads the object through object_. + if (tns::GetValue(isolate, jsObject) == nullptr) { + self->dataWrapper_ = new ObjCDataWrapper(self); + tns::SetValue(isolate, jsObject, self->dataWrapper_); + } } return self; @@ -107,23 +115,22 @@ - (void)dealloc { Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); wrapper_->GetCache()->Instances.erase(self); - Local value = self->object_->Get(isolate); - BaseDataWrapper* wrapper = tns::GetValue(isolate, value); - if (wrapper != nullptr) { - tns::DeleteValue(isolate, value); - // ensure we don't delete the same wrapper twice - // this is just needed as a failsafe in case some other wrapper is assigned to this object - if (wrapper == dataWrapper_) { - dataWrapper_ = nullptr; + // Detach and free only a wrapper that is still the one we attached: a + // finalizer or __releaseNativeCounterpart can have retired it already, and + // whatever else sits in the field belongs to another owner. Once the + // isolate is gone the field can no longer be read, so the claim is dropped + // rather than freed blind. + if (dataWrapper_ != nullptr) { + Local value = self->object_->Get(isolate); + if (tns::GetValue(isolate, value) == dataWrapper_) { + tns::DeleteValue(isolate, value); + delete dataWrapper_; } - delete wrapper; + dataWrapper_ = nullptr; } self->object_->Reset(); } delete wrapper_; - if (dataWrapper_ != nullptr) { - delete dataWrapper_; - } self->object_ = nullptr; [super dealloc]; } diff --git a/NativeScript/runtime/DictionaryAdapter.mm b/NativeScript/runtime/DictionaryAdapter.mm index 3cc5a6f0..60ac68ee 100644 --- a/NativeScript/runtime/DictionaryAdapter.mm +++ b/NativeScript/runtime/DictionaryAdapter.mm @@ -14,7 +14,7 @@ @interface DictionaryAdapterMapKeysEnumerator : NSEnumerator - (instancetype)initWithMap:(std::shared_ptr>)map isolate:(Isolate*)isolate - cache:(std::shared_ptr)cache; + owner:(id)owner; @end @@ -22,15 +22,19 @@ @implementation DictionaryAdapterMapKeysEnumerator { IsolateWrapper* wrapper_; uint32_t index_; std::shared_ptr> map_; + // The adapter owns the persistent this enumerator reads and resets it in + // -dealloc, so an enumeration keeps its adapter alive. + id owner_; } - (instancetype)initWithMap:(std::shared_ptr>)map isolate:(Isolate*)isolate - cache:(std::shared_ptr)cache { + owner:(id)owner { if (self) { self->wrapper_ = new IsolateWrapper(isolate); self->index_ = 0; self->map_ = map; + self->owner_ = [owner retain]; } return self; @@ -76,6 +80,8 @@ - (id)nextObject { - (void)dealloc { self->map_ = nil; delete self->wrapper_; + [self->owner_ release]; + self->owner_ = nil; [super dealloc]; } @@ -86,7 +92,7 @@ @interface DictionaryAdapterObjectKeysEnumerator : NSEnumerator - (instancetype)initWithProperties:(std::shared_ptr>)dictionary isolate:(Isolate*)isolate - cache:(std::shared_ptr)cache; + owner:(id)owner; - (Local)getProperties; @end @@ -95,15 +101,19 @@ @implementation DictionaryAdapterObjectKeysEnumerator { IsolateWrapper* wrapper_; std::shared_ptr> dictionary_; NSUInteger index_; + // The adapter owns the persistent this enumerator reads and resets it in + // -dealloc, so an enumeration keeps its adapter alive. + id owner_; } - (instancetype)initWithProperties:(std::shared_ptr>)dictionary isolate:(Isolate*)isolate - cache:(std::shared_ptr)cache { + owner:(id)owner { if (self) { self->wrapper_ = new IsolateWrapper(isolate); self->dictionary_ = dictionary; self->index_ = 0; + self->owner_ = [owner retain]; } return self; @@ -199,6 +209,8 @@ - (NSArray*)allObjects { - (void)dealloc { self->dictionary_ = nil; delete self->wrapper_; + [self->owner_ release]; + self->owner_ = nil; [super dealloc]; } @@ -208,6 +220,9 @@ - (void)dealloc { @implementation DictionaryAdapter { IsolateWrapper* wrapper_; std::shared_ptr> object_; + // The wrapper this adapter attached to the JS object, or nullptr when the + // field was already taken. Ownership lives with the field, not with this + // pointer: it is only the claim used to recognise our own wrapper there. ObjCDataWrapper* dataWrapper_; } @@ -215,8 +230,14 @@ - (instancetype)initWithJSObject:(Local)jsObject isolate:(Isolate*)isola if (self) { self->wrapper_ = new IsolateWrapper(isolate); self->object_ = std::make_shared>(isolate, jsObject); - self->wrapper_->GetCache()->Instances.emplace(self, self->object_); - tns::SetValue(isolate, jsObject, (self->dataWrapper_ = new ObjCDataWrapper(self))); + self->wrapper_->GetCache()->Instances[self] = self->object_; + // A JS object's internal field holds at most one wrapper, owned by whoever + // attached it first. An adapter that finds the field taken stays detached + // and never writes or clears it; it still reads the object through object_. + if (tns::GetValue(isolate, jsObject) == nullptr) { + self->dataWrapper_ = new ObjCDataWrapper(self); + tns::SetValue(isolate, jsObject, self->dataWrapper_); + } } return self; @@ -321,16 +342,14 @@ - (NSEnumerator*)keyEnumerator { Local obj = self->object_->Get(isolate); if (obj->IsMap()) { - return - [[[DictionaryAdapterMapKeysEnumerator alloc] initWithMap:self->object_ - isolate:isolate - cache:wrapper_->GetCache()] autorelease]; + return [[[DictionaryAdapterMapKeysEnumerator alloc] initWithMap:self->object_ + isolate:isolate + owner:self] autorelease]; } return [[[DictionaryAdapterObjectKeysEnumerator alloc] initWithProperties:self->object_ isolate:isolate - cache:wrapper_->GetCache()] - autorelease]; + owner:self] autorelease]; } - (void)dealloc { @@ -340,18 +359,23 @@ - (void)dealloc { Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); wrapper_->GetCache()->Instances.erase(self); - Local value = self->object_->Get(isolate); - BaseDataWrapper* wrapper = tns::GetValue(isolate, value); - if (wrapper != nullptr) { - if (wrapper == dataWrapper_) { - dataWrapper_ = nullptr; + // Detach and free only a wrapper that is still the one we attached: a + // finalizer or __releaseNativeCounterpart can have retired it already, and + // whatever else sits in the field belongs to another owner. Once the + // isolate is gone the field can no longer be read, so the claim is dropped + // rather than freed blind. + if (dataWrapper_ != nullptr) { + Local value = self->object_->Get(isolate); + if (tns::GetValue(isolate, value) == dataWrapper_) { + tns::DeleteValue(isolate, value); + delete dataWrapper_; } - tns::DeleteValue(isolate, value); - delete wrapper; + dataWrapper_ = nullptr; } - } - if (dataWrapper_ != nullptr) { - delete dataWrapper_; + // Persistent does not reset in its destructor; the enumerators + // vended by -keyEnumerator hold this adapter alive, so nothing can be + // reading the handle by the time this runs. + self->object_->Reset(); } self->object_ = nullptr; delete self->wrapper_; diff --git a/NativeScript/runtime/NSDataAdapter.mm b/NativeScript/runtime/NSDataAdapter.mm index 5176fe60..fcbba530 100644 --- a/NativeScript/runtime/NSDataAdapter.mm +++ b/NativeScript/runtime/NSDataAdapter.mm @@ -8,6 +8,9 @@ @implementation NSDataAdapter { IsolateWrapper* wrapper_; + // The wrapper this adapter attached to the JS object, or nullptr when the + // field was already taken. Ownership lives with the field, not with this + // pointer: it is only the claim used to recognise our own wrapper there. ObjCDataWrapper* dataWrapper_; std::shared_ptr> object_; } @@ -19,8 +22,14 @@ - (instancetype)initWithJSObject:(Local)jsObject isolate:(Isolate*)isola isolate); self->wrapper_ = new IsolateWrapper(isolate); self->object_ = std::make_shared>(isolate, jsObject); - self->wrapper_->GetCache()->Instances.emplace(self, self->object_); - tns::SetValue(isolate, jsObject, (dataWrapper_ = new ObjCDataWrapper(self))); + self->wrapper_->GetCache()->Instances[self] = self->object_; + // A JS object's internal field holds at most one wrapper, owned by whoever + // attached it first. An adapter that finds the field taken stays detached + // and never writes or clears it; it still reads the object through object_. + if (tns::GetValue(isolate, jsObject) == nullptr) { + self->dataWrapper_ = new ObjCDataWrapper(self); + tns::SetValue(isolate, jsObject, self->dataWrapper_); + } } return self; @@ -87,22 +96,21 @@ - (void)dealloc { Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); wrapper_->GetCache()->Instances.erase(self); - Local value = self->object_->Get(isolate); - BaseDataWrapper* wrapper = tns::GetValue(isolate, value); - if (wrapper != nullptr) { - tns::DeleteValue(isolate, value); - // ensure we don't delete the same wrapper twice - // this is just needed as a failsafe in case some other wrapper is assigned to this object - if (wrapper == dataWrapper_) { - dataWrapper_ = nullptr; + // Detach and free only a wrapper that is still the one we attached: a + // finalizer or __releaseNativeCounterpart can have retired it already, and + // whatever else sits in the field belongs to another owner. Once the + // isolate is gone the field can no longer be read, so the claim is dropped + // rather than freed blind. + if (dataWrapper_ != nullptr) { + Local value = self->object_->Get(isolate); + if (tns::GetValue(isolate, value) == dataWrapper_) { + tns::DeleteValue(isolate, value); + delete dataWrapper_; } - delete wrapper; + dataWrapper_ = nullptr; } self->object_->Reset(); } - if (dataWrapper_ != nullptr) { - delete dataWrapper_; - } delete self->wrapper_; self->object_ = nullptr; diff --git a/NativeScript/runtime/ObjectManager.mm b/NativeScript/runtime/ObjectManager.mm index c683ed9a..ba791b1e 100644 --- a/NativeScript/runtime/ObjectManager.mm +++ b/NativeScript/runtime/ObjectManager.mm @@ -317,9 +317,14 @@ void DisposeHandle(v8::Isolate* isolate, break; } - delete wrapper; - wrapper = nullptr; - tns::DeleteValue(isolate, obj); + // A branch above can run arbitrary code -- [target release] reaching an ObjC + // -dealloc is the reachable one -- and that code may detach this wrapper or + // attach a different one. The object's internal field is the wrapper's owner, + // so only a wrapper still sitting in it is ours to free. + if (tns::GetValue(isolate, obj) == wrapper) { + delete wrapper; + tns::DeleteValue(isolate, obj); + } return true; } @@ -387,8 +392,12 @@ void DisposeHandle(v8::Isolate* isolate, // were still referenced elsewhere and caused use-after-free crashes. [data release]; - delete wrapper; - tns::SetValue(isolate, value.As(), nullptr); + // The release above can run a -dealloc that detaches this wrapper; the + // internal field owns it, so free only what is still attached. + if (tns::GetValue(isolate, value) == wrapper) { + delete wrapper; + tns::SetValue(isolate, value.As(), nullptr); + } } } diff --git a/TestRunner/app/tests/GCFinalizerTests.js b/TestRunner/app/tests/GCFinalizerTests.js index 0482a9c0..d51ea954 100644 --- a/TestRunner/app/tests/GCFinalizerTests.js +++ b/TestRunner/app/tests/GCFinalizerTests.js @@ -236,6 +236,162 @@ describe("GC finalizer callbacks", function () { expect(survivor.objectAtIndex(1)).toBe(2); }); + // Allocation pressure shaped like the field workload: native lazy-global + // paths (TextDecoder, atob/btoa) interleaved with adapter marshalling, so + // a wrapper freed twice lands on somebody else's live allocation. + function churn(rounds) { + var decoder = new TextDecoder(); + var sink = 0; + for (var i = 0; i < rounds; i++) { + sink += decoder.decode(new Uint8Array([65, 66, 67, i % 128])).length; + sink += atob(btoa("churn-" + i)).length; + var probe = NSMutableArray.alloc().init(); + probe.addObject([i, i + 1]); + probe.addObject(new Uint8Array(8)); + probe.addObject({ k: i }); + sink += probe.count; + } + return sink; + } + + // The JS object's internal field owns the wrapper an adapter attaches to + // it. When a finalizer drops the last native reference to the adapter, the + // adapter's -dealloc detaches that wrapper from inside the disposal that + // released it, so the disposal must not free what it read beforehand. + it("releases adapters from inside a finalizer across repeated cycles", function () { + var cycles = 12; + + for (var c = 0; c < cycles; c++) { + (function () { + var holders = []; + for (var i = 0; i < 8; i++) { + // Each holder takes the only native reference to the + // adapters built for these collections. + var holder = NSMutableArray.alloc().init(); + holder.addObject([c, i, i + 1]); + holder.addObject(new Uint8Array(16)); + holder.addObject({ c: c, i: i }); + holders.push(holder); + } + })(); + + scrubStack(); + __collect(); + expect(churn(16)).toBeGreaterThan(0); + __collect(); + } + + var survivor = NSMutableArray.arrayWithArray([1, 2, 3]); + expect(survivor.count).toBe(3); + expect(survivor.objectAtIndex(2)).toBe(3); + }); + + // Marshalling the same collection twice builds a second adapter for a JS + // object whose field is already claimed. The second adapter must leave the + // field alone, so that neither adapter's -dealloc frees the other's + // wrapper, and both must still marshal back to the original JS object. + it("survives a JS collection marshalled to native twice", function () { + var rounds = 8; + + for (var r = 0; r < rounds; r++) { + var arr = [r, r + 1, r + 2]; + var obj = { id: r, param: "abc" }; + var types = TNSObjCTypes.alloc().init(); + + // objectAtIndex: on the outer adapter builds a fresh adapter for + // the nested collection on every call. + expect(types.methodWithNSArrayWrappingDictionary([obj])).toBe(obj); + expect(types.methodWithNSArrayWrappingDictionary([obj])).toBe(obj); + expect(types.methodWithNSArrayWrappingDictionary([arr])).toBe(arr); + expect(types.methodWithNSArrayWrappingDictionary([arr])).toBe(arr); + + var first = NSMutableArray.alloc().init(); + first.addObject(arr); + var second = NSMutableArray.alloc().init(); + second.addObject(arr); + expect(first.count).toBe(1); + expect(second.count).toBe(1); + + expect(churn(8)).toBeGreaterThan(0); + } + + scrubStack(); + __collect(); + expect(churn(16)).toBeGreaterThan(0); + __collect(); + + var survivor = NSMutableArray.arrayWithArray([4, 5]); + expect(survivor.count).toBe(2); + }); + + // A key enumerator reads the persistent its adapter owns, and the adapter + // resets that persistent in -dealloc, so an enumeration keeps its adapter + // alive for as long as the enumerator itself lives. + it("keeps a dictionary adapter alive for its keys enumerator", function () { + var rounds = 8; + + for (var r = 0; r < rounds; r++) { + var types = TNSObjCTypes.alloc().init(); + // Fast enumeration over a foreign NSDictionary goes through + // -keyEnumerator; the enumerator outlives the call that made it, + // draining with the pool rather than with the adapter. + var dictionary = { a: 3, b: { "-1": [4, 5] }, d: 6 }; + expect(types.methodWithNSDictionary(dictionary)).toBe(dictionary); + TNSClearOutput(); + + var map = new Map(); + map.set("a", 3); + map.set("d", 6); + expect(types.methodWithNSDictionary(map)).toBe(map); + TNSClearOutput(); + + expect(churn(8)).toBeGreaterThan(0); + } + + scrubStack(); + __collect(); + expect(churn(16)).toBeGreaterThan(0); + __collect(); + + // A dictionary enumerated after the sweep still reports its keys. + var late = { x: 1, y: 2 }; + expect(TNSObjCTypes.alloc().init().methodWithNSDictionary(late)).toBe(late); + expect(TNSGetOutput()).toBe("x 1y 2"); + TNSClearOutput(); + }); + + // The field crashes surfaced on worker isolates, where the same churn runs + // and the isolate is torn down while adapters may still be alive. + it("survives the same churn on a worker isolate", function (done) { + var originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL; + jasmine.DEFAULT_TIMEOUT_INTERVAL = 15000; + + var worker = new Worker("./adapterChurnWorker.js"); + var rounds = 0; + + var finish = function () { + jasmine.DEFAULT_TIMEOUT_INTERVAL = originalTimeout; + worker.terminate(); + done(); + }; + + worker.onmessage = function (msg) { + expect(msg.data.ok).toBe(true); + rounds++; + if (rounds === 6) { + finish(); + return; + } + worker.postMessage(rounds); + }; + worker.onerror = function (e) { + expect(String(e && e.message ? e.message : e)).toBe(""); + finish(); + }; + + worker.postMessage(0); + }); + // 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) { diff --git a/TestRunner/app/tests/adapterChurnWorker.js b/TestRunner/app/tests/adapterChurnWorker.js new file mode 100644 index 00000000..2eef82a8 --- /dev/null +++ b/TestRunner/app/tests/adapterChurnWorker.js @@ -0,0 +1,24 @@ +// Adapter marshalling interleaved with the native lazy-global paths, on a +// worker isolate: the shape the production heap corruption surfaced under. +onmessage = function (msg) { + var round = msg.data; + var decoder = new TextDecoder(); + var sink = 0; + + for (var i = 0; i < 24; i++) { + var holder = NSMutableArray.alloc().init(); + holder.addObject([round, i]); + holder.addObject(new Uint8Array(16)); + holder.addObject({ round: round, i: i }); + sink += holder.count; + + sink += decoder.decode(new Uint8Array([65, 66, 67, i % 128])).length; + sink += atob(btoa("worker-" + i)).length; + } + + __collect(); + __collect(); + + var survivor = NSMutableArray.arrayWithArray([1, 2, 3]); + postMessage({ ok: sink > 0 && survivor.count === 3 }); +}; From 693b618f6bfc8631f4a5a9c72f4189d61294c9d2 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Thu, 27 Aug 2026 01:59:09 -0300 Subject: [PATCH 02/10] fix(interop): zero-initialize the NSError out-parameter buffer A callee writes *error only on failure, so on success the read-back found whatever the malloc chunk last held. A non-null stale value was then sent localizedDescription and -- read through a __strong pointer -- retained and released by ARC: an over-release of whatever object now lives at that address, prematurely freeing live allocations whose owners keep writing through dangling references. Those writes landing in recycled GC bookkeeping produced the worker-isolate crash family this branch chases; under MallocScribble the stale read reproduces deterministically at boot as an unrecognized-selector throw on the scribble pattern. The other transient interop buffers are fully written before any read; this was the only uninitialized read-back. --- NativeScript/runtime/Interop.mm | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index d56a974d..10d2bac2 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -1675,7 +1675,11 @@ inline bool isBool() { void* errorRef = nullptr; if (methodCall.provideErrorOutParameter_) { void* dest = call.ArgumentBuffer(argsCount); - errorRef = malloc(ffi_type_pointer.size); + // Zero-initialized: a callee writes *error only on failure, so the + // success-path read below must find nil. Garbage here is read through a + // __strong pointer -- ARC retains and releases it -- so a stale non-null + // value over-releases whatever lives at that address now. + errorRef = calloc(1, ffi_type_pointer.size); Interop::SetValue(dest, errorRef); } From df236a8e1cf063b512b8bb5f9729133674eff6d2 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Thu, 27 Aug 2026 11:08:01 -0300 Subject: [PATCH 03/10] feat(worker): name looper threads after their entry script Crash reports previously showed every worker as an anonymous NSOperationQueue thread; the thread name now carries the worker id and script basename (worker3:pixelmap-socket.js). --- NativeScript/runtime/Worker.mm | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/NativeScript/runtime/Worker.mm b/NativeScript/runtime/Worker.mm index 4f2e14d5..31368507 100644 --- a/NativeScript/runtime/Worker.mm +++ b/NativeScript/runtime/Worker.mm @@ -1,4 +1,5 @@ #include "Worker.h" +#include #include #include "Caches.h" #include "Constants.h" @@ -181,6 +182,22 @@ throw NativeScriptException( tns::LoaderVocabulary inheritedVocabulary = tns::CaptureLoaderVocabulary(isolate); std::function func([worker, workerPath, inheritedVocabulary]() { + // Name the looper thread after its entry script so a crash report + // identifies which worker died instead of an anonymous NSOperationQueue + // thread. Darwin caps thread names at 63 bytes; keep the basename only. + { + std::string threadName = workerPath; + size_t slash = threadName.find_last_of('/'); + if (slash != std::string::npos) { + threadName = threadName.substr(slash + 1); + } + threadName = "worker" + std::to_string(worker->WorkerId()) + ":" + threadName; + if (threadName.size() > 63) { + threadName.resize(63); + } + pthread_setname_np(threadName.c_str()); + } + // Resolve tilde paths before creating the runtime std::string resolvedPath = workerPath; if (!workerPath.empty() && workerPath[0] == '~') { From ca00a178c2f2a61f62a2fb14881565003484a527 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Thu, 27 Aug 2026 11:10:27 -0300 Subject: [PATCH 04/10] fix(runtime): pin the backing store for the NSDataAdapter's lifetime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The adapter's persistent pins the JS object, not its bytes: a postMessage transfer detaches the ArrayBuffer and hands the store to another isolate, whose GC can free the memory while native code still holds this NSData — an async reader/writer then touches a freed, recycled chunk. Holding the BackingStore shared_ptr keeps the bytes alive for the adapter's lifetime, which is the contract NSData callers assume, and lets -bytes answer without unlocked cross-thread V8 access. The never-materialized-view branch now serves one stable copy freed in dealloc instead of leaking a fresh malloc per call. --- NativeScript/runtime/NSDataAdapter.mm | 71 ++++++++++++++++++--------- 1 file changed, 48 insertions(+), 23 deletions(-) diff --git a/NativeScript/runtime/NSDataAdapter.mm b/NativeScript/runtime/NSDataAdapter.mm index fcbba530..394ab4e7 100644 --- a/NativeScript/runtime/NSDataAdapter.mm +++ b/NativeScript/runtime/NSDataAdapter.mm @@ -13,6 +13,17 @@ @implementation NSDataAdapter { // pointer: it is only the claim used to recognise our own wrapper there. ObjCDataWrapper* dataWrapper_; std::shared_ptr> object_; + // Pins the bytes for the adapter's lifetime, which is the NSData contract + // native callers rely on. The persistent above pins only the JS OBJECT: a + // postMessage transfer detaches it and hands the store to another isolate, + // whose GC can free the memory while native code still holds this NSData — + // an async reader/writer then touches a freed, recycled chunk. + std::shared_ptr store_; + // View byte offset into store_, captured with it (immutable for a view). + size_t storeOffset_; + // Lazily-built stable copy for a view whose buffer was never materialized; + // owned here, freed in dealloc. + void* heapCopy_; } - (instancetype)initWithJSObject:(Local)jsObject isolate:(Isolate*)isolate { @@ -23,6 +34,19 @@ - (instancetype)initWithJSObject:(Local)jsObject isolate:(Isolate*)isola self->wrapper_ = new IsolateWrapper(isolate); self->object_ = std::make_shared>(isolate, jsObject); self->wrapper_->GetCache()->Instances[self] = self->object_; + self->storeOffset_ = 0; + self->heapCopy_ = nullptr; + if (jsObject->IsArrayBuffer()) { + self->store_ = jsObject.As()->GetBackingStore(); + } else if (jsObject->IsSharedArrayBuffer()) { + self->store_ = jsObject.As()->GetBackingStore(); + } else { + Local view = jsObject.As(); + if (view->HasBuffer()) { + self->store_ = view->Buffer()->GetBackingStore(); + self->storeOffset_ = view->ByteOffset(); + } + } // A JS object's internal field holds at most one wrapper, owned by whoever // attached it first. An adapter that finds the field taken stays detached // and never writes or clears it; it still reads the object through object_. @@ -40,36 +64,35 @@ - (const void*)bytes { } - (void*)mutableBytes { + // The pinned store answers without touching V8, so native callers on + // foreign threads need no isolate access (the old per-call + // GetBackingStore() lookup ran unlocked from any thread). + if (store_ != nullptr) { + void* data = store_->Data(); + if (data == nullptr) { + return nullptr; + } + return static_cast(data) + storeOffset_; + } + if (!wrapper_->IsValid()) { return nil; } + // Only reachable for a view whose buffer was never materialized. Serve one + // stable copy for the adapter's lifetime: NSData callers assume -bytes is + // stable, and a fresh allocation per call also never got freed. + if (heapCopy_ != nullptr) { + return heapCopy_; + } Isolate* isolate = wrapper_->Isolate(); Local obj = self->object_->Get(isolate).As(); - if (obj->IsArrayBuffer()) { - void* data = obj.As()->GetBackingStore()->Data(); - return data; - } - - if (obj->IsSharedArrayBuffer()) { - void* data = obj.As()->GetBackingStore()->Data(); - return data; - } - Local bufferView = obj.As(); - if (bufferView->HasBuffer()) { - uint8_t* data = static_cast(bufferView->Buffer()->GetBackingStore()->Data()); - if (data == nullptr) { - return nullptr; - } - - return data + bufferView->ByteOffset(); - } - size_t length = bufferView->ByteLength(); - void* data = malloc(length); - bufferView->CopyContents(data, length); - - return data; + heapCopy_ = malloc(length); + if (heapCopy_ != nullptr) { + bufferView->CopyContents(heapCopy_, length); + } + return heapCopy_; } - (NSUInteger)length { @@ -114,6 +137,8 @@ - (void)dealloc { delete self->wrapper_; self->object_ = nullptr; + free(self->heapCopy_); + self->heapCopy_ = nullptr; [super dealloc]; } From 4b63ca66233d66c22c129c0ee20ecd1fea386e2d Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Thu, 27 Aug 2026 12:21:41 -0300 Subject: [PATCH 05/10] fix(runtime): snapshot the NSDataAdapter length with its pinned store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NSData is immutable — length must not change for the object's lifetime — but the live ByteLength() read reported zero after a transfer detach while the pinned bytes stayed valid, and it was also the adapter's last unlocked cross-thread V8 access. --- NativeScript/runtime/NSDataAdapter.mm | 33 ++++++++++++--------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/NativeScript/runtime/NSDataAdapter.mm b/NativeScript/runtime/NSDataAdapter.mm index 394ab4e7..c2ec0e1c 100644 --- a/NativeScript/runtime/NSDataAdapter.mm +++ b/NativeScript/runtime/NSDataAdapter.mm @@ -21,6 +21,10 @@ @implementation NSDataAdapter { std::shared_ptr store_; // View byte offset into store_, captured with it (immutable for a view). size_t storeOffset_; + // Byte length snapshotted with the store. NSData is immutable — its length + // must not change for the object's lifetime — and the live ByteLength() + // reads zero after a transfer detach while the pinned bytes stay valid. + size_t length_; // Lazily-built stable copy for a view whose buffer was never materialized; // owned here, freed in dealloc. void* heapCopy_; @@ -37,11 +41,16 @@ - (instancetype)initWithJSObject:(Local)jsObject isolate:(Isolate*)isola self->storeOffset_ = 0; self->heapCopy_ = nullptr; if (jsObject->IsArrayBuffer()) { - self->store_ = jsObject.As()->GetBackingStore(); + Local buffer = jsObject.As(); + self->store_ = buffer->GetBackingStore(); + self->length_ = buffer->ByteLength(); } else if (jsObject->IsSharedArrayBuffer()) { - self->store_ = jsObject.As()->GetBackingStore(); + Local buffer = jsObject.As(); + self->store_ = buffer->GetBackingStore(); + self->length_ = buffer->ByteLength(); } else { Local view = jsObject.As(); + self->length_ = view->ByteLength(); if (view->HasBuffer()) { self->store_ = view->Buffer()->GetBackingStore(); self->storeOffset_ = view->ByteOffset(); @@ -87,29 +96,15 @@ - (void*)mutableBytes { Isolate* isolate = wrapper_->Isolate(); Local obj = self->object_->Get(isolate).As(); Local bufferView = obj.As(); - size_t length = bufferView->ByteLength(); - heapCopy_ = malloc(length); + heapCopy_ = malloc(length_); if (heapCopy_ != nullptr) { - bufferView->CopyContents(heapCopy_, length); + bufferView->CopyContents(heapCopy_, length_); } return heapCopy_; } - (NSUInteger)length { - if (!wrapper_->IsValid()) { - return 0; - } - Isolate* isolate = wrapper_->Isolate(); - Local obj = self->object_->Get(isolate).As(); - if (obj->IsArrayBuffer()) { - return obj.As()->ByteLength(); - } - - if (obj->IsSharedArrayBuffer()) { - return obj.As()->ByteLength(); - } - - return obj.As()->ByteLength(); + return length_; } - (void)dealloc { From 9d1d7a372642a6deb89cfbd8f711f93c99797e09 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Thu, 27 Aug 2026 12:43:38 -0300 Subject: [PATCH 06/10] fix(runtime): materialize the on-heap view copy at adapter init MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lazy first--bytes copy ran isolate APIs from whatever thread the caller was on, could race concurrent callers on the publication, and a view detached before the first call would expose an uninitialized allocation. Copying during init — isolate owned, view alive — removes the lazy path entirely, so every -bytes branch answers from native storage. --- NativeScript/runtime/NSDataAdapter.mm | 33 ++++++++++----------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/NativeScript/runtime/NSDataAdapter.mm b/NativeScript/runtime/NSDataAdapter.mm index c2ec0e1c..fdb49d1c 100644 --- a/NativeScript/runtime/NSDataAdapter.mm +++ b/NativeScript/runtime/NSDataAdapter.mm @@ -25,8 +25,9 @@ @implementation NSDataAdapter { // must not change for the object's lifetime — and the live ByteLength() // reads zero after a transfer detach while the pinned bytes stay valid. size_t length_; - // Lazily-built stable copy for a view whose buffer was never materialized; - // owned here, freed in dealloc. + // Stable copy for a view whose buffer was never materialized, built during + // init while the isolate is owned and the view alive — -bytes may run on + // threads that cannot touch V8. Owned here, freed in dealloc. void* heapCopy_; } @@ -54,6 +55,13 @@ - (instancetype)initWithJSObject:(Local)jsObject isolate:(Isolate*)isola if (view->HasBuffer()) { self->store_ = view->Buffer()->GetBackingStore(); self->storeOffset_ = view->ByteOffset(); + } else { + self->heapCopy_ = malloc(self->length_); + if (self->heapCopy_ != nullptr) { + view->CopyContents(self->heapCopy_, self->length_); + } else { + self->length_ = 0; + } } } // A JS object's internal field holds at most one wrapper, owned by whoever @@ -73,8 +81,8 @@ - (const void*)bytes { } - (void*)mutableBytes { - // The pinned store answers without touching V8, so native callers on - // foreign threads need no isolate access (the old per-call + // Every branch answers from native storage captured at init, so callers on + // foreign threads never need isolate access (the old per-call // GetBackingStore() lookup ran unlocked from any thread). if (store_ != nullptr) { void* data = store_->Data(); @@ -83,23 +91,6 @@ - (void*)mutableBytes { } return static_cast(data) + storeOffset_; } - - if (!wrapper_->IsValid()) { - return nil; - } - // Only reachable for a view whose buffer was never materialized. Serve one - // stable copy for the adapter's lifetime: NSData callers assume -bytes is - // stable, and a fresh allocation per call also never got freed. - if (heapCopy_ != nullptr) { - return heapCopy_; - } - Isolate* isolate = wrapper_->Isolate(); - Local obj = self->object_->Get(isolate).As(); - Local bufferView = obj.As(); - heapCopy_ = malloc(length_); - if (heapCopy_ != nullptr) { - bufferView->CopyContents(heapCopy_, length_); - } return heapCopy_; } From 467257c3e4a5b2fd8c599d117101ce1dbe1464b0 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Thu, 27 Aug 2026 13:26:28 -0300 Subject: [PATCH 07/10] fix(runtime): free the adapter's wrapper claim after isolate teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adapter released after its isolate died skipped the whole cleanup block and orphaned its attached wrapper — one 48-byte leak per adapter on every worker teardown. With the isolate gone the JS object and every other reader or deleter of the claim are gone too (all IsValid-gated), so the owner can free it unconditionally. --- NativeScript/runtime/ArrayAdapter.mm | 8 ++++++++ NativeScript/runtime/DictionaryAdapter.mm | 8 ++++++++ NativeScript/runtime/NSDataAdapter.mm | 8 ++++++++ 3 files changed, 24 insertions(+) diff --git a/NativeScript/runtime/ArrayAdapter.mm b/NativeScript/runtime/ArrayAdapter.mm index 904ace2c..5f3230d1 100644 --- a/NativeScript/runtime/ArrayAdapter.mm +++ b/NativeScript/runtime/ArrayAdapter.mm @@ -129,6 +129,14 @@ - (void)dealloc { dataWrapper_ = nullptr; } self->object_->Reset(); + } else if (dataWrapper_ != nullptr) { + // The isolate is gone, and with it the JS object and every other reader + // or deleter of the claim (all IsValid-gated): an attached claim only + // ever exists on a plain, never-registered object no finalizer visits, + // so the owner frees it here — adapters released after a worker isolate's + // teardown otherwise leak one wrapper each. + delete dataWrapper_; + dataWrapper_ = nullptr; } delete wrapper_; self->object_ = nullptr; diff --git a/NativeScript/runtime/DictionaryAdapter.mm b/NativeScript/runtime/DictionaryAdapter.mm index 60ac68ee..bee227d6 100644 --- a/NativeScript/runtime/DictionaryAdapter.mm +++ b/NativeScript/runtime/DictionaryAdapter.mm @@ -376,6 +376,14 @@ - (void)dealloc { // vended by -keyEnumerator hold this adapter alive, so nothing can be // reading the handle by the time this runs. self->object_->Reset(); + } else if (dataWrapper_ != nullptr) { + // The isolate is gone, and with it the JS object and every other reader + // or deleter of the claim (all IsValid-gated): an attached claim only + // ever exists on a plain, never-registered object no finalizer visits, + // so the owner frees it here — adapters released after a worker isolate's + // teardown otherwise leak one wrapper each. + delete dataWrapper_; + dataWrapper_ = nullptr; } self->object_ = nullptr; delete self->wrapper_; diff --git a/NativeScript/runtime/NSDataAdapter.mm b/NativeScript/runtime/NSDataAdapter.mm index fdb49d1c..8d00b233 100644 --- a/NativeScript/runtime/NSDataAdapter.mm +++ b/NativeScript/runtime/NSDataAdapter.mm @@ -119,6 +119,14 @@ - (void)dealloc { dataWrapper_ = nullptr; } self->object_->Reset(); + } else if (dataWrapper_ != nullptr) { + // The isolate is gone, and with it the JS object and every other reader + // or deleter of the claim (all IsValid-gated): an attached claim only + // ever exists on a plain, never-registered object no finalizer visits, + // so the owner frees it here — adapters released after a worker isolate's + // teardown otherwise leak one wrapper each. + delete dataWrapper_; + dataWrapper_ = nullptr; } delete self->wrapper_; From 668bba9938c7b4505bb76e228fc3c3e004465448 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Thu, 27 Aug 2026 13:53:33 -0300 Subject: [PATCH 08/10] fix(runtime): let the JSBlock own the block cache attached to its function Passing a JS function as a block argument caches a BlockWrapper on the function so repeat calls reuse the same block. That wrapper was freed only from the JSBlock dispose helper, and only by looking it up through the cached function -- which needs a live isolate. A block built on a worker isolate that outlives it, or released after it, skipped that branch entirely and orphaned the wrapper: two 48-byte tns::BlockWrapper leaks per TestRunner run, both allocated under WorkerWrapper::BackgroundLooper (NativeCallbackWorker and TeardownCrashWorker install an NSNotificationCenter observer block at module load). The block now carries the wrapper pointer, so disposal frees it without the isolate, and native code holding the block keeps the wrapper reachable in the meantime. While the isolate is alive the wrapper is freed only when the function's slot still points at it, the same ownership rule ObjectManager::DisposeValue applies -- __releaseNativeCounterpart can retire the same wrapper first. Disposal stays inline and callback_->Reset() stays unconditional. --- NativeScript/runtime/Interop.h | 5 +++++ NativeScript/runtime/Interop.mm | 17 +++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/NativeScript/runtime/Interop.h b/NativeScript/runtime/Interop.h index db7bd20f..535f2959 100644 --- a/NativeScript/runtime/Interop.h +++ b/NativeScript/runtime/Interop.h @@ -213,6 +213,11 @@ class Interop { JSBlockDescriptor* descriptor; void* userData; ffi_closure* ffiClosure; + // The wrapper caching this block on the JS function it was built from. It + // is owned here rather than through that function: the cache slot lives in + // a V8 heap that can be torn down (a worker isolate) while the block is + // still referenced by native code. + BlockWrapper* blockWrapper; static JSBlockDescriptor kJSBlockDescriptor; } JSBlock; diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index 10d2bac2..572fba6c 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -38,6 +38,7 @@ [](JSBlock* block) { if (block->descriptor == &JSBlock::kJSBlockDescriptor) { MethodCallbackWrapper* wrapper = static_cast(block->userData); + BlockWrapper* blockWrapper = block->blockWrapper; // 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, @@ -50,16 +51,22 @@ 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; + // The callback's slot is the cache's owner, so only a wrapper + // still sitting in it is ours to free. + if (tns::GetValue(isolate, callback) == blockWrapper) { + tns::DeleteValue(isolate, callback); + } else { + blockWrapper = nullptr; + } } // 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(); } + // Outside the isolate guard: once the isolate is gone the cache + // slot is unreachable and nothing else can free the wrapper. + delete blockWrapper; delete wrapper; ffi_closure_free(block->ffiClosure); block->~JSBlock(); @@ -109,6 +116,7 @@ .descriptor = &JSBlock::kJSBlockDescriptor, .userData = userData, .ffiClosure = result.second, + .blockWrapper = nullptr, }; object_setClass((__bridge id)blockPointer, objc_getClass("__NSMallocBlock__")); @@ -539,6 +547,7 @@ inline bool isBool() { userData); BlockWrapper* wrapper = new BlockWrapper((void*)blockPtr, blockTypeEncoding, false); + reinterpret_cast((void*)blockPtr)->blockWrapper = wrapper; tns::SetValue(isolate, arg.As(), wrapper); } From efcec448b604ee4f31efa286f87c9eb683bdd3d9 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Thu, 27 Aug 2026 14:17:48 -0300 Subject: [PATCH 09/10] docs(runtime): document the known losing-emplace leak in isImplementedInClass Freeing the loser is unsafe (never-initialized instance of an arbitrary class, arbitrary thread) and parking it merely converts the leak into perpetual retention; the leak stays, visible and explained, tracked by issue #459. --- NativeScript/runtime/Metadata.mm | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/NativeScript/runtime/Metadata.mm b/NativeScript/runtime/Metadata.mm index 65763ce0..3f2b9317 100644 --- a/NativeScript/runtime/Metadata.mm +++ b/NativeScript/runtime/Metadata.mm @@ -106,6 +106,11 @@ static UInt8 getSystemVersion() { @try { id instance = [klass alloc]; std::lock_guard lock(sampleInstancesMutex); + // A losing emplace (a +initialize re-entry or another thread populated + // the entry first) LEAKS `instance`, knowingly: it was never init'd, so + // releasing it would run -dealloc against zero-filled ivars of an + // arbitrary class on an arbitrary thread. + // https://github.com/NativeScript/ios/issues/459 sampleInstance = sampleInstances.emplace(klass, instance).first->second; } @catch (id err) { return false; From 2891f93028cfa5c661ec5163653c7f35450a5496 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Thu, 27 Aug 2026 14:30:53 -0300 Subject: [PATCH 10/10] fix(runtime): make the adapter the sole owner of its wrapper claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit __releaseNativeCounterpart could delete an adapter's attached claim while a native reference kept the adapter alive; if the isolate then died before the adapter's -dealloc, the teardown branch freed the stale pointer again. Claims are now marked and retirement paths leave them attached — the only deleter is the adapter's own -dealloc, in either isolate state. --- NativeScript/runtime/ArrayAdapter.mm | 17 ++++++++++------- NativeScript/runtime/DataWrapper.h | 8 ++++++++ NativeScript/runtime/DictionaryAdapter.mm | 17 ++++++++++------- NativeScript/runtime/NSDataAdapter.mm | 17 ++++++++++------- NativeScript/runtime/ObjectManager.mm | 12 ++++++++++-- 5 files changed, 48 insertions(+), 23 deletions(-) diff --git a/NativeScript/runtime/ArrayAdapter.mm b/NativeScript/runtime/ArrayAdapter.mm index 5f3230d1..2dfbdb9e 100644 --- a/NativeScript/runtime/ArrayAdapter.mm +++ b/NativeScript/runtime/ArrayAdapter.mm @@ -12,8 +12,10 @@ @implementation ArrayAdapter { IsolateWrapper* wrapper_; std::shared_ptr> object_; // The wrapper this adapter attached to the JS object, or nullptr when the - // field was already taken. Ownership lives with the field, not with this - // pointer: it is only the claim used to recognise our own wrapper there. + // field was already taken. The adapter owns the claim exclusively -- + // retirement paths leave adapter claims attached -- so -dealloc frees it in + // both isolate states; the field compare below guards the isolate-alive + // path against a slot someone else overwrote. ObjCDataWrapper* dataWrapper_; } @@ -27,6 +29,7 @@ - (instancetype)initWithJSObject:(Local)jsObject isolate:(Isolate*)isola // and never writes or clears it; it still reads the object through object_. if (tns::GetValue(isolate, jsObject) == nullptr) { self->dataWrapper_ = new ObjCDataWrapper(self); + self->dataWrapper_->MarkAdapterClaim(); tns::SetValue(isolate, jsObject, self->dataWrapper_); } } @@ -130,11 +133,11 @@ - (void)dealloc { } self->object_->Reset(); } else if (dataWrapper_ != nullptr) { - // The isolate is gone, and with it the JS object and every other reader - // or deleter of the claim (all IsValid-gated): an attached claim only - // ever exists on a plain, never-registered object no finalizer visits, - // so the owner frees it here — adapters released after a worker isolate's - // teardown otherwise leak one wrapper each. + // The isolate is gone, and with it the JS object and every reader of the + // claim; no other path deletes one (__releaseNativeCounterpart leaves + // adapter claims attached), so the owner frees it here — adapters + // released after a worker isolate's teardown otherwise leak one wrapper + // each. delete dataWrapper_; dataWrapper_ = nullptr; } diff --git a/NativeScript/runtime/DataWrapper.h b/NativeScript/runtime/DataWrapper.h index a72d37db..73e8f59a 100644 --- a/NativeScript/runtime/DataWrapper.h +++ b/NativeScript/runtime/DataWrapper.h @@ -375,6 +375,13 @@ class ObjCDataWrapper : public BaseDataWrapper { id Data() { return this->data_; } + // True for the claim a collection adapter attaches to the plain JS object it + // was built from. The adapter owns that claim exclusively -- it must stay + // deletable from the adapter's -dealloc even after isolate teardown -- so no + // other retirement path may free it. + bool IsAdapterClaim() { return this->adapterClaim_; } + void MarkAdapterClaim() { this->adapterClaim_ = true; } + const TypeEncoding* TypeEncoding() { return this->typeEncoding_; } // The class Data() had when this wrapper was built. Data() alone cannot tell @@ -383,6 +390,7 @@ class ObjCDataWrapper : public BaseDataWrapper { Class Klass() { return this->klass_; } private: + bool adapterClaim_ = false; id data_; const tns::TypeEncoding* typeEncoding_; Class klass_; diff --git a/NativeScript/runtime/DictionaryAdapter.mm b/NativeScript/runtime/DictionaryAdapter.mm index bee227d6..67a66606 100644 --- a/NativeScript/runtime/DictionaryAdapter.mm +++ b/NativeScript/runtime/DictionaryAdapter.mm @@ -221,8 +221,10 @@ @implementation DictionaryAdapter { IsolateWrapper* wrapper_; std::shared_ptr> object_; // The wrapper this adapter attached to the JS object, or nullptr when the - // field was already taken. Ownership lives with the field, not with this - // pointer: it is only the claim used to recognise our own wrapper there. + // field was already taken. The adapter owns the claim exclusively -- + // retirement paths leave adapter claims attached -- so -dealloc frees it in + // both isolate states; the field compare below guards the isolate-alive + // path against a slot someone else overwrote. ObjCDataWrapper* dataWrapper_; } @@ -236,6 +238,7 @@ - (instancetype)initWithJSObject:(Local)jsObject isolate:(Isolate*)isola // and never writes or clears it; it still reads the object through object_. if (tns::GetValue(isolate, jsObject) == nullptr) { self->dataWrapper_ = new ObjCDataWrapper(self); + self->dataWrapper_->MarkAdapterClaim(); tns::SetValue(isolate, jsObject, self->dataWrapper_); } } @@ -377,11 +380,11 @@ - (void)dealloc { // reading the handle by the time this runs. self->object_->Reset(); } else if (dataWrapper_ != nullptr) { - // The isolate is gone, and with it the JS object and every other reader - // or deleter of the claim (all IsValid-gated): an attached claim only - // ever exists on a plain, never-registered object no finalizer visits, - // so the owner frees it here — adapters released after a worker isolate's - // teardown otherwise leak one wrapper each. + // The isolate is gone, and with it the JS object and every reader of the + // claim; no other path deletes one (__releaseNativeCounterpart leaves + // adapter claims attached), so the owner frees it here — adapters + // released after a worker isolate's teardown otherwise leak one wrapper + // each. delete dataWrapper_; dataWrapper_ = nullptr; } diff --git a/NativeScript/runtime/NSDataAdapter.mm b/NativeScript/runtime/NSDataAdapter.mm index 8d00b233..2b2c6972 100644 --- a/NativeScript/runtime/NSDataAdapter.mm +++ b/NativeScript/runtime/NSDataAdapter.mm @@ -9,8 +9,10 @@ @implementation NSDataAdapter { IsolateWrapper* wrapper_; // The wrapper this adapter attached to the JS object, or nullptr when the - // field was already taken. Ownership lives with the field, not with this - // pointer: it is only the claim used to recognise our own wrapper there. + // field was already taken. The adapter owns the claim exclusively -- + // retirement paths leave adapter claims attached -- so -dealloc frees it in + // both isolate states; the field compare below guards the isolate-alive + // path against a slot someone else overwrote. ObjCDataWrapper* dataWrapper_; std::shared_ptr> object_; // Pins the bytes for the adapter's lifetime, which is the NSData contract @@ -69,6 +71,7 @@ - (instancetype)initWithJSObject:(Local)jsObject isolate:(Isolate*)isola // and never writes or clears it; it still reads the object through object_. if (tns::GetValue(isolate, jsObject) == nullptr) { self->dataWrapper_ = new ObjCDataWrapper(self); + self->dataWrapper_->MarkAdapterClaim(); tns::SetValue(isolate, jsObject, self->dataWrapper_); } } @@ -120,11 +123,11 @@ - (void)dealloc { } self->object_->Reset(); } else if (dataWrapper_ != nullptr) { - // The isolate is gone, and with it the JS object and every other reader - // or deleter of the claim (all IsValid-gated): an attached claim only - // ever exists on a plain, never-registered object no finalizer visits, - // so the owner frees it here — adapters released after a worker isolate's - // teardown otherwise leak one wrapper each. + // The isolate is gone, and with it the JS object and every reader of the + // claim; no other path deletes one (__releaseNativeCounterpart leaves + // adapter claims attached), so the owner frees it here — adapters + // released after a worker isolate's teardown otherwise leak one wrapper + // each. delete dataWrapper_; dataWrapper_ = nullptr; } diff --git a/NativeScript/runtime/ObjectManager.mm b/NativeScript/runtime/ObjectManager.mm index ba791b1e..2c12bc33 100644 --- a/NativeScript/runtime/ObjectManager.mm +++ b/NativeScript/runtime/ObjectManager.mm @@ -390,11 +390,19 @@ void DisposeHandle(v8::Isolate* isolate, // NSNotificationCenter observer token) the remaining owners keep it alive. // Calling [data dealloc] here, as this used to do, destroyed objects that // were still referenced elsewhere and caused use-after-free crashes. + // Read before the release below: an adapter claim's -dealloc frees the + // wrapper, so it must not be touched afterwards. + bool adapterClaim = objcWrapper->IsAdapterClaim(); + [data release]; // The release above can run a -dealloc that detaches this wrapper; the - // internal field owns it, so free only what is still attached. - if (tns::GetValue(isolate, value) == wrapper) { + // internal field owns it, so free only what is still attached. An + // adapter's claim is exempt: the adapter owns it exclusively and deletes + // it from its own -dealloc even after isolate teardown, so retiring it + // here would leave the adapter holding a stale pointer it later frees + // again. + if (!adapterClaim && tns::GetValue(isolate, value) == wrapper) { delete wrapper; tns::SetValue(isolate, value.As(), nullptr); }