diff options
author | Ryan Dwyer <ryandwyer1@gmail.com> | 2018-06-24 15:50:53 +1000 |
---|---|---|
committer | Ryan Dwyer <ryandwyer1@gmail.com> | 2018-06-24 15:50:53 +1000 |
commit | b864ac0149212adf753824366e20badfa971b29f (patch) | |
tree | 87f9a4117a84d3552c25a2434094d221c7d6bad0 | |
parent | Fix crash related to stacks and tabs (diff) | |
download | sway-b864ac0149212adf753824366e20badfa971b29f.tar.gz sway-b864ac0149212adf753824366e20badfa971b29f.tar.zst sway-b864ac0149212adf753824366e20badfa971b29f.zip |
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.
-rw-r--r-- | sway/commands/move.c | 14 | ||||
-rw-r--r-- | 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, | |||
177 | 177 | ||
178 | static void move_in_direction(struct sway_container *container, | 178 | static void move_in_direction(struct sway_container *container, |
179 | enum wlr_direction direction, int move_amt) { | 179 | enum wlr_direction direction, int move_amt) { |
180 | struct sway_container *old_parent = container->parent; | 180 | // For simplicity, we'll arrange the entire workspace. The reason for this |
181 | // is moving the container might reap the old parent, and container_move | ||
182 | // does not return a surviving parent. | ||
183 | // TODO: Make container_move return the surviving parent so we can arrange | ||
184 | // just that. | ||
185 | struct sway_container *old_ws = container_parent(container, C_WORKSPACE); | ||
181 | container_move(container, direction, move_amt); | 186 | container_move(container, direction, move_amt); |
187 | struct sway_container *new_ws = container_parent(container, C_WORKSPACE); | ||
182 | 188 | ||
183 | struct sway_transaction *txn = transaction_create(); | 189 | struct sway_transaction *txn = transaction_create(); |
184 | arrange_windows(old_parent, txn); | 190 | arrange_windows(old_ws, txn); |
185 | if (container->parent != old_parent) { | 191 | if (new_ws != old_ws) { |
186 | arrange_windows(container->parent, txn); | 192 | arrange_windows(new_ws, txn); |
187 | } | 193 | } |
188 | transaction_commit(txn); | 194 | transaction_commit(txn); |
189 | } | 195 | } |
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( | |||
293 | return &root_container; | 293 | return &root_container; |
294 | } | 294 | } |
295 | 295 | ||
296 | /** | ||
297 | * Implement the actual destroy logic, without reaping. | ||
298 | */ | ||
299 | struct sway_container *container_destroy_noreaping(struct sway_container *con) { | ||
300 | if (con == NULL) { | ||
301 | return NULL; | ||
302 | } | ||
303 | if (con->destroying) { | ||
304 | return NULL; | ||
305 | } | ||
306 | |||
307 | // The below functions move their children to somewhere else. | ||
308 | if (con->type == C_OUTPUT) { | ||
309 | container_output_destroy(con); | ||
310 | } else if (con->type == C_WORKSPACE) { | ||
311 | // Workspaces will refuse to be destroyed if they're the last workspace | ||
312 | // on their output. | ||
313 | if (!container_workspace_destroy(con)) { | ||
314 | wlr_log(L_ERROR, "workspace doesn't want to destroy"); | ||
315 | return NULL; | ||
316 | } | ||
317 | } | ||
318 | |||
319 | // At this point the container being destroyed shouldn't have any children | ||
320 | // unless sway is terminating. | ||
321 | if (!server.terminating) { | ||
322 | if (!sway_assert(!con->children || con->children->length == 0, | ||
323 | "Didn't expect to see children here")) { | ||
324 | return NULL; | ||
325 | } | ||
326 | } | ||
327 | |||
328 | wl_signal_emit(&con->events.destroy, con); | ||
329 | ipc_event_window(con, "close"); | ||
330 | |||
331 | con->destroying = true; | ||
332 | list_add(server.destroying_containers, con); | ||
333 | |||
334 | return container_remove_child(con); | ||
335 | } | ||
336 | |||
296 | bool container_reap_empty(struct sway_container *con) { | 337 | bool container_reap_empty(struct sway_container *con) { |
297 | if (con->layout == L_FLOATING) { | 338 | if (con->layout == L_FLOATING) { |
298 | // Don't reap the magical floating container that each workspace has | 339 | // Don't reap the magical floating container that each workspace has |
@@ -306,13 +347,13 @@ bool container_reap_empty(struct sway_container *con) { | |||
306 | case C_WORKSPACE: | 347 | case C_WORKSPACE: |
307 | if (!workspace_is_visible(con) && workspace_is_empty(con)) { | 348 | if (!workspace_is_visible(con) && workspace_is_empty(con)) { |
308 | wlr_log(L_DEBUG, "Destroying workspace via reaper"); | 349 | wlr_log(L_DEBUG, "Destroying workspace via reaper"); |
309 | container_destroy(con); | 350 | container_destroy_noreaping(con); |
310 | return true; | 351 | return true; |
311 | } | 352 | } |
312 | break; | 353 | break; |
313 | case C_CONTAINER: | 354 | case C_CONTAINER: |
314 | if (con->children->length == 0) { | 355 | if (con->children->length == 0) { |
315 | container_destroy(con); | 356 | container_destroy_noreaping(con); |
316 | return true; | 357 | return true; |
317 | } | 358 | } |
318 | case C_VIEW: | 359 | case C_VIEW: |
@@ -354,43 +395,16 @@ struct sway_container *container_flatten(struct sway_container *container) { | |||
354 | * events, detach it from the tree and mark it as destroying. The container will | 395 | * events, detach it from the tree and mark it as destroying. The container will |
355 | * remain in memory until it's no longer used by a transaction, then it will be | 396 | * remain in memory until it's no longer used by a transaction, then it will be |
356 | * freed via container_free(). | 397 | * freed via container_free(). |
398 | * | ||
399 | * This function just wraps container_destroy_noreaping(), then does reaping. | ||
357 | */ | 400 | */ |
358 | struct sway_container *container_destroy(struct sway_container *con) { | 401 | struct sway_container *container_destroy(struct sway_container *con) { |
359 | if (con == NULL) { | 402 | struct sway_container *parent = container_destroy_noreaping(con); |
360 | return NULL; | ||
361 | } | ||
362 | if (con->destroying) { | ||
363 | return NULL; | ||
364 | } | ||
365 | |||
366 | // The below functions move their children to somewhere else. | ||
367 | if (con->type == C_OUTPUT) { | ||
368 | container_output_destroy(con); | ||
369 | } else if (con->type == C_WORKSPACE) { | ||
370 | // Workspaces will refuse to be destroyed if they're the last workspace | ||
371 | // on their output. | ||
372 | if (!container_workspace_destroy(con)) { | ||
373 | return NULL; | ||
374 | } | ||
375 | } | ||
376 | 403 | ||
377 | // At this point the container being destroyed shouldn't have any children | 404 | if (!parent) { |
378 | // unless sway is terminating. | 405 | return NULL; |
379 | if (!server.terminating) { | ||
380 | if (!sway_assert(!con->children || con->children->length == 0, | ||
381 | "Didn't expect to see children here")) { | ||
382 | return NULL; | ||
383 | } | ||
384 | } | 406 | } |
385 | 407 | ||
386 | wl_signal_emit(&con->events.destroy, con); | ||
387 | ipc_event_window(con, "close"); | ||
388 | |||
389 | struct sway_container *parent = container_remove_child(con); | ||
390 | |||
391 | con->destroying = true; | ||
392 | list_add(server.destroying_containers, con); | ||
393 | |||
394 | return container_reap_empty_recursive(parent); | 408 | return container_reap_empty_recursive(parent); |
395 | } | 409 | } |
396 | 410 | ||