compositor: prevent adding hooks and blockers on destroyed surfaces

anvil/src/shell/mod.rs

@@ -115,7 +115,7 @@ impl<BackendData: Backend> CompositorHandler for AnvilState<BackendData> {
     }
 
     fn new_surface(&mut self, surface: &WlSurface) {
-        add_pre_commit_hook::<Self, _>(surface, move |state, _dh, surface| {
+        let _ = add_pre_commit_hook::<Self, _>(surface, move |state, _dh, surface| {
             #[cfg(feature = "udev")]
             let mut acquire_point = None;
             let maybe_dmabuf = with_states(surface, |surface_data| {
@@ -149,7 +149,7 @@ impl<BackendData: Backend> CompositorHandler for AnvilState<BackendData> {
                             Ok(())
                         });
                         if res.is_ok() {
-                            add_blocker(surface, blocker);
+                            let _ = add_blocker(surface, blocker);
                             return;
                         }
                     }
@@ -162,7 +162,7 @@ impl<BackendData: Backend> CompositorHandler for AnvilState<BackendData> {
                             Ok(())
                         });
                         if res.is_ok() {
-                            add_blocker(surface, blocker);
+                            let _ = add_blocker(surface, blocker);
                         }
                     }
                 }

anvil/src/shell/xdg.rs

@@ -50,7 +50,8 @@ impl<BackendData: Backend> XdgShellHandler for AnvilState<BackendData> {
         let window = WindowElement(Window::new_wayland_window(surface.clone()));
         place_new_window(&mut self.space, self.pointer.current_location(), &window, true);
 
-        compositor::add_post_commit_hook(surface.wl_surface(), |state: &mut Self, _, surface| {
+        // FIXME: On role re-usage this might be problematic
+        let _ = compositor::add_post_commit_hook(surface.wl_surface(), |state: &mut Self, _, surface| {
             handle_toplevel_commit(&mut state.space, surface);
         });
     }

src/backend/renderer/utils/wayland.rs

@@ -391,7 +391,7 @@ pub fn on_commit_buffer_handler<D: 'static>(surface: &WlSurface) {
             |_, _, _| true,
         );
         for surf in &new_surfaces {
-            add_destruction_hook(surf, |_: &mut D, surface| {
+            let _ = add_destruction_hook(surf, |_: &mut D, surface| {
                 // We reset the state on destruction before the user_data is dropped
                 // to prevent a deadlock which can happen if we try to send a buffer
                 // release during drop. This also enables us to free resources earlier

src/wayland/commit_timing/mod.rs

@@ -210,7 +210,7 @@ where
 
                 // Make sure we do not install the hook more then once in case the surface is being reused
                 if is_managed && is_initial {
-                    add_pre_commit_hook::<D, _>(&surface, |_, _, surface| {
+                    let _ = add_pre_commit_hook::<D, _>(&surface, |_, _, surface| {
                         let timestamp = with_states(surface, |states| {
                             states
                                 .data_map
@@ -226,7 +226,7 @@ where
                                 barrier_state.lock().unwrap().register(timestamp)
                             });
 
-                            add_blocker(surface, barrier);
+                            let _ = add_blocker(surface, barrier);
                         }
                     });
                 }

src/wayland/compositor/mod.rs

@@ -122,8 +122,8 @@ pub use self::transaction::{Barrier, Blocker, BlockerState};
 pub use self::tree::{AlreadyHasRole, TraversalAction};
 use self::tree::{PrivateSurfaceData, SuggestedSurfaceState};
 pub use crate::utils::hook::HookId;
-use crate::utils::Transform;
 use crate::utils::{user_data::UserDataMap, Buffer, Logical, Point, Rectangle};
+use crate::utils::{DeadResource, Transform};
 use atomic_float::AtomicF64;
 use wayland_server::backend::GlobalId;
 use wayland_server::protocol::wl_compositor::WlCompositor;
@@ -449,7 +449,7 @@ pub fn get_region_attributes(region: &wl_region::WlRegion) -> RegionAttributes {
 /// post-commit hook to apply state changes (i.e. copy last acked state to current).
 ///
 /// Compositors should use this for adding blockers if needed, e.g. the DMA-BUF readiness blocker.
-pub fn add_pre_commit_hook<D, F>(surface: &WlSurface, hook: F) -> HookId
+pub fn add_pre_commit_hook<D, F>(surface: &WlSurface, hook: F) -> Result<HookId, DeadResource>
 where
     F: Fn(&mut D, &DisplayHandle, &WlSurface) + Send + Sync + 'static,
     D: 'static,
@@ -463,11 +463,15 @@ where
         user_state_type,
     );
 
+    if !surface.is_alive() {
+        return Err(DeadResource);
+    }
+
     let hook = move |state: &mut dyn Any, dh: &DisplayHandle, surface: &WlSurface| {
         let state = state.downcast_mut::<D>().unwrap();
         hook(state, dh, surface);
     };
-    PrivateSurfaceData::add_pre_commit_hook(surface, hook)
+    Ok(PrivateSurfaceData::add_pre_commit_hook(surface, hook))
 }
 
 /// Register a post-commit hook to be invoked on surface commit
@@ -477,7 +481,7 @@ where
 ///
 /// Protocol implementations should apply state changes here, i.e. copy last acked state into
 /// current.
-pub fn add_post_commit_hook<D, F>(surface: &WlSurface, hook: F) -> HookId
+pub fn add_post_commit_hook<D, F>(surface: &WlSurface, hook: F) -> Result<HookId, DeadResource>
 where
     F: Fn(&mut D, &DisplayHandle, &WlSurface) + Send + Sync + 'static,
     D: 'static,
@@ -491,11 +495,15 @@ where
         user_state_type,
     );
 
+    if !surface.is_alive() {
+        return Err(DeadResource);
+    }
+
     let hook = move |state: &mut dyn Any, dh: &DisplayHandle, surface: &WlSurface| {
         let state = state.downcast_mut::<D>().unwrap();
         hook(state, dh, surface);
     };
-    PrivateSurfaceData::add_post_commit_hook(surface, hook)
+    Ok(PrivateSurfaceData::add_post_commit_hook(surface, hook))
 }
 
 /// Register a destruction hook to be invoked on surface destruction
@@ -504,7 +512,7 @@ where
 /// client disconnect).
 ///
 /// D generic is the compositor state, same as used in `CompositorState::new<D>()`
-pub fn add_destruction_hook<D, F>(surface: &WlSurface, hook: F) -> HookId
+pub fn add_destruction_hook<D, F>(surface: &WlSurface, hook: F) -> Result<HookId, DeadResource>
 where
     F: Fn(&mut D, &WlSurface) + Send + Sync + 'static,
     D: 'static,
@@ -518,11 +526,15 @@ where
         user_state_type,
     );
 
+    if !surface.is_alive() {
+        return Err(DeadResource);
+    }
+
     let hook = move |state: &mut dyn Any, surface: &WlSurface| {
         let state = state.downcast_mut::<D>().unwrap();
         hook(state, surface);
     };
-    PrivateSurfaceData::add_destruction_hook(surface, hook)
+    Ok(PrivateSurfaceData::add_destruction_hook(surface, hook))
 }
 
 /// Unregister a pre-commit hook
@@ -549,8 +561,12 @@ pub fn remove_destruction_hook(surface: &WlSurface, hook_id: HookId) {
 /// The module will only evaluate blocker states on commit. If a blocker
 /// becomes ready later, a call to [`CompositorClientState::blocker_cleared`] is necessary
 /// to trigger a re-evaluation.
-pub fn add_blocker(surface: &WlSurface, blocker: impl Blocker + Send + 'static) {
-    PrivateSurfaceData::add_blocker(surface, blocker)
+pub fn add_blocker(surface: &WlSurface, blocker: impl Blocker + Send + 'static) -> Result<(), DeadResource> {
+    if !surface.is_alive() {
+        return Err(DeadResource);
+    }
+    PrivateSurfaceData::add_blocker(surface, blocker);
+    Ok(())
 }
 
 /// Handler trait for compositor

src/wayland/drm_syncobj/mod.rs

@@ -308,9 +308,15 @@ where
                     );
                     return;
                 }
-                let commit_hook_id = compositor::add_pre_commit_hook::<D, _>(&surface, commit_hook);
-                let destruction_hook_id =
-                    compositor::add_destruction_hook::<D, _>(&surface, destruction_hook);
+                let Ok(commit_hook_id) = compositor::add_pre_commit_hook::<D, _>(&surface, commit_hook)
+                else {
+                    return;
+                };
+                let Ok(destruction_hook_id) =
+                    compositor::add_destruction_hook::<D, _>(&surface, destruction_hook)
+                else {
+                    return;
+                };
                 let syncobj_surface = data_init.init::<_, _>(
                     id,
                     DrmSyncobjSurfaceData {

src/wayland/fifo/mod.rs

@@ -206,7 +206,7 @@ where
 
                 // Make sure we do not install the hook more then once in case the surface is being reused
                 if is_managed && is_initial {
-                    add_pre_commit_hook::<D, _>(&surface, |_, _, surface| {
+                    let _ = add_pre_commit_hook::<D, _>(&surface, |_, _, surface| {
                         let fifo_barrier = with_states(surface, |states| {
                             let fifo_state = *states.cached_state.get::<FifoCachedState>().pending();
 
@@ -253,7 +253,7 @@ where
                             // sync subsurfaces
                             let skip = barrier.is_signaled() || is_sync_subsurface(surface);
                             if !skip {
-                                add_blocker(surface, barrier);
+                                let _ = add_blocker(surface, barrier);
                             }
                         }
                     });

src/wayland/pointer_constraints.rs

@@ -340,7 +340,7 @@ fn add_constraint<D: SeatHandler + PointerConstraintsHandler + 'static>(
     });
 
     if added {
-        compositor::add_post_commit_hook(surface, commit_hook::<D>);
+        let _ = compositor::add_post_commit_hook(surface, commit_hook::<D>);
     }
 }
 

src/wayland/seat/keyboard.rs

@@ -210,16 +210,14 @@ pub(crate) fn enter_internal<D: SeatHandler + 'static>(
     keys: impl Iterator<Item = Keycode>,
     serial: Serial,
 ) {
-    *seat.get_keyboard().unwrap().arc.last_enter.lock().unwrap() = Some(serial);
-    let serialized_keys = serialize_pressed_keys(keys);
-    for_each_focused_kbds(seat, surface, |kbd| {
-        kbd.enter(serial.into(), surface, serialized_keys.clone())
-    });
+    let seat_clone = seat.downgrade();
+    let Ok(hook_id) = add_destruction_hook::<D, _>(surface, move |_, surface| {
+        let Some(seat) = seat_clone.upgrade() else {
+            return;
+        };
 
-    let seat_clone = seat.clone();
-    let hook_id = add_destruction_hook::<D, _>(surface, move |_, surface| {
         if let Some(client) = surface.client() {
-            let keyboard = seat_clone.get_keyboard().unwrap();
+            let keyboard = seat.get_keyboard().unwrap();
             let inner = keyboard.arc.known_kbds.lock().unwrap();
             for kbd in &*inner {
                 let Ok(kbd) = kbd.upgrade() else {
@@ -231,7 +229,16 @@ pub(crate) fn enter_internal<D: SeatHandler + 'static>(
                 }
             }
         }
+    }) else {
+        return;
+    };
+
+    *seat.get_keyboard().unwrap().arc.last_enter.lock().unwrap() = Some(serial);
+    let serialized_keys = serialize_pressed_keys(keys);
+    for_each_focused_kbds(seat, surface, |kbd| {
+        kbd.enter(serial.into(), surface, serialized_keys.clone())
     });
+
     if let Some(old_hook_id) = with_states(surface, |states| {
         states
             .data_map

src/wayland/session_lock/lock.rs

@@ -81,7 +81,7 @@ where
                 let lock_surface = data_init.init(id, data);
 
                 // Initialize surface data.
-                compositor::with_states(&surface, |states| {
+                let initial = compositor::with_states(&surface, |states| {
                     let inserted = states.data_map.insert_if_missing_threadsafe(|| {
                         Mutex::new(LockSurfaceAttributes::new(lock_surface.clone()))
                     });
@@ -95,10 +95,14 @@ where
                             .unwrap();
                         attributes.surface = lock_surface.clone();
                     }
+
+                    inserted
                 });
 
-                // Add pre-commit hook for updating surface state.
-                compositor::add_pre_commit_hook::<D, _>(&surface, LockSurface::pre_commit_hook);
+                if initial {
+                    // Add pre-commit hook for updating surface state.
+                    let _ = compositor::add_pre_commit_hook::<D, _>(&surface, LockSurface::pre_commit_hook);
+                }
 
                 // Call compositor handler.
                 let lock_surface = LockSurface::new(surface, lock_surface);

src/wayland/shell/wlr_layer/handlers.rs

@@ -122,7 +122,7 @@ where
                 });
 
                 if initial {
-                    compositor::add_pre_commit_hook::<D, _>(
+                    let _ = compositor::add_pre_commit_hook::<D, _>(
                         &wl_surface,
                         super::LayerSurface::pre_commit_hook,
                     );

src/wayland/shell/xdg/handlers/surface.rs

@@ -124,7 +124,7 @@ where
                 });
 
                 if initial {
-                    compositor::add_pre_commit_hook::<D, _>(
+                    let _ = compositor::add_pre_commit_hook::<D, _>(
                         surface,
                         super::super::ToplevelSurface::pre_commit_hook,
                     );
@@ -202,7 +202,7 @@ where
                 });
 
                 if initial {
-                    compositor::add_pre_commit_hook::<D, _>(
+                    let _ = compositor::add_pre_commit_hook::<D, _>(
                         surface,
                         super::super::PopupSurface::pre_commit_hook,
                     );

src/wayland/viewporter/mod.rs

@@ -177,7 +177,7 @@ where
 
                 // only add the pre-commit hook once for the surface
                 if initial {
-                    compositor::add_pre_commit_hook::<D, _>(&surface, viewport_pre_commit_hook);
+                    let _ = compositor::add_pre_commit_hook::<D, _>(&surface, viewport_pre_commit_hook);
                 }
             }
             wp_viewporter::Request::Destroy => {

src/wayland/xwayland_shell.rs

@@ -264,7 +264,9 @@ where
                     return;
                 }
 
-                compositor::add_pre_commit_hook::<D, _>(&surface, serial_commit_hook);
+                // TODO: Can the xwayland shell role be re-used? In this case we should guard the pre-commit hook
+                // to avoid duplicates.
+                let _ = compositor::add_pre_commit_hook::<D, _>(&surface, serial_commit_hook);
 
                 data_init.init(id, XWaylandSurfaceUserData { wl_surface: surface });
                 // We call the handler callback once the serial is set.