From 947ab6437cb1f522627e78b66264f2485b71e9b7 Mon Sep 17 00:00:00 2001 From: timo <44401485+Timo972@users.noreply.github.com> Date: Wed, 26 Aug 2026 00:48:06 +0200 Subject: [PATCH 1/5] fix: harden thread-safety edges from the capture-load reliability work Follow-ups from the final review of #23 (DRO-2713 layers 1+2): - FBVideoStreamManager: hoist the XCUIScreen.mainScreen displayID read out of the @synchronized (self.sessions) region so a wedged XCUI call cannot block the control-marked capture routes on the same monitor. - /status: snapshot UIDevice systemName/systemVersion/userInterfaceIdiom behind a dispatch_once (FBSessionCommands.cachedDeviceInfo) and burn the once-token on the main thread in FBWebServer startServing, next to the existing status-cache pre-warm - the handler reads them from a connection queue while UIDevice is formally main-thread-only. - testAutomationRequestsDoNotNestInsideRunLoopSpin: widen the gap between the two probe requests from 0.1 s to 0.3 s (matching the analogous test) and the probe's spin window from 0.4 s to 1.0 s so the second request still lands well inside the first's spin on a loaded CI runner. Co-Authored-By: Claude Fable 5 --- .../Commands/FBSessionCommands.h | 7 ++++ .../Commands/FBSessionCommands.m | 35 ++++++++++++++----- WebDriverAgentLib/Routing/FBWebServer.m | 21 ++++++----- .../Utilities/FBVideoStreamManager.m | 7 +++- WebDriverAgentTests/UnitTests/FBRouteTests.m | 6 ++-- 5 files changed, 55 insertions(+), 21 deletions(-) diff --git a/WebDriverAgentLib/Commands/FBSessionCommands.h b/WebDriverAgentLib/Commands/FBSessionCommands.h index 95f3f258fd..ff1f4c58bb 100644 --- a/WebDriverAgentLib/Commands/FBSessionCommands.h +++ b/WebDriverAgentLib/Commands/FBSessionCommands.h @@ -14,6 +14,13 @@ NS_ASSUME_NONNULL_BEGIN @interface FBSessionCommands : NSObject +/** + Device properties served by /status (OS name, OS version, device kind), snapshotted once + behind a dispatch_once. /status runs off the main queue while UIDevice is formally + main-thread-only UIKit API, so FBWebServer burns the once-token on the main thread at startup. + */ ++ (NSDictionary *)cachedDeviceInfo; + @end NS_ASSUME_NONNULL_END diff --git a/WebDriverAgentLib/Commands/FBSessionCommands.m b/WebDriverAgentLib/Commands/FBSessionCommands.m index b23027daf0..800c86ff40 100644 --- a/WebDriverAgentLib/Commands/FBSessionCommands.m +++ b/WebDriverAgentLib/Commands/FBSessionCommands.m @@ -193,15 +193,10 @@ + (NSArray *)routes [buildInfo setObject:version forKey:@"version"]; } -#if TARGET_OS_WATCH - NSString *osName = @"watchOS"; - NSString *osVersion = WKInterfaceDevice.currentDevice.systemVersion; - NSString *deviceKind = @"watch"; -#else - NSString *osName = [[UIDevice currentDevice] systemName]; - NSString *osVersion = [[UIDevice currentDevice] systemVersion]; - NSString *deviceKind = [self.class deviceNameByUserInterfaceIdiom:[UIDevice currentDevice].userInterfaceIdiom]; -#endif + NSDictionary *deviceInfo = [self.class cachedDeviceInfo]; + NSString *osName = deviceInfo[@"osName"]; + NSString *osVersion = deviceInfo[@"osVersion"]; + NSString *deviceKind = deviceInfo[@"deviceKind"]; return FBResponseWithObject( @{ @@ -447,6 +442,28 @@ + (NSDictionary *)sessionInformation }; } ++ (NSDictionary *)cachedDeviceInfo +{ + static NSDictionary *deviceInfo; + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ +#if TARGET_OS_WATCH + deviceInfo = @{ + @"osName": @"watchOS", + @"osVersion": WKInterfaceDevice.currentDevice.systemVersion, + @"deviceKind": @"watch", + }; +#else + deviceInfo = @{ + @"osName": [[UIDevice currentDevice] systemName], + @"osVersion": [[UIDevice currentDevice] systemVersion], + @"deviceKind": [self deviceNameByUserInterfaceIdiom:[UIDevice currentDevice].userInterfaceIdiom], + }; +#endif + }); + return deviceInfo; +} + #if !TARGET_OS_WATCH /* Return the device kind as lower case diff --git a/WebDriverAgentLib/Routing/FBWebServer.m b/WebDriverAgentLib/Routing/FBWebServer.m index 4578f9993c..8bff5fbcfd 100644 --- a/WebDriverAgentLib/Routing/FBWebServer.m +++ b/WebDriverAgentLib/Routing/FBWebServer.m @@ -25,6 +25,7 @@ #import "FBRouteRequest.h" #import "FBRuntimeUtils.h" #import "FBSession.h" +#import "FBSessionCommands.h" #import "FBUnknownCommands.h" #import "FBConfiguration.h" #import "FBLogger.h" @@ -109,17 +110,19 @@ - (void)startServing #endif self.keepAlive = YES; - // /status is served off the main queue (it uses onControlQueue), but FBSDKVersion() and - // FBTestmanagerdVersion() cache their result behind a dispatch_once. Burn both once-tokens - // here, on the main thread, warmed only after the server has bound: FBTestmanagerdVersion()'s - // legacy branch waits (with a bounded timeout) on the daemon, and a degraded daemon must not - // be able to prevent the server from binding. An early request that races the warm-up just - // blocks on the dispatch_once for at most the bounded handshake. Warmed only after - // initialization is complete and keepAlive is set, so a shutdown that arrives while the - // bounded legacy handshake spins the run loop simply clears keepAlive via stopServing and the - // serving loop below never starts. + // /status is served off the main queue (it uses onControlQueue), but FBSDKVersion(), + // FBTestmanagerdVersion() and FBSessionCommands.cachedDeviceInfo cache their result behind a + // dispatch_once. Burn the once-tokens here, on the main thread (UIDevice, read by + // cachedDeviceInfo, is formally main-thread-only UIKit API), warmed only after the server has + // bound: FBTestmanagerdVersion()'s legacy branch waits (with a bounded timeout) on the daemon, + // and a degraded daemon must not be able to prevent the server from binding. An early request + // that races the warm-up just blocks on the dispatch_once for at most the bounded handshake. + // Warmed only after initialization is complete and keepAlive is set, so a shutdown that + // arrives while the bounded legacy handshake spins the run loop simply clears keepAlive via + // stopServing and the serving loop below never starts. FBSDKVersion(); FBTestmanagerdVersion(); + [FBSessionCommands cachedDeviceInfo]; NSRunLoop *runLoop = [NSRunLoop mainRunLoop]; while (self.keepAlive) { @try { diff --git a/WebDriverAgentLib/Utilities/FBVideoStreamManager.m b/WebDriverAgentLib/Utilities/FBVideoStreamManager.m index 261d043bd3..4c4fda50ce 100644 --- a/WebDriverAgentLib/Utilities/FBVideoStreamManager.m +++ b/WebDriverAgentLib/Utilities/FBVideoStreamManager.m @@ -119,6 +119,11 @@ - (nullable FBVideoStreamSession *)startSessionWithConfiguration:(FBScreenCaptur return nil; } + // Read outside the sessions lock: XCUIScreen goes through the automation machinery, and if it + // ever wedges it must not take the monitor down with it — the control-marked capture routes + // (stop/list/get/keyframe) block on this same lock and are supposed to stay wedge-immune. + long long mainScreenID = [XCUIScreen.mainScreen displayID]; + NSUInteger generation = 0; BOOL abortedByStopAll = NO; @synchronized (self.sessions) { @@ -130,7 +135,7 @@ - (nullable FBVideoStreamSession *)startSessionWithConfiguration:(FBScreenCaptur } else { self.pendingStarts -= 1; self.sessions[@(identifier)] = session; - self.mainScreenID = [XCUIScreen.mainScreen displayID]; + self.mainScreenID = mainScreenID; if (!self.isStreaming) { self.isStreaming = YES; self.loopGeneration += 1; diff --git a/WebDriverAgentTests/UnitTests/FBRouteTests.m b/WebDriverAgentTests/UnitTests/FBRouteTests.m index 01e420e843..4a79802abd 100644 --- a/WebDriverAgentTests/UnitTests/FBRouteTests.m +++ b/WebDriverAgentTests/UnitTests/FBRouteTests.m @@ -201,7 +201,7 @@ + (NSArray *)routes while (depth > prevMax && !atomic_compare_exchange_weak(&gSpinningProbeMaxDepth, &prevMax, depth)) { // retry until either our depth is recorded or another thread recorded a higher one } - [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:0.4]]; + [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:1.0]]; atomic_fetch_sub(&gSpinningProbeDepth, 1); atomic_fetch_add(&gSpinningProbeCompletions, 1); return FBResponseWithOK(); @@ -310,8 +310,10 @@ - (void)testAutomationRequestsDoNotNestInsideRunLoopSpin // handler; without the automation funnel a nested run loop drain would let the second // handler execute reentrantly inside the first (depth 2). With the funnel, the second // request blocks on its own connection queue until the first finishes on main (depth 1). + // The 0.3 s gap gives the first request time to reach its handler even on a loaded runner, + // while the probe's 1.0 s spin keeps the second request well inside the first's spin window. [self fireRequestForPath:@"/probe/spinning"]; - [NSThread sleepForTimeInterval:0.1]; + [NSThread sleepForTimeInterval:0.3]; [self fireRequestForPath:@"/probe/spinning"]; NSDate *deadline = [NSDate dateWithTimeIntervalSinceNow:10.0]; From 01a6d43fba62d4f526e73693471328e5af141306 Mon Sep 17 00:00:00 2001 From: timo <44401485+Timo972@users.noreply.github.com> Date: Wed, 26 Aug 2026 01:05:45 +0200 Subject: [PATCH 2/5] fix: close the remaining review nits from the DRO-2747 sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - fb_pixelBudget: reject the exact 2^64 boundary — (double)NSUIntegerMax rounds up to 2^64, so the > comparison accepted a value whose NSUInteger cast overflows (UB). Use >= and pin it with a test. - FBVideoStreamManager: give the aborted-start and session-limit errors distinct codes behind a named enum instead of a shared code 1. - Comment precision (review feedback): the FBWebServer pre-warm note no longer overstates the dispatch_once race guarantee, the hoisted displayID read documents the discarding abort path, and the retimed dispatch test explains what the 0.3 s gap actually buys. Co-Authored-By: Claude Fable 5 --- WebDriverAgentLib/Routing/FBWebServer.m | 4 +++- .../Utilities/FBVideoStreamManager.m | 17 +++++++++++++---- .../Utilities/FBVideoStreamSession.m | 4 +++- WebDriverAgentTests/UnitTests/FBRouteTests.m | 6 ++++-- .../UnitTests/FBVideoStreamSessionTests.m | 4 ++++ 5 files changed, 27 insertions(+), 8 deletions(-) diff --git a/WebDriverAgentLib/Routing/FBWebServer.m b/WebDriverAgentLib/Routing/FBWebServer.m index 8bff5fbcfd..ef75c40dc8 100644 --- a/WebDriverAgentLib/Routing/FBWebServer.m +++ b/WebDriverAgentLib/Routing/FBWebServer.m @@ -116,7 +116,9 @@ - (void)startServing // cachedDeviceInfo, is formally main-thread-only UIKit API), warmed only after the server has // bound: FBTestmanagerdVersion()'s legacy branch waits (with a bounded timeout) on the daemon, // and a degraded daemon must not be able to prevent the server from binding. An early request - // that races the warm-up just blocks on the dispatch_once for at most the bounded handshake. + // that races the warm-up just blocks on the dispatch_once for at most the bounded handshake — + // or, if it wins the race, runs the once-body itself off-main: the same exposure every /status + // request had before the snapshot, now at most once. // Warmed only after initialization is complete and keepAlive is set, so a shutdown that // arrives while the bounded legacy handshake spins the run loop simply clears keepAlive via // stopServing and the serving loop below never starts. diff --git a/WebDriverAgentLib/Utilities/FBVideoStreamManager.m b/WebDriverAgentLib/Utilities/FBVideoStreamManager.m index 4c4fda50ce..f1866cb522 100644 --- a/WebDriverAgentLib/Utilities/FBVideoStreamManager.m +++ b/WebDriverAgentLib/Utilities/FBVideoStreamManager.m @@ -30,6 +30,14 @@ static const NSTimeInterval FAILURE_BACKOFF_MAX = 10.0; static const char *QUEUE_NAME = "Screen Capture Encoder Queue"; +static NSString *const FBVideoStreamManagerErrorDomain = @"com.facebook.WebDriverAgent.FBVideoStreamManager"; +// No handler switches on these yet (they surface as unknown-error responses), but distinct +// codes keep the two start-failure modes distinguishable by more than the message text. +typedef NS_ENUM(NSInteger, FBVideoStreamManagerError) { + FBVideoStreamManagerErrorSessionLimitReached = 1, + FBVideoStreamManagerErrorStoppedWhileStarting = 2, +}; + @interface FBVideoStreamManager () @property (nonatomic) dispatch_queue_t backgroundQueue; @@ -90,8 +98,8 @@ - (nullable FBVideoStreamSession *)startSessionWithConfiguration:(FBScreenCaptur // and exceed MAX_SESSIONS. if (self.sessions.count + self.pendingStarts >= MAX_SESSIONS) { if (error) { - *error = [NSError errorWithDomain:@"com.facebook.WebDriverAgent.FBVideoStreamManager" - code:1 + *error = [NSError errorWithDomain:FBVideoStreamManagerErrorDomain + code:FBVideoStreamManagerErrorSessionLimitReached userInfo:@{NSLocalizedDescriptionKey: [NSString stringWithFormat:@"The maximum number of concurrent screen capture sessions (%@) has been reached", @(MAX_SESSIONS)]}]; } return nil; @@ -122,6 +130,7 @@ - (nullable FBVideoStreamSession *)startSessionWithConfiguration:(FBScreenCaptur // Read outside the sessions lock: XCUIScreen goes through the automation machinery, and if it // ever wedges it must not take the monitor down with it — the control-marked capture routes // (stop/list/get/keyframe) block on this same lock and are supposed to stay wedge-immune. + // Read unconditionally; the (rare) stop-all abort path below just discards it. long long mainScreenID = [XCUIScreen.mainScreen displayID]; NSUInteger generation = 0; @@ -157,8 +166,8 @@ - (nullable FBVideoStreamSession *)startSessionWithConfiguration:(FBScreenCaptur if (abortedByStopAll) { [session stop]; if (error) { - *error = [NSError errorWithDomain:@"com.facebook.WebDriverAgent.FBVideoStreamManager" - code:1 + *error = [NSError errorWithDomain:FBVideoStreamManagerErrorDomain + code:FBVideoStreamManagerErrorStoppedWhileStarting userInfo:@{NSLocalizedDescriptionKey: @"The screen capture session was stopped while it was starting"}]; } return nil; diff --git a/WebDriverAgentLib/Utilities/FBVideoStreamSession.m b/WebDriverAgentLib/Utilities/FBVideoStreamSession.m index 45b3c3d6ca..8a9387dba5 100644 --- a/WebDriverAgentLib/Utilities/FBVideoStreamSession.m +++ b/WebDriverAgentLib/Utilities/FBVideoStreamSession.m @@ -112,8 +112,10 @@ + (BOOL)fb_pixelBudget:(NSUInteger *)outBudget fromArgument:(nullable id)maxPixe } // Validate the original numeric value: integerValue would silently truncate fractions // (0.5 -> 0 disables the cap; -0.5 -> 0 passes a sign check but converts to garbage). + // The range check must be >=: (double)NSUIntegerMax rounds up to 2^64, so > would accept + // exactly 2^64 and the NSUInteger cast below would overflow (undefined behavior). double rawBudget = ((NSNumber *)maxPixels).doubleValue; - if (!isfinite(rawBudget) || rawBudget < 0 || rawBudget != floor(rawBudget) || rawBudget > (double)NSUIntegerMax) { + if (!isfinite(rawBudget) || rawBudget < 0 || rawBudget != floor(rawBudget) || rawBudget >= (double)NSUIntegerMax) { return NO; } // 1..3 cannot be honored: 2x2 = 4 is the minimum encodable size. diff --git a/WebDriverAgentTests/UnitTests/FBRouteTests.m b/WebDriverAgentTests/UnitTests/FBRouteTests.m index 4a79802abd..fe8ca3d2bb 100644 --- a/WebDriverAgentTests/UnitTests/FBRouteTests.m +++ b/WebDriverAgentTests/UnitTests/FBRouteTests.m @@ -310,8 +310,10 @@ - (void)testAutomationRequestsDoNotNestInsideRunLoopSpin // handler; without the automation funnel a nested run loop drain would let the second // handler execute reentrantly inside the first (depth 2). With the funnel, the second // request blocks on its own connection queue until the first finishes on main (depth 1). - // The 0.3 s gap gives the first request time to reach its handler even on a loaded runner, - // while the probe's 1.0 s spin keeps the second request well inside the first's spin window. + // The 0.3 s gap lets the first request get through the server and enqueued at the funnel + // before the second fires, even on a loaded runner (the handler itself only starts once the + // wait loop below spins the run loop), while the probe's 1.0 s spin keeps the second request + // well inside the first's spin window. [self fireRequestForPath:@"/probe/spinning"]; [NSThread sleepForTimeInterval:0.3]; [self fireRequestForPath:@"/probe/spinning"]; diff --git a/WebDriverAgentTests/UnitTests/FBVideoStreamSessionTests.m b/WebDriverAgentTests/UnitTests/FBVideoStreamSessionTests.m index 47134e8a31..4ca21a7b1e 100644 --- a/WebDriverAgentTests/UnitTests/FBVideoStreamSessionTests.m +++ b/WebDriverAgentTests/UnitTests/FBVideoStreamSessionTests.m @@ -206,6 +206,10 @@ - (void)testPixelBudgetArgumentParsingRejectsMalformedValues XCTAssertFalse([FBScreenCaptureConfiguration fb_pixelBudget:&budget fromArgument:@(2) deviceDefault:0]); XCTAssertFalse([FBScreenCaptureConfiguration fb_pixelBudget:&budget fromArgument:@(NAN) deviceDefault:0]); XCTAssertFalse([FBScreenCaptureConfiguration fb_pixelBudget:&budget fromArgument:@(INFINITY) deviceDefault:0]); + // (double)NSUIntegerMax rounds up to exactly 2^64, which does not fit in NSUInteger; the + // parser must reject it, not cast it (undefined behavior). + XCTAssertFalse([FBScreenCaptureConfiguration fb_pixelBudget:&budget fromArgument:@((double)NSUIntegerMax) deviceDefault:0]); + XCTAssertFalse([FBScreenCaptureConfiguration fb_pixelBudget:&budget fromArgument:@(0x1p64) deviceDefault:0]); } @end From 9889c685a27175f6b680466b6fb2299607ae7320 Mon Sep 17 00:00:00 2001 From: timo <44401485+Timo972@users.noreply.github.com> Date: Wed, 26 Aug 2026 01:15:38 +0200 Subject: [PATCH 3/5] fix: read the display ID before reserving or binding any session state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on the hoist: reading displayID between the bind and the insert left a wedge point where the socket and encoder were already live but the session was not yet visible to stopAllSessions — a stop would return while the listener, encoder, and pendingStarts reservation stayed stranded. Reading it first means a wedged XCUI call strands nothing: no monitor hold, no reservation, no bound-but-unstoppable session. Co-Authored-By: Claude Fable 5 --- WebDriverAgentLib/Utilities/FBVideoStreamManager.m | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/WebDriverAgentLib/Utilities/FBVideoStreamManager.m b/WebDriverAgentLib/Utilities/FBVideoStreamManager.m index f1866cb522..75ce5f8a79 100644 --- a/WebDriverAgentLib/Utilities/FBVideoStreamManager.m +++ b/WebDriverAgentLib/Utilities/FBVideoStreamManager.m @@ -87,6 +87,13 @@ - (instancetype)init - (nullable FBVideoStreamSession *)startSessionWithConfiguration:(FBScreenCaptureConfiguration *)configuration error:(NSError **)error { + // Read before any state is reserved or bound: XCUIScreen goes through the automation + // machinery, and if it ever wedges it must strand nothing — no sessions-monitor hold (the + // control-marked capture routes stop/list/get/keyframe block on that lock and are supposed + // to stay wedge-immune), no pendingStarts reservation, and no bound-but-uninserted session + // that stopAllSessions cannot see or stop. + long long mainScreenID = [XCUIScreen.mainScreen displayID]; + NSUInteger identifier; BOOL shouldStartLoop = NO; BOOL autoAssignPort = (0 == configuration.port); @@ -127,12 +134,6 @@ - (nullable FBVideoStreamSession *)startSessionWithConfiguration:(FBScreenCaptur return nil; } - // Read outside the sessions lock: XCUIScreen goes through the automation machinery, and if it - // ever wedges it must not take the monitor down with it — the control-marked capture routes - // (stop/list/get/keyframe) block on this same lock and are supposed to stay wedge-immune. - // Read unconditionally; the (rare) stop-all abort path below just discards it. - long long mainScreenID = [XCUIScreen.mainScreen displayID]; - NSUInteger generation = 0; BOOL abortedByStopAll = NO; @synchronized (self.sessions) { From 8dce653f1fdaf58f7353c40d9a173cb0215cf109 Mon Sep 17 00:00:00 2001 From: timo <44401485+Timo972@users.noreply.github.com> Date: Wed, 26 Aug 2026 08:38:15 +0200 Subject: [PATCH 4/5] fix: snapshot the stop generation before the display-ID lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up: moving the displayID read ahead of the reservation lock also moved it ahead of the stopGeneration snapshot, so a stop-all completing while a start was stuck in the lookup got absorbed into the start's baseline — both abort checks would pass and a live capture could appear after the stop-all had already returned success. Snapshot the generation under a brief lock before the lookup and reject the start at reservation time if it moved; the insert-time re-check keeps covering the bind window, and a wedged lookup still strands nothing. Co-Authored-By: Claude Fable 5 --- .../Utilities/FBVideoStreamManager.m | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/WebDriverAgentLib/Utilities/FBVideoStreamManager.m b/WebDriverAgentLib/Utilities/FBVideoStreamManager.m index 75ce5f8a79..5904e9c67b 100644 --- a/WebDriverAgentLib/Utilities/FBVideoStreamManager.m +++ b/WebDriverAgentLib/Utilities/FBVideoStreamManager.m @@ -87,6 +87,18 @@ - (instancetype)init - (nullable FBVideoStreamSession *)startSessionWithConfiguration:(FBScreenCaptureConfiguration *)configuration error:(NSError **)error { + NSUInteger identifier; + BOOL shouldStartLoop = NO; + BOOL autoAssignPort = (0 == configuration.port); + NSUInteger startGeneration; + // Snapshot the stop generation before the XCUIScreen read below, not inside the reservation + // lock: a stop-all that completes while this start is stuck in that lookup would otherwise be + // absorbed into the start's baseline and dodge both abort checks, letting a capture appear + // after the stop-all already returned success. + @synchronized (self.sessions) { + startGeneration = self.stopGeneration; + } + // Read before any state is reserved or bound: XCUIScreen goes through the automation // machinery, and if it ever wedges it must strand nothing — no sessions-monitor hold (the // control-marked capture routes stop/list/get/keyframe block on that lock and are supposed @@ -94,12 +106,17 @@ - (nullable FBVideoStreamSession *)startSessionWithConfiguration:(FBScreenCaptur // that stopAllSessions cannot see or stop. long long mainScreenID = [XCUIScreen.mainScreen displayID]; - NSUInteger identifier; - BOOL shouldStartLoop = NO; - BOOL autoAssignPort = (0 == configuration.port); - NSUInteger startGeneration; @synchronized (self.sessions) { - startGeneration = self.stopGeneration; + if (self.stopGeneration != startGeneration) { + // A stop-all ran to completion while this start was reading the display ID. Reject before + // reserving anything; the client's stop already promised this capture would not outlive it. + if (error) { + *error = [NSError errorWithDomain:FBVideoStreamManagerErrorDomain + code:FBVideoStreamManagerErrorStoppedWhileStarting + userInfo:@{NSLocalizedDescriptionKey: @"The screen capture session was stopped while it was starting"}]; + } + return nil; + } // Count in-flight starts toward the cap: their sessions are not inserted until after the // (slow) bind/encoder start, so without this two concurrent starts could both pass the check // and exceed MAX_SESSIONS. From 9df5b98c0b7c40a5108e7784a3d1264cd8586494 Mon Sep 17 00:00:00 2001 From: timo <44401485+Timo972@users.noreply.github.com> Date: Wed, 26 Aug 2026 08:57:07 +0200 Subject: [PATCH 5/5] fix: close the two remaining pre-warm and fast-fail ordering gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups: - Warm the /status device-info snapshot on the main thread BEFORE the server binds. Warming it after bind (next to the version pre-warms) left a window where an early /status request could win the dispatch_once and run the UIDevice reads on its connection queue — the exact off-main access the snapshot exists to remove. Unlike FBTestmanagerdVersion(), the read is cheap and local, so it cannot delay binding; the version pre-warms stay post-bind for the reasons already documented there. - Fast-fail over-cap starts before the XCUIScreen read. The hoist made a ninth start pay the displayID lookup before the capacity check, so under an XCUI stall an ineligible start could wedge the automation funnel. An advisory check in the early snapshot lock returns the limit error immediately; the authoritative check stays in the reservation lock to count starts that slip in during the lookup. Co-Authored-By: Claude Fable 5 --- WebDriverAgentLib/Routing/FBWebServer.m | 27 ++++++++++--------- .../Utilities/FBVideoStreamManager.m | 12 +++++++++ 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/WebDriverAgentLib/Routing/FBWebServer.m b/WebDriverAgentLib/Routing/FBWebServer.m index ef75c40dc8..738637f18c 100644 --- a/WebDriverAgentLib/Routing/FBWebServer.m +++ b/WebDriverAgentLib/Routing/FBWebServer.m @@ -100,6 +100,11 @@ - (void)startServing { [FBLogger logFmt:@"Built at %s %s", __DATE__, __TIME__]; self.exceptionHandler = [FBExceptionHandler new]; + // Snapshot the /status device info on the main thread BEFORE the server binds: once it + // accepts connections, an early /status request could win the dispatch_once and run the + // formally main-thread-only UIDevice reads on its connection queue. Unlike the version + // pre-warms below, this is a cheap local read that cannot delay binding. + [FBSessionCommands cachedDeviceInfo]; if (![self startHTTPServer]) { return; } @@ -110,21 +115,17 @@ - (void)startServing #endif self.keepAlive = YES; - // /status is served off the main queue (it uses onControlQueue), but FBSDKVersion(), - // FBTestmanagerdVersion() and FBSessionCommands.cachedDeviceInfo cache their result behind a - // dispatch_once. Burn the once-tokens here, on the main thread (UIDevice, read by - // cachedDeviceInfo, is formally main-thread-only UIKit API), warmed only after the server has - // bound: FBTestmanagerdVersion()'s legacy branch waits (with a bounded timeout) on the daemon, - // and a degraded daemon must not be able to prevent the server from binding. An early request - // that races the warm-up just blocks on the dispatch_once for at most the bounded handshake — - // or, if it wins the race, runs the once-body itself off-main: the same exposure every /status - // request had before the snapshot, now at most once. - // Warmed only after initialization is complete and keepAlive is set, so a shutdown that - // arrives while the bounded legacy handshake spins the run loop simply clears keepAlive via - // stopServing and the serving loop below never starts. + // /status is served off the main queue (it uses onControlQueue), but FBSDKVersion() and + // FBTestmanagerdVersion() cache their result behind a dispatch_once. Burn both once-tokens + // here, on the main thread, warmed only after the server has bound: FBTestmanagerdVersion()'s + // legacy branch waits (with a bounded timeout) on the daemon, and a degraded daemon must not + // be able to prevent the server from binding. An early request that races the warm-up just + // blocks on the dispatch_once for at most the bounded handshake. Warmed only after + // initialization is complete and keepAlive is set, so a shutdown that arrives while the + // bounded legacy handshake spins the run loop simply clears keepAlive via stopServing and the + // serving loop below never starts. FBSDKVersion(); FBTestmanagerdVersion(); - [FBSessionCommands cachedDeviceInfo]; NSRunLoop *runLoop = [NSRunLoop mainRunLoop]; while (self.keepAlive) { @try { diff --git a/WebDriverAgentLib/Utilities/FBVideoStreamManager.m b/WebDriverAgentLib/Utilities/FBVideoStreamManager.m index 5904e9c67b..d8bae7aa01 100644 --- a/WebDriverAgentLib/Utilities/FBVideoStreamManager.m +++ b/WebDriverAgentLib/Utilities/FBVideoStreamManager.m @@ -97,6 +97,18 @@ - (nullable FBVideoStreamSession *)startSessionWithConfiguration:(FBScreenCaptur // after the stop-all already returned success. @synchronized (self.sessions) { startGeneration = self.stopGeneration; + // Advisory fast-fail so a start that is already over the cap never reaches the XCUIScreen + // read: under an XCUI stall it would wedge the automation funnel despite never being + // eligible to start. The authoritative check runs in the reservation lock below, where + // concurrent starts that slipped in during the lookup are still counted. + if (self.sessions.count + self.pendingStarts >= MAX_SESSIONS) { + if (error) { + *error = [NSError errorWithDomain:FBVideoStreamManagerErrorDomain + code:FBVideoStreamManagerErrorSessionLimitReached + userInfo:@{NSLocalizedDescriptionKey: [NSString stringWithFormat:@"The maximum number of concurrent screen capture sessions (%@) has been reached", @(MAX_SESSIONS)]}]; + } + return nil; + } } // Read before any state is reserved or bound: XCUIScreen goes through the automation