From b864ac0149212adf753824366e20badfa971b29f Mon Sep 17 00:00:00 2001 From: Ryan Dwyer Date: Sun, 24 Jun 2018 15:50:53 +1000 Subject: [PATCH] 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. --- sway/commands/move.c | 14 +++++--- sway/tree/container.c | 82 +++++++++++++++++++++++++------------------ 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); }