Merge pull request #3028 from stapelberg/flakes2

Eliminate causes of flakyness

i3bar/src/xcb.c

@@ -83,7 +83,6 @@ int mod_pressed = 0;
 
 /* Event watchers, to interact with the user */
 ev_prepare *xcb_prep;
-ev_check *xcb_chk;
 ev_io *xcb_io;
 ev_io *xkb_io;
 
@@ -1075,21 +1074,11 @@ static void handle_resize_request(xcb_resize_request_event_t *event) {
 }
 
 /*
- * This function is called immediately before the main loop locks. We flush xcb
+ * This function is called immediately before the main loop locks. We check for
- * then (and only then)
+ * events from X11, handle them, then flush our outgoing queue.
  *
  */
 void xcb_prep_cb(struct ev_loop *loop, ev_prepare *watcher, int revents) {
-    xcb_flush(xcb_connection);
-}
-
-/*
- * This function is called immediately after the main loop locks, so when one
- * of the watchers registered an event.
- * We check whether an X-Event arrived and handle it.
- *
- */
-void xcb_chk_cb(struct ev_loop *loop, ev_check *watcher, int revents) {
     xcb_generic_event_t *event;
 
     if (xcb_connection_has_error(xcb_connection)) {
@@ -1210,6 +1199,8 @@ void xcb_chk_cb(struct ev_loop *loop, ev_check *watcher, int revents) {
         }
         free(event);
     }
+
+    xcb_flush(xcb_connection);
 }
 
 /*
@@ -1267,21 +1258,12 @@ char *init_xcb_early() {
     /* The various watchers to communicate with xcb */
     xcb_io = smalloc(sizeof(ev_io));
     xcb_prep = smalloc(sizeof(ev_prepare));
-    xcb_chk = smalloc(sizeof(ev_check));
 
     ev_io_init(xcb_io, &xcb_io_cb, xcb_get_file_descriptor(xcb_connection), EV_READ);
     ev_prepare_init(xcb_prep, &xcb_prep_cb);
-    ev_check_init(xcb_chk, &xcb_chk_cb);
-
-    /* Within an event loop iteration, run the xcb_chk watcher last: other
-     * watchers might call xcb_flush(), which, unexpectedly, can also read
-     * events into the queue (see _xcb_conn_wait). Hence, we need to drain xcb’s
-     * queue last, otherwise we risk dead-locking. */
-    ev_set_priority(xcb_chk, EV_MINPRI);
 
     ev_io_start(main_loop, xcb_io);
     ev_prepare_start(main_loop, xcb_prep);
-    ev_check_start(main_loop, xcb_chk);
 
     /* Now we get the atoms and save them in a nice data structure */
     get_atoms();
@@ -1535,11 +1517,9 @@ void clean_xcb(void) {
     xcb_aux_sync(xcb_connection);
     xcb_disconnect(xcb_connection);
 
-    ev_check_stop(main_loop, xcb_chk);
     ev_prepare_stop(main_loop, xcb_prep);
     ev_io_stop(main_loop, xcb_io);
 
-    FREE(xcb_chk);
     FREE(xcb_prep);
     FREE(xcb_io);
 }

include/i3.h

@@ -74,3 +74,4 @@ extern bool xcursor_supported, xkb_supported;
 extern xcb_window_t root;
 extern struct ev_loop *main_loop;
 extern bool only_check_config;
+extern bool force_xinerama;

src/floating.c

@@ -667,7 +667,7 @@ void floating_resize_window(Con *con, const bool proportional,
 
 /* Custom data structure used to track dragging-related events. */
 struct drag_x11_cb {
-    ev_check check;
+    ev_prepare prepare;
 
     /* Whether this modal event loop should be exited and with which result. */
     drag_result_t result;
@@ -686,7 +686,7 @@ struct drag_x11_cb {
     const void *extra;
 };
 
-static void xcb_drag_check_cb(EV_P_ ev_check *w, int revents) {
+static void xcb_drag_prepare_cb(EV_P_ ev_prepare *w, int revents) {
     struct drag_x11_cb *dragloop = (struct drag_x11_cb *)w->data;
     xcb_motion_notify_event_t *last_motion_notify = NULL;
     xcb_generic_event_t *event;
@@ -765,6 +765,8 @@ static void xcb_drag_check_cb(EV_P_ ev_check *w, int revents) {
             dragloop->extra);
     }
     free(last_motion_notify);
+
+    xcb_flush(conn);
 }
 
 /*
@@ -831,18 +833,18 @@ drag_result_t drag_pointer(Con *con, const xcb_button_press_event_t *event, xcb_
         .callback = callback,
         .extra = extra,
     };
-    ev_check *check = &loop.check;
+    ev_prepare *prepare = &loop.prepare;
     if (con)
         loop.old_rect = con->rect;
-    ev_check_init(check, xcb_drag_check_cb);
+    ev_prepare_init(prepare, xcb_drag_prepare_cb);
-    check->data = &loop;
+    prepare->data = &loop;
     main_set_x11_cb(false);
-    ev_check_start(main_loop, check);
+    ev_prepare_start(main_loop, prepare);
 
     while (loop.result == DRAGGING)
         ev_run(main_loop, EVRUN_ONCE);
 
-    ev_check_stop(main_loop, check);
+    ev_prepare_stop(main_loop, prepare);
     main_set_x11_cb(true);
 
     xcb_ungrab_keyboard(conn, XCB_CURRENT_TIME);

src/handlers.c

@@ -1264,6 +1264,9 @@ static void handle_configure_notify(xcb_configure_notify_event_t *event) {
     }
     DLOG("ConfigureNotify for root window 0x%08x\n", event->event);
 
+    if (force_xinerama) {
+        return;
+    }
     randr_query_outputs();
 }
 

src/main.c

@@ -35,9 +35,9 @@ struct rlimit original_rlimit_core;
 /** The number of file descriptors passed via socket activation. */
 int listen_fds;
 
-/* We keep the xcb_check watcher around to be able to enable and disable it
+/* We keep the xcb_prepare watcher around to be able to enable and disable it
  * temporarily for drag_pointer(). */
-static struct ev_check *xcb_check;
+static struct ev_prepare *xcb_prepare;
 
 extern Con *focused;
 
@@ -92,29 +92,26 @@ struct ws_assignments_head ws_assignments = TAILQ_HEAD_INITIALIZER(ws_assignment
 bool xcursor_supported = true;
 bool xkb_supported = true;
 
+bool force_xinerama = false;
+
 /*
- * This callback is only a dummy, see xcb_prepare_cb and xcb_check_cb.
+ * This callback is only a dummy, see xcb_prepare_cb.
  * See also man libev(3): "ev_prepare" and "ev_check" - customise your event loop
  *
  */
 static void xcb_got_event(EV_P_ struct ev_io *w, int revents) {
-    /* empty, because xcb_prepare_cb and xcb_check_cb are used */
+    /* empty, because xcb_prepare_cb are used */
 }
 
 /*
- * Flush before blocking (and waiting for new events)
+ * Called just before the event loop sleeps. Ensures xcb’s incoming and outgoing
+ * queues are empty so that any activity will trigger another event loop
+ * iteration, and hence another xcb_prepare_cb invocation.
  *
  */
 static void xcb_prepare_cb(EV_P_ ev_prepare *w, int revents) {
-    xcb_flush(conn);
+    /* Process all queued (and possibly new) events before the event loop
-}
+       sleeps. */
-
-/*
- * Instead of polling the X connection socket we leave this to
- * xcb_poll_for_event() which knows better than we can ever know.
- *
- */
-static void xcb_check_cb(EV_P_ ev_check *w, int revents) {
     xcb_generic_event_t *event;
 
     while ((event = xcb_poll_for_event(conn)) != NULL) {
@@ -137,6 +134,9 @@ static void xcb_check_cb(EV_P_ ev_check *w, int revents) {
 
         free(event);
     }
+
+    /* Flush all queued events to X11. */
+    xcb_flush(conn);
 }
 
 /*
@@ -148,12 +148,12 @@ static void xcb_check_cb(EV_P_ ev_check *w, int revents) {
 void main_set_x11_cb(bool enable) {
     DLOG("Setting main X11 callback to enabled=%d\n", enable);
     if (enable) {
-        ev_check_start(main_loop, xcb_check);
+        ev_prepare_start(main_loop, xcb_prepare);
         /* Trigger the watcher explicitly to handle all remaining X11 events.
          * drag_pointer()’s event handler exits in the middle of the loop. */
-        ev_feed_event(main_loop, xcb_check, 0);
+        ev_feed_event(main_loop, xcb_prepare, 0);
     } else {
-        ev_check_stop(main_loop, xcb_check);
+        ev_prepare_stop(main_loop, xcb_prepare);
     }
 }
 
@@ -197,7 +197,6 @@ int main(int argc, char *argv[]) {
     bool autostart = true;
     char *layout_path = NULL;
     bool delete_layout_path = false;
-    bool force_xinerama = false;
     bool disable_randr15 = false;
     char *fake_outputs = NULL;
     bool disable_signalhandler = false;
@@ -550,6 +549,10 @@ int main(int argc, char *argv[]) {
             config.ipc_socket_path = sstrdup(config.ipc_socket_path);
     }
 
+    if (config.force_xinerama) {
+        force_xinerama = true;
+    }
+
     xcb_void_cookie_t cookie;
     cookie = xcb_change_window_attributes_checked(conn, root, XCB_CW_EVENT_MASK, (uint32_t[]){ROOT_EVENT_MASK});
     xcb_generic_error_t *error = xcb_request_check(conn, cookie);
@@ -668,7 +671,7 @@ int main(int argc, char *argv[]) {
         fake_outputs_init(fake_outputs);
         FREE(fake_outputs);
         config.fake_outputs = NULL;
-    } else if (force_xinerama || config.force_xinerama) {
+    } else if (force_xinerama) {
         /* Force Xinerama (for drivers which don't support RandR yet, esp. the
          * nVidia binary graphics driver), when specified either in the config
          * file or on command-line */
@@ -776,15 +779,11 @@ int main(int argc, char *argv[]) {
     ewmh_update_desktop_viewport();
 
     struct ev_io *xcb_watcher = scalloc(1, sizeof(struct ev_io));
-    xcb_check = scalloc(1, sizeof(struct ev_check));
+    xcb_prepare = scalloc(1, sizeof(struct ev_prepare));
-    struct ev_prepare *xcb_prepare = scalloc(1, sizeof(struct ev_prepare));
 
     ev_io_init(xcb_watcher, xcb_got_event, xcb_get_file_descriptor(conn), EV_READ);
     ev_io_start(main_loop, xcb_watcher);
 
-    ev_check_init(xcb_check, xcb_check_cb);
-    ev_check_start(main_loop, xcb_check);
-
     ev_prepare_init(xcb_prepare, xcb_prepare_cb);
     ev_prepare_start(main_loop, xcb_prepare);
 

src/restore_layout.c

@@ -39,7 +39,6 @@ static TAILQ_HEAD(state_head, placeholder_state) state_head =
 static xcb_connection_t *restore_conn;
 
 static struct ev_io *xcb_watcher;
-static struct ev_check *xcb_check;
 static struct ev_prepare *xcb_prepare;
 
 static void restore_handle_event(int type, xcb_generic_event_t *event);
@@ -49,10 +48,6 @@ static void restore_xcb_got_event(EV_P_ struct ev_io *w, int revents) {
 }
 
 static void restore_xcb_prepare_cb(EV_P_ ev_prepare *w, int revents) {
-    xcb_flush(restore_conn);
-}
-
-static void restore_xcb_check_cb(EV_P_ ev_check *w, int revents) {
     xcb_generic_event_t *event;
 
     if (xcb_connection_has_error(restore_conn)) {
@@ -77,6 +72,8 @@ static void restore_xcb_check_cb(EV_P_ ev_check *w, int revents) {
 
         free(event);
     }
+
+    xcb_flush(restore_conn);
 }
 
 /*
@@ -91,7 +88,6 @@ void restore_connect(void) {
         /* This is not the initial connect, but a reconnect, most likely
          * because our X11 connection was killed (e.g. by a user with xkill. */
         ev_io_stop(main_loop, xcb_watcher);
-        ev_check_stop(main_loop, xcb_check);
         ev_prepare_stop(main_loop, xcb_prepare);
 
         placeholder_state *state;
@@ -107,7 +103,6 @@ void restore_connect(void) {
          */
         xcb_disconnect(restore_conn);
         free(xcb_watcher);
-        free(xcb_check);
         free(xcb_prepare);
     }
 
@@ -124,15 +119,11 @@ void restore_connect(void) {
     }
 
     xcb_watcher = scalloc(1, sizeof(struct ev_io));
-    xcb_check = scalloc(1, sizeof(struct ev_check));
     xcb_prepare = scalloc(1, sizeof(struct ev_prepare));
 
     ev_io_init(xcb_watcher, restore_xcb_got_event, xcb_get_file_descriptor(restore_conn), EV_READ);
     ev_io_start(main_loop, xcb_watcher);
 
-    ev_check_init(xcb_check, restore_xcb_check_cb);
-    ev_check_start(main_loop, xcb_check);
-
     ev_prepare_init(xcb_prepare, restore_xcb_prepare_cb);
     ev_prepare_start(main_loop, xcb_prepare);
 }

testcases/lib/i3test.pm.in

@@ -133,6 +133,22 @@ sub import {
     my ($class, %args) = @_;
     my $pkg = caller;
 
+    $x ||= i3test::X11->new;
+    # set the pointer to a predictable position in case a previous test has
+    # disturbed it
+    $x->warp_pointer(
+	0, # src_window (None)
+	$x->get_root_window(), # dst_window (None)
+	0, # src_x
+	0, # src_y
+	0, # src_width
+	0, # src_height
+	0, # dst_x
+	0); # dst_y
+    # Synchronize with X11 to ensure the pointer has been warped before i3
+    # starts up.
+    $x->get_input_focus_reply($x->get_input_focus()->{sequence});
+
     $i3_autostart = delete($args{i3_autostart}) // 1;
     my $i3_config = delete($args{i3_config}) // '-default';
 
@@ -155,10 +171,6 @@ __
     strict->import;
     warnings->import;
 
-    $x ||= i3test::X11->new;
-    # set the pointer to a predictable position in case a previous test has
-    # disturbed it
-    $x->root->warp_pointer(0, 0);
     $cv->recv if $i3_autostart;
 
     @_ = ($class);
@@ -181,29 +193,11 @@ received, etc.
 sub wait_for_event {
     my ($timeout, $cb) = @_;
 
-    my $cv = AE::cv;
-
     $x->flush;
 
-    # unfortunately, there is no constant for this
+    while (defined(my $event = $x->wait_for_event)) {
-    my $ae_read = 0;
+	return 1 if $cb->($event);
-
+    }
-    my $guard = AE::io $x->get_file_descriptor, $ae_read, sub {
-        while (defined(my $event = $x->poll_for_event)) {
-            if ($cb->($event)) {
-                $cv->send(1);
-                last;
-            }
-        }
-    };
-
-    # Trigger timeout after $timeout seconds (can be fractional)
-    my $t = AE::timer $timeout, 0, sub { warn "timeout ($timeout secs)"; $cv->send(0) };
-
-    my $result = $cv->recv;
-    undef $t;
-    undef $guard;
-    return $result;
 }
 
 =head2 wait_for_map($window)
@@ -350,6 +344,12 @@ sub open_window {
 
     $window->map;
     wait_for_map($window);
+
+    # MapWindow is sent before i3 even starts rendering: the window is placed at
+    # temporary off-screen coordinates first, and x_push_changes() sends further
+    # X11 requests to set focus etc. Hence, we sync with i3 before continuing.
+    sync_with_i3();
+
     return $window;
 }
 
@@ -688,6 +688,7 @@ sub sync_with_i3 {
         $_sync_window = open_window(
             rect => [ -15, -15, 10, 10 ],
             override_redirect => 1,
+            dont_map => 1,
         );
     }
 

testcases/t/219-ipc-window-focus.t

@@ -44,11 +44,11 @@ sub focus_subtest {
     is($events[0]->{container}->{name}, $name, "$name focused");
 }
 
-subtest 'focus left (1)', \&focus_subtest, 'focus left', 'Window 1';
+subtest 'focus left (1)', \&focus_subtest, 'focus left', $win1->name;
-subtest 'focus left (2)', \&focus_subtest, 'focus left', 'Window 0';
+subtest 'focus left (2)', \&focus_subtest, 'focus left', $win0->name;
-subtest 'focus right (1)', \&focus_subtest, 'focus right', 'Window 1';
+subtest 'focus right (1)', \&focus_subtest, 'focus right', $win1->name;
-subtest 'focus right (2)', \&focus_subtest, 'focus right', 'Window 2';
+subtest 'focus right (2)', \&focus_subtest, 'focus right', $win2->name;
-subtest 'focus right (3)', \&focus_subtest, 'focus right', 'Window 0';
+subtest 'focus right (3)', \&focus_subtest, 'focus right', $win0->name;
-subtest 'focus left', \&focus_subtest, 'focus left', 'Window 2';
+subtest 'focus left', \&focus_subtest, 'focus left', $win2->name;
 
 done_testing;

testcases/t/525-i3bar-mouse-bindings.t

@@ -87,8 +87,6 @@ diag('i3bar window = ' . $i3bar_window);
 my $left = open_window;
 my $right = open_window;
 sync_with_i3;
-my $con = $cv->recv;
-is($con->{window}, $right->{id}, 'focus is initially on the right container');
 
 sub focus_subtest {
     my ($subscribecb, $want, $msg) = @_;