From 2444c2f07b18ab36acde1fce173123d10dfafbc8 Mon Sep 17 00:00:00 2001 From: Shawn Wallace Date: Thu, 23 May 2024 23:53:47 -0400 Subject: [PATCH] xstate: Ignore Window/Drawable errors Sometimes, windows will have actually been destroyed before we even processed the events for it, so if we try to do something like get properties on windows (as we do whenever a window is mapped) we will get an error from the X server, but this is fine - we can just ignore this. Should fix #16, #20, #22, etc. --- src/server/dispatch.rs | 6 +- src/server/mod.rs | 1 + src/xstate.rs | 179 +++++++++++++++++++++++++---------------- tests/integration.rs | 62 ++++++++++++++ 4 files changed, 176 insertions(+), 72 deletions(-) diff --git a/src/server/dispatch.rs b/src/server/dispatch.rs index 7f17137..ef0e0ea 100644 --- a/src/server/dispatch.rs +++ b/src/server/dispatch.rs @@ -1150,7 +1150,11 @@ impl Dispatch for ServerState { .zip((data.surface_serial.is_some_and(|s| s == serial)).then_some(data)) }) { - debug!("associate surface {} with {:?}", data.server.id().protocol_id(), win); + debug!( + "associate surface {} with {:?}", + data.server.id().protocol_id(), + win + ); window_data.surface_key = Some(*key); state.associated_windows.insert(*key, win); if window_data.mapped { diff --git a/src/server/mod.rs b/src/server/mod.rs index cd9be8a..c1f2e72 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -511,6 +511,7 @@ impl ServerState { debug!("skipping setting window name to {name:?} because a _NET_WM_NAME title is already set"); None } else { + debug!("setting {window:?} title to {name:?}"); *w = name; Some(w) } diff --git a/src/xstate.rs b/src/xstate.rs index 45fc97b..7eeceab 100644 --- a/src/xstate.rs +++ b/src/xstate.rs @@ -13,6 +13,35 @@ pub struct XState { pub atoms: Atoms, } +// Sometimes we'll get events on windows that have already been destroyed +#[derive(Debug)] +enum MaybeBadWindow { + BadWindow, + Other(xcb::Error), +} +impl From for MaybeBadWindow { + fn from(value: xcb::Error) -> Self { + match value { + xcb::Error::Protocol(xcb::ProtocolError::X( + x::Error::Window(_) | x::Error::Drawable(_), + _, + )) => Self::BadWindow, + other => Self::Other(other), + } + } +} +impl From for MaybeBadWindow { + fn from(value: xcb::ProtocolError) -> Self { + match value { + xcb::ProtocolError::X(x::Error::Window(_) | x::Error::Drawable(_), _) => { + Self::BadWindow + } + other => Self::Other(xcb::Error::Protocol(other)), + } + } +} + +type XResult = Result; /// Essentially a trait alias. trait PropertyResolver { type Output; @@ -36,12 +65,12 @@ struct PropertyCookieWrapper<'a, F: PropertyResolver> { impl PropertyCookieWrapper<'_, F> { /// Get the result from our property cookie. - fn resolve(self) -> Option { - let reply = self.connection.wait_for_reply(self.cookie).unwrap(); + fn resolve(self) -> XResult> { + let reply = self.connection.wait_for_reply(self.cookie)?; if reply.r#type() == x::ATOM_NONE { - None + Ok(None) } else { - Some(self.resolver.resolve(reply)) + Ok(Some(self.resolver.resolve(reply))) } } } @@ -165,6 +194,20 @@ impl XState { } pub fn handle_events(&mut self, server_state: &mut super::RealServerState) { + macro_rules! unwrap_or_skip_bad_window { + ($err:expr) => { + match $err { + Ok(v) => v, + Err(e) => { + let err = MaybeBadWindow::from(e); + match err { + MaybeBadWindow::BadWindow => continue, + MaybeBadWindow::Other(other) => panic!("X11 protocol error: {other:?}"), + } + } + } + }; + } while let Some(event) = self.connection.poll_for_event().unwrap() { trace!("x11 event: {event:?}"); match event { @@ -181,7 +224,8 @@ impl XState { xcb::Event::X(x::Event::ReparentNotify(e)) => { debug!("reparent event: {e:?}"); if e.parent() == self.root { - let attrs = self.get_window_attributes(e.window()); + let attrs = + unwrap_or_skip_bad_window!(self.get_window_attributes(e.window())); server_state.new_window( e.window(), attrs.override_redirect, @@ -196,20 +240,20 @@ impl XState { } xcb::Event::X(x::Event::MapRequest(e)) => { debug!("requested to map {:?}", e.window()); - self.connection - .send_and_check_request(&x::MapWindow { window: e.window() }) - .unwrap(); + unwrap_or_skip_bad_window!(self + .connection + .send_and_check_request(&x::MapWindow { window: e.window() })); } xcb::Event::X(x::Event::MapNotify(e)) => { - let attrs = self.get_window_attributes(e.window()); - self.handle_window_attributes(server_state, e.window(), attrs); - server_state.map_window(e.window()); - self.connection - .send_and_check_request(&x::ChangeWindowAttributes { + unwrap_or_skip_bad_window!(self.connection.send_and_check_request( + &x::ChangeWindowAttributes { window: e.window(), value_list: &[x::Cw::EventMask(x::EventMask::PROPERTY_CHANGE)], - }) - .unwrap(); + } + )); + let attrs = unwrap_or_skip_bad_window!(self.get_window_attributes(e.window())); + self.handle_window_attributes(server_state, e.window(), attrs); + server_state.map_window(e.window()); } xcb::Event::X(x::Event::ConfigureNotify(e)) => { server_state.reconfigure_window(e); @@ -217,17 +261,12 @@ impl XState { xcb::Event::X(x::Event::UnmapNotify(e)) => { trace!("unmap event: {:?}", e.event()); server_state.unmap_window(e.window()); - match self - .connection - .send_and_check_request(&x::ChangeWindowAttributes { + unwrap_or_skip_bad_window!(self.connection.send_and_check_request( + &x::ChangeWindowAttributes { window: e.window(), value_list: &[x::Cw::EventMask(x::EventMask::empty())], - }) { - // Window error may occur if the window has been destroyed, - // which is fine - Ok(_) | Err(xcb::ProtocolError::X(x::Error::Window(_), _)) => {} - Err(other) => panic!("Error removing event mask from window: {other:?}"), - } + } + )); let active_win = self .connection @@ -277,12 +316,12 @@ impl XState { list.push(x::ConfigWindow::Height(e.height().into())); } - self.connection - .send_and_check_request(&x::ConfigureWindow { + unwrap_or_skip_bad_window!(self.connection.send_and_check_request( + &x::ConfigureWindow { window: e.window(), value_list: &list, - }) - .unwrap(); + } + )); } xcb::Event::X(x::Event::ClientMessage(e)) => match e.r#type() { x if x == self.atoms.wl_surface_id => { @@ -328,7 +367,7 @@ impl XState { } } - fn get_window_attributes(&self, window: x::Window) -> WindowAttributes { + fn get_window_attributes(&self, window: x::Window) -> XResult { let geometry = self.connection.send_request(&x::GetGeometry { drawable: x::Drawable::Window(window), }); @@ -341,16 +380,19 @@ impl XState { let wm_hints = self.get_wm_hints(window); let size_hints = self.get_wm_size_hints(window); - let geometry = self.connection.wait_for_reply(geometry).unwrap(); - let attrs = self.connection.wait_for_reply(attrs).unwrap(); - let title = name - .resolve() - .or_else(|| self.get_wm_name(window).resolve()); - let class = class.resolve(); - let wm_hints = wm_hints.resolve(); - let size_hints = size_hints.resolve(); + let geometry = self.connection.wait_for_reply(geometry)?; + let attrs = self.connection.wait_for_reply(attrs)?; + let mut title = name.resolve()?; + if title.is_none() { + title = self.get_wm_name(window).resolve()?; + } - WindowAttributes { + debug!("got title: {title:?}"); + let class = class.resolve()?; + let wm_hints = wm_hints.resolve()?; + let size_hints = size_hints.resolve()?; + + Ok(WindowAttributes { override_redirect: attrs.override_redirect(), popup_for: None, dims: WindowDims { @@ -363,7 +405,7 @@ impl XState { class, group: wm_hints.and_then(|h| h.window_group), size_hints, - } + }) } fn handle_window_attributes( @@ -373,15 +415,12 @@ impl XState { attrs: WindowAttributes, ) { if let Some(name) = attrs.title { - debug!("setting {window:?} title to {name:?}"); server_state.set_win_title(window, name); } if let Some(class) = attrs.class { - debug!("setting {window:?} class to {class}"); server_state.set_win_class(window, class); } if let Some(hints) = attrs.size_hints { - debug!("{window:?} size hints: {hints:?}"); server_state.set_size_hints(window, hints); } } @@ -506,46 +545,44 @@ impl XState { } let window = event.window(); - let get_prop = |r#type, long_length| { - self.connection - .wait_for_reply(self.connection.send_request(&x::GetProperty { - window, - property: event.atom(), - r#type, - long_offset: 0, - long_length, - delete: false, - })) - }; + macro_rules! unwrap_or_skip_bad_window { + ($err:expr) => { + match $err { + Ok(v) => v, + Err(e) => { + let err = MaybeBadWindow::from(e); + match err { + MaybeBadWindow::BadWindow => return, + MaybeBadWindow::Other(other) => panic!("X11 protocol error: {other:?}"), + } + } + } + }; + } match event.atom() { x if x == x::ATOM_WM_HINTS => { - let hints = self.get_wm_hints(window).resolve().unwrap(); + let hints = + unwrap_or_skip_bad_window!(self.get_wm_hints(window).resolve()).unwrap(); server_state.set_win_hints(window, hints); } x if x == x::ATOM_WM_NORMAL_HINTS => { - let hints = self.get_wm_size_hints(window).resolve().unwrap(); + let hints = + unwrap_or_skip_bad_window!(self.get_wm_size_hints(window).resolve()).unwrap(); server_state.set_size_hints(window, hints); } - x if x == x::ATOM_WM_NAME || x == self.atoms.net_wm_name => { - let ty = if x == x::ATOM_WM_NAME { - x::ATOM_STRING - } else { - self.atoms.utf8_string - }; - let prop = get_prop(ty, 256).unwrap(); - let data: &[u8] = prop.value(); - let name = String::from_utf8(data.to_vec()).unwrap(); - debug!("{:?} named: {name}", window); - let name = if x == x::ATOM_WM_NAME { - WmName::WmName(name) - } else { - WmName::NetWmName(name) - }; + x if x == x::ATOM_WM_NAME => { + let name = unwrap_or_skip_bad_window!(self.get_wm_name(window).resolve()).unwrap(); + server_state.set_win_title(window, name); + } + x if x == self.atoms.net_wm_name => { + let name = + unwrap_or_skip_bad_window!(self.get_net_wm_name(window).resolve()).unwrap(); server_state.set_win_title(window, name); } x if x == x::ATOM_WM_CLASS => { - let class = self.get_wm_class(window).resolve().unwrap(); + let class = + unwrap_or_skip_bad_window!(self.get_wm_class(window).resolve()).unwrap(); server_state.set_win_class(window, class); } _ => { diff --git a/tests/integration.rs b/tests/integration.rs index 23212fa..71780d6 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -295,11 +295,18 @@ impl Connection { wid } + #[track_caller] fn map_window(&self, window: x::Window) { self.send_and_check_request(&x::MapWindow { window }) .unwrap(); } + #[track_caller] + fn destroy_window(&self, window: x::Window) { + self.send_and_check_request(&x::DestroyWindow { window }) + .unwrap(); + } + fn set_property( &self, window: x::Window, @@ -357,6 +364,13 @@ fn toplevel_flow() { x::ATOM_WM_NORMAL_HINTS, &[flags, 0, 0, 0, 0, 50, 100, 300, 400], ); + println!("set title: window"); + connection.set_property( + window, + x::ATOM_STRING, + x::ATOM_WM_NAME, + c"window".to_bytes(), + ); connection.map_window(window); f.wait_and_dispatch(); @@ -514,3 +528,51 @@ fn input_focus() { |win, focus| assert_eq!(win, focus), ); } + +#[test] +fn quick_delete() { + let mut f = Fixture::new(); + let connection = Connection::new(&f.display); + + let window = connection.new_window(connection.root, 0, 0, 20, 20, false); + connection.map_window(window); + connection.set_property( + window, + x::ATOM_WM_HINTS, + x::ATOM_WM_HINTS, + &[WmHintsFlags::Input.bits(), 0], + ); + connection.set_property( + window, + x::ATOM_STRING, + x::ATOM_WM_NAME, + c"bindow".to_bytes(), + ); + connection.set_property( + window, + x::ATOM_STRING, + x::ATOM_WM_CLASS, + &[c"f".to_bytes_with_nul(), c"ssalc".to_bytes_with_nul()].concat(), + ); + let flags = (WmSizeHintsFlags::ProgramMaxSize | WmSizeHintsFlags::ProgramMinSize).bits(); + connection.set_property( + window, + x::ATOM_WM_SIZE_HINTS, + x::ATOM_WM_NORMAL_HINTS, + &[flags, 1, 2, 3, 4, 25, 50, 150, 200], + ); + connection + .send_and_check_request(&x::ConfigureWindow { + window, + value_list: &[x::ConfigWindow::X(10), x::ConfigWindow::Y(40)], + }) + .unwrap(); + connection.destroy_window(window); + f.wait_and_dispatch(); + + let last_surf = f + .testwl + .last_created_surface_id() + .expect("No surface created"); + assert_eq!(f.testwl.get_surface_data(last_surf), None); +}