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);
 }