feat: bound accessibility snapshot requests to avoid indefinite hangs - #1214
feat: bound accessibility snapshot requests to avoid indefinite hangs#1214mykola-mokhnach wants to merge 3 commits into
Conversation
|
I will review it and run some tests tomorrow, very hectic day today. |
Dan-Maor
left a comment
There was a problem hiding this comment.
I've ran some tests on my device and overall it works, but there may cases where false negatives would block snapshotting.
…#1210) An app that stops answering accessibility requests could previously block WDA forever, since XCTest offers no bounded timeout for the underlying snapshot request. Adds a new accessibilityDeadline setting/capability that, when set, aborts the request with a clear error instead of waiting indefinitely, caching the unresponsive state briefly so XCTest's own internal retries fail fast instead of each re-waiting the full timeout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bedab8a to
ea9588f
Compare
The accessibilityDeadline swizzle can now call monitoredApplicationWithProcessIdentifier: concurrently from different threads, exposing a pre-existing race on the unsynchronized appsCache dictionary. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Thanks for addressing the existing appsCache synchronization issue. I also reviewed the observed CI failures; they look simulator/environment-related rather than caused by this PR.
I still see three actionable gaps:
-
accessibilityDeadlineis not a hard bound on the snapshot request. The deadline only applies to thenotifyWhenEventLoopIsIdleForApplication:pre-check. Once that succeeds,original_requestSnapshotForElement:attributes:parameters:error:is called without a timeout and may still hang indefinitely. An idle main run loop does not guarantee that the AX subsystem—or the subsequent XCTest snapshot request—will respond. Please either bound the actual request or make the API name, documentation, PR description/title, and error semantics explicit that this is only a best-effort responsiveness guard rather than a deadline for the accessibility operation itself. -
The per-PID unresponsive cache also suppresses independent later commands. After one timeout, every request for that PID fails fast for up to the full configured deadline, even if the app recovers immediately and the client sends a new WebDriver command. The stated goal is to collapse XCTest's internal retries, but the cache is global by PID/time rather than scoped to one request/retry chain. Please scope it to the internal retry sequence, clear/re-probe it at a new command boundary, or otherwise avoid blocking recovered applications for the remainder of the deadline.
-
GET /wda/activeAppInfocan still hang before the swizzled snapshot method is reached. Making a nil.identifiersafe prevents the crash after an aborted fetch, but active-app resolution has earlier accessibility-dependent paths that may block beforerequestSnapshotForElement:. The new integration test invokes the lower-level snapshot path directly, so it does not validate the route end-to-end. Please add route-level coverage (including a frozen app) or document/test the remaining unbounded paths explicitly.
These issues do not negate the usefulness of the workaround, but the current implementation and wording overstate the guarantee and the cache can delay recovery in a way users would not expect.
(made this with codex)
It was already mentioned above that this solution is not a silver bullet, but rather a best-effort try to patch the known xctest API behavior. I will also document that in xcuitest's setting description. Do you want to add a note into more places?
I don't expect the request for pid api to ever block for too long or stall. It's not making request to the accessibility layer. Are you able to empirically reproduce a situation where this API can stall or know any particular steps to make that happen?
I did check this path empirically and figured out the only invocation that blocks is the snapshotting request, which is in turn only invoked if we try to fetch application identifier. Ofc, I can add the whole app info harness to that test, but I don't feel it's going to be useful as there are many other endpoints that might be invoked as well and I don't want to simply put them all into such test. What we have now is just the result of the common finding after experimenting locally. |
KazuCocoa
left a comment
There was a problem hiding this comment.
Lg, I did some real device testing
Summary
Fixes #1210: an app that stops answering accessibility requests could
previously block the whole WDA server forever, since XCTest offers no
bounded timeout for the underlying accessibility snapshot request that
backs most element/attribute lookups (including
.identifier, active-appdetection, etc.).
accessibilityDeadlinesetting/capability (NSTimeInterval,disabled by default). When set to a value > 0, a swizzle on
-[XCAXClient_iOS requestSnapshotForElement:attributes:parameters:error:]first confirms the target app's main run loop is genuinely responsive
(via the private
notifyWhenEventLoopIsIdleForApplication:API, wrappedin
FBXCAXClientProxy) before letting the real snapshot request proceed.If the app doesn't confirm responsiveness within the deadline, the
request is aborted with a clear "illegal/unresponsive application state"
error instead of risking an indefinite hang.
giving up. To avoid multiplying the configured deadline by the retry
count, a short-lived per-pid cache remembers "this app was just confirmed
unresponsive," so those retries fail fast instead of each re-running the
full wait.
handleActiveAppInfo:(GET /wda/activeAppInfo) is made resilient to.identifierreturningnil(which can legitimately happen once the newdeadline aborts the underlying fetch) - it now reports
"unknown"forthe app name instead of crashing.
Test plan
FBConfigurationTests.testAccessibilityDeadlineAbortsSnapshotRequestForDeadlockedAppfreezes the
IntegrationAppfixture's main thread (Deadlock appbutton,reworked to sleep instead of self-deadlock so it doesn't trip the OS
watchdog) and asserts that a snapshot request against it aborts within
bounds instead of hanging. Skipped on CI (
CIenv var) since itdeliberately freezes the app for ~20s.
accessibilityDeadlineset, a frozen app now fails gracefully in a fewseconds; with it unset (default), original unbounded behavior is
preserved unchanged.
WebDriverAgentLiband the new test target build and pass locally.