Merge pull request #2134 from Airblader/bug-1761

Properly validate containers when killing via criteria

include/con.h

@@ -30,6 +30,12 @@ Con *con_new(Con *parent, i3Window *window);
  */
 void con_focus(Con *con);
 
+/**
+ * Closes the given container.
+ *
+ */
+void con_close(Con *con, kill_window_t kill_window);
+
 /**
  * Returns true when this node is a leaf node (has no children)
  *
@@ -278,7 +284,7 @@ orientation_t con_orientation(Con *con);
 
 /**
  * Returns the container which will be focused next when the given container
- * is not available anymore. Called in tree_close and con_move_to_workspace
+ * is not available anymore. Called in tree_close_internal and con_move_to_workspace
  * to properly restore focus.
  *
  */

include/data.h

@@ -62,7 +62,7 @@ typedef enum { BS_NORMAL = 0,
                BS_NONE = 1,
                BS_PIXEL = 2 } border_style_t;
 
-/** parameter to specify whether tree_close() and x_window_kill() should kill
+/** parameter to specify whether tree_close_internal() and x_window_kill() should kill
  * only this specific window or the whole X11 client */
 typedef enum { DONT_KILL_WINDOW = 0,
                KILL_WINDOW = 1,

include/tree.h

@@ -56,12 +56,6 @@ bool level_down(void);
  */
 void tree_render(void);
 
-/**
- * Closes the current container using tree_close().
- *
- */
-void tree_close_con(kill_window_t kill_window);
-
 /**
  * Changes focus in the given way (next/previous) and given orientation
  * (horizontal/vertical).
@@ -78,11 +72,11 @@ void tree_next(char way, orientation_t orientation);
  * recursively while deleting a containers children.
  *
  * The force_set_focus flag is specified in the case of killing a floating
- * window: tree_close() will be invoked for the CT_FLOATINGCON (the parent
+ * window: tree_close_internal() will be invoked for the CT_FLOATINGCON (the parent
  * container) and focus should be set there.
  *
  */
-bool tree_close(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool force_set_focus);
+bool tree_close_internal(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool force_set_focus);
 
 /**
  * Loads tree from ~/.i3/_restart.json (used for in-place restarts).

src/commands.c

@@ -1258,7 +1258,6 @@ void cmd_split(I3_CMD, const char *direction) {
 void cmd_kill(I3_CMD, const char *kill_mode_str) {
     if (kill_mode_str == NULL)
         kill_mode_str = "window";
-    owindow *current;
 
     DLOG("kill_mode=%s\n", kill_mode_str);
 
@@ -1273,16 +1272,11 @@ void cmd_kill(I3_CMD, const char *kill_mode_str) {
         return;
     }
 
-    HANDLE_INVALID_MATCH;
+    HANDLE_EMPTY_MATCH;
 
-    /* check if the match is empty, not if the result is empty */
+    owindow *current;
-    if (match_is_empty(current_match))
+    TAILQ_FOREACH(current, &owindows, owindows) {
-        tree_close_con(kill_mode);
+        con_close(current->con, kill_mode);
-    else {
-        TAILQ_FOREACH(current, &owindows, owindows) {
-            DLOG("matching: %p / %s\n", current->con, current->con->name);
-            tree_close(current->con, kill_mode, false, false);
-        }
     }
 
     cmd_output->needs_tree_render = true;

src/con.c

@@ -220,6 +220,36 @@ void con_focus(Con *con) {
     }
 }
 
+/*
+ * Closes the given container.
+ *
+ */
+void con_close(Con *con, kill_window_t kill_window) {
+    assert(con != NULL);
+    DLOG("Closing con = %p.\n", con);
+
+    /* We never close output or root containers. */
+    if (con->type == CT_OUTPUT || con->type == CT_ROOT) {
+        DLOG("con = %p is of type %d, not closing anything.\n", con, con->type);
+        return;
+    }
+
+    if (con->type == CT_WORKSPACE) {
+        DLOG("con = %p is a workspace, closing all children instead.\n", con);
+        Con *child, *nextchild;
+        for (child = TAILQ_FIRST(&(con->focus_head)); child;) {
+            nextchild = TAILQ_NEXT(child, focused);
+            DLOG("killing child = %p.\n", child);
+            tree_close_internal(child, kill_window, false, false);
+            child = nextchild;
+        }
+
+        return;
+    }
+
+    tree_close_internal(con, kill_window, false, false);
+}
+
 /*
  * Returns true when this node is a leaf node (has no children)
  *
@@ -1162,7 +1192,7 @@ orientation_t con_orientation(Con *con) {
 
 /*
  * Returns the container which will be focused next when the given container
- * is not available anymore. Called in tree_close and con_move_to_workspace
+ * is not available anymore. Called in tree_close_internal and con_move_to_workspace
  * to properly restore focus.
  *
  */
@@ -1678,7 +1708,7 @@ static void con_on_remove_child(Con *con) {
         if (TAILQ_EMPTY(&(con->focus_head)) && !workspace_is_visible(con)) {
             LOG("Closing old workspace (%p / %s), it is empty\n", con, con->name);
             yajl_gen gen = ipc_marshal_workspace_event("empty", con, NULL);
-            tree_close(con, DONT_KILL_WINDOW, false, false);
+            tree_close_internal(con, DONT_KILL_WINDOW, false, false);
 
             const unsigned char *payload;
             ylength length;
@@ -1699,7 +1729,7 @@ static void con_on_remove_child(Con *con) {
     int children = con_num_children(con);
     if (children == 0) {
         DLOG("Container empty, closing\n");
-        tree_close(con, DONT_KILL_WINDOW, false, false);
+        tree_close_internal(con, DONT_KILL_WINDOW, false, false);
         return;
     }
 }

src/floating.c

@@ -162,7 +162,7 @@ void floating_enable(Con *con, bool automatic) {
     }
 
     /* 1: detach the container from its parent */
-    /* TODO: refactor this with tree_close() */
+    /* TODO: refactor this with tree_close_internal() */
     TAILQ_REMOVE(&(con->parent->nodes_head), con, nodes);
     TAILQ_REMOVE(&(con->parent->focus_head), con, focused);
 
@@ -180,7 +180,7 @@ void floating_enable(Con *con, bool automatic) {
     nc->layout = L_SPLITH;
     /* We insert nc already, even though its rect is not yet calculated. This
      * is necessary because otherwise the workspace might be empty (and get
-     * closed in tree_close()) even though it’s not. */
+     * closed in tree_close_internal()) even though it’s not. */
     TAILQ_INSERT_TAIL(&(ws->floating_head), nc, floating_windows);
     TAILQ_INSERT_TAIL(&(ws->focus_head), nc, focused);
 
@@ -188,7 +188,7 @@ void floating_enable(Con *con, bool automatic) {
     if ((con->parent->type == CT_CON || con->parent->type == CT_FLOATING_CON) &&
         con_num_children(con->parent) == 0) {
         DLOG("Old container empty after setting this child to floating, closing\n");
-        tree_close(con->parent, DONT_KILL_WINDOW, false, false);
+        tree_close_internal(con->parent, DONT_KILL_WINDOW, false, false);
     }
 
     char *name;
@@ -333,7 +333,7 @@ void floating_disable(Con *con, bool automatic) {
     /* 2: kill parent container */
     TAILQ_REMOVE(&(con->parent->parent->floating_head), con->parent, floating_windows);
     TAILQ_REMOVE(&(con->parent->parent->focus_head), con->parent, focused);
-    tree_close(con->parent, DONT_KILL_WINDOW, true, false);
+    tree_close_internal(con->parent, DONT_KILL_WINDOW, true, false);
 
     /* 3: re-attach to the parent of the currently focused con on the workspace
      * this floating con was on */

src/handlers.c

@@ -503,7 +503,7 @@ static void handle_unmap_notify_event(xcb_unmap_notify_event_t *event) {
         goto ignore_end;
     }
 
-    tree_close(con, DONT_KILL_WINDOW, false, false);
+    tree_close_internal(con, DONT_KILL_WINDOW, false, false);
     tree_render();
 
 ignore_end:
@@ -878,7 +878,7 @@ static void handle_client_message(xcb_client_message_event_t *event) {
             if (event->data.data32[0])
                 last_timestamp = event->data.data32[0];
 
-            tree_close(con, KILL_WINDOW, false, false);
+            tree_close_internal(con, KILL_WINDOW, false, false);
             tree_render();
         } else {
             DLOG("Couldn't find con for _NET_CLOSE_WINDOW request. (window = %d)\n", event->window);

src/randr.c

@@ -741,7 +741,7 @@ void randr_query_outputs(void) {
                     if (current != next && TAILQ_EMPTY(&(current->focus_head))) {
                         /* the workspace is empty and not focused, get rid of it */
                         DLOG("Getting rid of current = %p / %s (empty, unfocused)\n", current, current->name);
-                        tree_close(current, DONT_KILL_WINDOW, false, false);
+                        tree_close_internal(current, DONT_KILL_WINDOW, false, false);
                         continue;
                     }
                     DLOG("Detaching current = %p / %s\n", current, current->name);
@@ -783,7 +783,7 @@ void randr_query_outputs(void) {
                 }
 
                 DLOG("destroying disappearing con %p\n", output->con);
-                tree_close(output->con, DONT_KILL_WINDOW, true, false);
+                tree_close_internal(output->con, DONT_KILL_WINDOW, true, false);
                 DLOG("Done. Should be fine now\n");
                 output->con = NULL;
             }

src/tree.c

@@ -185,11 +185,11 @@ static bool _is_con_mapped(Con *con) {
  * recursively while deleting a containers children.
  *
  * The force_set_focus flag is specified in the case of killing a floating
- * window: tree_close() will be invoked for the CT_FLOATINGCON (the parent
+ * window: tree_close_internal() will be invoked for the CT_FLOATINGCON (the parent
  * container) and focus should be set there.
  *
  */
-bool tree_close(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool force_set_focus) {
+bool tree_close_internal(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool force_set_focus) {
     bool was_mapped = con->mapped;
     Con *parent = con->parent;
 
@@ -219,7 +219,7 @@ bool tree_close(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool
     for (child = TAILQ_FIRST(&(con->nodes_head)); child;) {
         nextchild = TAILQ_NEXT(child, nodes);
         DLOG("killing child=%p\n", child);
-        if (!tree_close(child, kill_window, true, false))
+        if (!tree_close_internal(child, kill_window, true, false))
             abort_kill = true;
         child = nextchild;
     }
@@ -310,7 +310,7 @@ bool tree_close(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool
      * underlying container, see ticket #660.
      *
      * Rendering has to be avoided when dont_kill_parent is set (when
-     * tree_close calls itself recursively) because the tree is in a
+     * tree_close_internal calls itself recursively) because the tree is in a
      * non-renderable state during that time. */
     if (!dont_kill_parent)
         tree_render();
@@ -320,7 +320,7 @@ bool tree_close(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool
 
     if (con_is_floating(con)) {
         DLOG("Container was floating, killing floating container\n");
-        tree_close(parent, DONT_KILL_WINDOW, false, (con == focused));
+        tree_close_internal(parent, DONT_KILL_WINDOW, false, (con == focused));
         DLOG("parent container killed\n");
     }
 
@@ -362,34 +362,6 @@ bool tree_close(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool
     return true;
 }
 
-/*
- * Closes the current container using tree_close().
- *
- */
-void tree_close_con(kill_window_t kill_window) {
-    assert(focused != NULL);
-
-    /* There *should* be no possibility to focus outputs / root container */
-    assert(focused->type != CT_OUTPUT);
-    assert(focused->type != CT_ROOT);
-
-    if (focused->type == CT_WORKSPACE) {
-        DLOG("Workspaces cannot be close, closing all children instead\n");
-        Con *child, *nextchild;
-        for (child = TAILQ_FIRST(&(focused->focus_head)); child;) {
-            nextchild = TAILQ_NEXT(child, focused);
-            DLOG("killing child=%p\n", child);
-            tree_close(child, kill_window, false, false);
-            child = nextchild;
-        }
-
-        return;
-    }
-
-    /* Kill con */
-    tree_close(focused, kill_window, false, false);
-}
-
 /*
  * Splits (horizontally or vertically) the given container by creating a new
  * container which contains the old one and the future ones.
@@ -773,7 +745,7 @@ void tree_flatten(Con *con) {
 
     /* 4: close the redundant cons */
     DLOG("closing redundant cons\n");
-    tree_close(con, DONT_KILL_WINDOW, true, false);
+    tree_close_internal(con, DONT_KILL_WINDOW, true, false);
 
     /* Well, we got to abort the recursion here because we destroyed the
      * container. However, if tree_flatten() is called sufficiently often,

src/workspace.c

@@ -442,7 +442,7 @@ static void _workspace_show(Con *workspace) {
 
     DLOG("old = %p / %s\n", old, (old ? old->name : "(null)"));
     /* Close old workspace if necessary. This must be done *after* doing
-     * urgency handling, because tree_close() will do a con_focus() on the next
+     * urgency handling, because tree_close_internal() will do a con_focus() on the next
      * client, which will clear the urgency flag too early. Also, there is no
      * way for con_focus() to know about when to clear urgency immediately and
      * when to defer it. */
@@ -451,7 +451,7 @@ static void _workspace_show(Con *workspace) {
         if (!workspace_is_visible(old)) {
             LOG("Closing old workspace (%p / %s), it is empty\n", old, old->name);
             yajl_gen gen = ipc_marshal_workspace_event("empty", old, NULL);
-            tree_close(old, DONT_KILL_WINDOW, false, false);
+            tree_close_internal(old, DONT_KILL_WINDOW, false, false);
 
             const unsigned char *payload;
             ylength length;

testcases/t/261-match-con_id-con_mark-combinations.t

@@ -27,9 +27,11 @@ $ws = fresh_workspace;
 open_window(wm_class => 'matchme');
 
 cmd '[con_id=__focused__ class=doesnotmatch] kill';
+sync_with_i3;
 is(@{get_ws($ws)->{nodes}}, 1, 'window was not killed');
 
 cmd '[con_id=__focused__ class=matchme] kill';
+sync_with_i3;
 is(@{get_ws($ws)->{nodes}}, 0, 'window was killed');
 
 ###############################################################################
@@ -41,9 +43,11 @@ open_window(wm_class => 'matchme');
 cmd 'mark marked';
 
 cmd '[con_mark=marked class=doesnotmatch] kill';
+sync_with_i3;
 is(@{get_ws($ws)->{nodes}}, 1, 'window was not killed');
 
 cmd '[con_mark=marked class=matchme] kill';
+sync_with_i3;
 is(@{get_ws($ws)->{nodes}}, 0, 'window was killed');
 
 ###############################################################################