From e862aca887d0fc4b88e7cb2f3d145d94f0f8ef28 Mon Sep 17 00:00:00 2001 From: Ben Buhse Date: Sun, 12 Apr 2026 10:31:45 -0500 Subject: [PATCH] Fix crash when killing WM with windows active In commit 75308a04, I added an assertion to Output.destroy() to verify that all of its windows had been removed already. The issue is, that only happened when Output received a .removed event, but that never came when killing the WM directly. Instead, I simplified the handling for the .removed event and moved most of that into destroy(). I also changed where structs have their wl_list.link removed (to inside of their destroys) to make it more consistent. Finally, XkbBinding.sendWindowToOutput asserts that the current output is actually focused. Users *should* never have a focused window if there's not also a focused output. --- src/Context.zig | 1 - src/Output.zig | 91 +++++++++++++++++++------------------------ src/Seat.zig | 1 + src/Window.zig | 2 +- src/WindowManager.zig | 3 -- src/XkbBindings.zig | 28 ++++++------- src/XkbConfig.zig | 2 +- 7 files changed, 56 insertions(+), 72 deletions(-) diff --git a/src/Context.zig b/src/Context.zig index 4cb9712..2c43bcd 100644 --- a/src/Context.zig +++ b/src/Context.zig @@ -125,7 +125,6 @@ pub fn manage(context: *Context) void { // Destroy all existing bindings var it = context.xkb_bindings.bindings.safeIterator(.forward); while (it.next()) |binding| { - binding.link.remove(); binding.destroy(); } diff --git a/src/Output.zig b/src/Output.zig index 49b2a3c..36ba31d 100644 --- a/src/Output.zig +++ b/src/Output.zig @@ -111,8 +111,46 @@ pub fn create(context: *Context, river_output_v1: *river.OutputV1) !*Output { } pub fn destroy(output: *Output) void { - // All of windows should've been removed from this output when handling the .removed event - assert(output.windows.first() == null); + const context = output.context; + const wm = context.wm; + + // Before we destroy the output, we wnant to take care of any window that are on it + // We chooe to move windows to the previous output in the WM's output list + // If this was the only output, windows go to the WM's orphan window list instead + const prev_output: ?*Output = if (wm.prevOutput(output)) |prev| blk: { + if (prev == output) break :blk null; // Only output; wrapped to itself + break :blk prev; + } else unreachable; + + const window_pending_output: Window.PendingManage.PendingOutput = + if (prev_output) |prev| + .{ .output = prev } + else + .clear_output; + + // Update each window's output before moving the list + var it = output.windows.iterator(.forward); + while (it.next()) |window| { + window.pending_manage.pending_output = window_pending_output; + } + + // Finally, move the windows to their new output (or the orphan list) + const dest_list = if (prev_output) |prev| &prev.windows else &wm.orphan_windows; + dest_list.appendList(&output.windows); + + seat_blk: { + // If the output was focused, move focus to the output that took this one's windows + // (and its first window, if any). + const seat = wm.seats.first() orelse break :seat_blk; + if (seat.focused_output != output) break :seat_blk; + + seat.pending_manage.output = if (prev_output) |prev| prev_blk: { + if (prev.windows.first()) |window| { + seat.pending_manage.window = .{ .window = window }; + } + break :prev_blk .{ .output = prev }; + } else .clear_focus; + } // Deinit optional surfaces if (output.bar) |*bar| bar.deinit(); @@ -128,6 +166,7 @@ pub fn destroy(output: *Output) void { output.river_output_v1.destroy(); if (output.wl_output) |wl_output| wl_output.release(); + output.link.remove(); utils.gpa.destroy(output); } @@ -166,54 +205,6 @@ fn riverOutputListener(river_output_v1: *river.OutputV1, event: river.OutputV1.E assert(output.river_output_v1 == river_output_v1); switch (event) { .removed => { - const context = output.context; - const wm = context.wm; - - // Move windows to the previous output in the list. - // If this was the only output, windows become orphans. - const prev_output: ?*Output = if (wm.prevOutput(output)) |prev| blk: { - if (prev == output) break :blk null; // Only output; wrapped to itself - break :blk prev; // We got the previous list - } else unreachable; - - const window_pending_output: Window.PendingManage.PendingOutput = - if (prev_output) |prev| - .{ .output = prev } - else - .clear_output; - - // Update each window's output before moving the list - var it = output.windows.iterator(.forward); - while (it.next()) |window| { - window.pending_manage.pending_output = window_pending_output; - } - - // Move windows to new destination - const dest_list = if (prev_output) |prev| &prev.windows else &wm.orphan_windows; - dest_list.appendList(&output.windows); - - blk: { - // If the removed output was focused, move focus to the next - // available output (and its first window, if any). - const seat = wm.seats.first() orelse break :blk; - if (seat.focused_output != output) break :blk; - - const next_output = wm.nextOutput(output); - if (next_output == output or next_output == null) { - // This was the last output; clear focus - seat.pending_manage.output = .clear_focus; - seat.pending_manage.window = .clear_focus; - break :blk; - } - const o = next_output.?; - - seat.pending_manage.output = .{ .output = o }; - if (o.windows.first()) |window| { - seat.pending_manage.window = .{ .window = window }; - } - } - - output.link.remove(); output.destroy(); }, .wl_output => |ev| { diff --git a/src/Seat.zig b/src/Seat.zig index 2f9ee8c..c828bbf 100644 --- a/src/Seat.zig +++ b/src/Seat.zig @@ -88,6 +88,7 @@ pub fn destroy(seat: *Seat) void { if (seat.resize_pointer_binding) |binding| binding.destroy(); seat.river_seat_v1.destroy(); seat.river_layer_shell_seat_v1.destroy(); + seat.link.remove(); utils.gpa.destroy(seat); } diff --git a/src/Window.zig b/src/Window.zig index 4196d56..ad1837f 100644 --- a/src/Window.zig +++ b/src/Window.zig @@ -126,6 +126,7 @@ pub fn destroy(window: *Window) void { window.river_node_v1.destroy(); window.river_window_v1.destroy(); + window.link.remove(); utils.gpa.destroy(window); } @@ -156,7 +157,6 @@ fn windowListener(river_window_v1: *river.WindowV1, event: river.WindowV1.Event, } window.nextFocus(); - window.link.remove(); window.destroy(); }, .dimensions => |ev| { diff --git a/src/WindowManager.zig b/src/WindowManager.zig index 32c7aad..a0e11be 100644 --- a/src/WindowManager.zig +++ b/src/WindowManager.zig @@ -41,21 +41,18 @@ pub fn destroy(wm: *WindowManager) void { { var it = wm.outputs.safeIterator(.forward); while (it.next()) |output| { - output.link.remove(); output.destroy(); } } { var it = wm.seats.safeIterator(.forward); while (it.next()) |seat| { - seat.link.remove(); seat.destroy(); } } { var it = wm.orphan_windows.safeIterator(.forward); while (it.next()) |window| { - window.link.remove(); window.destroy(); } } diff --git a/src/XkbBindings.zig b/src/XkbBindings.zig index ae19305..f5ea952 100644 --- a/src/XkbBindings.zig +++ b/src/XkbBindings.zig @@ -120,19 +120,15 @@ const XkbBinding = struct { pub fn destroy(xkb_binding: *XkbBinding) void { xkb_binding.xkb_binding_v1.destroy(); + xkb_binding.link.remove(); utils.gpa.destroy(xkb_binding); } fn xkbBindingListener(river_xkb_binding_v1: *river.XkbBindingV1, event: river.XkbBindingV1.Event, xkb_binding: *XkbBinding) void { assert(xkb_binding.xkb_binding_v1 == river_xkb_binding_v1); switch (event) { - .pressed => { - xkb_binding.executeCommand(); - }, - .released => {}, - else => |ev| { - log.debug("unhandled event: {s}", .{@tagName(ev)}); - }, + .pressed => xkb_binding.executeCommand(), + else => {}, } } @@ -375,20 +371,20 @@ const XkbBinding = struct { } } + /// This function requires that the window is currently on an output + /// AND that the output is the currently focused output on the first seat. fn sendWindowToOutput(context: *Context, direction: FocusDirection) void { const wm = context.wm; const seat = wm.seats.first() orelse return; const window = seat.focused_window orelse return; - assert(window.output == seat.focused_output); - const pending_output = if (seat.focused_output) |current| - switch (direction) { - .next => wm.nextOutput(current), - .prev => wm.prevOutput(current), - } - else switch (direction) { - .next => wm.outputs.first(), - .prev => wm.outputs.last(), + assert(window.output == seat.focused_output); + assert(seat.focused_output != null); + + const current_output = seat.focused_output.?; + const pending_output = switch (direction) { + .next => wm.nextOutput(current_output), + .prev => wm.prevOutput(current_output), }; if (pending_output) |output| blk: { diff --git a/src/XkbConfig.zig b/src/XkbConfig.zig index 0db374f..e6c9d95 100644 --- a/src/XkbConfig.zig +++ b/src/XkbConfig.zig @@ -42,8 +42,8 @@ const Keyboard = struct { } fn destroy(keyboard: *Keyboard) void { - keyboard.link.remove(); keyboard.xkb_keyboard_v1.destroy(); + keyboard.link.remove(); utils.gpa.destroy(keyboard); }