From: Dax Kelson <[email protected]>
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.
Fixes: 61e77a5f0c ("vnc: detect and optimize pageflips")
Fixes: ada8d2e436 ("ui: fix VNC client throttling when forced update is
requested")
Reviewed-by: Marc-André Lureau <[email protected]>
Signed-off-by: Dax Kelson <[email protected]>
Message-ID: <[email protected]>
---
ui/vnc.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index 8b1d03ef2434..10acb3e1e64b 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.56.0.116.g6de20f6092dc
