Skip to content

Commit

Permalink
Viewer: Synchronize viewport re-creation in EDT
Browse files Browse the repository at this point in the history
There are some CConn methods that access the viewport object in the RFB
thread:

- setName(), setCursor(), enableGII(), giiDeviceCreated(),
  enableQEMUExtKeyEventExt(), setLEDState()
  * invoked when a specific type of RFB message is received from the VNC
    server
- framebufferUpdateStart()
- framebufferUpdateEnd()
  * if firstUpdate is set and the desktop resizing mode is not "Server"
- beginRect()
  * if the RFB encoding has changed

There are also some CConn methods that re-create the viewport directly
in the EDT, as opposed to creating it in the RFB thread via
SwingUtilities.invokeAndWait():

- getOptions()
  * if the toolbar is visible and "View only" was toggled in the Options
    dialog
  * if the viewer is in windowed mode and "Show toolbar" was toggled in
    the Options dialog
  * if the viewer is not in macOS full-screen mode and "Scaling factor"
    was changed in the Options dialog
  * if the viewer is not in macOS full-screen mode and "Remote desktop
    size" was changed to something other than "Server" in the Options
    dialog
  * if the viewer is not in macOS full-screen mode and "Span mode" was
    changed in the Options dialog
- zoomIn(), zoomOut(), zoom100(), toggleToolbar(), toggleFullScreen(),
  toggleViewOnly()

In theory, if any operation from the first group coincides with any
operation from the second group, then the viewport object might be
accessed while it is being re-created.  The specific issue I observed
was triggered by the following procedure:

- Run GLXSpheres with VirtualGL in a TurboVNC session.
- Pop up the TurboVNC Viewer Options dialog, choose a remote desktop
  size that is larger than the client's screen, and click OK.
- Pop up the TurboVNC Viewer Options dialog, set the remote desktop size
  to "Auto", and click OK.

Setting the remote desktop size to "Auto" causes CConn.getOptions() to
set firstUpdate, which causes CConn.framebufferUpdateEnd() to invoke
CConn.sendDesktopSize(), which invokes CConn.computeScreenLayout().
Since framebuffer updates were being received rapidly from the server,
it was highly likely (100% reproducible on my systems) that
CConn.computeScreenLayout() would be invoked before the new viewport was
visible, which triggered an IllegalComponentStateException when
CConn.computeScreenLayout() attempted to invoke
viewport.getContentPane().getLocationOnScreen().

Since all of the methods in the first group are guarded by the
CConn/CConnection monitor lock in CConnection.processMsg(), the solution
was to obtain a CConn/CConnection monitor lock for all invocations of
CConn.recreateViewport() in the second group.  We already know that this
won't cause a deadlock, because in several places,
CConn.recreateViewport() is invoked via SwingUtilities.invokeAndWait()
(which causes it to execute in the EDT) while CConnection.processMsg()
owns the CConn/CConnection monitor lock.  (However, note that this is
also the reason why we can't simply add a 'synchronized' keyword to
CConn.recreateViewport().)

The specific failure mode that I observed was by far the most likely
failure mode, since other possible failure modes would have required
very unfortuitous timing.  It is also unknown whether other failure
modes would have triggered an exception or other visible problems.  They
might have been silent, leading to unexpected but not necessarily fatal
behavior.
  • Loading branch information
dcommander committed Feb 19, 2025
1 parent f2efc45 commit 63e3a1f
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 7 deletions.
5 changes: 5 additions & 0 deletions ChangeLog.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,11 @@ the SSH connection timeout specified using the `ConnectTimeout` OpenSSH config
file keyword was interpreted as milliseconds rather than seconds. This caused
the SSH connection to fail if the timeout was too low.

16. Fixed an issue whereby the Java TurboVNC Viewer threw an
IllegalComponentStateException if "Remote desktop size" was changed to "Auto"
in the TurboVNC Viewer Options dialog while the viewer was actively receiving
framebuffer updates from a VNC server.


2.2.9 ESR
=========
Expand Down
21 changes: 14 additions & 7 deletions java/com/turbovnc/vncviewer/CConn.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* Copyright (C) 2002-2005 RealVNC Ltd. All Rights Reserved.
* Copyright 2009-2011 Pierre Ossman <[email protected]> for Cendio AB
* Copyright (C) 2011-2023 D. R. Commander. All Rights Reserved.
* Copyright (C) 2011-2023, 2025 D. R. Commander. All Rights Reserved.
* Copyright (C) 2011-2015 Brian P. Hinz
*
* This is free software; you can redistribute it and/or modify
Expand Down Expand Up @@ -1763,9 +1763,11 @@ public void getOptions() {

if (options.fullScreen.isSelected() != opts.fullScreen)
toggleFullScreen();
else if (recreate)
recreateViewport();
else if (reconfigure)
else if (recreate) {
synchronized (this) {
recreateViewport();
}
} else if (reconfigure)
reconfigureAndRepaintViewport(false);
if (deleteRestore) {
savedState = -1;
Expand Down Expand Up @@ -1793,7 +1795,9 @@ public void toggleToolbar() {
return;
showToolbar = !showToolbar;
if (viewport != null) {
recreateViewport();
synchronized (this) {
recreateViewport();
}
viewport.showToolbar(showToolbar);
}
menu.showToolbar.setSelected(showToolbar);
Expand All @@ -1803,8 +1807,11 @@ public void toggleToolbar() {
public void toggleFullScreen() {
opts.fullScreen = !opts.fullScreen;
menu.fullScreen.setSelected(opts.fullScreen);
if (viewport != null)
recreateViewport(true);
if (viewport != null) {
synchronized (this) {
recreateViewport(true);
}
}
}

public boolean shouldGrab() {
Expand Down

0 comments on commit 63e3a1f

Please sign in to comment.