From 26b9c3d00aca75fe9b0a2e817ad3695e84f3994f Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sat, 22 Aug 2026 21:55:14 +0200 Subject: [PATCH 01/10] fix: let /status, /screenshot, and DELETE /session bypass the frozen route queue When the app under test freezes, every route handler funnels through the shared main-queue dispatch before it can run, so even side-effect-free routes like /status wait behind a stuck handler indefinitely (#1210). Add a standalone route flag that skips the shared queue: concurrent requests to the same endpoint are coalesced into one in-flight execution, everything else gets its own queue. Mark /status, /screenshot, and DELETE /session standalone. Also fixes two related hang sources found while auditing these routes for their own risk of blocking: FBTestmanagerdVersion() and stopScreenRecordingWithUUID:error: used an unbounded wait on a daemon RPC, and -kill unconditionally cleared _activeSession even if a newer session had since become active. --- .../Commands/FBScreenshotCommands.m | 4 +- .../Commands/FBSessionCommands.m | 4 +- WebDriverAgentLib/Routing/FBHTTPServer.h | 15 +++- WebDriverAgentLib/Routing/FBHTTPServer.m | 79 +++++++++++++++++-- WebDriverAgentLib/Routing/FBRoute.h | 9 +++ WebDriverAgentLib/Routing/FBRoute.m | 9 +++ WebDriverAgentLib/Routing/FBSession.m | 4 +- WebDriverAgentLib/Routing/FBWebServer.m | 2 +- .../Utilities/FBXCTestDaemonsProxy.m | 17 ++-- .../Utilities/FBXCodeCompatibility.m | 18 +++-- 10 files changed, 134 insertions(+), 27 deletions(-) diff --git a/WebDriverAgentLib/Commands/FBScreenshotCommands.m b/WebDriverAgentLib/Commands/FBScreenshotCommands.m index e2b0907223..9586a197fb 100644 --- a/WebDriverAgentLib/Commands/FBScreenshotCommands.m +++ b/WebDriverAgentLib/Commands/FBScreenshotCommands.m @@ -18,8 +18,8 @@ + (NSArray *)routes { return @[ - [[FBRoute GET:@"/screenshot"].withoutSession respondWithTarget:self action:@selector(handleGetScreenshot:)], - [[FBRoute GET:@"/screenshot"] respondWithTarget:self action:@selector(handleGetScreenshot:)], + [[FBRoute GET:@"/screenshot"].withoutSession.standalone respondWithTarget:self action:@selector(handleGetScreenshot:)], + [[FBRoute GET:@"/screenshot"].standalone respondWithTarget:self action:@selector(handleGetScreenshot:)], ]; } diff --git a/WebDriverAgentLib/Commands/FBSessionCommands.m b/WebDriverAgentLib/Commands/FBSessionCommands.m index c03cdd552d..edc04aec69 100644 --- a/WebDriverAgentLib/Commands/FBSessionCommands.m +++ b/WebDriverAgentLib/Commands/FBSessionCommands.m @@ -47,8 +47,8 @@ + (NSArray *)routes [[FBRoute POST:@"/wda/apps/state"] respondWithTarget:self action:@selector(handleSessionAppState:)], [[FBRoute GET:@"/wda/apps/list"] respondWithTarget:self action:@selector(handleGetActiveAppsList:)], [[FBRoute GET:@""] respondWithTarget:self action:@selector(handleGetActiveSession:)], - [[FBRoute DELETE:@""] respondWithTarget:self action:@selector(handleDeleteSession:)], - [[FBRoute GET:@"/status"].withoutSession respondWithTarget:self action:@selector(handleGetStatus:)], + [[FBRoute DELETE:@""].standalone respondWithTarget:self action:@selector(handleDeleteSession:)], + [[FBRoute GET:@"/status"].withoutSession.standalone respondWithTarget:self action:@selector(handleGetStatus:)], // Health check might modify simulator state so it should only be called in-between testing sessions [[FBRoute GET:@"/wda/healthcheck"].withoutSession respondWithTarget:self action:@selector(handleGetHealthCheck:)], diff --git a/WebDriverAgentLib/Routing/FBHTTPServer.h b/WebDriverAgentLib/Routing/FBHTTPServer.h index 42db7f1151..2ee07893cc 100644 --- a/WebDriverAgentLib/Routing/FBHTTPServer.h +++ b/WebDriverAgentLib/Routing/FBHTTPServer.h @@ -47,12 +47,25 @@ NS_ASSUME_NONNULL_BEGIN /** Registers a route handler for the given HTTP method and path pattern (":param" segments are - captured into the request's `params`). + captured into the request's `params`). Equivalent to -handleMethod:withPath:standalone:block: + with standalone:NO. */ - (void)handleMethod:(NSString *)method withPath:(NSString *)path block:(void (^)(RouteRequest *request, RouteResponse *response))block; +/** + Registers a route handler that, when `standalone` is YES, bypasses -routeQueue entirely so a + handler stuck on that queue can never block it. Concurrent requests to the same method+path are + coalesced into a single in-flight execution, whose response is delivered to all of them; anything + else runs on its own queue, so distinct standalone endpoints always execute in parallel with each + other and with whatever is stuck on -routeQueue. + */ +- (void)handleMethod:(NSString *)method + withPath:(NSString *)path + standalone:(BOOL)standalone + block:(void (^)(RouteRequest *request, RouteResponse *response))block; + /** Convenience for -handleMethod:@"GET" withPath:path block:block. */ diff --git a/WebDriverAgentLib/Routing/FBHTTPServer.m b/WebDriverAgentLib/Routing/FBHTTPServer.m index a8cdd3e364..77ca430530 100644 --- a/WebDriverAgentLib/Routing/FBHTTPServer.m +++ b/WebDriverAgentLib/Routing/FBHTTPServer.m @@ -35,6 +35,7 @@ @interface FBHTTPRoute : NSObject @property (nonatomic, strong) NSRegularExpression *regex; @property (nonatomic, copy, nullable) NSArray *keys; @property (nonatomic, copy) void (^block)(RouteRequest *request, RouteResponse *response); +@property (nonatomic, assign) BOOL isStandalone; @end @implementation FBHTTPRoute @@ -67,6 +68,9 @@ @interface FBHTTPServer () // Per-client cache of the already-parsed request line + headers while its body is still // arriving; nil while a client's next unread bytes start with an unparsed header block. @property (nonatomic, strong) NSMapTable *pendingRequestHeaders; +// Keyed by "METHOD path" - holds connections waiting on an already in-flight standalone request +// for that same endpoint. Guarded by @synchronized(self.standaloneWaiters). +@property (nonatomic, strong) NSMutableDictionary *standaloneWaiters; @end @@ -81,6 +85,7 @@ - (instancetype)init valueOptions:(NSPointerFunctionsOptions)NSMapTableStrongMemory]; _pendingRequestHeaders = [NSMapTable mapTableWithKeyOptions:(NSPointerFunctionsOptions)(NSMapTableObjectPointerPersonality | NSMapTableStrongMemory) valueOptions:(NSPointerFunctionsOptions)NSMapTableStrongMemory]; + _standaloneWaiters = [NSMutableDictionary dictionary]; } return self; } @@ -157,10 +162,19 @@ - (FBHTTPRoute *)compiledRouteWithPath:(NSString *)path - (void)handleMethod:(NSString *)method withPath:(NSString *)path block:(void (^)(RouteRequest *request, RouteResponse *response))block +{ + [self handleMethod:method withPath:path standalone:NO block:block]; +} + +- (void)handleMethod:(NSString *)method + withPath:(NSString *)path + standalone:(BOOL)standalone + block:(void (^)(RouteRequest *request, RouteResponse *response))block { FBHTTPRoute *route = [self compiledRouteWithPath:path]; route.verb = method.uppercaseString; route.block = block; + route.isStandalone = standalone; [self.routes addObject:route]; } @@ -281,8 +295,8 @@ - (void)processBufferForClient:(nw_connection_t)client // is rejected rather than risking the body being misread as empty and desyncing the rest // of the connection's request stream. RouteResponse *notImplemented = [RouteResponse new]; - id notImplementedPayload = FBResponseWithStatus([FBCommandStatus unknownCommandErrorWithMessage:@"Transfer-Encoding is not supported" - traceback:nil]); + id notImplementedPayload = FBResponseWithStatus([FBCommandStatus invalidArgumentErrorWithMessage:@"Transfer-Encoding is not supported" + traceback:nil]); [notImplementedPayload dispatchWithResponse:notImplemented]; [self failClient:client withResponse:notImplemented]; return; @@ -290,11 +304,10 @@ - (void)processBufferForClient:(nw_connection_t)client NSUInteger contentLength = (NSUInteger)requestHeaders[@"content-length"].integerValue; if (contentLength > FBConfiguration.sharedInstance.httpRequestBodySizeLimit) { - // Mirrors CocoaHTTPServer's maxRequestBodySize enforcement. Closes the connection after - // responding, since the rest of the oversized body is still incoming. + // Closes the connection after responding, since the rest of the oversized body is still incoming. RouteResponse *tooLarge = [RouteResponse new]; - id tooLargePayload = FBResponseWithStatus([FBCommandStatus unknownCommandErrorWithMessage:@"Request Entity Too Large" - traceback:nil]); + id tooLargePayload = FBResponseWithStatus([FBCommandStatus invalidArgumentErrorWithMessage:@"The request body exceeds the configured size limit" + traceback:nil]); [tooLargePayload dispatchWithResponse:tooLarge]; [self failClient:client withResponse:tooLarge]; return; @@ -344,8 +357,8 @@ - (void)failClient:(nw_connection_t)client withResponse:(RouteResponse *)respons - (void)respondBadRequestToClient:(nw_connection_t)client { RouteResponse *badRequest = [RouteResponse new]; - id payload = FBResponseWithStatus([FBCommandStatus unknownCommandErrorWithMessage:@"The request could not be parsed as valid HTTP" - traceback:nil]); + id payload = FBResponseWithStatus([FBCommandStatus invalidArgumentErrorWithMessage:@"The request could not be parsed as valid HTTP" + traceback:nil]); [payload dispatchWithResponse:badRequest]; [self failClient:client withResponse:badRequest]; } @@ -388,6 +401,11 @@ - (void)dispatchMethod:(NSString *)method pathAndQuery:(NSString *)pathAndQuery RouteResponse *response = [RouteResponse new]; [self applyDefaultHeadersToResponse:response]; + if (route.isStandalone) { + [self dispatchStandaloneRoute:route request:request response:response client:client method:method path:path]; + return; + } + void (^invoke)(void) = ^{ route.block(request, response); [self writeResponse:response toClient:client]; @@ -409,6 +427,51 @@ - (void)dispatchMethod:(NSString *)method pathAndQuery:(NSString *)pathAndQuery [self writeResponse:notFound toClient:client]; } +#pragma mark - Standalone route dispatch + +- (void)dispatchStandaloneRoute:(FBHTTPRoute *)route + request:(RouteRequest *)request + response:(RouteResponse *)response + client:(nw_connection_t)client + method:(NSString *)method + path:(NSString *)path +{ + NSString *key = [NSString stringWithFormat:@"%@ %@", method, path]; + BOOL isInFlight = NO; + @synchronized (self.standaloneWaiters) { + NSMutableArray *waiters = self.standaloneWaiters[key]; + if (nil != waiters) { + [waiters addObject:client]; + isInFlight = YES; + } else { + self.standaloneWaiters[key] = [NSMutableArray array]; + } + } + if (isInFlight) { + // An identical request is already executing; it will deliver this connection's response too. + return; + } + + dispatch_queue_t queue = dispatch_queue_create(key.UTF8String, DISPATCH_QUEUE_SERIAL); + __weak typeof(self) weakSelf = self; + dispatch_async(queue, ^{ + route.block(request, response); + __strong typeof(weakSelf) strongSelf = weakSelf; + if (nil == strongSelf) { + return; + } + NSArray *joinedClients; + @synchronized (strongSelf.standaloneWaiters) { + joinedClients = [strongSelf.standaloneWaiters[key] copy]; + [strongSelf.standaloneWaiters removeObjectForKey:key]; + } + [strongSelf writeResponse:response toClient:client]; + for (nw_connection_t joinedClient in joinedClients) { + [strongSelf writeResponse:response toClient:joinedClient]; + } + }); +} + - (void)writeResponse:(RouteResponse *)response toClient:(nw_connection_t)client { [self writeResponse:response toClient:client thenCloseConnection:NO]; diff --git a/WebDriverAgentLib/Routing/FBRoute.h b/WebDriverAgentLib/Routing/FBRoute.h index fce8dd8a98..35d55add21 100644 --- a/WebDriverAgentLib/Routing/FBRoute.h +++ b/WebDriverAgentLib/Routing/FBRoute.h @@ -27,6 +27,9 @@ typedef __nonnull id (^FBRouteSyncHandler)(FBRouteRequest *re /*! Route's path */ @property (nonatomic, copy, readonly) NSString *path; +/*! Whether this route bypasses the shared route queue - see -standalone */ +@property (nonatomic, assign, readonly) BOOL isStandalone; + /** Convenience constructor for GET route with given pathPattern */ @@ -67,6 +70,12 @@ typedef __nonnull id (^FBRouteSyncHandler)(FBRouteRequest *re */ - (instancetype)withoutSession; +/** + Chain-able constructor for a route that bypasses the shared route queue - see FBHTTPServer.h's + -handleMethod:withPath:standalone:block: for what that changes about how/when the handler runs. + */ +- (instancetype)standalone; + /** Dispatches response for request */ diff --git a/WebDriverAgentLib/Routing/FBRoute.m b/WebDriverAgentLib/Routing/FBRoute.m index fbe69b8c3c..47964014df 100644 --- a/WebDriverAgentLib/Routing/FBRoute.m +++ b/WebDriverAgentLib/Routing/FBRoute.m @@ -18,6 +18,7 @@ @interface FBRoute () @property (nonatomic, assign, readwrite) BOOL requiresSession; +@property (nonatomic, assign, readwrite) BOOL isStandalone; @property (nonatomic, copy, readwrite) NSString *verb; @property (nonatomic, copy, readwrite) NSString *path; @@ -126,10 +127,17 @@ - (instancetype)withoutSession return self; } +- (instancetype)standalone +{ + self.isStandalone = YES; + return self; +} + - (instancetype)respondWithBlock:(FBRouteSyncHandler)handler { FBRoute_Sync *route = [FBRoute_Sync withVerb:self.verb path:self.path requiresSession:self.requiresSession]; route.handler = handler; + route.isStandalone = self.isStandalone; return route; } @@ -138,6 +146,7 @@ - (instancetype)respondWithTarget:(id)target action:(SEL)action FBRoute_TargetAction *route = [FBRoute_TargetAction withVerb:self.verb path:self.path requiresSession:self.requiresSession]; route.target = target; route.action = action; + route.isStandalone = self.isStandalone; return route; } diff --git a/WebDriverAgentLib/Routing/FBSession.m b/WebDriverAgentLib/Routing/FBSession.m index 02889303a2..d9abe3acea 100644 --- a/WebDriverAgentLib/Routing/FBSession.m +++ b/WebDriverAgentLib/Routing/FBSession.m @@ -195,7 +195,9 @@ - (void)kill } } - _activeSession = nil; + if (self == _activeSession) { + _activeSession = nil; + } } - (XCUIApplication *)activeApplication diff --git a/WebDriverAgentLib/Routing/FBWebServer.m b/WebDriverAgentLib/Routing/FBWebServer.m index 46314f2a25..01b0e7e8a2 100644 --- a/WebDriverAgentLib/Routing/FBWebServer.m +++ b/WebDriverAgentLib/Routing/FBWebServer.m @@ -208,7 +208,7 @@ - (void)registerRouteHandlers:(NSArray *)commandHandlerClasses for (Class commandHandler in commandHandlerClasses) { NSArray *routes = [commandHandler routes]; for (FBRoute *route in routes) { - [self.server handleMethod:route.verb withPath:route.path block:^(RouteRequest *request, RouteResponse *response) { + [self.server handleMethod:route.verb withPath:route.path standalone:route.isStandalone block:^(RouteRequest *request, RouteResponse *response) { __strong typeof(weakSelf) strongSelf = weakSelf; if (nil == strongSelf) { return; diff --git a/WebDriverAgentLib/Utilities/FBXCTestDaemonsProxy.m b/WebDriverAgentLib/Utilities/FBXCTestDaemonsProxy.m index cec4a8bfaf..34fca95e4a 100644 --- a/WebDriverAgentLib/Utilities/FBXCTestDaemonsProxy.m +++ b/WebDriverAgentLib/Utilities/FBXCTestDaemonsProxy.m @@ -23,6 +23,7 @@ #import "XCUIDevice.h" #define LAUNCH_APP_TIMEOUT_SEC 300 +#define STOP_SCREEN_RECORDING_TIMEOUT_SEC 20 static void (*originalLaunchAppMethod)(id, SEL, NSString*, NSString*, NSArray*, NSDictionary*, void (^)(_Bool, NSError *)); @@ -342,14 +343,16 @@ + (BOOL)stopScreenRecordingWithUUID:(NSUUID *)uuid error:(NSError *__autoreleasi } __block NSError *innerError = nil; - [FBRunLoopSpinner spinUntilCompletion:^(void(^completion)(void)){ - [session stopScreenRecordingWithUUID:uuid withReply:^(NSError *invokeError) { - if (nil != invokeError) { - innerError = invokeError; - } - completion(); - }]; + dispatch_semaphore_t sem = dispatch_semaphore_create(0); + [session stopScreenRecordingWithUUID:uuid withReply:^(NSError *invokeError) { + innerError = invokeError; + dispatch_semaphore_signal(sem); }]; + int64_t timeoutNs = (int64_t)(STOP_SCREEN_RECORDING_TIMEOUT_SEC * NSEC_PER_SEC); + if (0 != dispatch_semaphore_wait(sem, dispatch_time(DISPATCH_TIME_NOW, timeoutNs)) && nil == innerError) { + NSString *message = [NSString stringWithFormat:@"Did not receive a reply to stop screen recording within %d seconds", STOP_SCREEN_RECORDING_TIMEOUT_SEC]; + innerError = [[[FBErrorBuilder builder] withDescription:message] build]; + } if (nil != innerError && error) { *error = innerError; } diff --git a/WebDriverAgentLib/Utilities/FBXCodeCompatibility.m b/WebDriverAgentLib/Utilities/FBXCodeCompatibility.m index e2fa950310..e8cf2901c8 100644 --- a/WebDriverAgentLib/Utilities/FBXCodeCompatibility.m +++ b/WebDriverAgentLib/Utilities/FBXCodeCompatibility.m @@ -77,6 +77,8 @@ + (BOOL)fb_areKeyEventsSupported @end +#define TESTMANAGERD_VERSION_TIMEOUT_SEC 20 + NSInteger FBTestmanagerdVersion(void) { static dispatch_once_t getTestmanagerdVersion; @@ -85,12 +87,18 @@ NSInteger FBTestmanagerdVersion(void) id proxy = [FBXCTestDaemonsProxy testRunnerProxy]; if ([(NSObject *)proxy respondsToSelector:@selector(_XCT_exchangeProtocolVersion:reply:)]) { id legacyProxy = (id)proxy; - [FBRunLoopSpinner spinUntilCompletion:^(void(^completion)(void)){ - [legacyProxy _XCT_exchangeProtocolVersion:testmanagerdVersion reply:^(unsigned long long code) { - testmanagerdVersion = (NSInteger) code; - completion(); - }]; + // Assume newest/full-featured on timeout, mirroring the modern-testmanagerd branch below. + __block NSInteger receivedVersion = 0xFFFF; + dispatch_semaphore_t sem = dispatch_semaphore_create(0); + [legacyProxy _XCT_exchangeProtocolVersion:testmanagerdVersion reply:^(unsigned long long code) { + receivedVersion = (NSInteger) code; + dispatch_semaphore_signal(sem); }]; + int64_t timeoutNs = (int64_t)(TESTMANAGERD_VERSION_TIMEOUT_SEC * NSEC_PER_SEC); + if (0 != dispatch_semaphore_wait(sem, dispatch_time(DISPATCH_TIME_NOW, timeoutNs))) { + [FBLogger logFmt:@"Did not receive a testmanagerd protocol version reply within %d seconds; assuming the newest/full-featured protocol", TESTMANAGERD_VERSION_TIMEOUT_SEC]; + } + testmanagerdVersion = receivedVersion; } else { // Modern testmanagerd (Xcode 15+) has already negotiated named XCTCapabilities by the time // a daemon session exists, instead of a single scalar protocol version. There is no direct From a06b98d06ca28103aa7b4d0a21f10fd9ee753b53 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sat, 22 Aug 2026 22:35:30 +0200 Subject: [PATCH 02/10] fix: cancel a killed session's pending requests instead of leaving them stuck DELETE /session already bypasses the frozen route queue, but other in-flight requests for that same session didn't: a request queued or executing when the session dies would either hang until it finally got a turn, or (worse) run against a session mid-teardown. -kill now clears the active-session pointer and posts a notification up front, before its own teardown; FBWebServer uses it to immediately fail every pending request for that session with a proper W3C "invalid session id" error instead of leaving their clients waiting. A request already executing keeps running in the background - GCD can't abort it - but its result is discarded rather than ever reaching a client. Also bounds a related unbounded wait found while testing this: -kill's check for whether the tested app is the system app goes through a shared accessibility client that can itself be stuck behind another in-flight request against a frozen app. It's now capped at 5s, defaulting to "assume it might be the system app" (skip termination) on timeout to stay on the safe side. --- WebDriverAgentLib/Routing/FBHTTPServer.h | 11 ++++++ WebDriverAgentLib/Routing/FBHTTPServer.m | 50 +++++++++++++++++++++++- WebDriverAgentLib/Routing/FBSession.h | 6 +++ WebDriverAgentLib/Routing/FBSession.m | 48 ++++++++++++++++++++--- WebDriverAgentLib/Routing/FBWebServer.m | 25 ++++++++++++ 5 files changed, 134 insertions(+), 6 deletions(-) diff --git a/WebDriverAgentLib/Routing/FBHTTPServer.h b/WebDriverAgentLib/Routing/FBHTTPServer.h index 2ee07893cc..75ea80ccd4 100644 --- a/WebDriverAgentLib/Routing/FBHTTPServer.h +++ b/WebDriverAgentLib/Routing/FBHTTPServer.h @@ -71,6 +71,17 @@ NS_ASSUME_NONNULL_BEGIN */ - (void)get:(NSString *)path withBlock:(void (^)(RouteRequest *request, RouteResponse *response))block; +/** + Immediately sends `response` to every non-standalone request currently pending for the given + "sessionID" path param - whether still queued on -routeQueue or already executing - instead of + leaving their HTTP clients waiting on a session that no longer exists. A request that has already + started executing keeps running to completion in the background regardless (GCD gives no way to + abort a block once it starts), but its eventual result is discarded rather than ever reaching a + client. `response` is written as-is to every pending client, so the caller is expected to supply + a fully-populated, protocol-correct error response (e.g. a W3C-shaped JSON body). + */ +- (void)abandonPendingRequestsForSessionID:(NSString *)sessionID withResponse:(RouteResponse *)response; + /** Starts listening on `port`. */ diff --git a/WebDriverAgentLib/Routing/FBHTTPServer.m b/WebDriverAgentLib/Routing/FBHTTPServer.m index 77ca430530..e752a88784 100644 --- a/WebDriverAgentLib/Routing/FBHTTPServer.m +++ b/WebDriverAgentLib/Routing/FBHTTPServer.m @@ -71,6 +71,10 @@ @interface FBHTTPServer () // Keyed by "METHOD path" - holds connections waiting on an already in-flight standalone request // for that same endpoint. Guarded by @synchronized(self.standaloneWaiters). @property (nonatomic, strong) NSMutableDictionary *standaloneWaiters; +// Keyed by the "sessionID" path param - holds clients with a non-standalone request currently +// queued on -routeQueue or executing for that session. See -abandonPendingRequestsForSessionID:. +// Guarded by @synchronized(self.pendingSessionRequests). +@property (nonatomic, strong) NSMutableDictionary *pendingSessionRequests; @end @@ -86,6 +90,7 @@ - (instancetype)init _pendingRequestHeaders = [NSMapTable mapTableWithKeyOptions:(NSPointerFunctionsOptions)(NSMapTableObjectPointerPersonality | NSMapTableStrongMemory) valueOptions:(NSPointerFunctionsOptions)NSMapTableStrongMemory]; _standaloneWaiters = [NSMutableDictionary dictionary]; + _pendingSessionRequests = [NSMutableDictionary dictionary]; } return self; } @@ -406,9 +411,38 @@ - (void)dispatchMethod:(NSString *)method pathAndQuery:(NSString *)pathAndQuery return; } + NSString *sessionID = params[@"sessionID"]; + if (nil != sessionID) { + @synchronized (self.pendingSessionRequests) { + NSMutableSet *pendingClients = self.pendingSessionRequests[sessionID]; + if (nil == pendingClients) { + pendingClients = [NSMutableSet set]; + self.pendingSessionRequests[sessionID] = pendingClients; + } + [pendingClients addObject:client]; + } + } + void (^invoke)(void) = ^{ route.block(request, response); - [self writeResponse:response toClient:client]; + // Whoever removes `client` from pendingSessionRequests first "wins" and gets to respond - + // either this normal completion, or -abandonPendingRequestsForSessionID: on another thread. + BOOL shouldRespond = YES; + if (nil != sessionID) { + @synchronized (self.pendingSessionRequests) { + NSMutableSet *pendingClients = self.pendingSessionRequests[sessionID]; + shouldRespond = [pendingClients containsObject:client]; + if (shouldRespond) { + [pendingClients removeObject:client]; + if (0 == pendingClients.count) { + [self.pendingSessionRequests removeObjectForKey:sessionID]; + } + } + } + } + if (shouldRespond) { + [self writeResponse:response toClient:client]; + } }; dispatch_queue_t routeQueue = self.routeQueue; if (routeQueue) { @@ -427,6 +461,20 @@ - (void)dispatchMethod:(NSString *)method pathAndQuery:(NSString *)pathAndQuery [self writeResponse:notFound toClient:client]; } +#pragma mark - Session-scoped request cancellation + +- (void)abandonPendingRequestsForSessionID:(NSString *)sessionID withResponse:(RouteResponse *)response +{ + NSSet *clients; + @synchronized (self.pendingSessionRequests) { + clients = [self.pendingSessionRequests[sessionID] copy]; + [self.pendingSessionRequests removeObjectForKey:sessionID]; + } + for (nw_connection_t client in clients) { + [self writeResponse:response toClient:client]; + } +} + #pragma mark - Standalone route dispatch - (void)dispatchStandaloneRoute:(FBHTTPRoute *)route diff --git a/WebDriverAgentLib/Routing/FBSession.h b/WebDriverAgentLib/Routing/FBSession.h index 61b1f87429..e4a810c394 100644 --- a/WebDriverAgentLib/Routing/FBSession.h +++ b/WebDriverAgentLib/Routing/FBSession.h @@ -16,6 +16,12 @@ NS_ASSUME_NONNULL_BEGIN /** Bundle identifier of Mobile Safari browser */ extern NSString* const FB_SAFARI_BUNDLE_ID; +/** + Posted (synchronously, on whatever thread calls -kill) once a session has been torn down. The + notification's object is the FBSession instance that was killed - see -identifier. + */ +extern NSString* const FBSessionWasKilledNotification; + /** Class that represents testing session */ diff --git a/WebDriverAgentLib/Routing/FBSession.m b/WebDriverAgentLib/Routing/FBSession.m index d9abe3acea..2dcf3faa9b 100644 --- a/WebDriverAgentLib/Routing/FBSession.m +++ b/WebDriverAgentLib/Routing/FBSession.m @@ -34,12 +34,20 @@ NSString *const FB_SAFARI_BUNDLE_ID = @"com.apple.mobilesafari"; +// +[XCUIApplication fb_systemApplication] goes through FBXCAXClientProxy's shared accessibility +// channel, which can be stuck for as long as some other in-flight request against a frozen app - +// see -fb_isTestedApplicationSameAsSystemAppWithTimeout: below. +static const NSTimeInterval FB_IS_SYSTEM_APP_CHECK_TIMEOUT_SEC = 5.; +NSString *const FBSessionWasKilledNotification = @"FBSessionWasKilledNotification"; + @interface FBSession () @property (nullable, nonatomic) XCUIApplication *testedApplication; @property (nonatomic) BOOL isTestedApplicationExpectedToRun; @property (nonatomic) BOOL shouldAppsWaitForQuiescence; @property (nonatomic, nullable) FBAlertsMonitor *alertsMonitor; @property (nonatomic, readwrite) NSMutableDictionary *> *elementsVisibilityCache; + +- (BOOL)fb_isTestedApplicationSameAsSystemAppWithTimeout:(NSTimeInterval)timeout; @end @interface FBSession (FBAlertsMonitorDelegate) @@ -173,6 +181,18 @@ - (void)kill return; } + // Cleared up front, before the (potentially slow) teardown below - not just at the very end - + // so that any request which arrives while that teardown is still running resolves to "no such + // session" (see +sessionWithIdentifier:) instead of racing in against a session that's already + // mid-teardown, and unlike the notification below, would never get abandoned either. + if (self == _activeSession) { + _activeSession = nil; + } + + // Posted early, before the (potentially slow) teardown below, so anything waiting on this + // session's pending HTTP requests can stop waiting as soon as possible. + [NSNotificationCenter.defaultCenter postNotificationName:FBSessionWasKilledNotification object:self]; + [self disableAlertsMonitor]; FBScreenRecordingPromise *activeScreenRecording = FBScreenRecordingContainer.sharedInstance.screenRecordingPromise; @@ -187,17 +207,13 @@ - (void)kill if (nil != self.testedApplication && FBConfiguration.sharedInstance.shouldTerminateApp && self.testedApplication.running - && ![self.testedApplication fb_isSameAppAs:XCUIApplication.fb_systemApplication]) { + && ![self fb_isTestedApplicationSameAsSystemAppWithTimeout:FB_IS_SYSTEM_APP_CHECK_TIMEOUT_SEC]) { @try { [self.testedApplication terminate]; } @catch (NSException *e) { [FBLogger logFmt:@"%@", e.description]; } } - - if (self == _activeSession) { - _activeSession = nil; - } } - (XCUIApplication *)activeApplication @@ -292,4 +308,26 @@ - (XCUIApplication *)makeApplicationWithBundleId:(NSString *)bundleIdentifier : [[XCUIApplication alloc] initWithBundleIdentifier:bundleIdentifier]; } +// +[XCUIApplication fb_systemApplication] has no async variant and can block for as long as +// FBXCAXClientProxy's shared accessibility channel is busy servicing some other (possibly stuck) +// request against a frozen app, unrelated to this session. Run it on its own thread and give up +// after `timeout`, assuming the tested app IS the system app - the safer assumption, since it +// means -kill skips terminating it rather than risking terminating springboard - if we can't find +// out in time. +- (BOOL)fb_isTestedApplicationSameAsSystemAppWithTimeout:(NSTimeInterval)timeout +{ + __block XCUIApplication *systemApp = nil; + dispatch_semaphore_t sem = dispatch_semaphore_create(0); + dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{ + systemApp = XCUIApplication.fb_systemApplication; + dispatch_semaphore_signal(sem); + }); + int64_t timeoutNs = (int64_t)(timeout * NSEC_PER_SEC); + if (0 != dispatch_semaphore_wait(sem, dispatch_time(DISPATCH_TIME_NOW, timeoutNs))) { + [FBLogger logFmt:@"Could not determine the system application within %@ seconds; assuming '%@' might be it and skipping its termination", @(timeout), self.testedApplication.bundleID]; + return YES; + } + return [self.testedApplication fb_isSameAppAs:systemApp]; +} + @end diff --git a/WebDriverAgentLib/Routing/FBWebServer.m b/WebDriverAgentLib/Routing/FBWebServer.m index 01b0e7e8a2..691ce44993 100644 --- a/WebDriverAgentLib/Routing/FBWebServer.m +++ b/WebDriverAgentLib/Routing/FBWebServer.m @@ -13,8 +13,10 @@ #import "FBTCPSocket.h" #import "FBCommandHandler.h" +#import "FBCommandStatus.h" #import "FBErrorBuilder.h" #import "FBExceptionHandler.h" +#import "FBResponsePayload.h" #import "FBRouteRequest.h" #import "FBRuntimeUtils.h" #import "FBSession.h" @@ -80,6 +82,11 @@ - (BOOL)startHTTPServer [self.server setDefaultHeader:@"Access-Control-Allow-Origin" value:@"*"]; [self.server setDefaultHeader:@"Access-Control-Allow-Headers" value:@"Content-Type, X-Requested-With"]; + [NSNotificationCenter.defaultCenter addObserver:self + selector:@selector(sessionWasKilled:) + name:FBSessionWasKilledNotification + object:nil]; + [self registerRouteHandlers:[self.class collectCommandHandlerClasses]]; [self registerServerKeyRouteHandlers]; @@ -167,8 +174,26 @@ - (void)readMjpegSettingsFromEnv } } +- (void)sessionWasKilled:(NSNotification *)notification +{ + FBSession *session = notification.object; + if (![session isKindOfClass:FBSession.class]) { + return; + } + // Same "invalid session id" shape a still-queued request would eventually get anyway, once + // -routeQueue drains and FBRoute.decorateRequest: finds the session gone - just delivered now + // instead of after however long the request would otherwise have been stuck waiting. + NSString *message = [NSString stringWithFormat:@"Session %@ was deleted while this request was still pending", session.identifier]; + id payload = FBResponseWithStatus([FBCommandStatus noSuchDriverErrorWithMessage:message + traceback:nil]); + RouteResponse *response = [RouteResponse new]; + [payload dispatchWithResponse:response]; + [self.server abandonPendingRequestsForSessionID:session.identifier withResponse:response]; +} + - (void)stopServing { + [NSNotificationCenter.defaultCenter removeObserver:self name:FBSessionWasKilledNotification object:nil]; [FBSession.activeSession kill]; [self stopScreenshotsBroadcaster]; if (self.server.isRunning) { From 7ab81c64f6b281cf37e6ec6159e326c6730ea348 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sat, 22 Aug 2026 22:40:56 +0200 Subject: [PATCH 03/10] fix: guard against fb_systemApplication asserting off the main thread +[XCUIApplication fb_systemApplication] is undocumented private API; its sibling -terminate is confirmed (this session, live) to hard-assert when called off the main thread, so the background-queue call added to bound -kill's system-app check needs the same @try/@catch already used around -terminate - an uncaught exception from inside a bare dispatch_async block has no handler and would crash the whole process, which is worse than the timeout this code already guards against. --- WebDriverAgentLib/Routing/FBSession.m | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/WebDriverAgentLib/Routing/FBSession.m b/WebDriverAgentLib/Routing/FBSession.m index 2dcf3faa9b..9c1462e32b 100644 --- a/WebDriverAgentLib/Routing/FBSession.m +++ b/WebDriverAgentLib/Routing/FBSession.m @@ -317,14 +317,23 @@ - (XCUIApplication *)makeApplicationWithBundleId:(NSString *)bundleIdentifier - (BOOL)fb_isTestedApplicationSameAsSystemAppWithTimeout:(NSTimeInterval)timeout { __block XCUIApplication *systemApp = nil; + __block NSException *caughtException = nil; dispatch_semaphore_t sem = dispatch_semaphore_create(0); dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{ - systemApp = XCUIApplication.fb_systemApplication; + // +fb_systemApplication is undocumented private API; some of its XCUIApplication siblings + // (e.g. -terminate) hard-assert when called off the main thread, so guard against this one + // doing the same on some other Xcode/iOS version - an uncaught exception thrown from inside a + // bare dispatch_async block has no handler and would crash the whole process. + @try { + systemApp = XCUIApplication.fb_systemApplication; + } @catch (NSException *e) { + caughtException = e; + } dispatch_semaphore_signal(sem); }); int64_t timeoutNs = (int64_t)(timeout * NSEC_PER_SEC); - if (0 != dispatch_semaphore_wait(sem, dispatch_time(DISPATCH_TIME_NOW, timeoutNs))) { - [FBLogger logFmt:@"Could not determine the system application within %@ seconds; assuming '%@' might be it and skipping its termination", @(timeout), self.testedApplication.bundleID]; + if (0 != dispatch_semaphore_wait(sem, dispatch_time(DISPATCH_TIME_NOW, timeoutNs)) || nil != caughtException) { + [FBLogger logFmt:@"Could not determine the system application within %@ seconds%@; assuming '%@' might be it and skipping its termination", @(timeout), nil == caughtException ? @"" : [NSString stringWithFormat:@" (%@)", caughtException.description], self.testedApplication.bundleID]; return YES; } return [self.testedApplication fb_isSameAppAs:systemApp]; From c89761ddbaad20338700d1f4b348bab2a9c2b8e1 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sat, 22 Aug 2026 22:50:41 +0200 Subject: [PATCH 04/10] refactor: drop the now-unnecessary self==_activeSession guard in -kill Now that the clear happens up front instead of after -kill's teardown, the scenario the guard protected against - a slow -kill finishing after a newer session had already taken over - can't happen: every call site resolves self/_activeSession and calls kill() in the same uninterrupted synchronous chain (+sessionWithIdentifier: literally hands back _activeSession itself). The only way self != _activeSession at that point is an unsynchronized data race on the static, already an accepted, out-of-scope risk elsewhere in this design, and one the guard couldn't protect against anyway. --- WebDriverAgentLib/Routing/FBSession.m | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/WebDriverAgentLib/Routing/FBSession.m b/WebDriverAgentLib/Routing/FBSession.m index 9c1462e32b..6e7ebfb139 100644 --- a/WebDriverAgentLib/Routing/FBSession.m +++ b/WebDriverAgentLib/Routing/FBSession.m @@ -181,13 +181,9 @@ - (void)kill return; } - // Cleared up front, before the (potentially slow) teardown below - not just at the very end - - // so that any request which arrives while that teardown is still running resolves to "no such - // session" (see +sessionWithIdentifier:) instead of racing in against a session that's already - // mid-teardown, and unlike the notification below, would never get abandoned either. - if (self == _activeSession) { - _activeSession = nil; - } + // Cleared up front, not at the end, so a request arriving mid-teardown resolves to "no such + // session" (+sessionWithIdentifier:) instead of running against a half-torn-down one. + _activeSession = nil; // Posted early, before the (potentially slow) teardown below, so anything waiting on this // session's pending HTTP requests can stop waiting as soon as possible. From d1873c50a52780164a7941357cd6d0e25e889525 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sun, 23 Aug 2026 10:02:49 +0200 Subject: [PATCH 05/10] fix: address PR #1222 review comments on session/request races - FBSession -kill: restore the self==_activeSession guard, but as an atomic check-and-clear under a lock this time. DELETE /session and session creation now genuinely run concurrently (both bypass the frozen route queue), so a session already superseded by a newer one can still reach -kill via a stale reference; without atomicity a belated call could win the race and null out the new session's pointer instead of its own. - FBHTTPServer: track session-scoped standalone requests (e.g. GET /session/:id/screenshot) in pendingSessionRequests too, not just non-standalone ones, so DELETE can abandon them the same way. DELETE /session's own request is excluded from tracking under its own session, since it's the one that performs the abandonment. - FBHTTPServer: track pending requests by a per-request identity object instead of by raw nw_connection_t, so two pipelined requests for the same session sharing one connection no longer collapse into a single tracked entry (which suppressed the second one's response permanently). - FBHTTPServer: allow only one in-flight request per connection at a time. Standalone routes now run on independent queues that can finish in any order, so without this, pipelined requests on one connection (e.g. /screenshot then /status) could have their responses written to the wire out of order. --- WebDriverAgentLib/Routing/FBHTTPServer.m | 317 ++++++++++++++--------- WebDriverAgentLib/Routing/FBSession.m | 19 +- 2 files changed, 208 insertions(+), 128 deletions(-) diff --git a/WebDriverAgentLib/Routing/FBHTTPServer.m b/WebDriverAgentLib/Routing/FBHTTPServer.m index e752a88784..62bfdbc1db 100644 --- a/WebDriverAgentLib/Routing/FBHTTPServer.m +++ b/WebDriverAgentLib/Routing/FBHTTPServer.m @@ -56,6 +56,26 @@ @implementation FBPendingHTTPRequestHeader @end +// Represents one dispatched-but-not-yet-answered request. Uses default (pointer) identity, so two +// pending requests that happen to share the same underlying connection - e.g. two pipelined +// requests for the same session - are never conflated into a single tracked entry. +@interface FBPendingRequest : NSObject +@property (nonatomic, strong, readonly) nw_connection_t client; +@end + +@implementation FBPendingRequest + +- (instancetype)initWithClient:(nw_connection_t)client +{ + if ((self = [super init])) { + _client = client; + } + return self; +} + +@end + + @interface FBHTTPServer () @property (nonatomic, nullable, strong) FBTCPSocket *socket; @@ -68,13 +88,20 @@ @interface FBHTTPServer () // Per-client cache of the already-parsed request line + headers while its body is still // arriving; nil while a client's next unread bytes start with an unparsed header block. @property (nonatomic, strong) NSMapTable *pendingRequestHeaders; -// Keyed by "METHOD path" - holds connections waiting on an already in-flight standalone request -// for that same endpoint. Guarded by @synchronized(self.standaloneWaiters). -@property (nonatomic, strong) NSMutableDictionary *standaloneWaiters; -// Keyed by the "sessionID" path param - holds clients with a non-standalone request currently -// queued on -routeQueue or executing for that session. See -abandonPendingRequestsForSessionID:. -// Guarded by @synchronized(self.pendingSessionRequests). -@property (nonatomic, strong) NSMutableDictionary *pendingSessionRequests; +// Connections with a request that's been parsed off the buffer but not yet answered. While a +// connection is in this set, -processBufferForClient: won't start any further pipelined request +// already sitting in its buffer - that keeps responses on one connection from being written out +// of order when e.g. a standalone /screenshot and /status are pipelined back to back and finish +// on independent queues. Guarded by @synchronized(self.connectionBuffers) (same lock as the +// buffers themselves, so the busy-check and the buffer consume-and-dispatch stay one atomic step). +@property (nonatomic, strong) NSMutableSet *connectionsAwaitingResponse; +// Keyed by "METHOD path" - holds requests waiting on an already in-flight standalone request for +// that same endpoint. Guarded by @synchronized(self.standaloneWaiters). +@property (nonatomic, strong) NSMutableDictionary *> *standaloneWaiters; +// Keyed by the "sessionID" path param - holds requests currently queued or executing for that +// session, standalone or not (except DELETE /session itself - see -dispatchMethod:...). +// See -abandonPendingRequestsForSessionID:. Guarded by @synchronized(self.pendingSessionRequests). +@property (nonatomic, strong) NSMutableDictionary *> *pendingSessionRequests; @end @@ -89,6 +116,7 @@ - (instancetype)init valueOptions:(NSPointerFunctionsOptions)NSMapTableStrongMemory]; _pendingRequestHeaders = [NSMapTable mapTableWithKeyOptions:(NSPointerFunctionsOptions)(NSMapTableObjectPointerPersonality | NSMapTableStrongMemory) valueOptions:(NSPointerFunctionsOptions)NSMapTableStrongMemory]; + _connectionsAwaitingResponse = [NSMutableSet set]; _standaloneWaiters = [NSMutableDictionary dictionary]; _pendingSessionRequests = [NSMutableDictionary dictionary]; } @@ -210,6 +238,7 @@ - (void)stop:(BOOL)immediately @synchronized (self.connectionBuffers) { [self.connectionBuffers removeAllObjects]; [self.pendingRequestHeaders removeAllObjects]; + [self.connectionsAwaitingResponse removeAllObjects]; } _isRunning = NO; } @@ -228,6 +257,7 @@ - (void)didClientDisconnect:(nw_connection_t)client @synchronized (self.connectionBuffers) { [self.connectionBuffers removeObjectForKey:client]; [self.pendingRequestHeaders removeObjectForKey:client]; + [self.connectionsAwaitingResponse removeObject:client]; } } @@ -246,104 +276,109 @@ - (void)client:(nw_connection_t)client didReceiveData:(NSData *)data #pragma mark - HTTP parsing +// Parses and dispatches at most one request per call. A connection with a request already +// in flight is left alone - see -connectionsAwaitingResponse - and picks back up, via a fresh +// call to this method, once that request's response has been written. - (void)processBufferForClient:(nw_connection_t)client { - while (YES) { - NSMutableData *buffer; - FBPendingHTTPRequestHeader *pending; - @synchronized (self.connectionBuffers) { - buffer = [self.connectionBuffers objectForKey:client]; - if (nil == buffer) { - return; - } - pending = [self.pendingRequestHeaders objectForKey:client]; + NSMutableData *buffer; + FBPendingHTTPRequestHeader *pending; + @synchronized (self.connectionBuffers) { + if ([self.connectionsAwaitingResponse containsObject:client]) { + return; } + buffer = [self.connectionBuffers objectForKey:client]; + if (nil == buffer) { + return; + } + pending = [self.pendingRequestHeaders objectForKey:client]; + } - if (nil == pending) { - NSRange headerEndRange = [buffer rangeOfData:FBCRLFCRLFData() options:(NSDataSearchOptions)0 range:NSMakeRange(0, buffer.length)]; - if (NSNotFound == headerEndRange.location) { - // Wait for the rest of the header block to arrive. - return; - } - - NSData *headerData = [buffer subdataWithRange:NSMakeRange(0, headerEndRange.location)]; - NSString *headerString = [[NSString alloc] initWithData:headerData encoding:NSUTF8StringEncoding]; - NSArray *lines = [headerString componentsSeparatedByString:@"\r\n"]; - if (lines.count < 1) { - [self respondBadRequestToClient:client]; - return; - } - - NSArray *requestLineParts = [lines.firstObject componentsSeparatedByString:@" "]; - if (requestLineParts.count < 2) { - [self respondBadRequestToClient:client]; - return; - } - - NSMutableDictionary *requestHeaders = [NSMutableDictionary dictionary]; - for (NSUInteger i = 1; i < lines.count; i++) { - NSString *line = lines[i]; - NSRange colonRange = [line rangeOfString:@":"]; - if (NSNotFound == colonRange.location) { - continue; - } - NSString *name = [line substringToIndex:colonRange.location]; - NSString *value = [[line substringFromIndex:colonRange.location + 1] - stringByTrimmingCharactersInSet:NSCharacterSet.whitespaceCharacterSet]; - requestHeaders[name.lowercaseString] = value; - } + if (nil == pending) { + NSRange headerEndRange = [buffer rangeOfData:FBCRLFCRLFData() options:(NSDataSearchOptions)0 range:NSMakeRange(0, buffer.length)]; + if (NSNotFound == headerEndRange.location) { + // Wait for the rest of the header block to arrive. + return; + } - NSString *transferEncoding = requestHeaders[@"transfer-encoding"]; - if (transferEncoding.length > 0) { - // No transfer decoder is implemented at all, so any encoding (chunked or otherwise - - // including a value only introduced by a duplicate header overwriting "chunked" above) - // is rejected rather than risking the body being misread as empty and desyncing the rest - // of the connection's request stream. - RouteResponse *notImplemented = [RouteResponse new]; - id notImplementedPayload = FBResponseWithStatus([FBCommandStatus invalidArgumentErrorWithMessage:@"Transfer-Encoding is not supported" - traceback:nil]); - [notImplementedPayload dispatchWithResponse:notImplemented]; - [self failClient:client withResponse:notImplemented]; - return; - } + NSData *headerData = [buffer subdataWithRange:NSMakeRange(0, headerEndRange.location)]; + NSString *headerString = [[NSString alloc] initWithData:headerData encoding:NSUTF8StringEncoding]; + NSArray *lines = [headerString componentsSeparatedByString:@"\r\n"]; + if (lines.count < 1) { + [self respondBadRequestToClient:client]; + return; + } - NSUInteger contentLength = (NSUInteger)requestHeaders[@"content-length"].integerValue; - if (contentLength > FBConfiguration.sharedInstance.httpRequestBodySizeLimit) { - // Closes the connection after responding, since the rest of the oversized body is still incoming. - RouteResponse *tooLarge = [RouteResponse new]; - id tooLargePayload = FBResponseWithStatus([FBCommandStatus invalidArgumentErrorWithMessage:@"The request body exceeds the configured size limit" - traceback:nil]); - [tooLargePayload dispatchWithResponse:tooLarge]; - [self failClient:client withResponse:tooLarge]; - return; - } + NSArray *requestLineParts = [lines.firstObject componentsSeparatedByString:@" "]; + if (requestLineParts.count < 2) { + [self respondBadRequestToClient:client]; + return; + } - pending = [FBPendingHTTPRequestHeader new]; - pending.method = requestLineParts[0].uppercaseString; - pending.pathAndQuery = requestLineParts[1]; - pending.bodyStart = headerEndRange.location + headerEndRange.length; - pending.contentLength = contentLength; - @synchronized (self.connectionBuffers) { - [self.pendingRequestHeaders setObject:pending forKey:client]; + NSMutableDictionary *requestHeaders = [NSMutableDictionary dictionary]; + for (NSUInteger i = 1; i < lines.count; i++) { + NSString *line = lines[i]; + NSRange colonRange = [line rangeOfString:@":"]; + if (NSNotFound == colonRange.location) { + continue; } + NSString *name = [line substringToIndex:colonRange.location]; + NSString *value = [[line substringFromIndex:colonRange.location + 1] + stringByTrimmingCharactersInSet:NSCharacterSet.whitespaceCharacterSet]; + requestHeaders[name.lowercaseString] = value; } - NSUInteger totalRequestLength = pending.bodyStart + pending.contentLength; - if (buffer.length < totalRequestLength) { - // Wait for the rest of the body to arrive - the parsed header stays cached above, so this - // doesn't re-scan/re-parse the header block on every subsequently arriving chunk. + NSString *transferEncoding = requestHeaders[@"transfer-encoding"]; + if (transferEncoding.length > 0) { + // No transfer decoder is implemented at all, so any encoding (chunked or otherwise - + // including a value only introduced by a duplicate header overwriting "chunked" above) + // is rejected rather than risking the body being misread as empty and desyncing the rest + // of the connection's request stream. + RouteResponse *notImplemented = [RouteResponse new]; + id notImplementedPayload = FBResponseWithStatus([FBCommandStatus invalidArgumentErrorWithMessage:@"Transfer-Encoding is not supported" + traceback:nil]); + [notImplementedPayload dispatchWithResponse:notImplemented]; + [self failClient:client withResponse:notImplemented]; return; } - NSData *body = pending.contentLength > 0 ? [buffer subdataWithRange:NSMakeRange(pending.bodyStart, pending.contentLength)] : [NSData data]; + NSUInteger contentLength = (NSUInteger)requestHeaders[@"content-length"].integerValue; + if (contentLength > FBConfiguration.sharedInstance.httpRequestBodySizeLimit) { + // Closes the connection after responding, since the rest of the oversized body is still incoming. + RouteResponse *tooLarge = [RouteResponse new]; + id tooLargePayload = FBResponseWithStatus([FBCommandStatus invalidArgumentErrorWithMessage:@"The request body exceeds the configured size limit" + traceback:nil]); + [tooLargePayload dispatchWithResponse:tooLarge]; + [self failClient:client withResponse:tooLarge]; + return; + } + pending = [FBPendingHTTPRequestHeader new]; + pending.method = requestLineParts[0].uppercaseString; + pending.pathAndQuery = requestLineParts[1]; + pending.bodyStart = headerEndRange.location + headerEndRange.length; + pending.contentLength = contentLength; @synchronized (self.connectionBuffers) { - [buffer replaceBytesInRange:NSMakeRange(0, totalRequestLength) withBytes:NULL length:0]; - [self.pendingRequestHeaders removeObjectForKey:client]; + [self.pendingRequestHeaders setObject:pending forKey:client]; } + } + + NSUInteger totalRequestLength = pending.bodyStart + pending.contentLength; + if (buffer.length < totalRequestLength) { + // Wait for the rest of the body to arrive - the parsed header stays cached above, so this + // doesn't re-scan/re-parse the header block on every subsequently arriving chunk. + return; + } + + NSData *body = pending.contentLength > 0 ? [buffer subdataWithRange:NSMakeRange(pending.bodyStart, pending.contentLength)] : [NSData data]; - [self dispatchMethod:pending.method pathAndQuery:pending.pathAndQuery body:body client:client]; + @synchronized (self.connectionBuffers) { + [buffer replaceBytesInRange:NSMakeRange(0, totalRequestLength) withBytes:NULL length:0]; + [self.pendingRequestHeaders removeObjectForKey:client]; + [self.connectionsAwaitingResponse addObject:client]; } + + [self dispatchMethod:pending.method pathAndQuery:pending.pathAndQuery body:body client:client]; } // Removes the client's buffered state and responds with a closing error response. Removing the @@ -406,40 +441,27 @@ - (void)dispatchMethod:(NSString *)method pathAndQuery:(NSString *)pathAndQuery RouteResponse *response = [RouteResponse new]; [self applyDefaultHeadersToResponse:response]; + NSString *sessionID = params[@"sessionID"]; if (route.isStandalone) { - [self dispatchStandaloneRoute:route request:request response:response client:client method:method path:path]; + // DELETE /session is what triggers -abandonPendingRequestsForSessionID: (see -kill / + // -sessionWasKilled:); tracking its own request here would make it abandon itself and write + // a response twice. + NSString *trackedSessionID = [route.verb isEqualToString:@"DELETE"] ? nil : sessionID; + [self dispatchStandaloneRoute:route request:request response:response client:client method:method path:path sessionID:trackedSessionID]; return; } - NSString *sessionID = params[@"sessionID"]; + FBPendingRequest *pendingRequest = nil; if (nil != sessionID) { - @synchronized (self.pendingSessionRequests) { - NSMutableSet *pendingClients = self.pendingSessionRequests[sessionID]; - if (nil == pendingClients) { - pendingClients = [NSMutableSet set]; - self.pendingSessionRequests[sessionID] = pendingClients; - } - [pendingClients addObject:client]; - } + pendingRequest = [[FBPendingRequest alloc] initWithClient:client]; + [self trackPendingRequest:pendingRequest forSessionID:sessionID]; } void (^invoke)(void) = ^{ route.block(request, response); - // Whoever removes `client` from pendingSessionRequests first "wins" and gets to respond - - // either this normal completion, or -abandonPendingRequestsForSessionID: on another thread. - BOOL shouldRespond = YES; - if (nil != sessionID) { - @synchronized (self.pendingSessionRequests) { - NSMutableSet *pendingClients = self.pendingSessionRequests[sessionID]; - shouldRespond = [pendingClients containsObject:client]; - if (shouldRespond) { - [pendingClients removeObject:client]; - if (0 == pendingClients.count) { - [self.pendingSessionRequests removeObjectForKey:sessionID]; - } - } - } - } + // Whoever untracks `pendingRequest` first "wins" and gets to respond - either this normal + // completion, or -abandonPendingRequestsForSessionID: on another thread. + BOOL shouldRespond = (nil == pendingRequest) || [self untrackPendingRequest:pendingRequest forSessionID:sessionID]; if (shouldRespond) { [self writeResponse:response toClient:client]; } @@ -463,15 +485,47 @@ - (void)dispatchMethod:(NSString *)method pathAndQuery:(NSString *)pathAndQuery #pragma mark - Session-scoped request cancellation +// `pendingRequest` identifies one dispatched request - see FBPendingRequest - so pipelined +// requests that happen to share a connection are tracked independently of one another. +- (void)trackPendingRequest:(FBPendingRequest *)pendingRequest forSessionID:(NSString *)sessionID +{ + @synchronized (self.pendingSessionRequests) { + NSMutableSet *pendingRequests = self.pendingSessionRequests[sessionID]; + if (nil == pendingRequests) { + pendingRequests = [NSMutableSet set]; + self.pendingSessionRequests[sessionID] = pendingRequests; + } + [pendingRequests addObject:pendingRequest]; + } +} + +// Returns YES if `pendingRequest` was still tracked (and is now removed) - i.e. this caller won +// the race to respond to it, as opposed to -abandonPendingRequestsForSessionID: having already +// claimed it on another thread. +- (BOOL)untrackPendingRequest:(FBPendingRequest *)pendingRequest forSessionID:(NSString *)sessionID +{ + @synchronized (self.pendingSessionRequests) { + NSMutableSet *pendingRequests = self.pendingSessionRequests[sessionID]; + BOOL wasPending = [pendingRequests containsObject:pendingRequest]; + if (wasPending) { + [pendingRequests removeObject:pendingRequest]; + if (0 == pendingRequests.count) { + [self.pendingSessionRequests removeObjectForKey:sessionID]; + } + } + return wasPending; + } +} + - (void)abandonPendingRequestsForSessionID:(NSString *)sessionID withResponse:(RouteResponse *)response { - NSSet *clients; + NSSet *pendingRequests; @synchronized (self.pendingSessionRequests) { - clients = [self.pendingSessionRequests[sessionID] copy]; + pendingRequests = [self.pendingSessionRequests[sessionID] copy]; [self.pendingSessionRequests removeObjectForKey:sessionID]; } - for (nw_connection_t client in clients) { - [self writeResponse:response toClient:client]; + for (FBPendingRequest *pendingRequest in pendingRequests) { + [self writeResponse:response toClient:pendingRequest.client]; } } @@ -483,13 +537,19 @@ - (void)dispatchStandaloneRoute:(FBHTTPRoute *)route client:(nw_connection_t)client method:(NSString *)method path:(NSString *)path + sessionID:(nullable NSString *)sessionID { NSString *key = [NSString stringWithFormat:@"%@ %@", method, path]; + FBPendingRequest *waiter = [[FBPendingRequest alloc] initWithClient:client]; + if (nil != sessionID) { + [self trackPendingRequest:waiter forSessionID:sessionID]; + } + BOOL isInFlight = NO; @synchronized (self.standaloneWaiters) { - NSMutableArray *waiters = self.standaloneWaiters[key]; + NSMutableArray *waiters = self.standaloneWaiters[key]; if (nil != waiters) { - [waiters addObject:client]; + [waiters addObject:waiter]; isInFlight = YES; } else { self.standaloneWaiters[key] = [NSMutableArray array]; @@ -508,14 +568,18 @@ - (void)dispatchStandaloneRoute:(FBHTTPRoute *)route if (nil == strongSelf) { return; } - NSArray *joinedClients; + NSArray *joinedWaiters; @synchronized (strongSelf.standaloneWaiters) { - joinedClients = [strongSelf.standaloneWaiters[key] copy]; + joinedWaiters = [strongSelf.standaloneWaiters[key] copy]; [strongSelf.standaloneWaiters removeObjectForKey:key]; } - [strongSelf writeResponse:response toClient:client]; - for (nw_connection_t joinedClient in joinedClients) { - [strongSelf writeResponse:response toClient:joinedClient]; + for (FBPendingRequest *joinedWaiter in [@[waiter] arrayByAddingObjectsFromArray:joinedWaiters]) { + // Whoever untracks a waiter first "wins" and gets to respond - either this normal + // completion, or -abandonPendingRequestsForSessionID: on another thread. + BOOL shouldRespond = (nil == sessionID) || [strongSelf untrackPendingRequest:joinedWaiter forSessionID:sessionID]; + if (shouldRespond) { + [strongSelf writeResponse:response toClient:joinedWaiter.client]; + } } }); } @@ -550,7 +614,13 @@ - (void)writeResponse:(RouteResponse *)response toClient:(nw_connection_t)client [weakSelf closeClient:client]; }]; } else { + // Submitted before this connection is allowed to move on to its next pipelined request (see + // -processBufferForClient:), so responses can't reach the wire out of order. [self.socket writeData:payload toClient:client]; + @synchronized (self.connectionBuffers) { + [self.connectionsAwaitingResponse removeObject:client]; + } + [self processBufferForClient:client]; } } @@ -558,6 +628,7 @@ - (void)closeClient:(nw_connection_t)client { @synchronized (self.connectionBuffers) { [self.connectionBuffers removeObjectForKey:client]; + [self.connectionsAwaitingResponse removeObject:client]; } nw_connection_cancel(client); } diff --git a/WebDriverAgentLib/Routing/FBSession.m b/WebDriverAgentLib/Routing/FBSession.m index 6e7ebfb139..dc0bd953cb 100644 --- a/WebDriverAgentLib/Routing/FBSession.m +++ b/WebDriverAgentLib/Routing/FBSession.m @@ -177,14 +177,23 @@ - (BOOL)disableAlertsMonitor - (void)kill { - if (nil == _activeSession) { + // DELETE /session and session creation now run concurrently (both can bypass the frozen route + // queue), so a session that's already been superseded by a newer one can still reach here via a + // stale reference. Check-and-clear must happen as one atomic step, else a belated -kill on the + // old session could win the write race and null out the new session's pointer instead of its + // own. self != _activeSession means someone else already killed/replaced this session - nothing + // left for us to do. + BOOL wasActive; + @synchronized (self.class) { + wasActive = (self == _activeSession); + if (wasActive) { + _activeSession = nil; + } + } + if (!wasActive) { return; } - // Cleared up front, not at the end, so a request arriving mid-teardown resolves to "no such - // session" (+sessionWithIdentifier:) instead of running against a half-torn-down one. - _activeSession = nil; - // Posted early, before the (potentially slow) teardown below, so anything waiting on this // session's pending HTTP requests can stop waiting as soon as possible. [NSNotificationCenter.defaultCenter postNotificationName:FBSessionWasKilledNotification object:self]; From dc8a71cc8b7c15fbee732d28c0fc9ca025943347 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sun, 23 Aug 2026 10:22:44 +0200 Subject: [PATCH 06/10] fix: address findings from self-review of standalone-route changes - FBSession -kill: dispatch [testedApplication terminate] to the main thread with a bounded wait instead of calling it inline. -kill can now run on a background queue (DELETE /session is standalone), and -terminate is confirmed to hard-assert off the main thread - the same reason the system-app check just above it is already background-dispatched. Waiting indefinitely for main would reintroduce the exact hang standalone routes exist to avoid if that's the queue currently stuck on another request, so give up after 5s and let the dispatched call finish on its own whenever main frees up. - FBSession -kill: a caller that loses the atomic active-session race now waits (bounded, via NSCondition) for the winner's teardown to actually finish before returning, instead of proceeding immediately. Session creation's own pre-kill of the outgoing session relies on this to not launch the new app while the old one's -terminate may still be in flight on another thread. - FBHTTPServer: route every call to -processBufferForClient: (from both new data arriving and a response completing) through one dedicated serial queue. The method parses a connection's buffer outside of any lock; that's only safe when no two calls for any connection can run concurrently, which no longer held once responses could complete on independent standalone-route queues and re-enter parsing directly on whichever thread finished the write. - FBHTTPServer: standalone-route coalescing now keys on the full path and query string instead of just the path, so a future standalone route that branches on query parameters can't have a second concurrent request silently served the first request's response. - FBXCodeCompatibility FBTestmanagerdVersion: stop caching the timeout fallback forever via dispatch_once. A merely-slow (not hung) first daemon reply would otherwise permanently lock in the fallback value for the rest of the process's life; only a real reply or the always-correct modern-testmanagerd branch is cached now, so a timeout is retried on the next call. --- WebDriverAgentLib/Routing/FBHTTPServer.m | 26 ++++- WebDriverAgentLib/Routing/FBSession.m | 97 +++++++++++++++---- .../Utilities/FBXCodeCompatibility.m | 37 +++++-- 3 files changed, 127 insertions(+), 33 deletions(-) diff --git a/WebDriverAgentLib/Routing/FBHTTPServer.m b/WebDriverAgentLib/Routing/FBHTTPServer.m index 62bfdbc1db..664f2012fe 100644 --- a/WebDriverAgentLib/Routing/FBHTTPServer.m +++ b/WebDriverAgentLib/Routing/FBHTTPServer.m @@ -88,6 +88,12 @@ @interface FBHTTPServer () // Per-client cache of the already-parsed request line + headers while its body is still // arriving; nil while a client's next unread bytes start with an unparsed header block. @property (nonatomic, strong) NSMapTable *pendingRequestHeaders; +// -processBufferForClient: parses a connection's buffer outside of any lock (cheap, and it can +// hand off to -dispatchMethod:...). It's only safe to do that unlocked because every call to it - +// from -client:didReceiveData: and from -writeResponse:toClient:thenCloseConnection: alike - is +// funneled through this single serial queue, so no two calls (even for different connections) ever +// run concurrently with each other. +@property (nonatomic, strong) dispatch_queue_t bufferProcessingQueue; // Connections with a request that's been parsed off the buffer but not yet answered. While a // connection is in this set, -processBufferForClient: won't start any further pipelined request // already sitting in its buffer - that keeps responses on one connection from being written out @@ -116,6 +122,7 @@ - (instancetype)init valueOptions:(NSPointerFunctionsOptions)NSMapTableStrongMemory]; _pendingRequestHeaders = [NSMapTable mapTableWithKeyOptions:(NSPointerFunctionsOptions)(NSMapTableObjectPointerPersonality | NSMapTableStrongMemory) valueOptions:(NSPointerFunctionsOptions)NSMapTableStrongMemory]; + _bufferProcessingQueue = dispatch_queue_create("com.facebook.wda.http.bufferProcessing", DISPATCH_QUEUE_SERIAL); _connectionsAwaitingResponse = [NSMutableSet set]; _standaloneWaiters = [NSMutableDictionary dictionary]; _pendingSessionRequests = [NSMutableDictionary dictionary]; @@ -271,7 +278,10 @@ - (void)client:(nw_connection_t)client didReceiveData:(NSData *)data } [buffer appendData:data]; } - [self processBufferForClient:client]; + __weak typeof(self) weakSelf = self; + dispatch_async(self.bufferProcessingQueue, ^{ + [weakSelf processBufferForClient:client]; + }); } #pragma mark - HTTP parsing @@ -447,7 +457,7 @@ - (void)dispatchMethod:(NSString *)method pathAndQuery:(NSString *)pathAndQuery // -sessionWasKilled:); tracking its own request here would make it abandon itself and write // a response twice. NSString *trackedSessionID = [route.verb isEqualToString:@"DELETE"] ? nil : sessionID; - [self dispatchStandaloneRoute:route request:request response:response client:client method:method path:path sessionID:trackedSessionID]; + [self dispatchStandaloneRoute:route request:request response:response client:client method:method pathAndQuery:pathAndQuery sessionID:trackedSessionID]; return; } @@ -536,10 +546,13 @@ - (void)dispatchStandaloneRoute:(FBHTTPRoute *)route response:(RouteResponse *)response client:(nw_connection_t)client method:(NSString *)method - path:(NSString *)path + pathAndQuery:(NSString *)pathAndQuery sessionID:(nullable NSString *)sessionID { - NSString *key = [NSString stringWithFormat:@"%@ %@", method, path]; + // Includes the query string, not just the path, so two concurrent requests that would run + // route.block with genuinely different `request` objects (e.g. differing query params) are + // never coalesced into sharing one response. + NSString *key = [NSString stringWithFormat:@"%@ %@", method, pathAndQuery]; FBPendingRequest *waiter = [[FBPendingRequest alloc] initWithClient:client]; if (nil != sessionID) { [self trackPendingRequest:waiter forSessionID:sessionID]; @@ -620,7 +633,10 @@ - (void)writeResponse:(RouteResponse *)response toClient:(nw_connection_t)client @synchronized (self.connectionBuffers) { [self.connectionsAwaitingResponse removeObject:client]; } - [self processBufferForClient:client]; + __weak typeof(self) weakSelf = self; + dispatch_async(self.bufferProcessingQueue, ^{ + [weakSelf processBufferForClient:client]; + }); } } diff --git a/WebDriverAgentLib/Routing/FBSession.m b/WebDriverAgentLib/Routing/FBSession.m index dc0bd953cb..fa08d2bc0e 100644 --- a/WebDriverAgentLib/Routing/FBSession.m +++ b/WebDriverAgentLib/Routing/FBSession.m @@ -38,6 +38,14 @@ // channel, which can be stuck for as long as some other in-flight request against a frozen app - // see -fb_isTestedApplicationSameAsSystemAppWithTimeout: below. static const NSTimeInterval FB_IS_SYSTEM_APP_CHECK_TIMEOUT_SEC = 5.; +// -[XCUIApplication terminate] hard-asserts off the main thread - see +// -fb_terminateTestedApplicationWithTimeout: below. +static const NSTimeInterval FB_APP_TERMINATE_TIMEOUT_SEC = 5.; +// Upper bound on -kill's own teardown (system-app check + terminate above, plus the existing 20s +// bound on stopping an active screen recording - see FBXCTestDaemonsProxy) - how long a caller that +// lost the -kill race below will wait for the winner to actually finish, rather than proceeding +// immediately against a session that's still mid-teardown. +static const NSTimeInterval FB_KILL_WAIT_TIMEOUT_SEC = 35.; NSString *const FBSessionWasKilledNotification = @"FBSessionWasKilledNotification"; @interface FBSession () @@ -46,8 +54,13 @@ @interface FBSession () @property (nonatomic) BOOL shouldAppsWaitForQuiescence; @property (nonatomic, nullable) FBAlertsMonitor *alertsMonitor; @property (nonatomic, readwrite) NSMutableDictionary *> *elementsVisibilityCache; +// Lets a -kill caller that loses the atomic race in -kill wait for the winner's teardown to +// actually finish, instead of returning immediately. Created once per session instance - see -init. +@property (nonatomic, strong, readonly) NSCondition *killCondition; +@property (nonatomic, assign) BOOL isKillFinished; - (BOOL)fb_isTestedApplicationSameAsSystemAppWithTimeout:(NSTimeInterval)timeout; +- (void)fb_terminateTestedApplicationWithTimeout:(NSTimeInterval)timeout; @end @interface FBSession (FBAlertsMonitorDelegate) @@ -99,6 +112,14 @@ @implementation FBSession static FBSession *_activeSession = nil; +- (instancetype)init +{ + if ((self = [super init])) { + _killCondition = [NSCondition new]; + } + return self; +} + + (instancetype)activeSession { return _activeSession; @@ -191,33 +212,46 @@ - (void)kill } } if (!wasActive) { + // Someone else is already tearing this exact session down (e.g. a concurrent DELETE and the + // pre-kill in session creation both targeting it). Wait for that teardown to actually finish, + // bounded, so a caller that's about to act as if the session is gone - e.g. launching a fresh + // app for a replacement session - doesn't race a still-in-flight -terminate. + [self.killCondition lock]; + NSDate *deadline = [NSDate dateWithTimeIntervalSinceNow:FB_KILL_WAIT_TIMEOUT_SEC]; + while (!self.isKillFinished && [self.killCondition waitUntilDate:deadline]) { + // Re-checks isKillFinished on every wake, in case of a spurious wakeup. + } + [self.killCondition unlock]; return; } - // Posted early, before the (potentially slow) teardown below, so anything waiting on this - // session's pending HTTP requests can stop waiting as soon as possible. - [NSNotificationCenter.defaultCenter postNotificationName:FBSessionWasKilledNotification object:self]; + @try { + // Posted early, before the (potentially slow) teardown below, so anything waiting on this + // session's pending HTTP requests can stop waiting as soon as possible. + [NSNotificationCenter.defaultCenter postNotificationName:FBSessionWasKilledNotification object:self]; - [self disableAlertsMonitor]; + [self disableAlertsMonitor]; - FBScreenRecordingPromise *activeScreenRecording = FBScreenRecordingContainer.sharedInstance.screenRecordingPromise; - if (nil != activeScreenRecording) { - NSError *error; - if (![FBXCTestDaemonsProxy stopScreenRecordingWithUUID:activeScreenRecording.identifier error:&error]) { - [FBLogger logFmt:@"%@", error]; + FBScreenRecordingPromise *activeScreenRecording = FBScreenRecordingContainer.sharedInstance.screenRecordingPromise; + if (nil != activeScreenRecording) { + NSError *error; + if (![FBXCTestDaemonsProxy stopScreenRecordingWithUUID:activeScreenRecording.identifier error:&error]) { + [FBLogger logFmt:@"%@", error]; + } + [FBScreenRecordingContainer.sharedInstance reset]; } - [FBScreenRecordingContainer.sharedInstance reset]; - } - if (nil != self.testedApplication - && FBConfiguration.sharedInstance.shouldTerminateApp - && self.testedApplication.running - && ![self fb_isTestedApplicationSameAsSystemAppWithTimeout:FB_IS_SYSTEM_APP_CHECK_TIMEOUT_SEC]) { - @try { - [self.testedApplication terminate]; - } @catch (NSException *e) { - [FBLogger logFmt:@"%@", e.description]; + if (nil != self.testedApplication + && FBConfiguration.sharedInstance.shouldTerminateApp + && self.testedApplication.running + && ![self fb_isTestedApplicationSameAsSystemAppWithTimeout:FB_IS_SYSTEM_APP_CHECK_TIMEOUT_SEC]) { + [self fb_terminateTestedApplicationWithTimeout:FB_APP_TERMINATE_TIMEOUT_SEC]; } + } @finally { + [self.killCondition lock]; + self.isKillFinished = YES; + [self.killCondition broadcast]; + [self.killCondition unlock]; } } @@ -344,4 +378,29 @@ - (BOOL)fb_isTestedApplicationSameAsSystemAppWithTimeout:(NSTimeInterval)timeout return [self.testedApplication fb_isSameAppAs:systemApp]; } +// -[XCUIApplication terminate] hard-asserts when called off the main thread, but -kill (the only +// caller) can now itself run on a background queue - DELETE /session is a standalone route (see +// FBHTTPServer.m) that bypasses the main routeQueue. Dispatching to main and waiting indefinitely +// would reintroduce the exact hang standalone routes exist to avoid, if that's the queue currently +// stuck servicing some other request against the frozen app; give up after `timeout` instead. The +// dispatched block still runs (and still terminates the app) whenever main frees up, even after +// this method has given up waiting on it. +- (void)fb_terminateTestedApplicationWithTimeout:(NSTimeInterval)timeout +{ + XCUIApplication *application = self.testedApplication; + dispatch_semaphore_t sem = dispatch_semaphore_create(0); + dispatch_async(dispatch_get_main_queue(), ^{ + @try { + [application terminate]; + } @catch (NSException *e) { + [FBLogger logFmt:@"%@", e.description]; + } + dispatch_semaphore_signal(sem); + }); + int64_t timeoutNs = (int64_t)(timeout * NSEC_PER_SEC); + if (0 != dispatch_semaphore_wait(sem, dispatch_time(DISPATCH_TIME_NOW, timeoutNs))) { + [FBLogger logFmt:@"Could not terminate '%@' within %@ seconds; the main thread may still be busy servicing another request", application.bundleID, @(timeout)]; + } +} + @end diff --git a/WebDriverAgentLib/Utilities/FBXCodeCompatibility.m b/WebDriverAgentLib/Utilities/FBXCodeCompatibility.m index e8cf2901c8..fda9f7200a 100644 --- a/WebDriverAgentLib/Utilities/FBXCodeCompatibility.m +++ b/WebDriverAgentLib/Utilities/FBXCodeCompatibility.m @@ -81,24 +81,42 @@ + (BOOL)fb_areKeyEventsSupported NSInteger FBTestmanagerdVersion(void) { - static dispatch_once_t getTestmanagerdVersion; - static NSInteger testmanagerdVersion; - dispatch_once(&getTestmanagerdVersion, ^{ + // Not a dispatch_once: a `dispatch_once` here would permanently cache the timeout fallback below + // if the very first call's daemon reply merely arrived late (busy, not hung), instead of the real + // negotiated version. -1 means "not yet successfully determined" - only a real reply (or the + // always-correct modern-testmanagerd branch) is cached; a timeout is retried on the next call. + static NSInteger cachedVersion = -1; + static dispatch_queue_t syncQueue; + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + syncQueue = dispatch_queue_create("com.facebook.wda.testmanagerdVersion", DISPATCH_QUEUE_SERIAL); + }); + + __block NSInteger result; + dispatch_sync(syncQueue, ^{ + if (cachedVersion >= 0) { + result = cachedVersion; + return; + } + id proxy = [FBXCTestDaemonsProxy testRunnerProxy]; if ([(NSObject *)proxy respondsToSelector:@selector(_XCT_exchangeProtocolVersion:reply:)]) { id legacyProxy = (id)proxy; - // Assume newest/full-featured on timeout, mirroring the modern-testmanagerd branch below. - __block NSInteger receivedVersion = 0xFFFF; + __block NSInteger receivedVersion = -1; dispatch_semaphore_t sem = dispatch_semaphore_create(0); - [legacyProxy _XCT_exchangeProtocolVersion:testmanagerdVersion reply:^(unsigned long long code) { + [legacyProxy _XCT_exchangeProtocolVersion:0 reply:^(unsigned long long code) { receivedVersion = (NSInteger) code; dispatch_semaphore_signal(sem); }]; int64_t timeoutNs = (int64_t)(TESTMANAGERD_VERSION_TIMEOUT_SEC * NSEC_PER_SEC); if (0 != dispatch_semaphore_wait(sem, dispatch_time(DISPATCH_TIME_NOW, timeoutNs))) { + // Assume newest/full-featured on timeout, mirroring the modern-testmanagerd branch below - + // but don't cache it, so a merely-slow (not hung) daemon gets a real answer on a later call. [FBLogger logFmt:@"Did not receive a testmanagerd protocol version reply within %d seconds; assuming the newest/full-featured protocol", TESTMANAGERD_VERSION_TIMEOUT_SEC]; + result = 0xFFFF; + return; } - testmanagerdVersion = receivedVersion; + result = receivedVersion; } else { // Modern testmanagerd (Xcode 15+) has already negotiated named XCTCapabilities by the time // a daemon session exists, instead of a single scalar protocol version. There is no direct @@ -109,8 +127,9 @@ NSInteger FBTestmanagerdVersion(void) if (nil == capabilities) { [FBLogger log:@"Could not retrieve testmanagerd capabilities"]; } - testmanagerdVersion = 0xFFFF; + result = 0xFFFF; } + cachedVersion = result; }); - return testmanagerdVersion; + return result; } From 924fe28a8cd0badc06de6c8a5c07a5e71bc93969 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sun, 23 Aug 2026 10:25:49 +0200 Subject: [PATCH 07/10] style: trim overlong comments to be more skimmable No behavior change - condenses several multi-line explanatory comments added in the prior two commits down to one or two lines each. --- WebDriverAgentLib/Routing/FBHTTPServer.m | 58 +++++++----------- WebDriverAgentLib/Routing/FBSession.m | 59 ++++++------------- .../Utilities/FBXCodeCompatibility.m | 16 ++--- 3 files changed, 44 insertions(+), 89 deletions(-) diff --git a/WebDriverAgentLib/Routing/FBHTTPServer.m b/WebDriverAgentLib/Routing/FBHTTPServer.m index 664f2012fe..9d5259060e 100644 --- a/WebDriverAgentLib/Routing/FBHTTPServer.m +++ b/WebDriverAgentLib/Routing/FBHTTPServer.m @@ -56,9 +56,8 @@ @implementation FBPendingHTTPRequestHeader @end -// Represents one dispatched-but-not-yet-answered request. Uses default (pointer) identity, so two -// pending requests that happen to share the same underlying connection - e.g. two pipelined -// requests for the same session - are never conflated into a single tracked entry. +// One dispatched-but-not-yet-answered request. Default (pointer) identity, so two pipelined +// requests sharing a connection are never conflated into a single tracked entry. @interface FBPendingRequest : NSObject @property (nonatomic, strong, readonly) nw_connection_t client; @end @@ -88,25 +87,20 @@ @interface FBHTTPServer () // Per-client cache of the already-parsed request line + headers while its body is still // arriving; nil while a client's next unread bytes start with an unparsed header block. @property (nonatomic, strong) NSMapTable *pendingRequestHeaders; -// -processBufferForClient: parses a connection's buffer outside of any lock (cheap, and it can -// hand off to -dispatchMethod:...). It's only safe to do that unlocked because every call to it - -// from -client:didReceiveData: and from -writeResponse:toClient:thenCloseConnection: alike - is -// funneled through this single serial queue, so no two calls (even for different connections) ever -// run concurrently with each other. +// -processBufferForClient: parses a connection's buffer unlocked; safe only because every caller +// (-client:didReceiveData: and -writeResponse:toClient:thenCloseConnection:) funnels through this +// one serial queue, so no two calls ever run concurrently. @property (nonatomic, strong) dispatch_queue_t bufferProcessingQueue; -// Connections with a request that's been parsed off the buffer but not yet answered. While a -// connection is in this set, -processBufferForClient: won't start any further pipelined request -// already sitting in its buffer - that keeps responses on one connection from being written out -// of order when e.g. a standalone /screenshot and /status are pipelined back to back and finish -// on independent queues. Guarded by @synchronized(self.connectionBuffers) (same lock as the -// buffers themselves, so the busy-check and the buffer consume-and-dispatch stay one atomic step). +// Connections with a request parsed off the buffer but not yet answered. Blocks +// -processBufferForClient: from starting the next pipelined request, so responses on one +// connection can't be written out of order. Guarded by @synchronized(self.connectionBuffers). @property (nonatomic, strong) NSMutableSet *connectionsAwaitingResponse; -// Keyed by "METHOD path" - holds requests waiting on an already in-flight standalone request for -// that same endpoint. Guarded by @synchronized(self.standaloneWaiters). +// Keyed by "METHOD path" - requests waiting on an already in-flight standalone request for that +// endpoint. Guarded by @synchronized(self.standaloneWaiters). @property (nonatomic, strong) NSMutableDictionary *> *standaloneWaiters; -// Keyed by the "sessionID" path param - holds requests currently queued or executing for that -// session, standalone or not (except DELETE /session itself - see -dispatchMethod:...). -// See -abandonPendingRequestsForSessionID:. Guarded by @synchronized(self.pendingSessionRequests). +// Keyed by the "sessionID" path param - requests currently queued or executing for that session, +// standalone or not (except DELETE /session itself - see -dispatchMethod:). See +// -abandonPendingRequestsForSessionID:. Guarded by @synchronized(self.pendingSessionRequests). @property (nonatomic, strong) NSMutableDictionary *> *pendingSessionRequests; @end @@ -286,9 +280,8 @@ - (void)client:(nw_connection_t)client didReceiveData:(NSData *)data #pragma mark - HTTP parsing -// Parses and dispatches at most one request per call. A connection with a request already -// in flight is left alone - see -connectionsAwaitingResponse - and picks back up, via a fresh -// call to this method, once that request's response has been written. +// Parses and dispatches at most one request per call; a connection with one already in flight is +// left alone (see -connectionsAwaitingResponse) until its response is written. - (void)processBufferForClient:(nw_connection_t)client { NSMutableData *buffer; @@ -453,9 +446,8 @@ - (void)dispatchMethod:(NSString *)method pathAndQuery:(NSString *)pathAndQuery NSString *sessionID = params[@"sessionID"]; if (route.isStandalone) { - // DELETE /session is what triggers -abandonPendingRequestsForSessionID: (see -kill / - // -sessionWasKilled:); tracking its own request here would make it abandon itself and write - // a response twice. + // DELETE triggers -abandonPendingRequestsForSessionID: itself; tracking its own request + // would make it abandon itself and write a response twice. NSString *trackedSessionID = [route.verb isEqualToString:@"DELETE"] ? nil : sessionID; [self dispatchStandaloneRoute:route request:request response:response client:client method:method pathAndQuery:pathAndQuery sessionID:trackedSessionID]; return; @@ -495,8 +487,6 @@ - (void)dispatchMethod:(NSString *)method pathAndQuery:(NSString *)pathAndQuery #pragma mark - Session-scoped request cancellation -// `pendingRequest` identifies one dispatched request - see FBPendingRequest - so pipelined -// requests that happen to share a connection are tracked independently of one another. - (void)trackPendingRequest:(FBPendingRequest *)pendingRequest forSessionID:(NSString *)sessionID { @synchronized (self.pendingSessionRequests) { @@ -509,9 +499,8 @@ - (void)trackPendingRequest:(FBPendingRequest *)pendingRequest forSessionID:(NSS } } -// Returns YES if `pendingRequest` was still tracked (and is now removed) - i.e. this caller won -// the race to respond to it, as opposed to -abandonPendingRequestsForSessionID: having already -// claimed it on another thread. +// Returns YES if this caller won the race to respond, vs. -abandonPendingRequestsForSessionID: +// already having claimed `pendingRequest` on another thread. - (BOOL)untrackPendingRequest:(FBPendingRequest *)pendingRequest forSessionID:(NSString *)sessionID { @synchronized (self.pendingSessionRequests) { @@ -549,9 +538,7 @@ - (void)dispatchStandaloneRoute:(FBHTTPRoute *)route pathAndQuery:(NSString *)pathAndQuery sessionID:(nullable NSString *)sessionID { - // Includes the query string, not just the path, so two concurrent requests that would run - // route.block with genuinely different `request` objects (e.g. differing query params) are - // never coalesced into sharing one response. + // Includes the query string so requests with different params are never coalesced together. NSString *key = [NSString stringWithFormat:@"%@ %@", method, pathAndQuery]; FBPendingRequest *waiter = [[FBPendingRequest alloc] initWithClient:client]; if (nil != sessionID) { @@ -587,8 +574,6 @@ - (void)dispatchStandaloneRoute:(FBHTTPRoute *)route [strongSelf.standaloneWaiters removeObjectForKey:key]; } for (FBPendingRequest *joinedWaiter in [@[waiter] arrayByAddingObjectsFromArray:joinedWaiters]) { - // Whoever untracks a waiter first "wins" and gets to respond - either this normal - // completion, or -abandonPendingRequestsForSessionID: on another thread. BOOL shouldRespond = (nil == sessionID) || [strongSelf untrackPendingRequest:joinedWaiter forSessionID:sessionID]; if (shouldRespond) { [strongSelf writeResponse:response toClient:joinedWaiter.client]; @@ -627,8 +612,7 @@ - (void)writeResponse:(RouteResponse *)response toClient:(nw_connection_t)client [weakSelf closeClient:client]; }]; } else { - // Submitted before this connection is allowed to move on to its next pipelined request (see - // -processBufferForClient:), so responses can't reach the wire out of order. + // Sent before unblocking the next pipelined request, so responses can't reach the wire out of order. [self.socket writeData:payload toClient:client]; @synchronized (self.connectionBuffers) { [self.connectionsAwaitingResponse removeObject:client]; diff --git a/WebDriverAgentLib/Routing/FBSession.m b/WebDriverAgentLib/Routing/FBSession.m index fa08d2bc0e..b589009d89 100644 --- a/WebDriverAgentLib/Routing/FBSession.m +++ b/WebDriverAgentLib/Routing/FBSession.m @@ -34,17 +34,11 @@ NSString *const FB_SAFARI_BUNDLE_ID = @"com.apple.mobilesafari"; -// +[XCUIApplication fb_systemApplication] goes through FBXCAXClientProxy's shared accessibility -// channel, which can be stuck for as long as some other in-flight request against a frozen app - -// see -fb_isTestedApplicationSameAsSystemAppWithTimeout: below. +// FBXCAXClientProxy's shared accessibility channel can be stuck servicing another request. static const NSTimeInterval FB_IS_SYSTEM_APP_CHECK_TIMEOUT_SEC = 5.; -// -[XCUIApplication terminate] hard-asserts off the main thread - see -// -fb_terminateTestedApplicationWithTimeout: below. +// -terminate hard-asserts off the main thread, which may itself be busy - see -fb_terminate...:. static const NSTimeInterval FB_APP_TERMINATE_TIMEOUT_SEC = 5.; -// Upper bound on -kill's own teardown (system-app check + terminate above, plus the existing 20s -// bound on stopping an active screen recording - see FBXCTestDaemonsProxy) - how long a caller that -// lost the -kill race below will wait for the winner to actually finish, rather than proceeding -// immediately against a session that's still mid-teardown. +// How long a -kill caller that lost the race below waits for the winner's teardown to finish. static const NSTimeInterval FB_KILL_WAIT_TIMEOUT_SEC = 35.; NSString *const FBSessionWasKilledNotification = @"FBSessionWasKilledNotification"; @@ -54,8 +48,7 @@ @interface FBSession () @property (nonatomic) BOOL shouldAppsWaitForQuiescence; @property (nonatomic, nullable) FBAlertsMonitor *alertsMonitor; @property (nonatomic, readwrite) NSMutableDictionary *> *elementsVisibilityCache; -// Lets a -kill caller that loses the atomic race in -kill wait for the winner's teardown to -// actually finish, instead of returning immediately. Created once per session instance - see -init. +// Lets a -kill caller that loses the race wait for the winner's teardown to finish. See -init. @property (nonatomic, strong, readonly) NSCondition *killCondition; @property (nonatomic, assign) BOOL isKillFinished; @@ -198,12 +191,9 @@ - (BOOL)disableAlertsMonitor - (void)kill { - // DELETE /session and session creation now run concurrently (both can bypass the frozen route - // queue), so a session that's already been superseded by a newer one can still reach here via a - // stale reference. Check-and-clear must happen as one atomic step, else a belated -kill on the - // old session could win the write race and null out the new session's pointer instead of its - // own. self != _activeSession means someone else already killed/replaced this session - nothing - // left for us to do. + // DELETE /session and session creation can now run concurrently, so a session already + // superseded by a newer one can still reach here via a stale reference. Check-and-clear must be + // atomic, else a belated -kill could null out the new session's pointer instead of its own. BOOL wasActive; @synchronized (self.class) { wasActive = (self == _activeSession); @@ -212,22 +202,18 @@ - (void)kill } } if (!wasActive) { - // Someone else is already tearing this exact session down (e.g. a concurrent DELETE and the - // pre-kill in session creation both targeting it). Wait for that teardown to actually finish, - // bounded, so a caller that's about to act as if the session is gone - e.g. launching a fresh - // app for a replacement session - doesn't race a still-in-flight -terminate. + // Someone else is already tearing this session down - wait for that to finish (bounded), so + // we don't act as if it's gone (e.g. launch a new app) while its -terminate is still in flight. [self.killCondition lock]; NSDate *deadline = [NSDate dateWithTimeIntervalSinceNow:FB_KILL_WAIT_TIMEOUT_SEC]; while (!self.isKillFinished && [self.killCondition waitUntilDate:deadline]) { - // Re-checks isKillFinished on every wake, in case of a spurious wakeup. } [self.killCondition unlock]; return; } @try { - // Posted early, before the (potentially slow) teardown below, so anything waiting on this - // session's pending HTTP requests can stop waiting as soon as possible. + // Posted before teardown so pending HTTP requests for this session can stop waiting sooner. [NSNotificationCenter.defaultCenter postNotificationName:FBSessionWasKilledNotification object:self]; [self disableAlertsMonitor]; @@ -347,22 +333,17 @@ - (XCUIApplication *)makeApplicationWithBundleId:(NSString *)bundleIdentifier : [[XCUIApplication alloc] initWithBundleIdentifier:bundleIdentifier]; } -// +[XCUIApplication fb_systemApplication] has no async variant and can block for as long as -// FBXCAXClientProxy's shared accessibility channel is busy servicing some other (possibly stuck) -// request against a frozen app, unrelated to this session. Run it on its own thread and give up -// after `timeout`, assuming the tested app IS the system app - the safer assumption, since it -// means -kill skips terminating it rather than risking terminating springboard - if we can't find -// out in time. +// Has no async variant and can block on the shared accessibility channel. Run off-thread and give +// up after `timeout`, assuming the tested app IS the system app - safer, since it means skipping +// termination rather than risking terminating springboard. - (BOOL)fb_isTestedApplicationSameAsSystemAppWithTimeout:(NSTimeInterval)timeout { __block XCUIApplication *systemApp = nil; __block NSException *caughtException = nil; dispatch_semaphore_t sem = dispatch_semaphore_create(0); dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{ - // +fb_systemApplication is undocumented private API; some of its XCUIApplication siblings - // (e.g. -terminate) hard-assert when called off the main thread, so guard against this one - // doing the same on some other Xcode/iOS version - an uncaught exception thrown from inside a - // bare dispatch_async block has no handler and would crash the whole process. + // Undocumented private API; guard in case it hard-asserts off-main like -terminate does on + // some Xcode/iOS version - uncaught, that would crash the whole process. @try { systemApp = XCUIApplication.fb_systemApplication; } @catch (NSException *e) { @@ -378,13 +359,9 @@ - (BOOL)fb_isTestedApplicationSameAsSystemAppWithTimeout:(NSTimeInterval)timeout return [self.testedApplication fb_isSameAppAs:systemApp]; } -// -[XCUIApplication terminate] hard-asserts when called off the main thread, but -kill (the only -// caller) can now itself run on a background queue - DELETE /session is a standalone route (see -// FBHTTPServer.m) that bypasses the main routeQueue. Dispatching to main and waiting indefinitely -// would reintroduce the exact hang standalone routes exist to avoid, if that's the queue currently -// stuck servicing some other request against the frozen app; give up after `timeout` instead. The -// dispatched block still runs (and still terminates the app) whenever main frees up, even after -// this method has given up waiting on it. +// -terminate hard-asserts off-main, but -kill can now run on a background queue. Dispatching to +// main and waiting indefinitely could hang just as long as main is stuck, so give up after +// `timeout`; the dispatched call still runs (and terminates the app) whenever main frees up. - (void)fb_terminateTestedApplicationWithTimeout:(NSTimeInterval)timeout { XCUIApplication *application = self.testedApplication; diff --git a/WebDriverAgentLib/Utilities/FBXCodeCompatibility.m b/WebDriverAgentLib/Utilities/FBXCodeCompatibility.m index fda9f7200a..26cd2dc6e6 100644 --- a/WebDriverAgentLib/Utilities/FBXCodeCompatibility.m +++ b/WebDriverAgentLib/Utilities/FBXCodeCompatibility.m @@ -81,10 +81,8 @@ + (BOOL)fb_areKeyEventsSupported NSInteger FBTestmanagerdVersion(void) { - // Not a dispatch_once: a `dispatch_once` here would permanently cache the timeout fallback below - // if the very first call's daemon reply merely arrived late (busy, not hung), instead of the real - // negotiated version. -1 means "not yet successfully determined" - only a real reply (or the - // always-correct modern-testmanagerd branch) is cached; a timeout is retried on the next call. + // Not dispatch_once: that would permanently cache the timeout fallback below if the first call's + // reply merely arrived late. -1 means "not yet determined"; a timeout isn't cached, so it retries. static NSInteger cachedVersion = -1; static dispatch_queue_t syncQueue; static dispatch_once_t onceToken; @@ -110,19 +108,15 @@ NSInteger FBTestmanagerdVersion(void) }]; int64_t timeoutNs = (int64_t)(TESTMANAGERD_VERSION_TIMEOUT_SEC * NSEC_PER_SEC); if (0 != dispatch_semaphore_wait(sem, dispatch_time(DISPATCH_TIME_NOW, timeoutNs))) { - // Assume newest/full-featured on timeout, mirroring the modern-testmanagerd branch below - - // but don't cache it, so a merely-slow (not hung) daemon gets a real answer on a later call. + // Assume newest/full-featured on timeout, but don't cache it - retry on the next call. [FBLogger logFmt:@"Did not receive a testmanagerd protocol version reply within %d seconds; assuming the newest/full-featured protocol", TESTMANAGERD_VERSION_TIMEOUT_SEC]; result = 0xFFFF; return; } result = receivedVersion; } else { - // Modern testmanagerd (Xcode 15+) has already negotiated named XCTCapabilities by the time - // a daemon session exists, instead of a single scalar protocol version. There is no direct - // integer equivalent to report here (this value is diagnostic-only, surfaced via the - // 'testmanagerdVersion' session capability), so keep reporting the existing "assume - // newest/full-featured" sentinel, while confirming capabilities did negotiate successfully. + // Modern testmanagerd (Xcode 15+) negotiates named XCTCapabilities instead of a scalar + // version; there's no direct integer equivalent, so just confirm capabilities negotiated. XCTCapabilities *capabilities = [XCTRunnerDaemonSession sharedSession].remoteInterfaceCapabilities; if (nil == capabilities) { [FBLogger log:@"Could not retrieve testmanagerd capabilities"]; From bb96680506553a7cf98ccec7a7f06266b60027d7 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sun, 23 Aug 2026 15:08:10 +0200 Subject: [PATCH 08/10] fix: resolve compiler/analyzer warnings in HTTP server files - FBConfiguration -bindingPortRange: collapse to a single return statement so the compiler can elide the copy (-Wnrvo). - FBTCPSocket -acceptConnection:: read weakSelf into a strong local before use, instead of a second direct weak read in the same block (-Warc-repeated-use-of-weak). - RouteResponse: mutableHeaders was declared `copy` on a mutable dictionary type, which would silently store an immutable object if ever assigned through the property setter; declare it `strong` instead, matching how it's actually used (osx.ObjCProperty). --- WebDriverAgentLib/Routing/FBTCPSocket.m | 3 ++- WebDriverAgentLib/Routing/RouteResponse.m | 2 +- WebDriverAgentLib/Utilities/FBConfiguration.m | 17 +++++++---------- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/WebDriverAgentLib/Routing/FBTCPSocket.m b/WebDriverAgentLib/Routing/FBTCPSocket.m index 0b8ea5112b..769add707a 100644 --- a/WebDriverAgentLib/Routing/FBTCPSocket.m +++ b/WebDriverAgentLib/Routing/FBTCPSocket.m @@ -135,7 +135,8 @@ - (void)acceptConnection:(nw_connection_t)connection } [strongSelf scheduleReceiveForConnection:connection]; } else if (nw_connection_state_failed == state || nw_connection_state_cancelled == state) { - [weakSelf handleDisconnectForConnection:connection]; + __strong typeof(weakSelf) strongSelf = weakSelf; + [strongSelf handleDisconnectForConnection:connection]; } }); nw_connection_start(connection); diff --git a/WebDriverAgentLib/Routing/RouteResponse.m b/WebDriverAgentLib/Routing/RouteResponse.m index 393ccab05c..eb383724bb 100644 --- a/WebDriverAgentLib/Routing/RouteResponse.m +++ b/WebDriverAgentLib/Routing/RouteResponse.m @@ -9,7 +9,7 @@ #import "RouteResponse.h" @interface RouteResponse () -@property (nonatomic, copy) NSMutableDictionary *mutableHeaders; +@property (nonatomic, strong) NSMutableDictionary *mutableHeaders; @end @implementation RouteResponse diff --git a/WebDriverAgentLib/Utilities/FBConfiguration.m b/WebDriverAgentLib/Utilities/FBConfiguration.m index 65bb372474..6a37b431ca 100644 --- a/WebDriverAgentLib/Utilities/FBConfiguration.m +++ b/WebDriverAgentLib/Utilities/FBConfiguration.m @@ -124,17 +124,14 @@ - (NSRange)bindingPortRange { // 'WebDriverAgent --port 8080' can be passed via the arguments to the process NSRange rangeFromArguments = [self.class bindingPortRangeFromArguments]; - if (rangeFromArguments.location != NSNotFound) { - return rangeFromArguments; + if (rangeFromArguments.location == NSNotFound) { + // Existence of USE_PORT in the environment implies the port range is managed by the launching process. + NSString *usePort = NSProcessInfo.processInfo.environment[@"USE_PORT"]; + rangeFromArguments = usePort.length > 0 + ? NSMakeRange((NSUInteger)usePort.integerValue, 1) + : NSMakeRange(DefaultStartingPort, DefaultPortRange); } - - // Existence of USE_PORT in the environment implies the port range is managed by the launching process. - if (NSProcessInfo.processInfo.environment[@"USE_PORT"] && - [NSProcessInfo.processInfo.environment[@"USE_PORT"] length] > 0) { - return NSMakeRange([NSProcessInfo.processInfo.environment[@"USE_PORT"] integerValue] , 1); - } - - return NSMakeRange(DefaultStartingPort, DefaultPortRange); + return rangeFromArguments; } - (NSString *)bindingIPAddress From 91d6a1cef46ff4e909f7706851b9afeed583fd8d Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sun, 23 Aug 2026 22:08:11 +0200 Subject: [PATCH 09/10] fix: address second round of PR #1222 review comments - FBHTTPServer: move the buffer append itself onto bufferProcessingQueue, not just the parse. -client:didReceiveData: previously appended under a separate lock, so a receive callback could still mutate a connection's buffer while -processBufferForClient: was reading it unlocked on the processing queue. - FBSession: replace the per-instance kill-wait with class-level teardown-in-progress state, and add +killActiveSessionAndWaitForTeardown. The per-instance wait only helped a caller that still held a reference to the outgoing session; handleCreateSession: instead reads FBSession.activeSession fresh, so once a concurrent -kill had already cleared the pointer, it saw nil and proceeded to launch the replacement app without waiting for that -kill's teardown - including its app termination - to actually finish. - FBSession -fb_terminateTestedApplicationWithTimeout:: make the deferred main-thread -terminate call cancelable. If the bounded wait times out, the call is marked "given up on" under the same lock the deferred block checks before actually calling -terminate, so it can no longer fire later against a replacement session's app once -kill has reported its teardown finished. All FBSessionTests pass; WebDriverAgentLib builds clean. --- .../Commands/FBSessionCommands.m | 4 +- WebDriverAgentLib/Routing/FBHTTPServer.m | 29 +++--- WebDriverAgentLib/Routing/FBSession.h | 7 ++ WebDriverAgentLib/Routing/FBSession.m | 91 ++++++++++++++----- 4 files changed, 91 insertions(+), 40 deletions(-) diff --git a/WebDriverAgentLib/Commands/FBSessionCommands.m b/WebDriverAgentLib/Commands/FBSessionCommands.m index edc04aec69..e8b886ac78 100644 --- a/WebDriverAgentLib/Commands/FBSessionCommands.m +++ b/WebDriverAgentLib/Commands/FBSessionCommands.m @@ -89,9 +89,7 @@ + (NSArray *)routes + (id)handleCreateSession:(FBRouteRequest *)request { - if (nil != FBSession.activeSession) { - [FBSession.activeSession kill]; - } + [FBSession killActiveSessionAndWaitForTeardown]; NSDictionary *capabilities; id errorResponse = [self capabilitiesFromCreateSessionRequest:request diff --git a/WebDriverAgentLib/Routing/FBHTTPServer.m b/WebDriverAgentLib/Routing/FBHTTPServer.m index 9d5259060e..024011655e 100644 --- a/WebDriverAgentLib/Routing/FBHTTPServer.m +++ b/WebDriverAgentLib/Routing/FBHTTPServer.m @@ -87,9 +87,8 @@ @interface FBHTTPServer () // Per-client cache of the already-parsed request line + headers while its body is still // arriving; nil while a client's next unread bytes start with an unparsed header block. @property (nonatomic, strong) NSMapTable *pendingRequestHeaders; -// -processBufferForClient: parses a connection's buffer unlocked; safe only because every caller -// (-client:didReceiveData: and -writeResponse:toClient:thenCloseConnection:) funnels through this -// one serial queue, so no two calls ever run concurrently. +// All buffer access - appending new data and -processBufferForClient:'s unlocked parse - is +// funneled through this one serial queue, so appends can never race a parse. @property (nonatomic, strong) dispatch_queue_t bufferProcessingQueue; // Connections with a request parsed off the buffer but not yet answered. Blocks // -processBufferForClient: from starting the next pipelined request, so responses on one @@ -264,17 +263,23 @@ - (void)didClientDisconnect:(nw_connection_t)client - (void)client:(nw_connection_t)client didReceiveData:(NSData *)data { - NSMutableData *buffer; - @synchronized (self.connectionBuffers) { - buffer = [self.connectionBuffers objectForKey:client]; - if (nil == buffer) { - return; - } - [buffer appendData:data]; - } + // The append itself, not just the parse, must run on bufferProcessingQueue: otherwise a receive + // callback here could still mutate the buffer while -processBufferForClient: is reading it + // unlocked on that queue. __weak typeof(self) weakSelf = self; dispatch_async(self.bufferProcessingQueue, ^{ - [weakSelf processBufferForClient:client]; + __strong typeof(weakSelf) strongSelf = weakSelf; + if (nil == strongSelf) { + return; + } + @synchronized (strongSelf.connectionBuffers) { + NSMutableData *buffer = [strongSelf.connectionBuffers objectForKey:client]; + if (nil == buffer) { + return; + } + [buffer appendData:data]; + } + [strongSelf processBufferForClient:client]; }); } diff --git a/WebDriverAgentLib/Routing/FBSession.h b/WebDriverAgentLib/Routing/FBSession.h index e4a810c394..821891ea36 100644 --- a/WebDriverAgentLib/Routing/FBSession.h +++ b/WebDriverAgentLib/Routing/FBSession.h @@ -50,6 +50,13 @@ extern NSString* const FBSessionWasKilledNotification; + (nullable instancetype)activeSession; +/** + Kills the active session, if any, and blocks until its teardown - including one already started + by a concurrent caller - is fully finished. Call this before preparing/launching a replacement + application, so it can't race a still-in-progress termination of the outgoing one. + */ ++ (void)killActiveSessionAndWaitForTeardown; + /** Fetches session for given identifier. If identifier doesn't match activeSession identifier, will return nil. diff --git a/WebDriverAgentLib/Routing/FBSession.m b/WebDriverAgentLib/Routing/FBSession.m index b589009d89..34a80db0ea 100644 --- a/WebDriverAgentLib/Routing/FBSession.m +++ b/WebDriverAgentLib/Routing/FBSession.m @@ -48,9 +48,6 @@ @interface FBSession () @property (nonatomic) BOOL shouldAppsWaitForQuiescence; @property (nonatomic, nullable) FBAlertsMonitor *alertsMonitor; @property (nonatomic, readwrite) NSMutableDictionary *> *elementsVisibilityCache; -// Lets a -kill caller that loses the race wait for the winner's teardown to finish. See -init. -@property (nonatomic, strong, readonly) NSCondition *killCondition; -@property (nonatomic, assign) BOOL isKillFinished; - (BOOL)fb_isTestedApplicationSameAsSystemAppWithTimeout:(NSTimeInterval)timeout; - (void)fb_terminateTestedApplicationWithTimeout:(NSTimeInterval)timeout; @@ -104,13 +101,30 @@ - (void)didDetectAlert:(FBAlert *)alert @implementation FBSession static FBSession *_activeSession = nil; +// Class-level, not per-instance: a caller that finds _activeSession already nil (a concurrent +// -kill beat it there) still needs to know whether that -kill's teardown is done, since it cleared +// the pointer before running it. See +waitForActiveTeardownWithTimeout:. +static BOOL _isTeardownInProgress = NO; -- (instancetype)init ++ (NSCondition *)teardownCondition { - if ((self = [super init])) { - _killCondition = [NSCondition new]; + static NSCondition *condition; + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + condition = [NSCondition new]; + }); + return condition; +} + +// Waits (bounded) for any -kill teardown currently in progress to finish. ++ (void)waitForActiveTeardownWithTimeout:(NSTimeInterval)timeout +{ + NSCondition *condition = self.teardownCondition; + [condition lock]; + NSDate *deadline = [NSDate dateWithTimeIntervalSinceNow:timeout]; + while (_isTeardownInProgress && [condition waitUntilDate:deadline]) { } - return self; + [condition unlock]; } + (instancetype)activeSession @@ -118,11 +132,23 @@ + (instancetype)activeSession return _activeSession; } -+ (void)markSessionActive:(FBSession *)session ++ (void)killActiveSessionAndWaitForTeardown { - if (_activeSession) { - [_activeSession kill]; + FBSession *session = _activeSession; + if (nil != session) { + // Runs the real teardown synchronously if this call wins the race in -kill, or waits for + // whoever did to finish if it lost - either way, blocks until torn down. + [session kill]; + } else { + // _activeSession is already nil, but a concurrent -kill (e.g. from DELETE /session) may still + // be mid-teardown - wait for it, so we don't launch a replacement app too early. + [self waitForActiveTeardownWithTimeout:FB_KILL_WAIT_TIMEOUT_SEC]; } +} + ++ (void)markSessionActive:(FBSession *)session +{ + [self killActiveSessionAndWaitForTeardown]; _activeSession = session; } @@ -204,14 +230,15 @@ - (void)kill if (!wasActive) { // Someone else is already tearing this session down - wait for that to finish (bounded), so // we don't act as if it's gone (e.g. launch a new app) while its -terminate is still in flight. - [self.killCondition lock]; - NSDate *deadline = [NSDate dateWithTimeIntervalSinceNow:FB_KILL_WAIT_TIMEOUT_SEC]; - while (!self.isKillFinished && [self.killCondition waitUntilDate:deadline]) { - } - [self.killCondition unlock]; + [self.class waitForActiveTeardownWithTimeout:FB_KILL_WAIT_TIMEOUT_SEC]; return; } + NSCondition *teardownCondition = self.class.teardownCondition; + [teardownCondition lock]; + _isTeardownInProgress = YES; + [teardownCondition unlock]; + @try { // Posted before teardown so pending HTTP requests for this session can stop waiting sooner. [NSNotificationCenter.defaultCenter postNotificationName:FBSessionWasKilledNotification object:self]; @@ -231,13 +258,16 @@ - (void)kill && FBConfiguration.sharedInstance.shouldTerminateApp && self.testedApplication.running && ![self fb_isTestedApplicationSameAsSystemAppWithTimeout:FB_IS_SYSTEM_APP_CHECK_TIMEOUT_SEC]) { + // Blocks until the app is either actually terminated or durably given up on (never left + // pending) - see -fb_terminateTestedApplicationWithTimeout: - so it's safe to report this + // teardown as finished as soon as this returns. [self fb_terminateTestedApplicationWithTimeout:FB_APP_TERMINATE_TIMEOUT_SEC]; } } @finally { - [self.killCondition lock]; - self.isKillFinished = YES; - [self.killCondition broadcast]; - [self.killCondition unlock]; + [teardownCondition lock]; + _isTeardownInProgress = NO; + [teardownCondition broadcast]; + [teardownCondition unlock]; } } @@ -361,22 +391,33 @@ - (BOOL)fb_isTestedApplicationSameAsSystemAppWithTimeout:(NSTimeInterval)timeout // -terminate hard-asserts off-main, but -kill can now run on a background queue. Dispatching to // main and waiting indefinitely could hang just as long as main is stuck, so give up after -// `timeout`; the dispatched call still runs (and terminates the app) whenever main frees up. +// `timeout` - but a "given up on" call must never still terminate whatever's running by the time +// main gets to it (e.g. a replacement session's app), so cancellation and the actual terminate +// call share a lock: whichever gets there first - the dispatched block, or the timeout - wins. - (void)fb_terminateTestedApplicationWithTimeout:(NSTimeInterval)timeout { XCUIApplication *application = self.testedApplication; + NSObject *lock = [NSObject new]; + __block BOOL isAllowedToTerminate = YES; dispatch_semaphore_t sem = dispatch_semaphore_create(0); dispatch_async(dispatch_get_main_queue(), ^{ - @try { - [application terminate]; - } @catch (NSException *e) { - [FBLogger logFmt:@"%@", e.description]; + @synchronized (lock) { + if (isAllowedToTerminate) { + @try { + [application terminate]; + } @catch (NSException *e) { + [FBLogger logFmt:@"%@", e.description]; + } + } } dispatch_semaphore_signal(sem); }); int64_t timeoutNs = (int64_t)(timeout * NSEC_PER_SEC); if (0 != dispatch_semaphore_wait(sem, dispatch_time(DISPATCH_TIME_NOW, timeoutNs))) { - [FBLogger logFmt:@"Could not terminate '%@' within %@ seconds; the main thread may still be busy servicing another request", application.bundleID, @(timeout)]; + @synchronized (lock) { + isAllowedToTerminate = NO; + } + [FBLogger logFmt:@"Could not terminate '%@' within %@ seconds; giving up on it rather than risk terminating a possible replacement session's app later", application.bundleID, @(timeout)]; } } From cdce07cb4cda6e38e0d2bcbc3e86e03b25277b31 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sun, 23 Aug 2026 22:24:10 +0200 Subject: [PATCH 10/10] docs: remove obsolete CocoaHTTPServer/RoutingHTTPServer mentions Those vendored dependencies were removed when the HTTP server was unified on Network.framework; nothing in the tree references them anymore. Co-Authored-By: Claude Sonnet 5 --- README.md | 10 ---------- WebDriverAgentLib/Routing/FBHTTPServer.m | 3 +-- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/README.md b/README.md index a367763f77..345e1d1571 100644 --- a/README.md +++ b/README.md @@ -42,15 +42,5 @@ Then, you find `WebDriverAgentRunner-Runner-sim-.zip` for iOS and `Web [`WebDriverAgent` is BSD-licensed](LICENSE). -## Third Party Sources - -WebDriverAgent depends on the following third-party frameworks: -- [CocoaHTTPServer](https://github.com/robbiehanson/CocoaHTTPServer) -- [RoutingHTTPServer](https://github.com/mattstevens/RoutingHTTPServer) - -These projects haven't been maintained in a while. That's why the source code of these -projects has been integrated directly in the WebDriverAgent source tree. - -You can find the source files and their licenses in the `WebDriverAgentLib/Vendor` directory. Have fun! diff --git a/WebDriverAgentLib/Routing/FBHTTPServer.m b/WebDriverAgentLib/Routing/FBHTTPServer.m index 024011655e..1532086a9c 100644 --- a/WebDriverAgentLib/Routing/FBHTTPServer.m +++ b/WebDriverAgentLib/Routing/FBHTTPServer.m @@ -145,8 +145,7 @@ - (FBHTTPRoute *)compiledRouteWithPath:(NSString *)path FBHTTPRoute *route = [FBHTTPRoute new]; NSMutableArray *keys = [NSMutableArray array]; - // Escape regex-significant characters before substituting :param placeholders, like - // RoutingHTTPServer.m used to. + // Escape regex-significant characters before substituting :param placeholders. NSRegularExpression *escapeRegex = [NSRegularExpression regularExpressionWithPattern:@"[.+()]" options:(NSRegularExpressionOptions)0 error:nil];