mirror/i3
3
0
Fork 0
mirror of https://github.com/i3/i3.git synced 2026-08-18 02:52:38 +00:00
Code Issues Packages Projects Releases Wiki Activity

Enforce tracking of ignored events by 32-bit full sequence (#6642)

Browse Source 
This PR is a follow-up to the discussion in #6372 .

I'm not too familiar with the internals of X11, but it seems that sequence numbers are sometimes of type `uint16_t` and sometimes of type `unsigned int` or `uint32_t`. In some places XCB provides the field `full_sequence` (for example as part of `xcb_generic_event_t`), but other structs only have the 16-bit sequence. Then there is also `xcb_void_cookie_t` which has `unsigned int` for `sequence`.

The current code for tracking ignored events mixes these different data types. This works fine, as long as the sequence number doesn't overflow a `uint16_t`. Afterwards, the tracking of ignored events will fail. This appears to be the reason for #6372 and probably also #6364 . There is also the open PR https://github.com/i3/i3/pull/6559 which notes: "[...] add the sequence to the ignore list, but under some (unknown) circumstances, that does not seem to work reliably."

This problem already existed in 4.23 (and before), but only became visible in 4.24, because it seems that is was masked by using `XCB_GRAB_MODE_SYNC` when executing key bindings. This seems to mask the bug, but sending a command via `i3-msg` also triggers it in 4.23 . Commit b42dc21068 changed `XCB_GRAB_MODE_SYNC` to `XCB_GRAB_MODE_ASYNC` and surfaced the bug. Additionally, the commit https://github.com/i3/i3/commit/cfa4cf16 increased the number of messages being sent and sequence numbers being used, which triggers this bug more quickly than before.

The first commit in this PR introduces a test for #6372 . I found a more performant way of overflowing the uint16_t sequence number (using sync_with_i3 to create fairly cheap messages). The test runs in 1.3 seconds on my system.

The second commit fixes the problem. It defines a new data type `ignore_event_sequence_t` and a macro `ENSURE_FULL_SEQUENCE` which is then used at every call site for `add_ignore_event` and `event_is_ignored`. This way the compiler checks that a full sequence number is used everywhere. Perhaps this is over-engineered? Please let me know - I don't work much in C.

This change makes the test pass and also fixes #6372 for me in my manual testing.

Looking forward to feedback!

Disclosure of AI assistance: I used Claude Fable 5 and GPT-5.6 Sol as sparring partners, but everything here is hand-written.
This commit is contained in:
javgh
2026-07-28 08:22:26 +02:00
committed by GitHub
parent f7d5b8983f
commit 9be3249ac5
8 changed files with 103 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
include/data.h
View File

@ -241,8 +241,24 @@ struct Workspace_Assignment {
TAILQ_ENTRY(Workspace_Assignment) ws_assignments;
};
typedef struct ignore_event_sequence_t {
uint32_t value;
} ignore_event_sequence_t;
/**
* We want to prevent confusion between a uint16_t sequence and a uint32_t full sequence.
* This macro will check that the provided argument has the correct sizeof() and will
* otherwise throw an error at compile-time ("Array size is negative"). At runtime,
* the macro will cause the argument to be wrapped and returned by use of the comma
* operator expression.
*
*/
#define ENSURE_FULL_SEQUENCE(sequence) \
((void)sizeof(char[(sizeof(sequence) == sizeof(uint32_t)) ? 1 : -1]), \
((ignore_event_sequence_t){.value = sequence}))
struct Ignore_Event {
int sequence;
ignore_event_sequence_t sequence;
int response_type;
time_t added;

6
include/handlers.h
View File

@ -14,6 +14,8 @@
#include <xcb/randr.h>
#include "data.h"
extern int randr_base;
extern int xkb_base;
extern int shape_base;
@ -26,13 +28,13 @@ extern int shape_base;
* Every ignored sequence number gets garbage collected after 5 seconds.
*
*/
void add_ignore_event(const int sequence, const int response_type);
void add_ignore_event(const ignore_event_sequence_t sequence, const int response_type);
/**
* Checks if the given sequence is ignored and returns true if so.
*
*/
bool event_is_ignored(const int sequence, const int response_type);
bool event_is_ignored(const ignore_event_sequence_t sequence, const int response_type);
/**
* Takes an xcb_generic_event_t and calls the appropriate handler, based on the

1
release-notes/bugfixes/3-losing-focus-when-moving-workspace Normal file
View File

@ -0,0 +1 @@
fix bug where moving a workspace to another screen would sometimes cause it to lose focus

34
src/handlers.c
View File

@ -35,7 +35,7 @@ static SLIST_HEAD(ignore_head, Ignore_Event) ignore_events;
* Every ignored sequence number gets garbage collected after 5 seconds.
*
*/
void add_ignore_event(const int sequence, const int response_type) {
void add_ignore_event(const ignore_event_sequence_t sequence, const int response_type) {
struct Ignore_Event *event = smalloc(sizeof(struct Ignore_Event));
event->sequence = sequence;
@ -49,7 +49,7 @@ void add_ignore_event(const int sequence, const int response_type) {
* Checks if the given sequence is ignored and returns true if so.
*
*/
bool event_is_ignored(const int sequence, const int response_type) {
bool event_is_ignored(const ignore_event_sequence_t sequence, const int response_type) {
struct Ignore_Event *event;
time_t now = time(NULL);
for (event = SLIST_FIRST(&ignore_events); event != SLIST_END(&ignore_events);) {
@ -64,7 +64,7 @@ bool event_is_ignored(const int sequence, const int response_type) {
}
SLIST_FOREACH (event, &ignore_events, ignore_events) {
if (event->sequence != sequence) {
if (event->sequence.value != sequence.value) {
continue;
}
@ -124,7 +124,7 @@ static void check_crossing_screen_boundary(uint32_t x, uint32_t y) {
* When the user moves the mouse pointer onto a window, this callback gets called.
*
*/
static void handle_enter_notify(xcb_enter_notify_event_t *event) {
static void handle_enter_notify(xcb_enter_notify_event_t *event, uint32_t full_sequence) {
Con *con;
last_timestamp = event->time;
@ -138,7 +138,7 @@ static void handle_enter_notify(xcb_enter_notify_event_t *event) {
}
/* Some events are not interesting, because they were not generated
* actively by the user, but by reconfiguration of windows */
if (event_is_ignored(event->sequence, XCB_ENTER_NOTIFY)) {
if (event_is_ignored(ENSURE_FULL_SEQUENCE(full_sequence), XCB_ENTER_NOTIFY)) {
DLOG("Event ignored\n");
return;
}
@ -280,11 +280,11 @@ static void handle_mapping_notify(xcb_mapping_notify_event_t *event) {
* A new window appeared on the screen (=was mapped), so let’s manage it.
*
*/
static void handle_map_request(const xcb_map_request_event_t *event) {
static void handle_map_request(const xcb_map_request_event_t *event, uint32_t full_sequence) {
xcb_get_window_attributes_cookie_t cookie = xcb_get_window_attributes_unchecked(conn, event->window);
DLOG("window = 0x%08x, serial is %d.\n", event->window, event->sequence);
add_ignore_event(event->sequence, -1);
add_ignore_event(ENSURE_FULL_SEQUENCE(full_sequence), -1);
manage_window(event->window, cookie, false);
}
@ -479,7 +479,7 @@ static void handle_screen_change(xcb_generic_event_t *e) {
* now, so we better clean up before.
*
*/
static void handle_unmap_notify_event(xcb_unmap_notify_event_t *event) {
static void handle_unmap_notify_event(xcb_unmap_notify_event_t *event, uint32_t full_sequence) {
DLOG("UnmapNotify for 0x%08x (received from 0x%08x), serial %d\n", event->window, event->event, event->sequence);
xcb_get_input_focus_cookie_t cookie;
Con *con = con_by_window_id(event->window);
@ -532,7 +532,7 @@ ignore_end:
*
* Therefore, we ignore all EnterNotify events which have the same sequence
* as an UnmapNotify event. */
add_ignore_event(event->sequence, XCB_ENTER_NOTIFY);
add_ignore_event(ENSURE_FULL_SEQUENCE(full_sequence), XCB_ENTER_NOTIFY);
/* Since we just ignored the sequence of this UnmapNotify, we want to make
* sure that following events use a different sequence. When putting xterm
@ -551,7 +551,7 @@ ignore_end:
* important fields in the event data structure).
*
*/
static void handle_destroy_notify_event(xcb_destroy_notify_event_t *event) {
static void handle_destroy_notify_event(xcb_destroy_notify_event_t *event, uint32_t full_sequence) {
DLOG("destroy notify for 0x%08x, 0x%08x\n", event->event, event->window);
xcb_unmap_notify_event_t unmap;
@ -559,7 +559,7 @@ static void handle_destroy_notify_event(xcb_destroy_notify_event_t *event) {
unmap.event = event->event;
unmap.window = event->window;
handle_unmap_notify_event(&unmap);
handle_unmap_notify_event(&unmap, full_sequence);
}
static bool window_name_changed(i3Window *window, char *old_name) {
@ -1432,11 +1432,11 @@ void handle_event(int type, xcb_generic_event_t *event) {
translate_keysyms();
grab_all_keys(conn);
} else if (state->xkbType == XCB_XKB_MAP_NOTIFY) {
if (event_is_ignored(event->sequence, type)) {
if (event_is_ignored(ENSURE_FULL_SEQUENCE(event->full_sequence), type)) {
DLOG("Ignoring map notify event for sequence %d.\n", state->sequence);
} else {
DLOG("xkb map notify, sequence %d, time %d\n", state->sequence, state->time);
add_ignore_event(event->sequence, type);
add_ignore_event(ENSURE_FULL_SEQUENCE(event->full_sequence), type);
xcb_key_symbols_free(keysyms);
keysyms = xcb_key_symbols_alloc(conn);
ungrab_all_keys(conn);
@ -1490,15 +1490,15 @@ void handle_event(int type, xcb_generic_event_t *event) {
break;
case XCB_MAP_REQUEST:
handle_map_request((xcb_map_request_event_t *)event);
handle_map_request((xcb_map_request_event_t *)event, event->full_sequence);
break;
case XCB_UNMAP_NOTIFY:
handle_unmap_notify_event((xcb_unmap_notify_event_t *)event);
handle_unmap_notify_event((xcb_unmap_notify_event_t *)event, event->full_sequence);
break;
case XCB_DESTROY_NOTIFY:
handle_destroy_notify_event((xcb_destroy_notify_event_t *)event);
handle_destroy_notify_event((xcb_destroy_notify_event_t *)event, event->full_sequence);
break;
case XCB_EXPOSE:
@ -1514,7 +1514,7 @@ void handle_event(int type, xcb_generic_event_t *event) {
/* Enter window = user moved their mouse over the window */
case XCB_ENTER_NOTIFY:
handle_enter_notify((xcb_enter_notify_event_t *)event);
handle_enter_notify((xcb_enter_notify_event_t *)event, event->full_sequence);
break;
/* Client message are sent to the root window. The only interesting

2
src/main.c
View File

@ -133,7 +133,7 @@ static void xcb_prepare_cb(EV_P_ ev_prepare *w, int revents) {
while ((event = xcb_poll_for_event(conn)) != NULL) {
if (event->response_type == 0) {
if (event_is_ignored(event->sequence, 0)) {
if (event_is_ignored(ENSURE_FULL_SEQUENCE(event->full_sequence), 0)) {
DLOG("Expected X11 Error received for sequence %x\n", event->sequence);
} else {
xcb_generic_error_t *error = (xcb_generic_error_t *)event;

4
src/tree.c
View File

@ -232,7 +232,7 @@ bool tree_close_internal(Con *con, kill_window_t kill_window, bool dont_kill_par
/* Ignore X11 errors for the ReparentWindow request.
* X11 Errors are returned when the window was already destroyed */
add_ignore_event(cookie.sequence, 0);
add_ignore_event(ENSURE_FULL_SEQUENCE(cookie.sequence), 0);
/* We are no longer handling this window, thus set WM_STATE to
* WM_STATE_WITHDRAWN (see ICCCM 4.1.3.1) */
@ -254,7 +254,7 @@ bool tree_close_internal(Con *con, kill_window_t kill_window, bool dont_kill_par
/* Ignore X11 errors for the ReparentWindow request.
* X11 Errors are returned when the window was already destroyed */
add_ignore_event(cookie.sequence, 0);
add_ignore_event(ENSURE_FULL_SEQUENCE(cookie.sequence), 0);
ipc_send_window_event("close", con);
window_free(con->window);
con->window = NULL;

2
src/xcb.c
View File

@ -112,7 +112,7 @@ void xcb_set_window_rect(xcb_connection_t *conn, const xcb_window_t window, Rect
XCB_CONFIG_WINDOW_HEIGHT,
&(r.x));
/* ignore events which are generated because we configured a window */
add_ignore_event(cookie.sequence, -1);
add_ignore_event(ENSURE_FULL_SEQUENCE(cookie.sequence), -1);
}
/*

60
testcases/t/556-workspace-keeps-focus-after-move.t Normal file
View File

@ -0,0 +1,60 @@
#!perl
# vim:ts=4:sw=4:expandtab
#
# Please read the following documents before working on tests:
# • https://build.i3wm.org/docs/testsuite.html
# (or docs/testsuite)
#
# • https://build.i3wm.org/docs/lib-i3test.html
# (alternatively: perldoc ./testcases/lib/i3test.pm)
#
# • https://build.i3wm.org/docs/ipc.html
# (or docs/ipc)
#
# • https://i3wm.org/downloads/modern_perl_a4.pdf
# (unless you are already familiar with Perl)
#
# Tests that a workspace keeps its focus after moving even in event-heavy sessions.
# Ticket: #6372
# Bug still in: 4.25-24-gf7d5b898
use i3test i3_autostart => 0;
my $config = <<EOT;
font -misc-fixed-medium-r-normal--13-120-75-75-C-70-iso10646-1
fake-outputs 1024x768+0+0,1024x768+1024+0
EOT
my $pid = launch_with_config($config);
cmd 'workspace ws1';
cmd 'layout default';
cmd 'split v';
my $first = open_window;
is($x->input_focus, $first->id, 'window focused');
# We use 'sync_with_i3' to push the sequence number for xcb events.
# The reply is a fairly cheap way of increasing the sequence number.
for (1 .. 1000) {
sync_with_i3;
}
is($x->input_focus, $first->id, 'window still focused after some events');
cmd 'move workspace to output right';
is($x->input_focus, $first->id, 'window still focused after some events and after workspace has been moved');
cmd 'move workspace to output left';
# We push the sequence number until it overflows a 16-bit counter.
# This triggers bugs where a uint16_t sequence is compared with a uint32_t full_sequence.
for (1 .. 1 << 16) {
sync_with_i3;
}
is($x->input_focus, $first->id, 'window still focused after many events');
cmd 'move workspace to output right';
is($x->input_focus, $first->id, 'window still focused after many events and after workspace has been moved');
exit_gracefully($pid);
done_testing;