Skip to content

Commit

Permalink
[core] fix SharedObjectRegistry crash for accessing dict from multith…
Browse files Browse the repository at this point in the history
…reads (expo#25997)

# Why

fix the crash from https://github.com/ospfranco/expo-sqlite-benchmark

# How

the hermes gc thread may be different to js thread. we should protect
the internal data structures with thread-safety. this pr added a
DispatchQueue to protect the internal data structures.

# Test Plan

test repro on https://github.com/ospfranco/expo-sqlite-benchmark
  • Loading branch information
Kudo authored Dec 19, 2023
1 parent e67eb98 commit fe2f516
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 16 deletions.
2 changes: 2 additions & 0 deletions packages/expo-modules-core/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

### 🐛 Bug fixes

- [iOS] Fixed `SharedObjectRegistry` crash for accessing internal data structures from multi-threads. ([#25997](https://github.com/expo/expo/pull/25997) by [@kudo](https://github.com/kudo))

### 💡 Others

## 1.11.2 — 2023-12-15
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,28 +31,39 @@ public final class SharedObjectRegistry {
*/
internal static var pairs = [SharedObjectId: SharedObjectPair]()

/**
The lock queue to keep thread safety for internal data structures.
*/
private static let lockQueue = DispatchQueue(label: "expo.modules.core.SharedObjectRegistry")

/**
A number of all pairs stored in the registry.
*/
internal static var size: Int {
return pairs.count
return Self.lockQueue.sync {
return pairs.count
}
}

/**
Returns the next shared object ID and increases the counter.
*/
@discardableResult
internal static func pullNextId() -> SharedObjectId {
let id = nextId
nextId += 1
return id
return Self.lockQueue.sync {
let id = nextId
nextId += 1
return id
}
}

/**
Returns a pair of shared objects with given ID or `nil` when there is no such pair in the registry.
*/
internal static func get(_ id: SharedObjectId) -> SharedObjectPair? {
return pairs[id]
return Self.lockQueue.sync {
return pairs[id]
}
}

/**
Expand All @@ -70,7 +81,10 @@ public final class SharedObjectRegistry {
jsObject.setObjectDeallocator { delete(id) }

// Save the pair in the dictionary.
pairs[id] = (native: nativeObject, javaScript: jsObject.createWeak())
let jsWeakObject = jsObject.createWeak()
Self.lockQueue.async {
pairs[id] = (native: nativeObject, javaScript: jsWeakObject)
}

return id
}
Expand All @@ -79,12 +93,14 @@ public final class SharedObjectRegistry {
Deletes the shared objects pair with a given ID.
*/
internal static func delete(_ id: SharedObjectId) {
if let pair = pairs[id] {
// Reset an ID on the objects.
pair.native.sharedObjectId = 0

// Delete the pair from the dictionary.
pairs[id] = nil
Self.lockQueue.async {
if let pair = pairs[id] {
// Reset an ID on the objects.
pair.native.sharedObjectId = 0

// Delete the pair from the dictionary.
pairs[id] = nil
}
}
}

Expand All @@ -93,7 +109,9 @@ public final class SharedObjectRegistry {
*/
internal static func toNativeObject(_ jsObject: JavaScriptObject) -> SharedObject? {
if let objectId = try? jsObject.getProperty(sharedObjectIdPropertyName).asInt() {
return pairs[objectId]?.native
return Self.lockQueue.sync {
return pairs[objectId]?.native
}
}
return nil
}
Expand All @@ -103,7 +121,9 @@ public final class SharedObjectRegistry {
*/
internal static func toJavaScriptObject(_ nativeObject: SharedObject) -> JavaScriptObject? {
let objectId = nativeObject.sharedObjectId
return pairs[objectId]?.javaScript.lock()
return Self.lockQueue.sync {
return pairs[objectId]?.javaScript.lock()
}
}

/**
Expand All @@ -127,6 +147,8 @@ public final class SharedObjectRegistry {
}

internal static func clear() {
pairs.removeAll()
Self.lockQueue.async {
pairs.removeAll()
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ final class SharedObjectRegistrySpec: ExpoSpec {
let nativeObject = TestSharedObject()
let id = SharedObjectRegistry.add(native: nativeObject, javaScript: runtime.createObject())
SharedObjectRegistry.delete(id)
expect(nativeObject.sharedObjectId) == 0
expect(nativeObject.sharedObjectId).toEventually(equal(0))
}
}

Expand Down

0 comments on commit fe2f516

Please sign in to comment.