Skip to content

fix(ios): guard plugin event listener state against concurrent access #8635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
OrLeib wants to merge 1 commit into
base: main
Choose a base branch
from
+226 −42
Open

fix(ios): guard plugin event listener state against concurrent access #8635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ios/Capacitor/Capacitor.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
A38C3D7B2848BE6F004B3680 /* CapacitorCookieManager.swift in Sources */ = {isa = PBXBuildFile; fileRef = A38C3D7A2848BE6F004B3680 /* CapacitorCookieManager.swift */; };
A71289E627F380A500DADDF3 /* Router.swift in Sources */ = {isa = PBXBuildFile; fileRef = A71289E527F380A500DADDF3 /* Router.swift */; };
A71289EB27F380FD00DADDF3 /* RouterTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A71289EA27F380FD00DADDF3 /* RouterTests.swift */; };
8157E0A22F0A000000C0FFEE /* PluginEventListenerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8157E0A12F0A000000C0FFEE /* PluginEventListenerTests.swift */; };
AA01F00D0000000000000002 /* HttpInterceptorNavigationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = AA01F00D0000000000000001 /* HttpInterceptorNavigationTests.swift */; };
A7187FD22BD1CB7D00093C45 /* CAPPluginMethod.swift in Sources */ = {isa = PBXBuildFile; fileRef = A7187FD12BD1CB7D00093C45 /* CAPPluginMethod.swift */; };
A76739792B98E09700795F7B /* PrivacyInfo.xcprivacy in Resources */ = {isa = PBXBuildFile; fileRef = A76739782B98E09700795F7B /* PrivacyInfo.xcprivacy */; };
Expand Down Expand Up @@ -249,6 +250,7 @@
A38C3D7A2848BE6F004B3680 /* CapacitorCookieManager.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CapacitorCookieManager.swift; sourceTree = "<group>"; };
A71289E527F380A500DADDF3 /* Router.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Router.swift; sourceTree = "<group>"; };
A71289EA27F380FD00DADDF3 /* RouterTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RouterTests.swift; sourceTree = "<group>"; };
8157E0A12F0A000000C0FFEE /* PluginEventListenerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PluginEventListenerTests.swift; sourceTree = "<group>"; };
AA01F00D0000000000000001 /* HttpInterceptorNavigationTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HttpInterceptorNavigationTests.swift; sourceTree = "<group>"; };
A7187FD12BD1CB7D00093C45 /* CAPPluginMethod.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CAPPluginMethod.swift; sourceTree = "<group>"; };
A76739782B98E09700795F7B /* PrivacyInfo.xcprivacy */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.xml; path = PrivacyInfo.xcprivacy; sourceTree = "<group>"; };
Expand Down Expand Up @@ -353,6 +355,7 @@
62959BBD2526510200A3D7F1 /* Info.plist */,
A71289EA27F380FD00DADDF3 /* RouterTests.swift */,
AA01F00D0000000000000001 /* HttpInterceptorNavigationTests.swift */,
8157E0A12F0A000000C0FFEE /* PluginEventListenerTests.swift */,
);
path = CapacitorTests;
sourceTree = "<group>";
Expand Down Expand Up @@ -781,6 +784,7 @@
621ECCC3254204B700D3D615 /* BridgedTypesTests.swift in Sources */,
A71289EB27F380FD00DADDF3 /* RouterTests.swift in Sources */,
AA01F00D0000000000000002 /* HttpInterceptorNavigationTests.swift in Sources */,
8157E0A22F0A000000C0FFEE /* PluginEventListenerTests.swift in Sources */,
6263686025F6EC0100576C1C /* PluginCallAccessorTests.m in Sources */,
);
runOnlyForDeploymentPostprocessing = 0;
Expand Down
97 changes: 55 additions & 42 deletions ios/Capacitor/Capacitor/CAPPlugin.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,15 @@
#import "CAPBridgedJSTypes.h"
#import <Capacitor/Capacitor-Swift.h>
#import <Foundation/Foundation.h>
#import <os/lock.h>

@implementation CAPPlugin
@implementation CAPPlugin {
// Guards eventListeners and retainedEventArguments, which are read and mutated from
// the plugin's dispatch queue (addListener/removeListener) and from any thread that
// calls notifyListeners. Zero-initialized memory is a valid unlocked os_unfair_lock,
// so this works regardless of which initializer created the plugin.
os_unfair_lock _eventListenersLock;
}

-(instancetype) initWithBridge:(id<CAPBridgeProtocol>)bridge pluginId:(NSString *)pluginId pluginName:(NSString *)pluginName {
self.bridge = bridge;
Expand Down Expand Up @@ -40,65 +47,69 @@ -(PluginConfig*)getConfig {
-(void)load {}

- (void)addEventListener:(NSString *)eventName listener:(CAPPluginCall *)listener {
NSArray *retained = nil;
os_unfair_lock_lock(&_eventListenersLock);
NSMutableArray *listenersForEvent = [self.eventListeners objectForKey:eventName];
if(listenersForEvent == nil || [listenersForEvent count] == 0) {
listenersForEvent = [[NSMutableArray alloc] initWithObjects:listener, nil];
[self.eventListeners setValue:listenersForEvent forKey:eventName];

[self sendRetainedArgumentsForEvent:eventName];
retained = [self takeRetainedArgumentsForEvent:eventName];
} else {
[listenersForEvent addObject:listener];
}
os_unfair_lock_unlock(&_eventListenersLock);

// Deliver outside of the lock, since listeners may call back into this plugin
for(id data in retained) {
[self notifyListeners:eventName data:data];
}
}

- (void)sendRetainedArgumentsForEvent:(NSString *)eventName {
// copy retained args and null source to prevent potential race conditions
NSMutableArray *retained = [self.retainedEventArguments objectForKey:eventName];
if (retained == nil) {
return;
}

// Must be called while holding _eventListenersLock
- (NSArray *)takeRetainedArgumentsForEvent:(NSString *)eventName {
NSArray *retained = [self.retainedEventArguments objectForKey:eventName];
if (retained != nil) {
[self.retainedEventArguments removeObjectForKey:eventName];

for(id data in retained) {
[self notifyListeners:eventName data:data];
}
}
return retained;
}

- (void)removeEventListener:(NSString *)eventName listener:(CAPPluginCall *)listener {
os_unfair_lock_lock(&_eventListenersLock);
NSMutableArray *listenersForEvent = [self.eventListeners objectForKey:eventName];
if(!listenersForEvent) { return; }
NSUInteger listenerIndex = [listenersForEvent indexOfObject:listener];
if(listenerIndex == NSNotFound) {
return;
NSUInteger listenerIndex = listenersForEvent ? [listenersForEvent indexOfObject:listener] : NSNotFound;
if(listenerIndex != NSNotFound) {
[listenersForEvent removeObjectAtIndex:listenerIndex];
}
[listenersForEvent removeObjectAtIndex:listenerIndex];
os_unfair_lock_unlock(&_eventListenersLock);
}

- (void)notifyListeners:(NSString *)eventName data:(NSDictionary<NSString *,id> *)data {
[self notifyListeners:eventName data:data retainUntilConsumed:NO];
}

- (void)notifyListeners:(NSString *)eventName data:(NSDictionary<NSString *,id> *)data retainUntilConsumed:(BOOL)retain {
NSArray<CAPPluginCall *> *listenersForEvent = [self.eventListeners objectForKey:eventName];
if(listenersForEvent == nil || [listenersForEvent count] == 0) {
if (retain == YES) {

if ([self.retainedEventArguments objectForKey:eventName] == nil) {
[self.retainedEventArguments setObject:[[NSMutableArray alloc] init] forKey:eventName];
}

[[self.retainedEventArguments objectForKey:eventName] addObject:data];
NSArray<CAPPluginCall *> *listenersForEvent = nil;
os_unfair_lock_lock(&_eventListenersLock);
NSArray<CAPPluginCall *> *currentListeners = [self.eventListeners objectForKey:eventName];
if(currentListeners == nil || [currentListeners count] == 0) {
if (retain == YES && data != nil) {
NSMutableArray *retained = [self.retainedEventArguments objectForKey:eventName];
if (retained == nil) {
retained = [[NSMutableArray alloc] init];
[self.retainedEventArguments setObject:retained forKey:eventName];
}
[retained addObject:data];
}
return;
} else {
// Snapshot the listeners so they can be called without holding the lock
listenersForEvent = [currentListeners copy];
}
os_unfair_lock_unlock(&_eventListenersLock);

for (int i=0; i < listenersForEvent.count; i++) {
CAPPluginCall *call = listenersForEvent[i];
if (call != nil) {
CAPPluginCallResult *result = [[CAPPluginCallResult alloc] init:data];
call.successHandler(result, call);
}
for (CAPPluginCall *call in listenersForEvent) {
CAPPluginCallResult *result = [[CAPPluginCallResult alloc] init:data];
call.successHandler(result, call);
}
}

Expand All @@ -117,22 +128,24 @@ - (void)removeListener:(CAPPluginCall *)call {
}

- (void)removeAllListeners:(CAPPluginCall *)call {
os_unfair_lock_lock(&_eventListenersLock);
[self.eventListeners removeAllObjects];
os_unfair_lock_unlock(&_eventListenersLock);
[call resolve];
}

- (NSArray<CAPPluginCall *>*)getListeners:(NSString *)eventName {
NSArray<CAPPluginCall *>* listeners = [self.eventListeners objectForKey:eventName];
os_unfair_lock_lock(&_eventListenersLock);
NSArray<CAPPluginCall *>* listeners = [[self.eventListeners objectForKey:eventName] copy];
os_unfair_lock_unlock(&_eventListenersLock);
return listeners;
}

- (BOOL)hasListeners:(NSString *)eventName {
NSArray<CAPPluginCall *>* listeners = [self.eventListeners objectForKey:eventName];

if (listeners == nil) {
return false;
}
return [listeners count] > 0;
os_unfair_lock_lock(&_eventListenersLock);
BOOL hasListeners = [[self.eventListeners objectForKey:eventName] count] > 0;
os_unfair_lock_unlock(&_eventListenersLock);
return hasListeners;
}

- (void)checkPermissions:(CAPPluginCall *)call {
Expand Down
167 changes: 167 additions & 0 deletions ios/Capacitor/CapacitorTests/PluginEventListenerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
import XCTest
@testable import Capacitor

class PluginEventListenerTests: XCTestCase {
// XCTest creates a new test case instance per test method, so each test gets a fresh plugin
private let plugin: CAPPlugin = {
let plugin = CAPPlugin()
plugin.eventListeners = [:]
plugin.retainedEventArguments = [:]
return plugin
}()

private func makeListener(
_ callbackId: String,
eventName: String = "event",
onEvent: @escaping (CAPPluginCallResult?, CAPPluginCall?) -> Void = { _, _ in }
) -> CAPPluginCall {
CAPPluginCall(
callbackId: callbackId,
methodName: "addListener",
options: ["eventName": eventName],
success: onEvent,
error: { _ in }
)
}

// Runs `body` off the main thread and fails instead of hanging if it deadlocks.
private func assertCompletes(_ description: String, _ body: @escaping () -> Void) {
let done = expectation(description: description)
DispatchQueue.global().async {
body()
done.fulfill()
}
wait(for: [done], timeout: 5)
}

// https://github.com/ionic-team/capacitor/issues/8157
// Listeners are added and removed on the plugin queue while plugins notify from any thread.
func testConcurrentListenerMutationAndNotifyIsThreadSafe() {
let plugin = self.plugin
let listeners = (0..<64).map { makeListener("listener-\($0)") }

assertCompletes("concurrent access") {
DispatchQueue.concurrentPerform(iterations: 20_000) { iteration in
let listener = listeners[iteration % listeners.count]
switch iteration % 8 {
case 0, 1:
plugin.addEventListener("event", listener: listener)
case 2:
plugin.removeEventListener("event", listener: listener)
case 3:
plugin.notifyListeners("event", data: ["iteration": iteration])
case 4:
plugin.notifyListeners("event", data: ["iteration": iteration], retainUntilConsumed: true)
case 5:
_ = plugin.hasListeners("event")
_ = plugin.getListeners("event")?.count
case 6:
plugin.removeEventListener("event", listener: listeners[(iteration / 8) % listeners.count])
default:
if iteration % 1_000 == 7 {
plugin.removeAllListeners(self.makeListener("clear-\(iteration)"))
} else {
plugin.notifyListeners("event", data: nil)
}
}
}
}
}

func testRetainedEventsAreDeliveredOnceInOrderToFirstListener() {
for index in 0..<5 {
plugin.notifyListeners("event", data: ["index": index], retainUntilConsumed: true)
}

var firstReceived: [Int] = []
plugin.addEventListener("event", listener: makeListener("first") { result, _ in
firstReceived.append(result?.data?["index"] as? Int ?? -1)
})
XCTAssertEqual(firstReceived, [0, 1, 2, 3, 4])
XCTAssertNil(plugin.retainedEventArguments?["event"])

var secondReceived: [Int] = []
plugin.addEventListener("event", listener: makeListener("second") { result, _ in
secondReceived.append(result?.data?["index"] as? Int ?? -1)
})
XCTAssertEqual(secondReceived, [])

plugin.notifyListeners("event", data: ["index": 5], retainUntilConsumed: true)
XCTAssertEqual(firstReceived, [0, 1, 2, 3, 4, 5])
XCTAssertEqual(secondReceived, [5])
XCTAssertNil(plugin.retainedEventArguments?["event"])
}

func testRetainedEventsAreDeliveredAgainAfterAllListenersAreRemoved() {
let first = makeListener("first")
plugin.addEventListener("event", listener: first)
plugin.removeEventListener("event", listener: first)
plugin.notifyListeners("event", data: ["index": 1], retainUntilConsumed: true)

var received: [Int] = []
plugin.addEventListener("event", listener: makeListener("second") { result, _ in
received.append(result?.data?["index"] as? Int ?? -1)
})
XCTAssertEqual(received, [1])
}

func testListenerRemovingItselfDuringNotifyDoesNotDeadlock() {
let plugin = self.plugin
var calls = 0
let selfRemoving = makeListener("self-removing") { _, call in
calls += 1
if let call {
plugin.removeEventListener("event", listener: call)
}
}
var otherCalls = 0
plugin.addEventListener("event", listener: selfRemoving)
plugin.addEventListener("event", listener: makeListener("other") { _, _ in otherCalls += 1 })

assertCompletes("notify with self-removing listener") {
plugin.notifyListeners("event", data: [:])
plugin.notifyListeners("event", data: [:])
}
XCTAssertEqual(calls, 1)
XCTAssertEqual(otherCalls, 2)
XCTAssertEqual(plugin.getListeners("event")?.count, 1)
}

func testListenerReenteringPluginDuringRetainedDeliveryDoesNotDeadlock() {
let plugin = self.plugin
plugin.notifyListeners("event", data: ["index": 0], retainUntilConsumed: true)

var nestedReceived = 0
plugin.addEventListener("nested", listener: makeListener("nested", eventName: "nested") { _, _ in
nestedReceived += 1
})
var hadListeners = false
let reentrant = makeListener("reentrant") { _, _ in
hadListeners = plugin.hasListeners("event")
plugin.notifyListeners("nested", data: [:])
plugin.addEventListener("event", listener: self.makeListener("late"))
}

assertCompletes("add listener with retained events") {
plugin.addEventListener("event", listener: reentrant)
}
XCTAssertTrue(hadListeners)
XCTAssertEqual(nestedReceived, 1)
XCTAssertEqual(plugin.getListeners("event")?.count, 2)
}

func testGetListenersReturnsSnapshot() {
let first = makeListener("first")
plugin.addEventListener("event", listener: first)
let snapshot = plugin.getListeners("event")
plugin.addEventListener("event", listener: makeListener("second"))
plugin.removeEventListener("event", listener: first)

XCTAssertEqual(snapshot?.count, 1)
XCTAssertTrue(snapshot?.first === first)
XCTAssertEqual(plugin.getListeners("event")?.count, 1)
XCTAssertTrue(plugin.hasListeners("event"))
XCTAssertFalse(plugin.hasListeners("missing"))
XCTAssertNil(plugin.getListeners("missing"))
}
}