Skip to content

Rely on drop order #360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
madsmtm wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Rely on drop order #360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 15 additions & 25 deletions src/backends/wayland/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>> {
Expand All @@ -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,
Expand All @@ -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>
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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>>,
Expand Down
58 changes: 25 additions & 33 deletions src/backends/x11.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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>> {
Expand Down Expand Up @@ -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,

Expand All @@ -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.
Expand Down Expand Up @@ -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")?,
)
Expand All @@ -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,
Expand Down Expand Up @@ -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.
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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();
}

Expand All @@ -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();
}
}
Expand Down