From d1974575c4977289a7e700a660893c079f0cbdbe Mon Sep 17 00:00:00 2001 From: Christian Meissl Date: Fri, 6 Feb 2026 14:14:43 +0100 Subject: [PATCH 1/4] remove pre-commit hook when surface is destroyed this re-uses the already existing remove_default_dmabuf_pre_commit_hook during surface destruction instead of just removing the hook from the stored list of hooks. --- src/handlers/compositor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/handlers/compositor.rs b/src/handlers/compositor.rs index f98a9824ef..6d9d0546c4 100644 --- a/src/handlers/compositor.rs +++ b/src/handlers/compositor.rs @@ -498,7 +498,7 @@ impl CompositorHandler for State { .root_surface .retain(|k, v| k != surface && v != surface); - self.niri.dmabuf_pre_commit_hook.remove(surface); + self.remove_default_dmabuf_pre_commit_hook(surface); } } From cb6e06d48f88c8b28c9d572a2ce65084ef301df6 Mon Sep 17 00:00:00 2001 From: Christian Meissl Date: Fri, 6 Feb 2026 18:55:20 +0100 Subject: [PATCH 2/4] do not add dmabuf pre-commit hook for destroyed surfaces this prevents surfaces getting stored indefinitely in case some logic tries to add the hook for an already destroyed surface. --- src/handlers/compositor.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/handlers/compositor.rs b/src/handlers/compositor.rs index 6d9d0546c4..8557eddb31 100644 --- a/src/handlers/compositor.rs +++ b/src/handlers/compositor.rs @@ -517,6 +517,11 @@ delegate_shm!(State); impl State { pub fn add_default_dmabuf_pre_commit_hook(&mut self, surface: &WlSurface) { + if !surface.is_alive() { + error!("tried to add dmabuf pre-commit hook for a dead surface"); + return; + } + let hook = add_pre_commit_hook::(surface, move |state, _dh, surface| { let maybe_dmabuf = with_states(surface, |surface_data| { surface_data From 1f74aa281e04d6d9a4f47154753ec47fc00560f9 Mon Sep 17 00:00:00 2001 From: Christian Meissl Date: Sun, 8 Feb 2026 13:01:22 +0100 Subject: [PATCH 3/4] align surface/toplevel destruction order for client destruction resource destruction has undefined order in case the client does not explicitly destroy the resourced and wait for destruction to complete. the same applies for clients exiting unexpectedly. --- src/handlers/compositor.rs | 6 ++++-- src/handlers/xdg_shell.rs | 9 ++++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/handlers/compositor.rs b/src/handlers/compositor.rs index 8557eddb31..ee9a9281ca 100644 --- a/src/handlers/compositor.rs +++ b/src/handlers/compositor.rs @@ -566,10 +566,12 @@ impl State { } pub fn remove_default_dmabuf_pre_commit_hook(&mut self, surface: &WlSurface) { + // In case a client disconnects the hook might not be active at the time + // the surface is destroyed because it was previously mapped. + // This can happen because implicit resource destruction is done with + // undefined order, so the surface might get destroyed before a toplevel. if let Some(hook) = self.niri.dmabuf_pre_commit_hook.remove(surface) { remove_pre_commit_hook(surface, hook); - } else { - error!("tried to remove dmabuf pre-commit hook but there was none"); } } } diff --git a/src/handlers/xdg_shell.rs b/src/handlers/xdg_shell.rs index 74cf5bc6a2..7b31c2fc7d 100644 --- a/src/handlers/xdg_shell.rs +++ b/src/handlers/xdg_shell.rs @@ -863,7 +863,14 @@ impl XdgShellHandler for State { self.niri.window_mru_ui.remove_window(id); self.niri.layout.remove_window(&window, transaction.clone()); - self.add_default_dmabuf_pre_commit_hook(surface.wl_surface()); + let surface = surface.wl_surface(); + // This check is necessary because implicit resource destruction is done with + // undefined order, so the surface might get destroyed before a toplevel. + if surface.is_alive() { + // Re-add the default dmabuf pre commit hook in case the surface + // role is re-used. + self.add_default_dmabuf_pre_commit_hook(surface); + } // If this is the only instance, then this transaction will complete immediately, so no // need to set the timer. From 11134b9d63164486510ec59a968cd10e31eb852b Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Tue, 10 Feb 2026 08:38:30 +0300 Subject: [PATCH 4/4] rearrange some things --- src/handlers/compositor.rs | 17 ++++++++++++----- src/handlers/xdg_shell.rs | 7 ++++--- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/handlers/compositor.rs b/src/handlers/compositor.rs index ee9a9281ca..daf342296f 100644 --- a/src/handlers/compositor.rs +++ b/src/handlers/compositor.rs @@ -498,7 +498,16 @@ impl CompositorHandler for State { .root_surface .retain(|k, v| k != surface && v != surface); - self.remove_default_dmabuf_pre_commit_hook(surface); + // The object destruction order is not guaranteed to follow the logical role order. So for + // example when a client disconnects unexpectedly, WlSurface::destroyed() may be called + // before XdgShellHandler::toplevel_destroyed(). In this case, the surface will *not* have + // the default dmabuf pre-commit hook: it will still have the toplevel pre-commit hook. + // + // So, this may come out empty, and then the toplevel pre-commit hook will be removed in the + // subsequent toplevel_destroyed() call. + if let Some(hook) = self.niri.dmabuf_pre_commit_hook.remove(surface) { + remove_pre_commit_hook(surface, hook); + } } } @@ -566,12 +575,10 @@ impl State { } pub fn remove_default_dmabuf_pre_commit_hook(&mut self, surface: &WlSurface) { - // In case a client disconnects the hook might not be active at the time - // the surface is destroyed because it was previously mapped. - // This can happen because implicit resource destruction is done with - // undefined order, so the surface might get destroyed before a toplevel. if let Some(hook) = self.niri.dmabuf_pre_commit_hook.remove(surface) { remove_pre_commit_hook(surface, hook); + } else { + error!("tried to remove dmabuf pre-commit hook but there was none"); } } } diff --git a/src/handlers/xdg_shell.rs b/src/handlers/xdg_shell.rs index 7b31c2fc7d..8512f85107 100644 --- a/src/handlers/xdg_shell.rs +++ b/src/handlers/xdg_shell.rs @@ -863,12 +863,13 @@ impl XdgShellHandler for State { self.niri.window_mru_ui.remove_window(id); self.niri.layout.remove_window(&window, transaction.clone()); + let surface = surface.wl_surface(); // This check is necessary because implicit resource destruction is done with - // undefined order, so the surface might get destroyed before a toplevel. + // undefined order, so the surface might get destroyed before toplevel_destroyed() is + // called. In this case, adding the default pre-commit hook here would leak it, since the + // place that removes it is WlSurface::destroyed(), which had already been called by now. if surface.is_alive() { - // Re-add the default dmabuf pre commit hook in case the surface - // role is re-used. self.add_default_dmabuf_pre_commit_hook(surface); }