Repository navigation
fix(warp): do not warp into an app that is no longer frontmost - #35
Merged
Merged
Conversation
warpAfterHandover reads the frontmost app, then makes about eight cross-process Accessibility calls to find its focused window and judge it, and only then moves the pointer. Switching again during those reads left it dragging the pointer into a window that had gone to the back, and filing the hold against it. Reproduced by widening the window with a temporary delay and switching twice: master strands the pointer in the background app 2 runs in 6, this 0 in 6, with the refusal logged. Without the delay the race is too narrow to hit by hand, which is why it goes in on its own and can be reverted on its own. Ordinary single switches still warp, 5 for 5.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six lines, deliberately on its own branch so it can be reverted on its own. This is a behaviour
change rather than a refactor, and it is the item left out of #34 for that reason.
The bug
warpAfterHandoverreads the frontmost app, then makes about eight cross-process Accessibilitycalls to find its focused window and judge it against the window policy: the app element, the
focused window, its position and size, its title, role, subrole, modal and minimized flags. Each of
those is bounded by the 100ms messaging timeout. Only then does it move the pointer.
Switch again during that interval and the pointer is dragged into a window that has gone to the
back, and the hold is filed against it. You notice it as the cursor jumping to the wrong screen
after a quick double Cmd-Tab.
The fix
Re-read the frontmost app immediately before warping, and refuse if it changed. The refusal is
logged at debug level, because a warp that silently does not happen is the kind of thing you would
otherwise write off as flakiness.
Verification
The race is too narrow to hit by hand with healthy apps, so I widened it: a temporary 400ms delay
in the same place, then two switches in quick succession, six runs each.
The delay was then removed and the branch re-verified clean: 5 of 5 ordinary single switches still
warp, 8 of 8 pointer crossings between displays land, the ring and directional shortcuts resolve
correctly, and a 30-second sweep gives 67 of 67 crossings with the guard never firing spuriously.
290 unit tests, the release build and
make check-packageare green.What this does not fix
Two bounded limits, both inherent to checking and then acting, and both far smaller than the window
being closed. Stating them so the next reader does not have to rediscover them:
warpPointerstill callsscreenFrames()and reads the cursor after the guard and beforeCGWarpMouseCursorPosition. Those are window server calls rather than the cross-app Accessibilityround trips the guard closes, so the residual window is orders of magnitude smaller, but it is not
zero. Closing it would mean threading the expectation into
warpPointer, which the focusshortcuts also call and where "still frontmost" means something different.
the warp would have happened, this refuses a warp master would have performed. That is the cost of
a guard that can only ever suppress.
Codex reviewed it and found no defects; the two items above are its observations, not findings.