fix(control): bound a hold whose release was lost - #52
Merged
Conversation
Taking a host's machine away from them needed only a dropped "up". `armHoldWatchdog` runs at the end of `trackHeldState`, which runs for every event — including mouse moves. So the 5s idle timer was reset by movement. If a button's "up" was lost while the guest kept moving the mouse, every move re-armed the timer and it never fired, and the button stayed held forever. A held button is not passive. `dispatch` reads `heldButtons.size > 0` as "dragging", and a drag is the one case where remote movement is injected instead of being tracked virtually. So the stuck hold turned every remote move into a real pointer move: the guest's mouse drove the host's, the host could not move their own cursor, and their clicks did nothing because the button was already down and everything had become a drag. Revoking control was the only escape, which is precisely what `disable()` -> `releaseAll()` does — and why it recovered without a reboot. Fixing it by not resetting the idle timer on movement would trade one bug for another: a legitimate drag longer than the timeout would be torn apart mid-drag. Both properties are needed at once, so there are now two timers. The idle timer keeps its reset-on-every-event behaviour, so an active drag is never cut short. A second, absolute timer starts once when the first button or key goes down, is never reset, and releases everything after 30s. It is the only thing that can bound a hold whose release was lost, and no real drag reaches it. Also surfaces heldButtons/heldKeys in getDiagnostics. A stuck hold is otherwise invisible: it presents as "my mouse is possessed", with nothing anywhere reporting that the injector still thinks a button is down.
ThreatCrush Security Scan160 finding(s) HIGH/CRITICAL: 5 | MEDIUM: 49 | LOW: 106
…and 110 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
Both diagnosed from documentation and build output rather than inference.
**dbus-next could never load in a packaged build.** Its `require('x11')` sits
inside `getDbusAddressFromWindowSelection`, a function `connection.js` never
calls — it only imports `getDbusAddressFromFs`. Lazy, and dead. But rollup's
commonjs plugin hoisted it to module scope, line 11 of the chunk, beside the
node builtins:
const path$1 = require("path");
const require$$1$2 = require("os");
require("x11"); <-- hoisted out of a function that never runs
`x11` is a dependency of nothing here — not of dbus-next (it is not even in
its optionalDependencies), not of remote-input, and it ships nowhere. So
loading the chunk threw `Cannot find module 'x11'`, KWin cursor reporting was
never available in any packaged build on any Linux host, `getCursorPosition()`
always returned null, and `restoreLocalPointer()` was permanently a no-op.
dbus-next was only bundled because remote-input is excluded from
externalization and drags its dependencies in with it. Declaring dbus-next in
the desktop app makes externalizeDepsPlugin leave it alone and electron-builder
ship it, so the lazy require stays lazy. The chunk is gone and the require with
it; the module is now a genuine runtime `import("dbus-next")`.
**The desktop cursor overlay is unsupported on Wayland.** Electron documents
`showInactive()` — the call that shows this window *without* taking focus — as
"Not supported on Wayland (Linux)", along with `setPosition()`, and notes that
positioning, moving, focusing and blurring generally are not possible there
without user input. The `level` argument to `setAlwaysOnTop` is documented
macOS/Windows only.
This window is fullscreen and always-on-top over the host's real desktop, and
the file's own header already warned that one which failed to be inert "would
lock the user out of their own desktop". Building it out of operations the
platform documents as unsupported is not a risk worth carrying, so it is not
created on Wayland. The in-app cursor still draws inside the PairUX window,
so the guest's pointer stays visible over the video; only the desktop-wide
overlay is given up.
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
1 participant
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.
Diagnosed from a host-side log. The host is Linux/Wayland on the
wayland-ydotoolbackend — worth stating because the previous two attempts atthis bug (#49, #50) both landed in the nut-js backend and never executed on
the affected machine.
Root cause
armHoldWatchdogis called at the end oftrackHeldState, which runs forevery event — including mouse moves. The 5s idle timer was therefore reset by
movement. If a button's "up" was lost while the guest kept moving the mouse,
every move re-armed the timer, it never fired, and the button stayed held
indefinitely.
A held button is not passive.
dispatchreadsheldButtons.size > 0as"dragging", and a drag is the one case where remote movement is injected
rather than tracked virtually:
So a stuck hold turns every remote move into a real pointer move. Observed
consequences, all from this one cause:
cannot move their own cursor
interaction has become a drag
disable()->releaseAll()Fix
Not resetting the idle timer on movement would trade this for a different bug:
any drag longer than the timeout would be torn apart mid-drag. Both properties
are required simultaneously, so there are two timers:
is never cut short. Catches a viewer that disappears entirely.
never reset, cleared on genuine release. The only thing that can bound a hold
whose release was lost. No real drag reaches it.
Also surfaces
heldButtons/heldKeysingetDiagnostics(). A stuck hold isotherwise invisible — it presents to the user as "my mouse is possessed" with
nothing anywhere reporting that the injector still believes a button is down.
Tests
Three regressions, all using fake timers:
Validation: lint, typecheck, 1454 tests, 34 script tests, format, build.