GUACAMOLE-288: Six fixes for the multi-monitor branch (incl. two WIP items)#3
Open
ciroiriarte wants to merge 21 commits into
Open
Conversation
…onitors on each RDP connection.
…annot use the clientController.
… group or when multiple clients are inside.
The multi-monitor BroadcastChannel was created with the fixed name "guac_monitors", shared by every connection open in the browser. With two connections active at once, a secondary monitor window sent its keyboard and mouse events onto the same channel the other connection's windows were listening on, so input intended for one session -- including anything typed into a credential prompt -- reached the other session's guacd stream, and display instructions were mirrored across the connection boundary. Separately, broadcast.onmessage was wired directly to the monitor type's handler table, so any same-origin page could reach client.runHandler() by posting to a channel whose name it could guess. The primary window now generates a per-connection channel id (crypto.randomUUID(), with a timestamp+random fallback for insecure contexts) and uses it both in the channel name and as a _cid tag on every message; messages whose _cid does not match are dropped before dispatch. Secondary windows inherit the id through the window URL rather than generating their own, so the route becomes /secondaryMonitor/:id/:channel?. This resolves the "ensure that there is no mixing of data between multiple connections" TODO in guacManageMonitor.js.
… gaps. getOffsetX() derived a monitor's horizontal offset by summing the rendered widths of every monitor at a lower position. That is correct only when the monitors form a gapless left-to-right row, which is the common arrangement but not the only one: a vertical stack, a gap between monitors, or any arrangement where a monitor is not immediately adjacent to its predecessor produced a wrong offset for both draw instructions and mouse coordinates. It was also asymmetric with getOffsetY(), which already used absolute geometry. getOffsetX() now reads the monitor's absolute left coordinate and normalises it against the leftmost monitor, via a new getLowestLeftOffset() helper that mirrors the existing getLowestTopOffset(). onmultimonlayout() also closed a monitor as soon as it was absent from a single layout update. A guest that briefly reports a reduced layout while renegotiating its heads would therefore destroy a window the user had opened, with no way to recover it. Absence is now counted and closeMonitor() fires only after 8 consecutive misses, with the counter reset whenever the monitor reappears. Adds getMonitorsInfos() so the resize handlers can consult the current layout.
Display.setMonitorSize() recorded monitorWidth and monitorHeight but did not act on them. When the server had already sized the default layer to the full combined desktop and sent no further resize after the per-window monitor rectangle became known, the window kept rendering the entire desktop instead of its own region. setMonitorSize() now re-runs the resize of the default layer whenever the current display size differs from the monitor size, re-entering the existing clamping path so the visible area is constrained to the monitor bounds. Adds DisplaySpec.js covering both orderings: resize followed by setMonitorSize, and setMonitorSize followed by a later oversized resize.
…hoes. Resizing a monitor window produced a feedback loop. A resize request sent to the server came back as a remote display resize, which rescaled the display element, which changed the measured size of the container, which fired the element-resize sensor again, which sent a further request. Because requests were sent synchronously on every sensor event, a single user drag generated a burst of requests and the combined display did not settle. Resize requests are now debounced by 250ms, and requests are suppressed for 700ms after the display is resized by the server, since element resizes in that window are echoes of the remote resize rather than user intent. A request arriving inside the suppression window is rescheduled for the remainder of that window rather than dropped, so a genuine pending resize is not lost. The primary window detects remote resizes through the existing managedDisplay.size watch; the secondary window gains a display.onresize handler for the same purpose. Both handlers also now guard against a missing client, display, or zero-size container, and the secondary no longer assumes .client-main is present in the document.
The multimon-layout layer parameter is parsed and consumed without validation. A malformed payload throws out of JSON.parse inside the instruction handler, which aborts the handler loop and tears down the client, and geometry values that are not finite numbers propagate into the offset and size calculations as NaN, leaving monitor windows positioned at NaN with no error surfaced. Client.js now guards both the parse and the callback dispatch, and requires the parsed value to be an object before invoking the layout handler, so the transport layer is self-protecting regardless of what a consumer does with the value. guacManageMonitor.js validates each entry: width, height, top and left must be finite, with width and height strictly positive. Negative top and left remain valid, since a monitor may sit above or to the left of the primary. An entry that fails validation is treated the same as an absent monitor and therefore flows into the existing missing-count tolerance rather than immediately closing the window. Adds ClientSpec.js covering the transport-level guard: malformed JSON, JSON null, non-object JSON values, a handler that throws, and an unassigned handler. Five of the seven specs fail against the unguarded parser.
…order. The monitor map assigned positions by iterating the monitors object, so a monitor's position was the order its window happened to be opened. That position is what sendAllSizes() reports to guacd as the monitor index, so a user who opens a window and then moves it to the left of an existing one gets an arrangement that does not match what is on screen. It also prevented the vertical arrangement noted as outstanding in the pull request, since a stacked layout has no meaningful opening order. Secondary monitors are now ordered by their reported geometry, left then top, which orders a horizontal row, a vertical stack, and any combination of the two. The main window remains position 0. A monitor that has not yet reported a size keeps its relative opening order and sorts last, taking its place once a size arrives. This also closes a gap in the position sequence when a monitor is removed. Positions were assigned before the closing monitor's entry was deleted, so closing the middle of three left positions 1 and 3 with 2 missing. Since the closing monitor now has no geometry to sort by, it is placed last and the survivors keep a contiguous sequence. Guacd rejects a monitor index greater than the current monitor count in order to keep the layout contiguous, so the gap silently dropped a subsequent resize for the monitor that landed above it. Replaces the "use the left value to reorder monitors if needed" TODO.
corentin-soriano
force-pushed
the
GUACAMOLE-288_multi_screen
branch
from
July 21, 2026 13:17
18d3783 to
a36a6c3
Compare
corentin-soriano
left a comment
corentin-soriano
left a comment
Owner
There was a problem hiding this comment.
Thanks for your work!
I've rebased my branch as you requested.
I have a few quick comments, even without having reviewed it in detail.
I'll try to test it quickly!
| } | ||
|
|
||
| // Close additional monitors when window is unloaded | ||
| // Close additional monitors when window is unloaded |
Comment on lines
+130
to
+131
|
|
||
|
|
Comment on lines
+455
to
+463
| /** | ||
| * Sends the current size of the main element (the display container) | ||
| * to the Guacamole server, requesting that the remote display be | ||
| * resized. If the Guacamole client is not yet connected, it will be | ||
| * connected and the current size will sent through the initial | ||
| * handshake. If the size of the main element is not yet known, this | ||
| * function may need to be invoked multiple times until the size is | ||
| * known and the client may be connected. | ||
| */ |
Owner
There was a problem hiding this comment.
Is anything missing from this documentation?
| @@ -0,0 +1 @@ | |||
| /home/ciro/code/guacamole-client_proto-spice/guacamole-common-js/node_modules No newline at end of file | |||
Owner
There was a problem hiding this comment.
I think this line is not part of the PR.
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
2 participants
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.
Fixes for the GUACAMOLE-288 multi-monitor branch
Base:
corentin-soriano:GUACAMOLE-288_multi_screenI've been running your multi-monitor work as the basis for multi-monitor support in
the SPICE protocol client (apache#1227, apache/guacamole-server#560
is the corresponding server work). Doing so surfaced six issues in the shared
multi-monitor code, two of which are on your own WIP list. All six are
protocol-agnostic and apply to RDP as much as to SPICE, so they belong on your branch
rather than mine.
Anything SPICE-specific I needed has stayed on my branch — nothing here mentions
SPICE or assumes it.
The changes
1. Per-connection BroadcastChannel namespacing
The most significant one, and the reason I'd suggest looking at this PR before the
others. The channel name is the fixed string
guac_monitors, so all connections openin a browser share one channel. With two connections active, a secondary window's
keyboard and mouse events reach the other connection's guacd stream — including
anything typed into a credential prompt — and display instructions are mirrored
across the boundary.
broadcast.onmessagealso dispatches straight intomessageHandlers, so any same-origin page can reachclient.runHandler().This resolves the
// TODO: ensure that there is no mixing of data between multiple connectionsyou'd already left inguacManageMonitor.js, so I assume it was on yourlist. The approach: a per-connection id from
crypto.randomUUID()used in both thechannel name and a
_cidtag on each message, with non-matching messages droppedbefore dispatch. Secondaries inherit the id via the window URL rather than generating
their own, which changes the route to
/secondaryMonitor/:id/:channel?.2. Absolute left offset, and tolerance for transient layout gaps
getOffsetX()sums the rendered widths of all lower-positioned monitors. That'scorrect for a gapless left-to-right row — likely every arrangement you tested — but
wrong for a vertical stack or any layout with a gap, and asymmetric with
getOffsetY(), which already uses absolute geometry. Changed to use the absoluteleft coordinate normalised against the leftmost monitor.
Separately,
onmultimonlayout()closes a monitor the first time it's absent from alayout update. A guest that briefly reports a reduced layout while renegotiating
heads destroys a window the user opened. Now requires 8 consecutive misses.
3. Crop the display on monitor size change
Display.setMonitorSize()records the dimensions but doesn't act on them. If theserver has already sized the default layer to the full combined desktop and sends no
further resize, the window renders the whole desktop rather than its own region.
Adds
DisplaySpec.jswith two specs. The first (resize, thensetMonitorSize) is theone that exercises the fix — it fails against the unpatched
Display.jswithExpected 5120 to be 2560. The second (setMonitorSize, then a larger resize) passeseither way, since the existing clamping inside
resize()already handled thatordering; it's there as a regression guard for that path rather than as coverage of
the new code.
4. Debounce resize requests, suppress remote resize echoes
A resize request comes back as a remote display resize → rescales the display element
→ changes the container's measured size → fires the resize sensor → sends another
request. Requests were sent synchronously per sensor event, so one drag produced a
burst and the display didn't settle. Now debounced 250ms, with a 700ms suppression
window after a server-driven resize. A request arriving inside that window is
rescheduled rather than dropped.
5. Validate multimon-layout before use
Malformed JSON throws out of
JSON.parseinside the instruction handler, aborting thehandler loop and tearing down the client; non-finite geometry propagates as NaN into
the offset math.
Client.jsnow guards parse and dispatch;guacManageMonitor.jsvalidates each entry, treating a bad one the same as an absent monitor so it flows
into the tolerance added in (2).
Adds
ClientSpec.jsfor the transport-level guard — malformed JSON, JSONnull,non-object values, a throwing handler, and an unassigned handler. Five of the seven
specs fail against the unguarded parser; the other two are a positive control and a
pre-existing null-handler guard.
The
guacManageMonitor.jshalf is untested, becauseguacamole/src/main/frontendhasno test harness at all — no karma, no jasmine, no specs. Same gap applies to (1), (2)
and (4), and to (6). I'd be glad to propose adding one under a separate issue if you
think that would be welcome; it seemed like too much to fold into this PR.
6. Order monitors by screen position rather than opening order
This is the "reorder monitors vertically if needed" item on your WIP list.
updateMonitorsInfos()assigns each monitor's position by iterating themonitorsobject, so the position is the order the window happened to be opened. That position
is what
sendAllSizes()reports to guacd as the monitor index, so opening a windowand then moving it left of an existing one produces an arrangement that doesn't match
what's on screen. A vertical stack has no meaningful opening order at all, which I
think is why the item was still outstanding.
Secondary monitors are now ordered by their reported geometry, left then top, which
handles a row, a stack, or any mix. The main window stays at position 0. A monitor
that hasn't reported a size yet keeps its relative opening order and sorts last,
taking its place once a size arrives.
It also closes a gap in the position sequence when a monitor is removed. Positions
were assigned before the closing monitor's entry was deleted, so closing the middle of
three left positions 1 and 3 with 2 missing. Since the closing monitor now has no
geometry to sort by it is placed last, and the survivors keep a contiguous sequence.
That matters at least for SPICE, where guacd rejects a monitor index greater than the
current monitor count to keep the layout contiguous, so the gap silently dropped a
later resize; I haven't checked whether the RDP path is equally strict.
Notes
is the one exception: it depends on commit 2, because it rewrites the
if (!layout[pos])check intoif (!isValidGeometry(layout[pos])), and that checkonly takes its current form after commit 2. Taking 5 without 2 conflicts in
guacManageMonitor.js. Verified by cherry-picking each of the six onto thisbranch's base in isolation.
secondaryMonitorroutegaining an optional
:channelsegment.Testing, run against this branch:
guacamole-common-jskarma suite: 24/24 pass under headless Firefox (17 pre-existingplus 7 added here).
guacamole/src/main/frontendwebpack build: succeeds, no errors (the twoentrypoint-size warnings are pre-existing and unrelated).
Two environment notes you may already know about, neither caused by these commits but
both hit when building this branch from a clean checkout:
NODE_OPTIONS=--openssl-legacy-provideron OpenSSL 3, because thepinned
css-minimizer-webpack-pluginstill asks for an MD4 hash.apache/mainhas since moved fromangular-templatecache-webpack-plugintoangular-templatecache-webpack5-plugin, so this branch and current main no longershare a dependency tree.
Both suggest a rebase onto current main is due before the ASF reviewers pick apache#1061 back
up. Happy to redo these six commits on top of a rebased branch if that's useful.