diff --git a/NativeScript/runtime/ArrayAdapter.mm b/NativeScript/runtime/ArrayAdapter.mm index ed7d10f3..2dfbdb9e 100644 --- a/NativeScript/runtime/ArrayAdapter.mm +++ b/NativeScript/runtime/ArrayAdapter.mm @@ -11,7 +11,11 @@ @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. 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_; } @@ -19,8 +23,15 @@ - (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); + self->dataWrapper_->MarkAdapterClaim(); + tns::SetValue(isolate, jsObject, self->dataWrapper_); + } } return self; @@ -107,23 +118,30 @@ - (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) { + } else if (dataWrapper_ != nullptr) { + // 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; } + delete wrapper_; self->object_ = nullptr; [super dealloc]; } 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 3cc5a6f0..67a66606 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,11 @@ - (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. 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_; } @@ -215,8 +232,15 @@ - (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); + self->dataWrapper_->MarkAdapterClaim(); + tns::SetValue(isolate, jsObject, self->dataWrapper_); + } } return self; @@ -321,16 +345,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 +362,31 @@ - (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) { + // 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(); + } else if (dataWrapper_ != nullptr) { + // 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; } self->object_ = nullptr; delete self->wrapper_; 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 d56a974d..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); } @@ -1675,7 +1684,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); } 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; diff --git a/NativeScript/runtime/NSDataAdapter.mm b/NativeScript/runtime/NSDataAdapter.mm index 5176fe60..2b2c6972 100644 --- a/NativeScript/runtime/NSDataAdapter.mm +++ b/NativeScript/runtime/NSDataAdapter.mm @@ -8,8 +8,29 @@ @implementation NSDataAdapter { IsolateWrapper* wrapper_; + // The wrapper this adapter attached to the JS object, or nullptr when the + // 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 + // 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_; + // 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_; + // 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_; } - (instancetype)initWithJSObject:(Local)jsObject isolate:(Isolate*)isolate { @@ -19,8 +40,40 @@ - (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_; + self->storeOffset_ = 0; + self->heapCopy_ = nullptr; + if (jsObject->IsArrayBuffer()) { + Local buffer = jsObject.As(); + self->store_ = buffer->GetBackingStore(); + self->length_ = buffer->ByteLength(); + } else if (jsObject->IsSharedArrayBuffer()) { + 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(); + } 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 + // 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); + self->dataWrapper_->MarkAdapterClaim(); + tns::SetValue(isolate, jsObject, self->dataWrapper_); + } } return self; @@ -31,53 +84,21 @@ - (const void*)bytes { } - (void*)mutableBytes { - if (!wrapper_->IsValid()) { - return nil; - } - 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()); + // 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(); if (data == nullptr) { return nullptr; } - - return data + bufferView->ByteOffset(); + return static_cast(data) + storeOffset_; } - - size_t length = bufferView->ByteLength(); - void* data = malloc(length); - bufferView->CopyContents(data, length); - - return data; + 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 { @@ -87,25 +108,34 @@ - (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) { + } else if (dataWrapper_ != nullptr) { + // 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; } delete self->wrapper_; self->object_ = nullptr; + free(self->heapCopy_); + self->heapCopy_ = nullptr; [super dealloc]; } diff --git a/NativeScript/runtime/ObjectManager.mm b/NativeScript/runtime/ObjectManager.mm index c683ed9a..2c12bc33 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; } @@ -385,10 +390,22 @@ 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]; - 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. 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); + } } } 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] == '~') { 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 }); +};