Fix crash when unmapping a view with reapable parents

container_destroy was calling container_reap_empty, which calls
container_destroy and so on. Eventually the original container_destroy
would return a NULL pointer to the caller which caused a crash.

This also fixes an arrange on the wrong container when moving views in
and out of stacks.

2 files changed, 58 insertions, 38 deletions

diff --git a/sway/commands/move.c b/sway/commands/move.c
index 2c9fb77a..da0f89e9 100644
--- a/sway/commands/move.c
+++ b/sway/commands/move.c
@@ -177,13 +177,19 @@ static struct cmd_results *cmd_move_workspace(struct sway_container *current,
 
 static void move_in_direction(struct sway_container *container,
                 enum wlr_direction direction, int move_amt) {
-        struct sway_container *old_parent = container->parent;
+        // For simplicity, we'll arrange the entire workspace. The reason for this
+        // is moving the container might reap the old parent, and container_move
+        // does not return a surviving parent.
+        // TODO: Make container_move return the surviving parent so we can arrange
+        // just that.
+        struct sway_container *old_ws = container_parent(container, C_WORKSPACE);
         container_move(container, direction, move_amt);
+        struct sway_container *new_ws = container_parent(container, C_WORKSPACE);
 
         struct sway_transaction *txn = transaction_create();
-        arrange_windows(old_parent, txn);
-        if (container->parent != old_parent) {
-                arrange_windows(container->parent, txn);
+        arrange_windows(old_ws, txn);
+        if (new_ws != old_ws) {
+                arrange_windows(new_ws, txn);
         }
         transaction_commit(txn);
 }
diff --git a/sway/tree/container.c b/sway/tree/container.c
index 484d26a5..075c508c 100644
--- a/sway/tree/container.c
+++ b/sway/tree/container.c
@@ -293,6 +293,47 @@ static struct sway_container *container_output_destroy(
         return &root_container;
 }
 
+/**
+ * Implement the actual destroy logic, without reaping.
+ */
+struct sway_container *container_destroy_noreaping(struct sway_container *con) {
+        if (con == NULL) {
+                return NULL;
+        }
+        if (con->destroying) {
+                return NULL;
+        }
+
+        // The below functions move their children to somewhere else.
+        if (con->type == C_OUTPUT) {
+                container_output_destroy(con);
+        } else if (con->type == C_WORKSPACE) {
+                // Workspaces will refuse to be destroyed if they're the last workspace
+                // on their output.
+                if (!container_workspace_destroy(con)) {
+                        wlr_log(L_ERROR, "workspace doesn't want to destroy");
+                        return NULL;
+                }
+        }
+
+        // At this point the container being destroyed shouldn't have any children
+        // unless sway is terminating.
+        if (!server.terminating) {
+                if (!sway_assert(!con->children || con->children->length == 0,
+                                        "Didn't expect to see children here")) {
+                        return NULL;
+                }
+        }
+
+        wl_signal_emit(&con->events.destroy, con);
+        ipc_event_window(con, "close");
+
+        con->destroying = true;
+        list_add(server.destroying_containers, con);
+
+        return container_remove_child(con);
+}
+
 bool container_reap_empty(struct sway_container *con) {
         if (con->layout == L_FLOATING) {
                 // Don't reap the magical floating container that each workspace has
@@ -306,13 +347,13 @@ bool container_reap_empty(struct sway_container *con) {
         case C_WORKSPACE:
                 if (!workspace_is_visible(con) && workspace_is_empty(con)) {
                         wlr_log(L_DEBUG, "Destroying workspace via reaper");
-                        container_destroy(con);
+                        container_destroy_noreaping(con);
                         return true;
                 }
                 break;
         case C_CONTAINER:
                 if (con->children->length == 0) {
-                        container_destroy(con);
+                        container_destroy_noreaping(con);
                         return true;
                 }
         case C_VIEW:
@@ -354,43 +395,16 @@ struct sway_container *container_flatten(struct sway_container *container) {
  * events, detach it from the tree and mark it as destroying. The container will
  * remain in memory until it's no longer used by a transaction, then it will be
  * freed via container_free().
+ *
+ * This function just wraps container_destroy_noreaping(), then does reaping.
  */
 struct sway_container *container_destroy(struct sway_container *con) {
-        if (con == NULL) {
-                return NULL;
-        }
-        if (con->destroying) {
-                return NULL;
-        }
-
-        // The below functions move their children to somewhere else.
-        if (con->type == C_OUTPUT) {
-                container_output_destroy(con);
-        } else if (con->type == C_WORKSPACE) {
-                // Workspaces will refuse to be destroyed if they're the last workspace
-                // on their output.
-                if (!container_workspace_destroy(con)) {
-                        return NULL;
-                }
-        }
+        struct sway_container *parent = container_destroy_noreaping(con);
 
-        // At this point the container being destroyed shouldn't have any children
-        // unless sway is terminating.
-        if (!server.terminating) {
-                if (!sway_assert(!con->children || con->children->length == 0,
-                                        "Didn't expect to see children here")) {
-                        return NULL;
-                }
+        if (!parent) {
+                return NULL;
         }
 
-        wl_signal_emit(&con->events.destroy, con);
-        ipc_event_window(con, "close");
-
-        struct sway_container *parent = container_remove_child(con);
-
-        con->destroying = true;
-        list_add(server.destroying_containers, con);
-
         return container_reap_empty_recursive(parent);
 }