From abfd337c14292e0a1dd80ef498d92c8be0b47621 Mon Sep 17 00:00:00 2001
From: Ian Douglas Scott <idscott@system76.com>
Date: Tue, 25 Feb 2025 13:47:28 -0800
Subject: [PATCH 1/2] Fix leaks of `SeatRc` and `KbdRc`, but using `Weak` in
user data
This fixes https://github.com/Smithay/smithay/issues/1422.
This can be tested by adding `Drop` impls that print to those two types,
or using tools that detect leaks.
---
src/wayland/seat/keyboard.rs | 19 ++++-----
src/wayland/seat/mod.rs | 80 +++++++++++++++++++++++-------------
2 files changed, 59 insertions(+), 40 deletions(-)
diff --git a/src/wayland/seat/keyboard.rs b/src/wayland/seat/keyboard.rs
index 951c76082500..d48f7a28e49d 100644
--- a/src/wayland/seat/keyboard.rs
+++ b/src/wayland/seat/keyboard.rs
@@ -1,4 +1,4 @@
-use std::fmt;
+use std::{fmt, sync::Weak};
use tracing::{error, instrument, trace, warn};
use wayland_server::{
@@ -14,7 +14,7 @@ use super::WaylandFocus;
use crate::{
backend::input::{KeyState, Keycode},
input::{
- keyboard::{KeyboardHandle, KeyboardTarget, KeysymHandle, ModifiersState},
+ keyboard::{KbdRc, KeyboardHandle, KeyboardTarget, KeysymHandle, ModifiersState},
Seat, SeatHandler, SeatState,
},
utils::Serial,
@@ -88,13 +88,15 @@ impl<D: SeatHandler + 'static> KeyboardHandle<D> {
/// May return `None` for a valid `WlKeyboard` that was created without
/// the keyboard capability.
pub fn from_resource(seat: &WlKeyboard) -> Option<Self> {
- seat.data::<KeyboardUserData<D>>()?.handle.clone()
+ Some(Self {
+ arc: seat.data::<KeyboardUserData<D>>()?.handle.as_ref()?.upgrade()?,
+ })
}
}
/// User data for keyboard
pub struct KeyboardUserData<D: SeatHandler> {
- pub(crate) handle: Option<KeyboardHandle<D>>,
+ pub(crate) handle: Option<Weak<KbdRc<D>>>,
}
impl<D: SeatHandler> fmt::Debug for KeyboardUserData<D> {
@@ -122,13 +124,8 @@ where
}
fn destroyed(_state: &mut D, _client_id: ClientId, keyboard: &WlKeyboard, data: &KeyboardUserData<D>) {
- if let Some(ref handle) = data.handle {
- handle
- .arc
- .known_kbds
- .lock()
- .unwrap()
- .retain(|k| k.id() != keyboard.id())
+ if let Some(ref arc) = data.handle.as_ref().and_then(|h| h.upgrade()) {
+ arc.known_kbds.lock().unwrap().retain(|k| k.id() != keyboard.id())
}
}
}
diff --git a/src/wayland/seat/mod.rs b/src/wayland/seat/mod.rs
index 0e33ec0e5df6..62dee5817031 100644
--- a/src/wayland/seat/mod.rs
+++ b/src/wayland/seat/mod.rs
@@ -70,7 +70,11 @@ pub(crate) mod keyboard;
pub(crate) mod pointer;
mod touch;
-use std::{borrow::Cow, fmt, sync::Arc};
+use std::{
+ borrow::Cow,
+ fmt,
+ sync::{Arc, Weak},
+};
use crate::input::{Inner, Seat, SeatHandler, SeatRc, SeatState};
@@ -146,7 +150,8 @@ impl<D: SeatHandler> Inner<D> {
/// Global data of WlSeat
pub struct SeatGlobalData<D: SeatHandler> {
- arc: Arc<SeatRc<D>>,
+ // Seat arc
+ arc: Weak<SeatRc<D>>,
}
impl<D: SeatHandler> fmt::Debug for SeatGlobalData<D> {
@@ -171,12 +176,17 @@ impl<D: SeatHandler + 'static> SeatState<D> {
<D as SeatHandler>::KeyboardFocus: WaylandFocus,
N: Into<String>,
{
- let Seat { arc } = self.new_seat(name);
+ let seat = self.new_seat(name);
- let global_id = display.create_global::<D, _, _>(9, SeatGlobalData { arc: arc.clone() });
- arc.inner.lock().unwrap().global = Some(global_id);
+ let global_id = display.create_global::<D, _, _>(
+ 9,
+ SeatGlobalData {
+ arc: Arc::downgrade(&seat.arc),
+ },
+ );
+ seat.arc.inner.lock().unwrap().global = Some(global_id);
- Seat { arc }
+ seat
}
}
@@ -190,7 +200,7 @@ impl<D: SeatHandler + 'static> Seat<D> {
/// Attempt to retrieve a [`Seat`] from an existing resource
pub fn from_resource(seat: &WlSeat) -> Option<Self> {
seat.data::<SeatUserData<D>>()
- .map(|d| d.arc.clone())
+ .and_then(|d| d.arc.upgrade())
.map(|arc| Self { arc })
}
@@ -215,7 +225,7 @@ impl<D: SeatHandler + 'static> Seat<D> {
/// User data for seat
pub struct SeatUserData<D: SeatHandler> {
- arc: Arc<SeatRc<D>>,
+ arc: Weak<SeatRc<D>>,
}
impl<D: SeatHandler> fmt::Debug for SeatUserData<D> {
@@ -269,18 +279,21 @@ where
) {
match request {
wl_seat::Request::GetPointer { id } => {
- let inner = data.arc.inner.lock().unwrap();
+ let ptr_handle = data
+ .arc
+ .upgrade()
+ .and_then(|arc| arc.inner.lock().unwrap().pointer.clone());
let client_scale = state.client_compositor_state(client).clone_client_scale();
let pointer = data_init.init(
id,
PointerUserData {
- handle: inner.pointer.clone(),
+ handle: ptr_handle.clone(),
client_scale,
},
);
- if let Some(ref ptr_handle) = inner.pointer {
+ if let Some(ref ptr_handle) = ptr_handle {
ptr_handle.wl_pointer.new_pointer(pointer);
} else {
// we should send a protocol error... but the protocol does not allow
@@ -288,34 +301,40 @@ where
}
}
wl_seat::Request::GetKeyboard { id } => {
- let inner = data.arc.inner.lock().unwrap();
+ let kbd_handle = data
+ .arc
+ .upgrade()
+ .and_then(|arc| arc.inner.lock().unwrap().keyboard.clone());
let keyboard = data_init.init(
id,
KeyboardUserData {
- handle: inner.keyboard.clone(),
+ handle: kbd_handle.as_ref().map(|h| Arc::downgrade(&h.arc)),
},
);
- if let Some(ref h) = inner.keyboard {
+ if let Some(ref h) = kbd_handle {
h.new_kbd(keyboard);
} else {
// same as pointer, should error but cannot
}
}
wl_seat::Request::GetTouch { id } => {
- let inner = data.arc.inner.lock().unwrap();
+ let touch_handle = data
+ .arc
+ .upgrade()
+ .and_then(|arc| arc.inner.lock().unwrap().touch.clone());
let client_scale = state.client_compositor_state(client).clone_client_scale();
let touch = data_init.init(
id,
TouchUserData {
- handle: inner.touch.clone(),
+ handle: touch_handle.clone(),
client_scale,
},
);
- if let Some(ref h) = inner.touch {
+ if let Some(ref h) = touch_handle {
h.new_touch(touch);
} else {
// same as pointer, should error but cannot
@@ -329,12 +348,13 @@ where
}
fn destroyed(_state: &mut D, _: ClientId, seat: &WlSeat, data: &SeatUserData<D>) {
- data.arc
- .inner
- .lock()
- .unwrap()
- .known_seats
- .retain(|s| s.id() != seat.id());
+ if let Some(arc) = data.arc.upgrade() {
+ arc.inner
+ .lock()
+ .unwrap()
+ .known_seats
+ .retain(|s| s.id() != seat.id());
+ }
}
}
@@ -362,12 +382,14 @@ where
let resource = data_init.init(resource, data);
- if resource.version() >= 2 {
- resource.name(global_data.arc.name.clone());
- }
+ if let Some(arc) = global_data.arc.upgrade() {
+ if resource.version() >= 2 {
+ resource.name(arc.name.clone());
+ }
- let mut inner = global_data.arc.inner.lock().unwrap();
- resource.capabilities(inner.compute_caps());
- inner.known_seats.push(resource.downgrade());
+ let mut inner = arc.inner.lock().unwrap();
+ resource.capabilities(inner.compute_caps());
+ inner.known_seats.push(resource.downgrade());
+ }
}
}
From 2620147d451625e3caf0ab3fb3569eab7d492498 Mon Sep 17 00:00:00 2001
From: Ian Douglas Scott <idscott@system76.com>
Date: Tue, 25 Feb 2025 14:22:40 -0800
Subject: [PATCH 2/2] WIP Use `WeakLoopHandle` to avoid leaking `LoopInner`
Pending `calloop` release.
---
Cargo.toml | 2 +-
src/xwayland/xwm/mod.rs | 8 +++++---
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/Cargo.toml b/Cargo.toml
index 5c21e4ffd498..8ead20706fea 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -29,7 +29,7 @@ members = [
appendlist = "1.4"
ash = { version = "0.38.0", optional = true }
bitflags = "2.2.1"
-calloop = "0.14.0"
+calloop = { git = "https://github.com/Smithay/calloop" } # XXX
cursor-icon = "1.0.0"
cgmath = "0.18.0"
downcast-rs = "1.2.0"
diff --git a/src/xwayland/xwm/mod.rs b/src/xwayland/xwm/mod.rs
index ba0819018d65..3751b4c98f2d 100644
--- a/src/xwayland/xwm/mod.rs
+++ b/src/xwayland/xwm/mod.rs
@@ -880,11 +880,13 @@ impl X11Wm {
span,
};
- let event_handle = handle.clone();
+ let event_handle = handle.downgrade();
handle.insert_source(source, move |event, _, data| match event {
calloop::channel::Event::Msg(event) => {
- if let Err(err) = handle_event(&event_handle, data, id, event) {
- warn!(id = id.0, err = ?err, "Failed to handle X11 event");
+ if let Some(event_handle) = event_handle.upgrade() {
+ if let Err(err) = handle_event(&event_handle, data, id, event) {
+ warn!(id = id.0, err = ?err, "Failed to handle X11 event");
+ }
}
}
calloop::channel::Event::Closed => {