Fix a race in the VNC server that corrupts a client's display output (the client keeps stale pixels because their update is never sent), or stops updating a client entirely, when the display surface is switched while an update for that client is queued but not yet encoded.
virtio-gpu guests with a double-buffering compositor (GNOME/mutter on Wayland) switch surfaces on every frame, so they hit it in normal use: fast scrolling in a terminal leaves scattered 16-pixel slivers of old text that stay until the client reconnects. A reproducer (a page-flipping virtio-gpu guest plus a VNC/QMP harness) loses updates in 35 of 150 four-client cycles on master, and in none with this patch. How it happens: vnc_update_client() clears a client's dirty bits for every rectangle it puts into a job, resets vs->has_dirty, and moves vs->update into vs->job_update, before the worker thread encodes anything. When the display surface is switched, vnc_dpy_switch() calls vnc_abort_display_jobs(), and a job the worker has not started yet is discarded (vnc_worker_thread_loop() sees vs->abort and skips it). The abort path only handles that case when no newer request is pending, and even then it restores vs->update but not the cleared dirty state: 1. Lost pixels. On the page-flip path vnc_dpy_switch() then re-dirties only the GUEST dirty map. vnc_refresh_server_surface() compares the new guest surface against the server surface, which already holds the dropped rectangles' pixels, so every chunk that did not change again is never marked dirty for the client, and with has_dirty at zero an incremental update is not even attempted. The client keeps the stale pixels until those chunks change again or it sends a non-incremental request. 2. A stuck client. If the client sent another request while the job was queued (vs->update != NONE), the dropped job's vs->job_update is never cleared, and vnc_should_update() refuses every later update for that client until it reconnects. Retire a dropped job whenever job_update is still set after the join (a completed job's output was consumed by vnc_jobs_join(), which clears it), keep a pending forced update, and re-dirty the whole client area plus has_dirty. This costs one full update, and only when a job was actually discarded. Signed-off-by: Dax Kelson <[email protected]> --- Notes: Tested on master 73294d0 (x86_64, KVM): virtio-gpu-pci, a minimal guest that shows a screen of text and then page-flips (DRM SETCRTC, 60 Hz) between two black buffers, and VNC clients that use Raw and keep one incremental request outstanding. Once the guest is static, each client's framebuffer is compared with QMP screendump. - 4 clients, 150 cycles: unpatched lost the switch to black in 35 cycles (78 client failures, all on clients 1-3; client 0's job always starts first). One non-incremental request healed each. Patched: 0. - 2 clients that also send a non-incremental request every 10 ms, 60 cycles: unpatched, one client got no further update at all, forced or not, for the rest of the run. Patched: 0. - 1 client, 450 cycles: not hit either way on this idle host. We see the symptom with a single client on loaded KubeVirt nodes (GNOME/Wayland guests). The reproducer (guest init + VNC/QMP harness) is small; happy to post it. ui/vnc.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/ui/vnc.c b/ui/vnc.c index 656768f9..c9187e7f 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -748,11 +748,24 @@ static void vnc_abort_display_jobs(VncDisplay *vd) } QTAILQ_FOREACH(vs, &vd->clients, next) { vnc_lock_output(vs); - if (vs->update == VNC_STATE_UPDATE_NONE && - vs->job_update != VNC_STATE_UPDATE_NONE) { - /* job aborted before completion */ - vs->update = vs->job_update; + if (vs->job_update != VNC_STATE_UPDATE_NONE) { + /* + * Job aborted before completion (a completed job's output was + * consumed by vnc_jobs_join(), which cleared job_update). + * Retire it even if the client sent a newer request meanwhile, + * or vnc_should_update() refuses every later update. + * vnc_update_client() already cleared the dirty bits of the + * rectangles it carried and the server surface already holds + * their pixels, so nothing would mark them dirty again: + * re-dirty the whole client area and its pending-dirty count. + */ + if (vs->update != VNC_STATE_UPDATE_FORCE) { + vs->update = vs->job_update; + } vs->job_update = VNC_STATE_UPDATE_NONE; + vnc_set_area_dirty(vs->dirty, vd, 0, 0, + vnc_width(vd), vnc_height(vd)); + vs->has_dirty++; } vs->abort = false; vnc_unlock_output(vs); -- 2.54.0 (Apple Git-157)
