Skip to content

Commit

Permalink
fix(bridge): Synchronize access to Strong handles in collection adapters
Browse files Browse the repository at this point in the history 
`TNSArrayAdapter`, `TNSDataAdapter` and `TNSDictionaryAdapter` hold `Strong`
references to their underlying JS objects. When the Objective-C instances of these
classes were `dealloc`-ed, these references' destructors were being called outside
of the JSC VM's lock which lead to nasty and rarely occurring crashes when such
adapters are being used in threads different than the main thread of the JSC engine.

* Do not store `ExecState`s as calls to an adapter can be made long after the JS
state has changed. Use global object's `globalExec` instead.
* Reset `Strong` handles inside the locked section. This way the C++ destructor
of a nullptr handle will not have to modify anything in the VM's heap.
  • Loading branch information
@mbektchiev
mbektchiev committed May 23, 2019
1 parent 11304ed commit b1cfb82
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 60 deletions.
46 changes: 27 additions & 19 deletions src/NativeScript/ObjC/TNSArrayAdapter.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,53 +16,57 @@

@implementation TNSArrayAdapter {
Strong<JSObject> _object;
ExecState* _execState;
VM* _vm;
JSGlobalObject* _globalObject;
}

- (instancetype)initWithJSObject:(JSObject*)jsObject execState:(ExecState*)execState {
if (self) {
self->_object = Strong<JSObject>(execState->vm(), jsObject);
self->_execState = execState;
self->_vm = &execState->vm();
[TNSRuntime runtimeForVM:self->_vm] -> _objectMap.get()->set(self, jsObject);
self->_globalObject = execState->lexicalGlobalObject();
VM& vm = execState->vm();
[TNSRuntime runtimeForVM:&vm] -> _objectMap.get()->set(self, jsObject);
}

return self;
}

- (NSUInteger)count {
RELEASE_ASSERT_WITH_MESSAGE([TNSRuntime runtimeForVM:self->_vm], "The runtime is deallocated.");
JSLockHolder lock(self->_execState);
VM& vm = self->_globalObject->vm();
RELEASE_ASSERT_WITH_MESSAGE([TNSRuntime runtimeForVM:&vm], "The runtime is deallocated.");
JSLockHolder lock(vm);

JSObject* object = self->_object.get();
if (JSArray* array = jsDynamicCast<JSArray*>(self->_execState->vm(), object)) {
if (JSArray* array = jsDynamicCast<JSArray*>(vm, object)) {
return array->length();
}

return object->get(self->_execState, self->_execState->vm().propertyNames->length).toUInt32(self->_execState);
ExecState* execState = self->_globalObject->globalExec();
return object->get(execState, vm.propertyNames->length).toUInt32(execState);
}

- (id)objectAtIndex:(NSUInteger)index {
RELEASE_ASSERT_WITH_MESSAGE([TNSRuntime runtimeForVM:self->_vm], "The runtime is deallocated.");
VM& vm = self->_globalObject->vm();
RELEASE_ASSERT_WITH_MESSAGE([TNSRuntime runtimeForVM:&vm], "The runtime is deallocated.");
if (!(index < [self count])) {
@throw [NSException exceptionWithName:NSRangeException reason:[NSString stringWithFormat:@"Index (%tu) out of bounds", index] userInfo:nil];
}

JSLockHolder lock(self->_execState);
return toObject(self->_execState, self->_object.get()->get(self->_execState, index));
JSLockHolder lock(vm);
ExecState* execState = self->_globalObject->globalExec();
return toObject(execState, self->_object.get()->get(execState, index));
}

- (NSUInteger)countByEnumeratingWithState:(NSFastEnumerationState*)state objects:(id[])buffer count:(NSUInteger)len {
RELEASE_ASSERT_WITH_MESSAGE([TNSRuntime runtimeForVM:self->_vm], "The runtime is deallocated.");
VM& vm = self->_globalObject->vm();
RELEASE_ASSERT_WITH_MESSAGE([TNSRuntime runtimeForVM:&vm], "The runtime is deallocated.");

JSLockHolder lock(self->_execState);
JSLockHolder lock(vm);
ExecState* execState = self->_globalObject->globalExec();

if (state->state == 0) { // uninitialized
state->state = 1;
state->mutationsPtr = reinterpret_cast<unsigned long*>(self);
state->extra[0] = 0; // current index
state->extra[1] = self->_object->get(self->_execState, self->_vm->propertyNames->length).toUInt32(self->_execState);
state->extra[1] = self->_object->get(execState, vm.propertyNames->length).toUInt32(execState);
}

NSUInteger currentIndex = state->extra[0];
Expand All @@ -71,7 +75,7 @@ - (NSUInteger)countByEnumeratingWithState:(NSFastEnumerationState*)state objects
state->itemsPtr = buffer;

while (count < len && currentIndex < length) {
*buffer++ = toObject(self->_execState, self->_object->get(self->_execState, currentIndex));
*buffer++ = toObject(execState, self->_object->get(execState, currentIndex));
currentIndex++;
count++;
}
Expand All @@ -83,10 +87,14 @@ - (NSUInteger)countByEnumeratingWithState:(NSFastEnumerationState*)state objects

- (void)dealloc {
{
if (TNSRuntime* runtime = [TNSRuntime runtimeForVM:self->_vm]) {
JSLockHolder lock(self->_execState);
VM& vm = self->_globalObject->vm();
JSLockHolder lock(vm);
if (TNSRuntime* runtime = [TNSRuntime runtimeForVM:&vm]) {
runtime->_objectMap.get()->remove(self);
}
// Clear Strong reference inside the locked section. Otherwise it would be done when the
// C++ members' destructros are run. This races with other threads and can corrupt the VM's heap.
self->_object.clear();
}

[super dealloc];
Expand Down
37 changes: 21 additions & 16 deletions src/NativeScript/ObjC/TNSDataAdapter.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,15 @@

@implementation TNSDataAdapter {
Strong<JSObject> _object;
ExecState* _execState;
VM* _vm;
JSGlobalObject* _globalObject;
}

- (instancetype)initWithJSObject:(JSObject*)jsObject execState:(ExecState*)execState {
if (self) {
self->_object.set(execState->vm(), jsObject);
self->_execState = execState;
self->_vm = &execState->vm();
[TNSRuntime runtimeForVM:self->_vm] -> _objectMap.get()->set(self, jsObject);
VM& vm = execState->vm();
self->_object.set(vm, jsObject);
self->_globalObject = execState->lexicalGlobalObject();
[TNSRuntime runtimeForVM:&vm] -> _objectMap.get()->set(self, jsObject);
}

return self;
Expand All @@ -37,10 +36,11 @@ - (const void*)bytes {
}

- (void*)mutableBytes {
RELEASE_ASSERT_WITH_MESSAGE([TNSRuntime runtimeForVM:self->_vm], "The runtime is deallocated.");
JSLockHolder lock(self->_execState);
VM& vm = self->_globalObject->vm();
RELEASE_ASSERT_WITH_MESSAGE([TNSRuntime runtimeForVM:&vm], "The runtime is deallocated.");
JSLockHolder lock(vm);

if (JSArrayBuffer* arrayBuffer = jsDynamicCast<JSArrayBuffer*>(self->_execState->vm(), self->_object.get())) {
if (JSArrayBuffer* arrayBuffer = jsDynamicCast<JSArrayBuffer*>(vm, self->_object.get())) {
return arrayBuffer->impl()->data();
}

Expand All @@ -53,21 +53,26 @@ - (void*)mutableBytes {
}

- (NSUInteger)length {
RELEASE_ASSERT_WITH_MESSAGE([TNSRuntime runtimeForVM:self->_vm], "The runtime is deallocated.");
VM& vm = self->_execState->vm();
JSLockHolder lock(self->_execState);
VM& vm = self->_globalObject->vm();
ExecState* execState = self->_globalObject->globalExec();
RELEASE_ASSERT_WITH_MESSAGE([TNSRuntime runtimeForVM:&vm], "The runtime is deallocated.");
JSLockHolder lock(vm);
auto scope = DECLARE_CATCH_SCOPE(vm);
NSUInteger length = self->_object->get(self->_execState, vm.propertyNames->byteLength).toUInt32(self->_execState);
reportErrorIfAny(self->_execState, scope);
NSUInteger length = self->_object->get(execState, vm.propertyNames->byteLength).toUInt32(execState);
reportErrorIfAny(execState, scope);
return length;
}

- (void)dealloc {
{
if (TNSRuntime* runtime = [TNSRuntime runtimeForVM:self->_vm]) {
JSLockHolder lock(self->_execState);
VM& vm = self->_globalObject->vm();
JSLockHolder lock(vm);
if (TNSRuntime* runtime = [TNSRuntime runtimeForVM:&vm]) {
runtime->_objectMap.get()->remove(self);
}
// Clear Strong reference inside the locked section. Otherwise it would be done when the
// C++ members' destructros are run. This races with other threads and can corrupt the VM's heap.
self->_object.clear();
}

[super dealloc];
Expand Down
70 changes: 45 additions & 25 deletions src/NativeScript/ObjC/TNSDictionaryAdapter.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,29 +22,46 @@ @interface TNSDictionaryAdapterMapKeysEnumerator : NSEnumerator

@implementation TNSDictionaryAdapterMapKeysEnumerator {
Strong<JSMapIterator> _iterator;
ExecState* _execState;
JSGlobalObject* _globalObject;
}

- (instancetype)initWithMap:(JSMap*)map execState:(ExecState*)execState {
if (self) {
_iterator.set(execState->vm(), JSMapIterator::create(execState->vm(), execState->vm().mapIteratorStructure.get(), map, JSC::IterateKey));
self->_execState = execState;
self->_globalObject = execState->lexicalGlobalObject();
VM& vm = execState->vm();
_iterator.set(vm, JSMapIterator::create(vm, vm.mapIteratorStructure.get(), map, JSC::IterateKey));
}

return self;
}

- (id)nextObject {
JSLockHolder lock(self->_execState);
JSLockHolder lock(self->_globalObject->vm());

JSValue key, value;
if (_iterator->nextKeyValue(self->_execState, key, value)) {
return toObject(_execState, key);
ExecState* exec = self->_globalObject->globalExec();
if (_iterator->nextKeyValue(exec, key, value)) {
return toObject(exec, key);
}

return nil;
}

- (void)dealloc {
{
VM& vm = self->_globalObject->vm();
JSLockHolder lock(vm);
if (TNSRuntime* runtime = [TNSRuntime runtimeForVM:&vm]) {
runtime->_objectMap.get()->remove(self);
}
// Clear Strong reference inside the locked section. Otherwise it would be done when the
// C++ members' destructros are run. This races with other threads and can corrupt the VM's heap.
self->_iterator.clear();
}

[super dealloc];
}

@end

@interface TNSDictionaryAdapterObjectKeysEnumerator : NSEnumerator
Expand Down Expand Up @@ -88,14 +105,14 @@ - (NSArray*)allObjects {

@implementation TNSDictionaryAdapter {
Strong<JSObject> _object;
ExecState* _execState;
JSGlobalObject* _globalObject;
VM* _vm;
}

- (instancetype)initWithJSObject:(JSObject*)jsObject execState:(ExecState*)execState {
if (self) {
self->_object = Strong<JSObject>(execState->vm(), jsObject);
self->_execState = execState;
self->_globalObject = execState->lexicalGlobalObject();
self->_vm = &execState->vm();
[TNSRuntime runtimeForVM:self->_vm] -> _objectMap.get()->set(self, jsObject);
}
Expand All @@ -104,57 +121,60 @@ - (instancetype)initWithJSObject:(JSObject*)jsObject execState:(ExecState*)execS
}

- (NSUInteger)count {
JSLockHolder lock(self->_execState);
JSLockHolder lock(self->_vm);

JSObject* object = self->_object.get();
if (JSMap* map = jsDynamicCast<JSMap*>(self->_execState->vm(), object)) {
if (JSMap* map = jsDynamicCast<JSMap*>(*self->_vm, object)) {
return map->size();
}

PropertyNameArray properties(&self->_execState->vm(), PropertyNameMode::Strings, PrivateSymbolMode::Include);
object->methodTable(*self->_vm)->getOwnPropertyNames(object, self->_execState, properties, EnumerationMode());
PropertyNameArray properties(self->_vm, PropertyNameMode::Strings, PrivateSymbolMode::Include);
object->methodTable(*self->_vm)->getOwnPropertyNames(object, self->_globalObject->globalExec(), properties, EnumerationMode());
return properties.size();
}

- (id)objectForKey:(id)aKey {
RELEASE_ASSERT_WITH_MESSAGE([TNSRuntime runtimeForVM:self->_vm], "The runtime is deallocated.");
JSLockHolder lock(self->_execState);
JSLockHolder lock(self->_vm);

JSObject* object = self->_object.get();
if (JSMap* map = jsDynamicCast<JSMap*>(self->_execState->vm(), object)) {
JSValue key = toValue(self->_execState, aKey);
return toObject(self->_execState, map->get(self->_execState, key));
if (JSMap* map = jsDynamicCast<JSMap*>(*self->_vm, object)) {
JSValue key = toValue(self->_globalObject->globalExec(), aKey);
return toObject(self->_globalObject->globalExec(), map->get(self->_globalObject->globalExec(), key));
} else if ([aKey isKindOfClass:[NSString class]]) {
Identifier key{ Identifier::fromString(self->_execState, WTF::String(reinterpret_cast<CFStringRef>(aKey))) };
return toObject(self->_execState, object->get(self->_execState, key));
Identifier key{ Identifier::fromString(_globalObject->globalExec(), WTF::String(reinterpret_cast<CFStringRef>(aKey))) };
return toObject(_globalObject->globalExec(), object->get(self->_globalObject->globalExec(), key));
} else if ([aKey isKindOfClass:[NSNumber class]]) {
NSUInteger key = [aKey unsignedIntegerValue];
return toObject(self->_execState, object->get(self->_execState, key));
return toObject(self->_globalObject->globalExec(), object->get(self->_globalObject->globalExec(), key));
}

return nil;
}

- (NSEnumerator*)keyEnumerator {
RELEASE_ASSERT_WITH_MESSAGE([TNSRuntime runtimeForVM:self->_vm], "The runtime is deallocated.");
JSLockHolder lock(self->_execState);
JSLockHolder lock(self->_globalObject->globalExec());

JSObject* object = self->_object.get();
if (JSMap* map = jsDynamicCast<JSMap*>(self->_execState->vm(), object)) {
return [[[TNSDictionaryAdapterMapKeysEnumerator alloc] initWithMap:map execState:self->_execState] autorelease];
if (JSMap* map = jsDynamicCast<JSMap*>(*self->_vm, object)) {
return [[[TNSDictionaryAdapterMapKeysEnumerator alloc] initWithMap:map execState:self->_globalObject->globalExec()] autorelease];
}

PropertyNameArray properties(&self->_execState->vm(), PropertyNameMode::Strings, PrivateSymbolMode::Include);
object->methodTable(*self->_vm)->getOwnPropertyNames(object, self->_execState, properties, EnumerationMode());
PropertyNameArray properties(self->_vm, PropertyNameMode::Strings, PrivateSymbolMode::Include);
object->methodTable(*self->_vm)->getOwnPropertyNames(object, self->_globalObject->globalExec(), properties, EnumerationMode());
return [[[TNSDictionaryAdapterObjectKeysEnumerator alloc] initWithProperties:properties.releaseData()] autorelease];
}

- (void)dealloc {
{
JSLockHolder lock(self->_vm);
if (TNSRuntime* runtime = [TNSRuntime runtimeForVM:self->_vm]) {
JSLockHolder lock(self->_execState);
runtime->_objectMap.get()->remove(self);
}
// Clear Strong reference inside the locked section. Otherwise it would be done when the
// C++ members' destructros are run. This races with other threads and can corrupt the VM's heap.
self->_object.clear();
}

[super dealloc];
Expand Down

0 comments on commit b1cfb82

Please sign in to comment.