ionic-team / ionic-team/capacitor

[Bug]: CAPPlugin Event Handling Causing Data Races on iOS

Open
#8,157 1 comment 0 reactions 0 assignees View on GitHub
needs reproduction
Dominant language
TypeScript
Stars
16.7k
Forks
1.3k
Avg merge
3d 15h
Merged PRs (30d)
10

Description

### Capacitor Version

Capacitor 6.1.1

Though this is reported on 6.1.1 as that's the version we have in our codebase and not the latest 7.x release, as best as I can tell, the code around `CAPPlugin`'s handling of event listeners hasn't changed between those major versions, so I believe that 7.x is affected as well.

### Other API Details

```Shell

```

### Platforms Affected

- [x] iOS
- [ ] Android
- [ ] Web

### Current Behavior

We've noticed that we get crashes sometimes when sending events on the main thread. I can't share the entire stack trace, but the two frames at the top of the crash report are:

```
0 CoreFoundation 0x12c0c -[__NSDictionaryM objectForKey:] + 136
1 Capacitor 0x917c -[CAPPlugin notifyListeners:data:retainUntilConsumed:] + 83 (CAPPlugin.m:83)
```

My current working theory is that there is some mutation that's happening on a background thread at the same time as calling the plugin to send an event. The `eventListeners` property of `CAPPlugin` itself isn't guarded by a lock or any other synchronization mechanism, but it is mutated on `CapacitorBridge`'s `dispatchQueue` for the various methods that the JS side calls to add or remove a listener. However, there's no direct way to get access to that dispatch queue if I wanted to synchronize sending events to that queue, nor is the `-notifyListeners:data:retainUntilConsumed:` method run on that dispatch queue, so I've got to go through some additional steps if I want to try to enforce some data safety.

`CAPBridgeViewController`'s `bridge` property is typed as `CAPBridgeProtocol`, but `dispatchQueue` isn't a property on the protocol. I could check the type of the class returned by `bridge` and cast that so I could get access that property, but if you all change the type of the thing returned from the `bridge` property, then my potential work around to get access to `dispatchQueue` to synchronize sending events stops working.

This crash happening can be inconsistent as you might expect for a data race. I've attached a test that exhibits this behavior in the "Additional Info" section, but it can sometimes run without encountering an issue, just with the nature of this being a timing sensitive crash. Utilizing Xcode's functionality to run a test repeatedly helps here to get it to crash.

### Expected Behavior

The app would not crash.

### Project Reproduction

See "Additional Information" for a test that exhibits the issue.

### Additional Information

A test case that exercises the data race:

```swift
import Capacitor
import XCTest

final class CAPPluginSynchronizationTest: XCTestCase {
func testConcurrentAccess() {
let plugin = CAPPlugin()
// Need to assign this since it's not assigned in CAPPlugin's initializer, otherwise we're just messaging `nil` everywhere and not actually showcasing the data race.
plugin.eventListeners = [:]

let group = DispatchGroup()
let iterations = 100_000

group.enter()
Thread.detachNewThread {
for _ in 0 ..< iterations {
let call = CAPPluginCall(callbackId: UUID().uuidString) { _, _ in

} error: { _ in

}!

call.options["eventName"] = "event-name"

plugin.addListener(call)
}

group.leave()
}

group.enter()
Thread.detachNewThread {
for _ in 0 ..< iterations {
let call = CAPPluginCall(callbackId: "") { _, _ in

} error: { _ in

}!

plugin.removeAllListeners(call)
}

group.leave()
}

group.wait()
}
}
```

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.