Guard seat_get_focus() NULL dereferences

[hack-heart]

seat_get_focus() can return NULL when the focus stack is empty. A few call sites don't check for this and just dereference the result, which crashes scroll.

I hit this for two main reasons: linux-wallpaperengine crashing (according to logs), and restarting noctalia shell (necessary after an update). For the linux-wallpaperengine crashes, each crash is an abrupt client disconnect, which leaves the seat with a stale has_focus or empty focus stack. Then whatever touches focus next dereferences NULL and crashes scroll.

I added NULL checks in seat_set_workspace_focus, seat_set_focus_surface, seat_unfocus_unless_client, and view_unmap. The first two caused crashes (coredumps pasted below), the other two sites, I haven't gotten a crash from yet but added checks to be safe.

Coredumps:

I cannot remember how i triggered this one:

#0  seat_send_unfocus ()
#1  seat_set_workspace_focus ()
#2  seat_set_focus ()
#3  wl_signal_emit_mutable ()
#4  container_begin_destroy ()
#5  view_unmap ()
#6  handle_unmap ()
#7  wl_signal_emit_mutable ()
#8  wlr_surface_unmap ()
#9  destroy_xdg_toplevel ()
#10 destroy_xdg_surface_role_object ()
#11 destroy_xdg_surface ()
#12 xdg_client_handle_resource_destroy ()
#13 wl_client_destroy ()

Mouse click on empty container (clicked wallpaper, likely was just after a wallpaper engine crash or something):

#0  seat_send_unfocus ()
#1  seat_set_workspace_focus ()
#2  seat_set_focus ()
#3  handle_button ()
#4  wl_signal_emit_mutable ()
#5  handle_pointer_button ()
#6  handle_libinput_readable ()
#7  wl_event_loop_dispatch ()

IPC workspace switch (with noctalia workspace widget):

#0  seat_send_unfocus ()
#1  seat_set_workspace_focus ()
#2  seat_set_focus ()
#3  workspace_switch ()
#4  cmd_workspace ()
#5  execute_command ()
#6  ipc_client_handle_command ()
#7  ipc_client_handle_readable ()
#8  wl_event_loop_dispatch ()

Layer surface teardown (restarted noctalia shell):

#0  seat_send_unfocus ()
#1  seat_set_focus_surface ()
#2  seat_set_focus_layer ()
#3  handle_node_destroy ()
#4  wl_signal_emit_mutable ()
#5  sway_scene_node_destroy ()
#6  layer_surface_destroy ()
#7  surface_handle_role_resource_destroy ()

[dawsers]

Thank you for taking the time to do this. I added one comment about content, and another about how to go about this, in the sense that the first three cases are in "pure" sway's code, so you may have to decide whether you want the code to be scroll-specific, or go upstream and submit a PR to sway. I am good either way, it is completely your choice.

[hack-heart]

thanks for taking a look! where can I see the comments?

[dawsers]

thanks for taking a look! where can I see the comments?

Don't you see them here and in the Files Changed tab?

It is just one comment, about enclosing the if {} else if {}, and the rest is good.

Edit: Sorry, I had forgotten to submit

[dawsers]

Here, I would just enclose both into an outer if (node) to make clear both are only valid when node != NULL. Something like:

if (node) {
	if (node->type == N_WORKSPACE) {
		output_layer_shell_enable(node->sway_workspace->output, LAYER_SHELL_ALL);
	} else if (node->type == N_CONTAINER) {
		struct sway_container *con = node->sway_container;
		if (con->fullscreen || (fullscreen && config->fullscreen_movefocus == FULLSCREEN_MOVEFOCUS_FOLLOW)) {
			container_set_fullscreen(con, FULLSCREEN_WORKSPACE);
			container_set_fullscreen_container(con, FULLSCREEN_ENABLED);
			arrange_root();
		}
	}
}

I think it is OK to add this check too, even though I cannot think of a case where a view is unmapped and scroll left without a focused surface or workspace, but it is OK to add, it doesn't hurt.

[dawsers]

This is all good, thank you. However, you may have to decide whether to merge this into scroll or go upstream to sway, because these are all cases from sway's code. I mean, your use case would also make sway crash. I will merge it here if that is your preference, I understand a PR in sway may take a lot longer to process, but if you are also a sway user, or would rather submit a PR there, I can also merge it later on when it gets accepted, which I cannot think of a reason why it wouldn't.