Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VNCServerST: Add a timeout to pointer button ownership #1718

Merged

Conversation

MikeLooijmans
Copy link
Contributor

When one clients holds down a button on the pointer device (probably dragging something), other clients' attempts at pointer operations are denied. This yields a sane user experience, but with limits.

When one clients starts dragging, and then his network connection fails, other clients are denied access to the pointer until the VNC server finally discovers that the connection is dead and closes it. This can take about 15 minutes.

Add a timeout to this policy: If we don't hear from the client for 3 seconds, other clients are allowed to control the pointer once more.

This solves the problem that one failing network could make the server completely deaf to other clients for a long time.

@LMattsson LMattsson added the bug Something isn't working label Jan 22, 2024
@LMattsson
Copy link
Contributor

That sounds like an annoying issue, thank you for looking into this.

From your description, I assume this is seen with "shared" connections. A colleague and I were able to reproduce the issue with input being locked if a viewer loses connection.

Steps we used to reproduce:

  1. Running Xvnc with -alwaysshared=true
  2. Start an application and a window manager, e.g. xterm and metacity. Also start e.g. glxgears to show continuous graphical updates.
  3. Connected viewer A and viewer B. Viewer A is run on a VM where we control the network connection
  4. Start dragging the xterm window in viewer A
  5. While dragging, disconnect the network from the VM.
  6. In viewer B, note that input (both keyboard and mouse) are blocked. However, we still get graphical updates.

We also tested the regular case when two viewers are connected and one user is dragging a window. On master, the second user cannot interfere while the first user is dragging.

Copy link
Contributor

@LMattsson LMattsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We built and tested the regular case with your changes. Unfortunately, it seems something is off. When a user is dragging the window in viewer A, viewer B can immediately interfere with the mouse.

We get the impression that the timeout isn't working as intended. Generally, it seems reasonable to keep blocking user B unless we detect a period of inactivity from user A.

if (buttonMask) {
pointerClient = client;
pointerClientTime = now;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's an "else pointerClient = NULL;" missing here

@MikeLooijmans
Copy link
Contributor Author

We built and tested the regular case with your changes. Unfortunately, it seems something is off. When a user is dragging the window in viewer A, viewer B can immediately interfere with the mouse.

We get the impression that the timeout isn't working as intended. Generally, it seems reasonable to keep blocking user B unless we detect a period of inactivity from user A.

My use case was with x0vncserver, maybe it sets up things differently? Anyway, that missing "else" makes is harder to interfere, so that's not related to what you're seeing

return;
// to provide a bit more sane user experience. But limit the time to prevent
// locking out all others when e.g. the network is down.
if (pointerClient != NULL) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "if" will never trigger, as nothing will ever assign anything else than NULL to pointerclient...

When one clients holds down a button on the pointer device (probably
dragging something), other clients' attempts at pointer operations
are denied. This yields a sane user experience, but with limits.

When one clients starts dragging, and then his network connection fails,
other clients are denied access to the pointer until the VNC server
finally discovers that the connection is dead and closes it. This can
take about 15 minutes.

Add a timeout to this policy: If we don't hear from the client for 3
seconds, other clients are allowed to control the pointer once more.

This solves the problem that one failing network could make the server
completely deaf to other clients for a long time.

Signed-off-by: Mike Looijmans <[email protected]>
@MikeLooijmans
Copy link
Contributor Author

Yep, my attempt to avoid a time(0) syscall when not needed backfired... Better to keep the code path simple.

Copy link
Contributor

@LMattsson LMattsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the quick update, looks good now!

Rebuilt with the updated commit and tested both the regular use case as well as when one of the clients lost network. Tested the same way as described earlier.

Lastly, we also tested when viewer A stopped moving the mouse but still had the mouse button down. Viewer B could not interfere until more than 3 seconds had passed since viewer A stopped moving.

@LMattsson LMattsson merged commit 6050b15 into TigerVNC:master Jan 25, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants