From 2083ca7c88bc3e4b52ec68af384f334be1da2141 Mon Sep 17 00:00:00 2001 From: srdusr <99972264+srdusr@users.noreply.github.com> Date: Thu, 4 Apr 2024 14:18:00 +0200 Subject: Fix XWayland: it now actually renders and receives keyboard input Three real bugs found and fixed via WAYLAND_DEBUG=1 protocol tracing in the QEMU VM, plus a fourth found along the way: - XWayland tried glamor (GBM rendering) first, which fails against this deliberately software-only compositor; its post-failure fallback path never used the xwayland_shell_v1 protocol at all, so X11Surface:: wl_surface() never resolved. Fixed by shadowing `Xwayland` on PATH with a wrapper script that always re-execs it with -shm (smithay's XWayland::spawn hardcodes its own argv and can't be bypassed either, since XWaylandClientData's fields are private). - Even with -shm, set_mapped(true) was only called after wl_surface() already resolved, deadlocking XWayland (it never advances a window past surface creation until the map is granted). Fixed by calling set_mapped(true) unconditionally in map_window_request. - The window then rendered as a ~1px sliver: initial geometry was seeded from X11Surface::geometry(), which can still be a tiny default at MapRequest time. Fixed by using the same 800x600 default the xdg-shell path already uses. - Typing didn't reach the window until a broader, XWayland-independent bug was fixed: nothing in the Wayland backend ever called KeyboardHandle::set_focus, so no window (native or X11) could ever receive keyboard input. Fixed in handle_pointer_button, along with TitlebarHit::Close being X11-surface-blind. Verified live: xterm launched via XWayland renders correctly sized and decorated, and a synthetic keypress sequence (ls + Enter) executed in its shell, screendump-confirmed. --- crates/wayland/src/lib.rs | 46 ++++++++++++++++--- crates/wayland/src/xwayland.rs | 100 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 138 insertions(+), 8 deletions(-) (limited to 'crates/wayland') diff --git a/crates/wayland/src/lib.rs b/crates/wayland/src/lib.rs index b2fa9dc..507102e 100644 --- a/crates/wayland/src/lib.rs +++ b/crates/wayland/src/lib.rs @@ -562,6 +562,39 @@ fn handle_pointer_position(state: &mut CompState, pos: Point, time } } +/// The underlying `wl_surface` for a mapped window, regardless of whether +/// it's a native `xdg-shell` toplevel or an XWayland `X11Surface` -- +/// `desktop::Window` exposes these as two separate accessors with no +/// shared one. +fn dwindow_wl_surface(w: &DWindow) -> Option { + if let Some(top) = w.toplevel() { + return Some(top.wl_surface().clone()); + } + w.x11_surface().and_then(|x| x.wl_surface()) +} + +/// Requests a client close its window, whichever kind it is. +fn close_dwindow(w: &DWindow) { + if let Some(top) = w.toplevel() { + top.send_close(); + } else if let Some(x11) = w.x11_surface() { + let _ = x11.close(); + } +} + +/// Focuses `id` in our own `WindowManager` *and* gives its surface real +/// Wayland/X11 keyboard focus - without this, a window can be raised and +/// tiled correctly yet never receive a single keystroke. +fn focus_window(state: &mut CompState, id: WindowId) { + state.wm.borrow_mut().focus_window(id); + state.pending.borrow_mut().push(CoreEvent::WindowFocused(id)); + let surface = state.id_to_window.get(&id).and_then(dwindow_wl_surface); + if let Some(keyboard) = state.seat.get_keyboard() { + let serial = SERIAL_COUNTER.next_serial(); + keyboard.set_focus(state, surface, serial); + } +} + fn handle_pointer_button(state: &mut CompState, pos: Point, button: u32, pressed: bool, time: u32) { const BTN_LEFT: u32 = 0x110; let serial = SERIAL_COUNTER.next_serial(); @@ -569,13 +602,12 @@ fn handle_pointer_button(state: &mut CompState, pos: Point, button if pressed && button == BTN_LEFT { let hit = state.wm.borrow().hit_test(pos.x as i32, pos.y as i32); if let Some((id, hit)) = hit { - state.wm.borrow_mut().focus_window(id); - state.pending.borrow_mut().push(CoreEvent::WindowFocused(id)); + focus_window(state, id); match hit { TitlebarHit::Drag => state.wm.borrow_mut().start_drag(id, pos.x as i32, pos.y as i32), TitlebarHit::Close => { - if let Some(w) = state.id_to_window.get(&id).and_then(|w| w.toplevel()) { - w.send_close(); + if let Some(w) = state.id_to_window.get(&id) { + close_dwindow(w); } } TitlebarHit::Maximize => { @@ -586,7 +618,11 @@ fn handle_pointer_button(state: &mut CompState, pos: Point, button TitlebarHit::Resize(edge) => state.wm.borrow_mut().start_resize(id, edge, pos.x as i32, pos.y as i32), } } else if let Some((window, _loc)) = state.space.element_under(pos) { - state.space.raise_element(&window.clone(), true); + let window = window.clone(); + state.space.raise_element(&window, true); + if let Some(&id) = dwindow_wl_surface(&window).and_then(|s| state.surface_to_id.get(&s)) { + focus_window(state, id); + } } } else if !pressed { let mut wm = state.wm.borrow_mut(); diff --git a/crates/wayland/src/xwayland.rs b/crates/wayland/src/xwayland.rs index dc7a88b..ca54f87 100644 --- a/crates/wayland/src/xwayland.rs +++ b/crates/wayland/src/xwayland.rs @@ -40,7 +40,33 @@ pub(crate) type X11Window = smithay::xwayland::xwm::X11Window; /// internal X11-connection source. Both are owned by the event loop after /// `insert_source`, not by any struct here - dropping the loop (or the /// `X11Wm` on disconnect) is what shuts things down. +/// +/// Before spawning, arranges for XWayland to run with `-shm`: this +/// compositor only ever supports `wl_shm` (see `udev.rs`'s module docs on +/// why it's deliberately software-only, no GBM/DMA-BUF), and XWayland's +/// default behavior of trying `glamor` first and falling back to +/// shared-memory buffers on failure does *not* fall back to the +/// `xwayland_shell_v1` protocol for associating X11 windows with +/// `wl_surface`s - confirmed by tracing the actual Wayland protocol +/// exchange with `WAYLAND_DEBUG=1`. Starting with `-shm` from the outset +/// avoids the failed glamor attempt entirely, which keeps XWayland on the +/// code path that does use `xwayland_shell_v1` correctly. +/// +/// `smithay::xwayland::XWayland::spawn` builds its `Xwayland` command line +/// internally with a fixed argument list (no way to add `-shm` directly), +/// and can't be bypassed either: the `XWaylandClientData` type it inserts +/// as the spawned client's data has private fields, so nothing outside +/// smithay can construct one, and `X11Wm`/the internal surface-association +/// commit hook both depend on the client's data specifically being that +/// type. Instead, a tiny wrapper script shadows `Xwayland` on `PATH` +/// (`Command::new("Xwayland")`'s lookup honors the `PATH` smithay copies +/// from this process's own environment) and always re-execs the real +/// binary with `-shm` prepended. pub(crate) fn spawn(handle: &LoopHandle<'static, CompState>, display_handle: &smithay::reexports::wayland_server::DisplayHandle) -> std::io::Result<()> { + if let Err(e) = ensure_shm_wrapper_on_path() { + log::warn!("could not set up an -shm wrapper for XWayland ({e}); XWayland windows will likely fail to render - see xwayland.rs's `spawn` docs"); + } + let (xwayland, client) = XWayland::spawn(display_handle, None, std::iter::empty::<(String, String)>(), true, std::process::Stdio::null(), std::process::Stdio::null(), |_| ())?; let handle_for_ready = handle.clone(); @@ -59,6 +85,60 @@ pub(crate) fn spawn(handle: &LoopHandle<'static, CompState>, display_handle: &sm Ok(()) } +/// Writes a small shell script named `Xwayland` to a private directory and +/// prepends that directory to this process's own `PATH` - the next +/// `Command::new("Xwayland")` (namely `XWayland::spawn`'s, which copies +/// `PATH` from this process's environment into the child's) resolves to +/// the wrapper instead of the real binary. The wrapper always re-execs the +/// real `Xwayland` with `-shm` prepended to whatever arguments it was +/// given, so it's transparent to everything else `spawn` sets up. +fn ensure_shm_wrapper_on_path() -> std::io::Result<()> { + use std::os::unix::fs::PermissionsExt; + + let real_xwayland = find_on_path("Xwayland").ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "Xwayland not found on PATH"))?; + + let wrapper_dir = std::env::var_os("XDG_RUNTIME_DIR").map(std::path::PathBuf::from).unwrap_or_else(std::env::temp_dir).join("srdwm-xwayland-shm-wrapper"); + std::fs::create_dir_all(&wrapper_dir)?; + + let wrapper_path = wrapper_dir.join("Xwayland"); + let quoted = shell_single_quote(&real_xwayland.to_string_lossy()); + std::fs::write(&wrapper_path, format!("#!/bin/sh\nexec {quoted} -shm \"$@\"\n"))?; + let mut perms = std::fs::metadata(&wrapper_path)?.permissions(); + perms.set_mode(0o755); + std::fs::set_permissions(&wrapper_path, perms)?; + + let old_path = std::env::var_os("PATH").unwrap_or_default(); + let mut new_path = wrapper_dir.into_os_string(); + new_path.push(":"); + new_path.push(old_path); + // SAFETY: called once, synchronously, before any XWayland process (or + // any other thread) is spawned. + unsafe { std::env::set_var("PATH", new_path) }; + Ok(()) +} + +fn find_on_path(name: &str) -> Option { + let path_var = std::env::var_os("PATH")?; + std::env::split_paths(&path_var).map(|dir| dir.join(name)).find(|candidate| candidate.is_file()) +} + +/// POSIX single-quoting: safe for any byte sequence, including embedded +/// single quotes (`'` -> `'\''`). +fn shell_single_quote(s: &str) -> String { + format!("'{}'", s.replace('\'', r"'\''")) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn shell_single_quote_handles_embedded_quotes() { + assert_eq!(shell_single_quote("/usr/bin/Xwayland"), "'/usr/bin/Xwayland'"); + assert_eq!(shell_single_quote("/it's/here"), r"'/it'\''s/here'"); + } +} + fn to_core_resize_edge(edge: X11ResizeEdge) -> ResizeEdge { match edge { X11ResizeEdge::Top => ResizeEdge::Top, @@ -112,7 +192,6 @@ impl CompState { let geom = self.wm.borrow().window(id).map(|w| w.geometry).unwrap_or_default(); let dwindow = DWindow::new_x11_window(surface.clone()); - let _ = surface.set_mapped(true); let _ = surface.configure(Rectangle::new((geom.x, geom.y + TITLEBAR_HEIGHT as i32).into(), (geom.width as i32, (geom.height - TITLEBAR_HEIGHT) as i32).into())); self.space.map_element(dwindow.clone(), (geom.x, geom.y + TITLEBAR_HEIGHT as i32), true); @@ -168,12 +247,27 @@ impl XwmHandler for CompState { let id = wm.alloc_window_id(); let mut w = CoreWindow::new(id, window.title()); w.app_id = window.class(); - let size = window.geometry().size; - w.geometry = srdwm_core::Rect::new(0, 0, size.w.max(1) as u32, size.h.max(1) as u32 + TITLEBAR_HEIGHT); + // Not `window.geometry()`: at `MapRequest` time this can still + // be whatever tiny/default size the X11 window was *created* + // with, before XWayland ever applies a `ConfigureRequest` -- + // and our own `configure_request` handler is deliberately a + // no-op (we own layout for managed windows, matching + // `new_managed_window`'s xdg-shell path below, which doesn't + // trust the client's initial size either). + w.geometry = srdwm_core::Rect::new(0, 0, 800, 600 + TITLEBAR_HEIGHT); wm.add_window(w); id }; self.xwayland_windows.insert(window.window_id(), id); + // Grant the map request *now*, unconditionally: per `X11Surface`'s + // docs this is what tells XWayland the window may proceed, and it + // does so before ever finishing our own wl_surface-dependent setup + // (`finish_x11_window_setup` bails out until `wl_surface()` + // resolves). Deferring `set_mapped` until after that check would + // deadlock - XWayland doesn't seem to advance the window past + // surface creation (no `get_xwayland_surface`/`set_serial`, no + // buffer attach) until the map is granted. + let _ = window.set_mapped(true); self.finish_x11_window_setup(&window); if !self.id_to_window.contains_key(&id) { self.xwayland_pending.push(window); -- cgit v1.2.3