From 556ab18e556ac98772743be73b39daf634dfcc85 Mon Sep 17 00:00:00 2001 From: orleib-lab Date: Tue, 29 Sep 2026 20:53:49 +0300 Subject: [PATCH] fix(ios): guard plugin event listener state against concurrent access CAPPlugin stored eventListeners and retainedEventArguments in plain NSMutableDictionary/NSMutableArray instances with no synchronization. addListener/removeListener run on the plugin's dispatch queue while plugins call notifyListeners from arbitrary threads, so concurrent reads and mutations crashed in -[__NSDictionaryM objectForKey:] inside notifyListeners:data:retainUntilConsumed:. Guard every access with an os_unfair_lock and never invoke listener callbacks while holding it: notifyListeners and retained event delivery snapshot the listeners inside the critical section and call out afterwards, so listeners can re-enter the plugin (for example remove themselves) without deadlocking. getListeners now returns a snapshot. Fixes #8157 --- .../Capacitor.xcodeproj/project.pbxproj | 4 + ios/Capacitor/Capacitor/CAPPlugin.m | 97 +++++----- .../PluginEventListenerTests.swift | 167 ++++++++++++++++++ 3 files changed, 226 insertions(+), 42 deletions(-) create mode 100644 ios/Capacitor/CapacitorTests/PluginEventListenerTests.swift diff --git a/ios/Capacitor/Capacitor.xcodeproj/project.pbxproj b/ios/Capacitor/Capacitor.xcodeproj/project.pbxproj index 1bfc57d944..72640dbb23 100644 --- a/ios/Capacitor/Capacitor.xcodeproj/project.pbxproj +++ b/ios/Capacitor/Capacitor.xcodeproj/project.pbxproj @@ -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 */; }; @@ -249,6 +250,7 @@ A38C3D7A2848BE6F004B3680 /* CapacitorCookieManager.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CapacitorCookieManager.swift; sourceTree = ""; }; A71289E527F380A500DADDF3 /* Router.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Router.swift; sourceTree = ""; }; A71289EA27F380FD00DADDF3 /* RouterTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RouterTests.swift; sourceTree = ""; }; + 8157E0A12F0A000000C0FFEE /* PluginEventListenerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PluginEventListenerTests.swift; sourceTree = ""; }; AA01F00D0000000000000001 /* HttpInterceptorNavigationTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HttpInterceptorNavigationTests.swift; sourceTree = ""; }; A7187FD12BD1CB7D00093C45 /* CAPPluginMethod.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CAPPluginMethod.swift; sourceTree = ""; }; A76739782B98E09700795F7B /* PrivacyInfo.xcprivacy */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.xml; path = PrivacyInfo.xcprivacy; sourceTree = ""; }; @@ -353,6 +355,7 @@ 62959BBD2526510200A3D7F1 /* Info.plist */, A71289EA27F380FD00DADDF3 /* RouterTests.swift */, AA01F00D0000000000000001 /* HttpInterceptorNavigationTests.swift */, + 8157E0A12F0A000000C0FFEE /* PluginEventListenerTests.swift */, ); path = CapacitorTests; sourceTree = ""; @@ -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; diff --git a/ios/Capacitor/Capacitor/CAPPlugin.m b/ios/Capacitor/Capacitor/CAPPlugin.m index afa7c9a8b2..0e55fa4d0f 100644 --- a/ios/Capacitor/Capacitor/CAPPlugin.m +++ b/ios/Capacitor/Capacitor/CAPPlugin.m @@ -2,8 +2,15 @@ #import "CAPBridgedJSTypes.h" #import #import +#import -@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)bridge pluginId:(NSString *)pluginId pluginName:(NSString *)pluginName { self.bridge = bridge; @@ -40,39 +47,41 @@ -(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 *)data { @@ -80,25 +89,27 @@ - (void)notifyListeners:(NSString *)eventName data:(NSDictionary } - (void)notifyListeners:(NSString *)eventName data:(NSDictionary *)data retainUntilConsumed:(BOOL)retain { - NSArray *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 *listenersForEvent = nil; + os_unfair_lock_lock(&_eventListenersLock); + NSArray *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); } } @@ -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*)getListeners:(NSString *)eventName { - NSArray* listeners = [self.eventListeners objectForKey:eventName]; + os_unfair_lock_lock(&_eventListenersLock); + NSArray* listeners = [[self.eventListeners objectForKey:eventName] copy]; + os_unfair_lock_unlock(&_eventListenersLock); return listeners; } - (BOOL)hasListeners:(NSString *)eventName { - NSArray* 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 { diff --git a/ios/Capacitor/CapacitorTests/PluginEventListenerTests.swift b/ios/Capacitor/CapacitorTests/PluginEventListenerTests.swift new file mode 100644 index 0000000000..fbb186a48c --- /dev/null +++ b/ios/Capacitor/CapacitorTests/PluginEventListenerTests.swift @@ -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")) + } +}