diff options
| author | srdusr <[email protected]> | 2025-10-28 20:09:00 +0200 |
|---|---|---|
| committer | srdusr <[email protected]> | 2025-10-28 20:09:00 +0200 |
| commit | 9592fd7acb5b68f3fae139dfac22b728c35b6198 (patch) | |
| tree | ebdaab6064388b67ffe06a611d934d7a29120959 | |
| parent | 304f3a374408bb6a5a04ebb7d4652c631395f5c6 (diff) | |
| download | srdwm-9592fd7acb5b68f3fae139dfac22b728c35b6198.tar.gz srdwm-9592fd7acb5b68f3fae139dfac22b728c35b6198.zip | |
Fix a fake monitor's layer-shell surfaces misrouting onto the real primary
Live incident, root-caused jointly with the AGS peer session: creating a
fake monitor visibly shrank the real primary output's usable area
(full_y stayed 0 throughout - its true position never moved) each time,
tracking almost exactly one bar height per fake monitor created.
create_virtual_head registered its new Output in udev.virtual_heads but
never in CompState::outputs, the list output_for_wl searches to resolve
a client-named wl_output back to anything. new_layer_surface's own
fallback for an output it can't resolve is landing on the primary output
- so AGS's own per-monitor bar, aimed at the fake monitor it reasonably
believed was a new real one, silently landed on the real primary output
instead, stacking its own exclusive-zone reservation on top of the real
bar already there. Two fake monitors, two misrouted bars, two zone
increments, matching the observed climb exactly.
Fixed by registering (and, on removal, deregistering) a virtual head's
Output in CompState::outputs the same way bring_up_head already does for
a real one. Also adds Monitor::is_virtual / MonitorInfo's "virtual" JSON
field, requested directly by the AGS peer session as the real
discriminator their own temporary FAKE- name-pattern match was standing
in for.
The X-position half of this same incident was AGS's own remembered-
layout restore treating a fake monitor's wl_output as a real hotplug --
already fixed on their side (readArrangeable() now filters split/virtual
outputs).
| -rw-r--r-- | crates/core/src/monitor.rs | 32 | ||||
| -rw-r--r-- | crates/platform/src/ipc/tests.rs | 24 | ||||
| -rw-r--r-- | crates/platform/src/ipc/types.rs | 20 | ||||
| -rw-r--r-- | crates/wayland/src/udev/platform.rs | 1 | ||||
| -rw-r--r-- | crates/wayland/src/udev/virtual_heads.rs | 21 | ||||
| -rw-r--r-- | docs/TODO.md | 10 |
6 files changed, 107 insertions, 1 deletions
diff --git a/crates/core/src/monitor.rs b/crates/core/src/monitor.rs index 6ea053f..f18edea 100644 --- a/crates/core/src/monitor.rs +++ b/crates/core/src/monitor.rs @@ -65,11 +65,41 @@ pub struct Monitor { /// other than `1.0`) traced back to exactly this missing piece of /// information. pub scale: f64, + /// `true` for a fully virtual/headless output created by `srd dispatch + /// create fake-monitor` (`crates/wayland/src/udev/virtual_heads.rs`) -- + /// a real, independent `wl_output` global with no DRM connector behind + /// it. `false` for every ordinary connected output, split part + /// included (`split` and `is_virtual` are independent: a split part is + /// still a real output's own rectangle, not a second `wl_output`). + /// + /// Requested directly by the AGS peer session after a fake monitor's + /// `wl_output` caused a real live incident: a fake output looks like an + /// ordinary new monitor to any client watching the core Wayland + /// registry (not just `wlr-output-management-v1`, which already + /// deliberately excludes it - see `virtual_heads.rs`'s own module doc + /// comment), so AGS's own remembered-layout restore treated it as a + /// real hotplug and repositioned the *real* monitor to make room for + /// it, twice, once per fake monitor created. AGS's own fix was a + /// name-pattern match (`/^FAKE-/i`) since nothing else in `srd + /// monitors`' output let it tell a fake output apart from a real one -- + /// this field is the real discriminator that match was standing in for. + pub is_virtual: bool, } impl Monitor { pub fn new(id: MonitorId, name: impl Into<String>, geometry: Rect) -> Self { - Self { id, name: name.into(), geometry, full_geometry: geometry, maximize_geometry: geometry, refresh_rate_mhz: 60_000, primary: false, split: false, scale: 1.0 } + Self { + id, + name: name.into(), + geometry, + full_geometry: geometry, + maximize_geometry: geometry, + refresh_rate_mhz: 60_000, + primary: false, + split: false, + scale: 1.0, + is_virtual: false, + } } } diff --git a/crates/platform/src/ipc/tests.rs b/crates/platform/src/ipc/tests.rs index aa86e95..591be56 100644 --- a/crates/platform/src/ipc/tests.rs +++ b/crates/platform/src/ipc/tests.rs @@ -497,6 +497,30 @@ fn set_monitor_split_with_neither_name_nor_a_resolvable_id_errors() { } #[test] +fn monitors_query_marks_a_virtual_output_so_a_client_does_not_treat_it_as_a_real_hotplug() { + let dir = tempfile::tempdir().unwrap(); + let mut server = IpcServer::bind_in(dir.path(), "test").unwrap(); + let wm = Rc::new(RefCell::new(WindowManager::new())); + let real = srdwm_core::Monitor::new(0, "eDP-1", srdwm_core::Rect::new(0, 0, 1920, 1080)); + let mut fake = srdwm_core::Monitor::new(1, "FAKE-1", srdwm_core::Rect::new(1920, 0, 1920, 1080)); + fake.is_virtual = true; + wm.borrow_mut().set_monitors(vec![real, fake]); + + let mut client = UnixStream::connect(&server.path).unwrap(); + let mut reader = std::io::BufReader::new(client.try_clone().unwrap()); + client.write_all(b"{\"cmd\":\"monitors\"}\n").unwrap(); + server.poll(&wm); + let line = read_line(&mut reader); + + let parsed: serde_json::Value = serde_json::from_str(&line).unwrap(); + let monitors = parsed["monitors"].as_array().unwrap(); + let real = monitors.iter().find(|m| m["name"] == "eDP-1").unwrap(); + let fake = monitors.iter().find(|m| m["name"] == "FAKE-1").unwrap(); + assert_eq!(real["virtual"], false, "a real output must not be marked virtual"); + assert_eq!(fake["virtual"], true, "a fake monitor must be marked virtual so a client can tell it apart from a real hotplug"); +} + +#[test] fn monitors_query_lists_a_disabled_output_alongside_live_ones() { // What the AGS peer session asked for directly: a disabled output // must not just vanish from `srd monitors` - it needs a row diff --git a/crates/platform/src/ipc/types.rs b/crates/platform/src/ipc/types.rs index 1dc95f6..df11592 100644 --- a/crates/platform/src/ipc/types.rs +++ b/crates/platform/src/ipc/types.rs @@ -288,6 +288,21 @@ pub(crate) struct MonitorInfo { // actually displaying. See `WorkspaceInfo::monitor` for the same fact // indexed from the other direction. pub(crate) active_workspace: usize, + // `true` for a fully virtual/headless output (`srd dispatch create + // fake-monitor`) - a real `wl_output` global with no DRM connector + // behind it. Requested directly by the AGS peer session after a fake + // monitor's `wl_output` caused a real live incident: it looks like an + // ordinary new physical monitor to any client watching the core + // Wayland registry (unlike `wlr-output-management-v1`, which already + // excludes it), so AGS's own remembered-layout restore treated one + // appearing as a real hotplug and repositioned the *real* monitor to + // make room for it. Before this field existed, AGS's only option was + // matching the name against `^FAKE-` - this is the real + // discriminator that pattern was standing in for. `#[serde(rename)]` + // rather than a field literally named `virtual` because that word is + // a reserved identifier in Rust. + #[serde(rename = "virtual")] + pub(crate) is_virtual: bool, } /// Pushed to every subscriber (and used as `subscribe`'s own initial @@ -446,6 +461,7 @@ pub(crate) fn monitor_snapshot(wm: &std::rc::Rc<std::cell::RefCell<WindowManager split: m.split, scale: m.scale, active_workspace: wm.workspace_for_monitor(m.id), + is_virtual: m.is_virtual, }); // Disabled-but-still-connected outputs, appended rather than merged in // by name - see `MonitorInfo::enabled`'s own doc comment for why @@ -482,6 +498,10 @@ pub(crate) fn monitor_snapshot(wm: &std::rc::Rc<std::cell::RefCell<WindowManager // "shows nothing, not tracked" the same way `id: u32::MAX` above // is a deliberate not-a-real-value sentinel for this same entry. active_workspace: 0, + // A fake monitor is never administratively disabled/re-enabled -- + // see `virtual_heads.rs`'s own module doc comment - so this + // branch (disabled-but-still-connected outputs) can never be one. + is_virtual: false, }); live.chain(disabled).collect() } diff --git a/crates/wayland/src/udev/platform.rs b/crates/wayland/src/udev/platform.rs index bf82b32..4012e3c 100644 --- a/crates/wayland/src/udev/platform.rs +++ b/crates/wayland/src/udev/platform.rs @@ -864,6 +864,7 @@ impl Platform for UdevPlatform { m.full_geometry = full; m.maximize_geometry = full; m.primary = false; + m.is_virtual = true; out.push(m); next_id += 1; } diff --git a/crates/wayland/src/udev/virtual_heads.rs b/crates/wayland/src/udev/virtual_heads.rs index 38444e3..a1e3569 100644 --- a/crates/wayland/src/udev/virtual_heads.rs +++ b/crates/wayland/src/udev/virtual_heads.rs @@ -90,6 +90,23 @@ impl CompState { output.set_preferred(mode); let global = output.create_global::<CompState>(&self.dh); + // Also registered in `self.outputs` (mirroring `bring_up_head`'s own + // `entry` for a real head), not just `udev.virtual_heads` - without + // this, `output_for_wl` (which only ever searches `self.outputs`) + // can never resolve *this* output's own `wl_output` back to + // anything, so any client naming it in a `zwlr_layer_shell_v1:: + // get_layer_surface` request (a per-monitor bar/panel, concretely) + // silently falls through `new_layer_surface`'s "or land on the + // primary output" fallback instead - landing a bar meant for this + // fake monitor on the real primary one, stacking its exclusive + // zone on top of whatever real bar is already reserving space + // there. Confirmed live: a second fake monitor's own bar attempt + // was exactly what pushed the real monitor's reserved top strip up + // by another zone's worth, on top of the real bar's own - reported + // as the real monitor's position drifting after a fake monitor + // appeared, which was actually its *usable* (bar-shrunk) area + // shrinking further, not its true position moving at all. + self.outputs.push(crate::state::OutputEntry { output: output.clone(), location }); self.udev.as_mut().unwrap().virtual_heads.push(VirtualHead { name, output, global, size: (width, height), location }); // Payload discarded unread - `main.rs`'s own handler for this // event just re-queries the whole monitor list, same as a real @@ -111,6 +128,10 @@ impl CompState { }; let head = udev.virtual_heads.remove(index); self.dh.remove_global::<CompState>(head.global); + // Mirrors `create_virtual_head`'s own registration - see that + // function's doc comment for why this output was in `self.outputs` + // at all. + self.outputs.retain(|e| e.output != head.output); self.pending.borrow_mut().push(CoreEvent::MonitorRemoved(0)); Ok(()) } diff --git a/docs/TODO.md b/docs/TODO.md index 0db3a23..f58f1df 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -13,6 +13,16 @@ that has the full story. Keep this list current as items close or open; update the source doc's own entry too, don't let this drift into a second stale copy the way `PANEL_SUPPORT_TODO.md` did. +## Fake-monitor incident, root-caused and fixed jointly with the AGS peer session (2026-08-27) + +Follow-up to the incident entry directly below. `dotfiles-1a` (AGS) confirmed from AGS's own session log (`~/.local/state/wm-session-*.log` - not `~/.cache/ags/ags.log`, which was stale) that the X-position jump was AGS's own remembered "extend-left" layout restore firing twice, once per fake monitor appearing (`arrange()` places the primary at 0,0 and walks the rest `left -= width`, then normalizes - correct behavior for a *real* second monitor; the only fault was a fake one being in the arrangeable set at all). Fixed on AGS's side: `readArrangeable()` now filters `!m.split && !m.virtual`. Their match was keyed on connector *name*, not index, ruling out the index-shift half of my own original theory. + +The Y drift (34 -> 66 -> 97, `full_y` staying `0` throughout every snapshot - confirmed from my own captured `srd monitors` output, so this was never the real monitor's true position moving, only its *usable*/bar-shrunk area shrinking further) was left unexplained by AGS's fix and flagged back to me to root-cause srdwm-side. Found it: `create_virtual_head` (`virtual_heads.rs`) created a real `wl_output` global and pushed the new head into `udev.virtual_heads`, but never registered it in `CompState::outputs` - the list `output_for_wl` (`state/mod.rs`) searches to resolve a client-named `wl_output` back to anything. `protocols/layer_shell.rs::new_layer_surface`'s own fallback for an output it can't resolve is "land on the primary output" (its own comment says so directly) - so AGS's own per-monitor bar, spawned for what it (reasonably, before its own fix above) believed was a new real monitor and aimed at that fake output, silently landed on the real primary output instead, each one stacking its own exclusive-zone reservation on top of the real bar already there. Two fake monitors, two misrouted bars, two zone increments (~32px, ~31px) - exactly the observed climb, and exactly reproducible from reading the code, not guessed. + +Fixed by registering (and, on removal, deregistering) a virtual head's `Output` in `CompState::outputs` the same way `bring_up_head` already does for a real one, so `output_for_wl` can actually resolve it and a client's layer-shell surface lands where it was actually aimed. Also added the discriminator `dotfiles-1a` asked for directly: `Monitor::is_virtual` / `MonitorInfo`'s `"virtual"` JSON field (`#[serde(rename = "virtual")]`, `is_virtual` Rust-side since `virtual` is a reserved identifier), `true` only for a fake monitor - AGS's own name-pattern match (`/^FAKE-/i`) was standing in for exactly this and can now be retired in favor of a real field. Full workspace build/test/clippy clean (34 platform tests, up from 33). + +Fake monitors are safe to create again. Not yet live-verified against a real repro (needs a restart); `dotfiles-1a` asked for a heads-up before the next live test so AGS's log can be watched at the same time. + ## Live incident: creating a fake/virtual monitor corrupted the real monitor's position, repeatedly, with no further input (2026-08-27) Asked to demo fake monitors live. `srd dispatch create fake-monitor FAKE-1 1920x1080` was harmless (`eDP-1` stayed at `full_x=0`), but creating a *second* one (`FAKE-2`) immediately moved `eDP-1` to `full_x=1920`, and it kept drifting on its own for at least one more tick afterward (`full_x=3840, y` climbing ~31px per tick) with zero further commands issued. Removing both fake monitors stopped the drift but did not self-restore `eDP-1`'s position; fixed by hand via `srd dispatch set output position eDP-1 0 0`, confirmed restored. The user's actual laptop panel visibly shifted during this. |