From b80281dcc8482a90777bab69cd8b2cdbfbc4036f Mon Sep 17 00:00:00 2001 From: i509VCB Date: Wed, 15 Dec 2021 20:04:53 -0600 Subject: [PATCH] x11: fix window drop not destroying --- src/backend/x11/buffer.rs | 17 ++- src/backend/x11/error.rs | 4 + src/backend/x11/mod.rs | 177 ++++++++++++++++---------------- src/backend/x11/window_inner.rs | 9 +- 4 files changed, 108 insertions(+), 99 deletions(-) diff --git a/src/backend/x11/buffer.rs b/src/backend/x11/buffer.rs index ea2aa15..8389124 100644 --- a/src/backend/x11/buffer.rs +++ b/src/backend/x11/buffer.rs @@ -109,10 +109,8 @@ where window: &Window, dmabuf: &Dmabuf, ) -> Result, CreatePixmapError> { - let window_inner = window.0.upgrade().unwrap(); - - if dmabuf.format().code != window_inner.format { - return Err(CreatePixmapError::IncorrectFormat(window_inner.format)); + if dmabuf.format().code != window.format() { + return Err(CreatePixmapError::IncorrectFormat(window.format())); } let mut fds = Vec::new(); @@ -129,7 +127,7 @@ where } // We need dri3 >= 1.2 in order to use the enhanced dri3_pixmap_from_buffers function. - let xid = if window_inner.extensions.dri3 >= Some((1, 2)) { + let xid = if window.0.extensions.dri3 >= Some((1, 2)) { if dmabuf.num_planes() > 4 { return Err(CreatePixmapError::TooManyPlanes); } @@ -154,7 +152,7 @@ where offsets.next().unwrap_or(x11rb::NONE), window.depth(), // In the future this could be made nicer. - match window.format().unwrap() { + match window.format() { DrmFourcc::Argb8888 => 32, DrmFourcc::Xrgb8888 => 24, _ => unreachable!(), @@ -183,7 +181,7 @@ where stride as u16, window.depth(), // In the future this could be made nicer. - match window.format().unwrap() { + match window.format() { DrmFourcc::Argb8888 => 32, DrmFourcc::Xrgb8888 => 24, _ => unreachable!(), @@ -198,10 +196,9 @@ where } fn present(self, connection: &C, window: &Window) -> Result { - let window_inner = window.0.upgrade().unwrap(); // We have the connection and window alive. - let next_serial = window_inner.next_serial.fetch_add(1, Ordering::SeqCst); + let next_serial = window.0.next_serial.fetch_add(1, Ordering::SeqCst); // We want to present as soon as possible, so wait 1ms so the X server will present when next convenient. - let msc = window_inner.last_msc.load(Ordering::SeqCst) + 1; + let msc = window.0.last_msc.load(Ordering::SeqCst) + 1; // options parameter does not take the enum but a u32. const OPTIONS: present::Option = present::Option::NONE; diff --git a/src/backend/x11/error.rs b/src/backend/x11/error.rs index 54ba73b..90c4d32 100644 --- a/src/backend/x11/error.rs +++ b/src/backend/x11/error.rs @@ -145,6 +145,10 @@ pub enum AllocateBuffersError { /// No free slots #[error("No free slots in the swapchain")] NoFreeSlots, + + /// The window has been destroyed + #[error("The window has been destroyed")] + WindowDestroyed, } impl From for AllocateBuffersError { diff --git a/src/backend/x11/mod.rs b/src/backend/x11/mod.rs index d57e874..5eadebf 100644 --- a/src/backend/x11/mod.rs +++ b/src/backend/x11/mod.rs @@ -259,7 +259,7 @@ impl X11Backend { #[derive(Debug)] pub struct X11Surface { connection: Weak, - window: Window, + window: Weak, resize: Receiver>, swapchain: Swapchain>>, BufferObject<()>>, format: DrmFourcc, @@ -420,56 +420,62 @@ impl X11Handle { device: Arc>>, modifiers: impl Iterator, ) -> Result { - match window.0.upgrade() { - Some(window) => { - let has_resize = { window.resize.lock().unwrap().is_some() }; + let has_resize = { window.0.resize.lock().unwrap().is_some() }; - if has_resize { - return Err(X11Error::SurfaceExists); - } - - let inner = self.inner.clone(); - let inner_guard = inner.lock().unwrap(); - - // Fail if the window is not managed by this backend or is destroyed - if !inner_guard.windows.contains_key(&window.id) { - return Err(X11Error::InvalidWindow); - } - - let modifiers = modifiers.collect::>(); - - let format = window.format; - let size = window.size(); - let swapchain = Swapchain::new(device, size.w as u32, size.h as u32, format, modifiers); - - let (sender, recv) = mpsc::channel(); - - { - let mut resize = window.resize.lock().unwrap(); - *resize = Some(sender); - } - - Ok(X11Surface { - connection: Arc::downgrade(&inner_guard.connection), - window: window.into(), - swapchain, - format, - width: size.w, - height: size.h, - buffer: None, - resize: recv, - }) - } - - None => Err(X11Error::InvalidWindow), + if has_resize { + return Err(X11Error::SurfaceExists); } + + let inner = self.inner.clone(); + let inner_guard = inner.lock().unwrap(); + + // Fail if the window is not managed by this backend or is destroyed + if !inner_guard.windows.contains_key(&window.id()) { + return Err(X11Error::InvalidWindow); + } + + let modifiers = modifiers.collect::>(); + + let format = window.0.format; + let size = window.size(); + let swapchain = Swapchain::new(device, size.w as u32, size.h as u32, format, modifiers); + + let (sender, recv) = mpsc::channel(); + + { + let mut resize = window.0.resize.lock().unwrap(); + *resize = Some(sender); + } + + Ok(X11Surface { + connection: Arc::downgrade(&inner_guard.connection), + window: Arc::downgrade(&window.0), + swapchain, + format, + width: size.w, + height: size.h, + buffer: None, + resize: recv, + }) } } impl X11Surface { /// Returns the window the surface presents to. - pub fn window(&self) -> &Window { - &self.window + /// + /// This will return [`None`] if the window has been destroyed. + pub fn window(&self) -> Option + '_> { + let window = self.window.upgrade().map(Window).map(WindowTemporary); + + struct WindowTemporary(Window); + + impl AsRef for WindowTemporary { + fn as_ref(&self) -> &Window { + &self.0 + } + } + + window } /// Returns a handle to the GBM device used to allocate buffers. @@ -528,13 +534,15 @@ impl X11Surface { mem::swap(&mut next, current); } + let window = self.window().ok_or(AllocateBuffersError::WindowDestroyed)?; + if let Ok(pixmap) = PixmapWrapper::with_dmabuf( &*connection, - &self.window, + window.as_ref(), next.userdata().get::().unwrap(), ) { // Now present the current buffer - let _ = pixmap.present(&*connection, &self.window); + let _ = pixmap.present(&*connection, window.as_ref()); } self.swapchain.submitted(next); @@ -617,79 +625,67 @@ impl<'a> WindowBuilder<'a> { )?); let downgrade = Arc::downgrade(&window); - inner.windows.insert(window.id, window); + inner.windows.insert(window.id, downgrade); - Ok(Window(downgrade)) + Ok(Window(window)) } } /// An X11 window. +/// +/// Dropping an instance of the window will destroy it. #[derive(Debug)] -pub struct Window(Weak); +pub struct Window(Arc); impl Window { /// Sets the title of the window. pub fn set_title(&self, title: &str) { - if let Some(inner) = self.0.upgrade() { - inner.set_title(title); - } + self.0.set_title(title); } /// Maps the window, making it visible. pub fn map(&self) { - if let Some(inner) = self.0.upgrade() { - inner.map(); - } + self.0.map(); } /// Unmaps the window, making it invisible. pub fn unmap(&self) { - if let Some(inner) = self.0.upgrade() { - inner.unmap(); - } + self.0.unmap(); } /// Returns the size of this window. /// /// If the window has been destroyed, the size is `0 x 0`. pub fn size(&self) -> Size { - self.0 - .upgrade() - .map(|inner| inner.size()) - .unwrap_or_else(|| (0, 0).into()) + self.0.size() } /// Changes the visibility of the cursor within the confines of the window. /// /// If `false`, this will hide the cursor. If `true`, this will show the cursor. pub fn set_cursor_visible(&self, visible: bool) { - if let Some(inner) = self.0.upgrade() { - inner.set_cursor_visible(visible); - } + self.0.set_cursor_visible(visible); } /// Returns the XID of the window. pub fn id(&self) -> u32 { - self.0.upgrade().map(|inner| inner.id).unwrap_or(0) + self.0.id } /// Returns the depth id of this window. pub fn depth(&self) -> u8 { - self.0.upgrade().map(|inner| inner.depth.depth).unwrap_or(0) + self.0.depth.depth } /// Returns the format expected by the window. - pub fn format(&self) -> Option { - self.0.upgrade().map(|inner| inner.format) + pub fn format(&self) -> DrmFourcc { + self.0.format } } impl PartialEq for Window { fn eq(&self, other: &Self) -> bool { - match (self.0.upgrade(), other.0.upgrade()) { - (Some(self_), Some(other)) => self_ == other, - _ => false, - } + self.0 == other.0 } } @@ -742,7 +738,7 @@ pub(crate) struct X11Inner { log: Logger, connection: Arc, screen_number: usize, - windows: HashMap>, + windows: HashMap>, key_counter: Arc, window_format: DrmFourcc, extensions: Extensions, @@ -760,7 +756,13 @@ impl X11Inner { use self::X11Event::Input; fn window_from_id(inner: &Arc>, id: &u32) -> Option> { - inner.lock().unwrap().windows.get(id).cloned() + inner + .lock() + .unwrap() + .windows + .get(id) + .cloned() + .and_then(|weak| weak.upgrade()) } // If X11 is deadlocking somewhere here, make sure you drop your mutex guards. @@ -812,7 +814,7 @@ impl X11Inner { }, }, }), - &mut Window(Arc::downgrade(&window)), + &mut Window(window), ) } else { callback( @@ -823,7 +825,7 @@ impl X11Inner { state: ButtonState::Pressed, }, }), - &mut Window(Arc::downgrade(&window)), + &mut Window(window), ) } } @@ -846,7 +848,7 @@ impl X11Inner { state: ButtonState::Released, }, }), - &mut Window(Arc::downgrade(&window)), + &mut Window(window), ); } } @@ -870,7 +872,7 @@ impl X11Inner { state: KeyState::Pressed, }, }), - &mut Window(Arc::downgrade(&window)), + &mut Window(window), ) } } @@ -902,7 +904,7 @@ impl X11Inner { state: KeyState::Released, }, }), - &mut Window(Arc::downgrade(&window)), + &mut Window(window), ); } } @@ -924,7 +926,7 @@ impl X11Inner { size: window_size, }, }), - &mut Window(Arc::downgrade(&window)), + &mut Window(window), ) } } @@ -942,15 +944,16 @@ impl X11Inner { // requests a resize or does something which causes a resize // inside the callback. { - *window.size.lock().unwrap() = configure_notify_size; + let mut resize_guard = window.size.lock().unwrap(); + *resize_guard = configure_notify_size; } (callback)( X11Event::Resized(configure_notify_size), - &mut Window(Arc::downgrade(&window)), + &mut Window(window.clone()), ); - if let Some(resize_sender) = &*window.resize.lock().unwrap() { + if let Some(resize_sender) = window.resize.lock().unwrap().as_ref() { let _ = resize_sender.send(configure_notify_size); } } @@ -974,7 +977,7 @@ impl X11Inner { if client_message.data.as_data32()[0] == window.atoms.WM_DELETE_WINDOW // Destroy the window? { - (callback)(X11Event::CloseRequested, &mut Window(Arc::downgrade(&window))); + (callback)(X11Event::CloseRequested, &mut Window(window)); } } } @@ -982,7 +985,7 @@ impl X11Inner { x11::Event::Expose(expose) => { if let Some(window) = window_from_id(inner, &expose.window) { if expose.count == 0 { - (callback)(X11Event::Refresh, &mut Window(Arc::downgrade(&window))); + (callback)(X11Event::Refresh, &mut Window(window)); } } } @@ -991,7 +994,7 @@ impl X11Inner { if let Some(window) = window_from_id(inner, &complete_notify.window) { window.last_msc.store(complete_notify.msc, Ordering::SeqCst); - (callback)(X11Event::PresentCompleted, &mut Window(Arc::downgrade(&window))); + (callback)(X11Event::PresentCompleted, &mut Window(window)); } } diff --git a/src/backend/x11/window_inner.rs b/src/backend/x11/window_inner.rs index 95790bb..42467c0 100644 --- a/src/backend/x11/window_inner.rs +++ b/src/backend/x11/window_inner.rs @@ -34,7 +34,7 @@ use x11rb::{ impl From> for Window { fn from(inner: Arc) -> Self { - Window(Arc::downgrade(&inner)) + Window(inner) } } @@ -287,7 +287,12 @@ impl WindowInner { impl PartialEq for WindowInner { fn eq(&self, other: &Self) -> bool { - self.id == other.id + match (self.connection.upgrade(), other.connection.upgrade()) { + (Some(conn1), Some(conn2)) => Arc::ptr_eq(&conn1, &conn2) && self.id == other.id, + + // A window with no connection to the X server cannot be validated. + _ => false, + } } }