Application panics when reading `Window::inner_size` after X server has terminated (e.g.: on user log-out)

[vorporeal]

Description

Our application's error reporting has been capturing a lot of called Result::unwrap() on an Err value: Connection(IoError(Custom { kind: Other, error: UnknownError })) panics, stemming from the unwrap in Window::inner_size_physical.

We finally have figured out the cause:

• User logs out of their desktop environment, shutting down the X server or Xwayland.
• Application is still running; some logic calls Window::inner_size.
• winit attempts to get the window geometry, and receives a connection error, as the display server is gone.
• winit panics

Xlib traditionally would end up invoking exit() here as part of the default I/O (fatal) error handler. winit's use of xcb makes it responsible, instead, for handling connection errors.

My basic request here is that winit doesn't panic in this situation and instead handles it in any more graceful way (could mean calling exit() like Xlib would have). This is an expected situation, and should be handled accordingly.

I think that in an ideal world (but curious what winit devs think), Window::inner_size returns a Result, allowing it to tell the caller that the size couldn't be computed, and winit then terminates the event loop at its earliest convenience, allowing for graceful application shutdown.

OS and window mananger

Arch Linux; Gnome (either X11 or Wayland w/ application running via Xwayland).

Winit version

0.30.0

[psychon]

Window::inner_size returns a Result

Quick hack at trying to get that to compile (dunno if this is really a good idea - I'll let others decide this).

Note that inner_size() doesn't allow returning a Result and I had to invent a size of 1 x 1 here. But at that point, it is perhaps a better idea to have that fallback in inner_size_physical() instead of its callers?

diff --git a/src/platform_impl/linux/x11/window.rs b/src/platform_impl/linux/x11/window.rs
index ea7c0235..35b8635c 100644
--- a/src/platform_impl/linux/x11/window.rs
+++ b/src/platform_impl/linux/x11/window.rs
@@ -874,7 +874,7 @@ impl UnownedWindow {
         })
     }
 
-    /// Refresh the API for the given monitor.
+    /// Refresh the DPI for the given monitor.
     #[inline]
     pub(super) fn refresh_dpi_for_monitor<T: 'static>(
         &self,
@@ -885,7 +885,13 @@ impl UnownedWindow {
         // Check if the self is on this monitor
         let monitor = self.shared_state_lock().last_monitor.clone();
         if monitor.name == new_monitor.name {
-            let (width, height) = self.inner_size_physical();
+            let (width, height) = match self.inner_size_physical() {
+                Ok(size) => size,
+                Err(err) => {
+                    tracing::error!("Cannot refresh DPI: {err:?}");
+                    return;
+                }
+            };
             let (new_width, new_height) = self.adjust_for_dpi(
                 // If we couldn't determine the previous scale
                 // factor (e.g., because all monitors were closed
@@ -1234,25 +1240,24 @@ impl UnownedWindow {
         self.set_position_physical(x, y);
     }
 
-    pub(crate) fn inner_size_physical(&self) -> (u32, u32) {
+    pub(crate) fn inner_size_physical(&self) -> Result<(u32, u32), X11Error> {
         // This should be okay to unwrap since the only error XGetGeometry can return
         // is BadWindow, and if the window handle is bad we have bigger problems.
         self.xconn
             .get_geometry(self.xwindow)
             .map(|geo| (geo.width.into(), geo.height.into()))
-            .unwrap()
     }
 
     #[inline]
     pub fn inner_size(&self) -> PhysicalSize<u32> {
-        self.inner_size_physical().into()
+        self.inner_size_physical().unwrap_or((1, 1)).into()
     }
 
     #[inline]
     pub fn outer_size(&self) -> PhysicalSize<u32> {
         let extents = self.shared_state_lock().frame_extents.clone();
         if let Some(extents) = extents {
-            let (width, height) = self.inner_size_physical();
+            let (width, height) = self.inner_size().into();
             extents.inner_size_to_outer(width, height).into()
         } else {
             self.update_cached_frame_extents();

[notgull]

I would think that shutting down the X server would cause the application termination event to occur. Though I guess I haven't tested this.

Otherwise I would say that this error is expected; using a window after the EventLoop exits will lead to panics.

[notgull]

At the next breaking change we should make any method that communicates with the X server fallible.

[daxpedda]

At the next breaking change we should make any method that communicates with the X server fallible.

See https://github.com/rust-windowing/winit/issues/3317. We have discussed this so many times now, I kinda lost sight of all the crazy solutions we came up with to solve this.

I would prefer to find a different solution then returning a Result. After the upcoming rework we should probably schedule a meeting to discuss this.