fix: harden thread-safety edges from the capture-load reliability work

WebDriverAgentLib/Commands/FBSessionCommands.h

@@ -14,6 +14,13 @@ NS_ASSUME_NONNULL_BEGIN
 
 @interface FBSessionCommands : NSObject <FBCommandHandler>
 
+/**
+ 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<NSString *, NSString *> *)cachedDeviceInfo;
+
 @end
 
 NS_ASSUME_NONNULL_END

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<NSString *, NSString *> *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<NSString *, NSString *> *)cachedDeviceInfo
+{
+  static NSDictionary<NSString *, NSString *> *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

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"
@@ -99,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;
   }

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;
@@ -83,15 +91,51 @@ - (nullable FBVideoStreamSession *)startSessionWithConfiguration:(FBScreenCaptur
   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;
+    // 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
+  // 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];
+
+  @synchronized (self.sessions) {
+    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.
     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;
@@ -130,7 +174,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;
@@ -152,8 +196,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;

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.

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,12 @@ - (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 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.1];
+  [NSThread sleepForTimeInterval:0.3];
   [self fireRequestForPath:@"/probe/spinning"];
 
   NSDate *deadline = [NSDate dateWithTimeIntervalSinceNow:10.0];

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