Rely on drop order (#360)

Certain fields must be dropped before the source window/display is
dropped. Before, this was done using `Option<T>`, which is a bit
annoying, and it also wasn't handled properly (e.g. `WaylandDisplayImpl`
also has to be dropped after the `WlSurface`).

Author: madsmtm

src/backends/wayland/mod.rs

@@ -22,22 +22,18 @@ struct State;
 
 #[derive(Debug)]
 pub struct WaylandDisplayImpl<D: ?Sized> {
-    conn: Option<Connection>,
+    conn: Connection,
     event_queue: Mutex<EventQueue<State>>,
     qh: QueueHandle<State>,
     shm: wl_shm::WlShm,
 
     /// The object that owns the display handle.
     ///
     /// This has to be dropped *after* the `conn` field, because the `conn` field implicitly borrows
-    /// this.
+    /// this. This is done by placing this field last, see:
+    /// <https://doc.rust-lang.org/std/ops/trait.Drop.html#drop-order>
     _display: D,
-}
-
-impl<D: HasDisplayHandle + ?Sized> WaylandDisplayImpl<D> {
-    fn conn(&self) -> &Connection {
-        self.conn.as_ref().unwrap()
-    }
+    // NO FIELDS AFTER THIS!
 }
 
 impl<D: HasDisplayHandle + ?Sized> ContextInterface<D> for Arc<WaylandDisplayImpl<D>> {
@@ -64,7 +60,7 @@ impl<D: HasDisplayHandle + ?Sized> ContextInterface<D> for Arc<WaylandDisplayImp
         globals.destroy();
 
         Ok(Arc::new(WaylandDisplayImpl {
-            conn: Some(conn),
+            conn,
             event_queue: Mutex::new(event_queue),
             qh,
             shm,
@@ -78,23 +74,24 @@ impl<D: ?Sized> Drop for WaylandDisplayImpl<D> {
         if self.shm.version() >= 2 {
             self.shm.release();
         }
-        // Make sure the connection is dropped first.
-        self.conn = None;
     }
 }
 
 #[derive(Debug)]
-pub struct WaylandImpl<D: ?Sized, W: ?Sized> {
-    display: Arc<WaylandDisplayImpl<D>>,
-    surface: Option<wl_surface::WlSurface>,
+pub struct WaylandImpl<D: ?Sized, W> {
+    surface: wl_surface::WlSurface,
     buffers: Option<(WaylandBuffer, WaylandBuffer)>,
     size: Option<(NonZeroI32, NonZeroI32)>,
 
     /// The pointer to the window object.
     ///
     /// This has to be dropped *after* the `surface` field, because the `surface` field implicitly
-    /// borrows this.
+    /// borrows this. This is done by placing this field last, see:
+    /// <https://doc.rust-lang.org/std/ops/trait.Drop.html#drop-order>
     window_handle: W,
+    /// Drop the display last.
+    display: Arc<WaylandDisplayImpl<D>>,
+    // NO FIELDS AFTER THIS!
 }
 
 impl<D: HasDisplayHandle + ?Sized, W: HasWindowHandle> SurfaceInterface<D, W>
@@ -120,11 +117,11 @@ impl<D: HasDisplayHandle + ?Sized, W: HasWindowHandle> SurfaceInterface<D, W>
             )
         }
         .swbuf_err("Failed to create proxy for surface ID.")?;
-        let surface = wl_surface::WlSurface::from_id(display.conn(), surface_id)
+        let surface = wl_surface::WlSurface::from_id(&display.conn, surface_id)
             .swbuf_err("Failed to create proxy for surface ID.")?;
         Ok(Self {
             display: display.clone(),
-            surface: Some(surface),
+            surface,
             buffers: Default::default(),
             size: None,
             window_handle: window,
@@ -221,7 +218,7 @@ impl<D: HasDisplayHandle + ?Sized, W: HasWindowHandle> SurfaceInterface<D, W>
         let age = back.age;
         Ok(BufferImpl {
             event_queue: &self.display.event_queue,
-            surface: self.surface.as_ref().unwrap(),
+            surface: &self.surface,
             front,
             back,
             width,
@@ -231,13 +228,6 @@ impl<D: HasDisplayHandle + ?Sized, W: HasWindowHandle> SurfaceInterface<D, W>
     }
 }
 
-impl<D: ?Sized, W: ?Sized> Drop for WaylandImpl<D, W> {
-    fn drop(&mut self) {
-        // Make sure the surface is dropped first.
-        self.surface = None;
-    }
-}
-
 #[derive(Debug)]
 pub struct BufferImpl<'surface> {
     event_queue: &'surface Mutex<EventQueue<State>>,

src/backends/x11.rs

@@ -40,7 +40,7 @@ use x11rb::xcb_ffi::XCBConnection;
 #[derive(Debug)]
 pub struct X11DisplayImpl<D: ?Sized> {
     /// The handle to the XCB connection.
-    connection: Option<XCBConnection>,
+    connection: XCBConnection,
 
     /// SHM extension is available.
     is_shm_available: bool,
@@ -53,7 +53,11 @@ pub struct X11DisplayImpl<D: ?Sized> {
     /// Without `&mut`, the underlying connection cannot be closed without other unsafe behavior.
     /// With `&mut`, the connection can be dropped without us knowing about it. Therefore, we
     /// cannot provide `&mut` access to this field.
+    ///
+    /// This has to be dropped *after* the `conn` field. This is done by placing this field last:
+    /// <https://doc.rust-lang.org/std/ops/trait.Drop.html#drop-order>
     _display: D,
+    // NO FIELDS AFTER THIS!
 }
 
 impl<D: HasDisplayHandle + ?Sized> ContextInterface<D> for Arc<X11DisplayImpl<D>> {
@@ -110,28 +114,17 @@ impl<D: HasDisplayHandle + ?Sized> ContextInterface<D> for Arc<X11DisplayImpl<D>
         let supported_visuals = supported_visuals(&connection);
 
         Ok(Arc::new(X11DisplayImpl {
-            connection: Some(connection),
+            connection,
             is_shm_available,
             supported_visuals,
             _display: display,
         }))
     }
 }
 
-impl<D: ?Sized> X11DisplayImpl<D> {
-    fn connection(&self) -> &XCBConnection {
-        self.connection
-            .as_ref()
-            .expect("X11DisplayImpl::connection() called after X11DisplayImpl::drop()")
-    }
-}
-
 /// The handle to an X11 drawing context.
 #[derive(Debug)]
-pub struct X11Impl<D: ?Sized, W: ?Sized> {
-    /// X display this window belongs to.
-    display: Arc<X11DisplayImpl<D>>,
-
+pub struct X11Impl<D: ?Sized, W> {
     /// The window to draw to.
     window: xproto::Window,
 
@@ -154,7 +147,13 @@ pub struct X11Impl<D: ?Sized, W: ?Sized> {
     size: Option<(NonZeroU16, NonZeroU16)>,
 
     /// Keep the window alive.
+    /// NOTE: Drop this second-to-last!
     window_handle: W,
+
+    /// X display this window belongs to.
+    /// NOTE: Drop this last!
+    display: Arc<X11DisplayImpl<D>>,
+    // NO FIELDS AFTER THIS!
 }
 
 /// The buffer that is being drawn to.
@@ -219,13 +218,13 @@ impl<D: HasDisplayHandle + ?Sized, W: HasWindowHandle> SurfaceInterface<D, W> fo
         let display2 = display.clone();
         let tokens = {
             let geometry_token = display2
-                .connection()
+                .connection
                 .get_geometry(window)
                 .swbuf_err("Failed to send geometry request")?;
             let window_attrs_token = if window_handle.visual_id.is_none() {
                 Some(
                     display2
-                        .connection()
+                        .connection
                         .get_window_attributes(window)
                         .swbuf_err("Failed to send window attributes request")?,
                 )
@@ -238,11 +237,11 @@ impl<D: HasDisplayHandle + ?Sized, W: HasWindowHandle> SurfaceInterface<D, W> fo
 
         // Create a new graphics context to draw to.
         let gc = display
-            .connection()
+            .connection
             .generate_id()
             .swbuf_err("Failed to generate GC ID")?;
         display
-            .connection()
+            .connection
             .create_gc(
                 gc,
                 window,
@@ -344,7 +343,7 @@ impl<D: HasDisplayHandle + ?Sized, W: HasWindowHandle> SurfaceInterface<D, W> fo
         if self.size != Some((width, height)) {
             self.buffer_presented = false;
             self.buffer
-                .resize(self.display.connection(), num_bytes)
+                .resize(&self.display.connection, num_bytes)
                 .swbuf_err("Failed to resize X11 buffer")?;
 
             // We successfully resized the buffer.
@@ -358,11 +357,11 @@ impl<D: HasDisplayHandle + ?Sized, W: HasWindowHandle> SurfaceInterface<D, W> fo
         tracing::trace!("next_buffer: window={:X}", self.window);
 
         // Finish waiting on the previous `shm::PutImage` request, if any.
-        self.buffer.finish_wait(self.display.connection())?;
+        self.buffer.finish_wait(&self.display.connection)?;
 
         // We can now safely call `next_buffer` on the buffer.
         Ok(BufferImpl {
-            connection: self.display.connection(),
+            connection: &self.display.connection,
             window: self.window,
             gc: self.gc,
             depth: self.depth,
@@ -382,7 +381,7 @@ impl<D: HasDisplayHandle + ?Sized, W: HasWindowHandle> SurfaceInterface<D, W> fo
         // TODO: Is it worth it to do SHM here? Probably not.
         let reply = self
             .display
-            .connection()
+            .connection
             .get_image(
                 xproto::ImageFormat::Z_PIXMAP,
                 self.window,
@@ -775,25 +774,18 @@ impl Drop for ShmSegment {
     }
 }
 
-impl<D: ?Sized> Drop for X11DisplayImpl<D> {
-    fn drop(&mut self) {
-        // Make sure that the x11rb connection is dropped before its source is.
-        self.connection = None;
-    }
-}
-
-impl<D: ?Sized, W: ?Sized> Drop for X11Impl<D, W> {
+impl<D: ?Sized, W> Drop for X11Impl<D, W> {
     fn drop(&mut self) {
         // If we used SHM, make sure it's detached from the server.
         if let Buffer::Shm(mut shm) = mem::replace(
             &mut self.buffer,
             Buffer::Wire(util::PixelBuffer(Vec::new())),
         ) {
             // If we were in the middle of processing a buffer, wait for it to finish.
-            shm.finish_wait(self.display.connection()).ok();
+            shm.finish_wait(&self.display.connection).ok();
 
             if let Some((segment, seg_id)) = shm.seg.take() {
-                if let Ok(token) = self.display.connection().shm_detach(seg_id) {
+                if let Ok(token) = self.display.connection.shm_detach(seg_id) {
                     token.ignore_error();
                 }
 
@@ -803,7 +795,7 @@ impl<D: ?Sized, W: ?Sized> Drop for X11Impl<D, W> {
         }
 
         // Close the graphics context that we created.
-        if let Ok(token) = self.display.connection().free_gc(self.gc) {
+        if let Ok(token) = self.display.connection.free_gc(self.gc) {
             token.ignore_error();
         }
     }