xdg-surface: handle inert toplevels and popups in when committing

[ammen99]

This is an attempt to Fixes #2432
@dkondor please test

[dkondor]

Just tested this (this branch applied on top of latest wlroots master + wayfire master).

The crash after dismissing the dialog is gone, but there is a new crash when closing a toplevel that used to have a dialog.

This happens in my simplified example: https://github.com/dkondor/wlroots/blob/issue-2432/issue-2432/wlr-crasher-2.c

To reproduce, close the blue dialog either by clicking on it or by activating another app. After that, when the main toplevel is closed (clicking on it or even Ctrl-C from terminal), wayfire crashes.

I cannot test my original issues with cairo-dock, since trying to start it already crashes wayfire with this patch.

[dkondor]

Stackrace:

EE 28-10-20 11:56:08.144 - [src/main.cpp:233] Fatal error: Segmentation fault
EE 28-10-20 11:56:08.183 - #1  signal_handler(int) ../src/main.cpp:234
EE 28-10-20 11:56:08.361 - #2  killpg ??:?
EE 28-10-20 11:56:08.370 - #3  wl_list_remove /home/dkondor/program/wayland/src/wayland-util.c:55
EE 28-10-20 11:56:08.383 - #4  reset_xdg_surface ../types/xdg_shell/wlr_xdg_surface.c:513
EE 28-10-20 11:56:08.397 - #5  destroy_xdg_surface ../types/xdg_shell/wlr_xdg_surface.c:517
EE 28-10-20 11:56:08.410 - #6  xdg_surface_handle_resource_destroy ../types/xdg_shell/wlr_xdg_surface.c:325
EE 28-10-20 11:56:08.418 - #7  destroy_resource /home/dkondor/program/wayland/src/wayland-server.c:726
EE 28-10-20 11:56:08.429 - #8  wl_resource_destroy /home/dkondor/program/wayland/src/wayland-server.c:743
EE 28-10-20 11:56:08.441 - #9  xdg_surface_handle_destroy ../types/xdg_shell/wlr_xdg_surface.c:309
EE 28-10-20 11:56:08.449 - #10 ffi_call_unix64 ??:?
EE 28-10-20 11:56:08.459 - #11 ffi_call ??:?
EE 28-10-20 11:56:08.467 - #12 wl_closure_invoke /home/dkondor/program/wayland/src/connection.c:1020
EE 28-10-20 11:56:08.476 - #13 wl_client_connection_data /home/dkondor/program/wayland/src/wayland-server.c:432
EE 28-10-20 11:56:08.486 - #14 wl_event_loop_dispatch /home/dkondor/program/wayland/src/event-loop.c:1027
EE 28-10-20 11:56:08.496 - #15 wl_display_run /home/dkondor/program/wayland/src/wayland-server.c:1349
EE 28-10-20 11:56:08.527 - #16 main ../src/main.cpp:394
EE 28-10-20 11:56:08.662 - #17 __libc_start_main ../csu/libc-start.c:344
EE 28-10-20 11:56:08.703 - #18 ?? ??:0

[ammen99]

@dkondor I think I saw the problem, reset_xdg_surface might be called multiple times for the same toplevel/popup. I have fixed this issue, please try again.

[dkondor]

Yes, it's working now; also, my original issue with cairo-dock is resolved as well. Thanks!

[travankor]

Should it send a protocol error, too? See: #2057

[ammen99]

Should it send a protocol error, too? See: #2057

No, it shouldn't. This is simply a race condition which neither wlroots nor the client can prevent, and necessitates an inert state of objects on the wlroots side.

@travankor does this also fix your issue with chromium on Wayland?

[travankor]

does this also fix your issue with chromium on Wayland?

I can't test that anymore since the bug only worked on v80 and previous releases. And I cleaned my package cache in September, unfortunately, because of disk space running out.

[ammen99]

I did not know that you can destroy and re-create the xdg_toplevel without doing the same for xdg_surface. Unfortunately, this means we just need to remove one of the errors for clients, let me know if you find this acceptable.

[emersion]

Can we add an explanation of why this is necessary to the commit message? Otherwise I won't remember. Something among those lines:

xdg_popups can be destroyed by the compositor when closed. When this happens,
wlroots makes the xdg_popup surface inert and resets the xdg_surface role to
NONE.

Currently, wlroots sends a protocol error and asserts that an xdg_surface has
a role when committed. This is racy if at the same time the client commits an
xdg_popup and the compositor closes it. This patch removes the assertion and
ignores commits on xdg_surfaces without a role set.

[ammen99]

I updated the patch as you suggested. Thanks for the in-depth review, I know so little about the xdg-shell implementation.

[emersion]

Thanks for the in-depth review, I know so little about the xdg-shell implementation.

No problem, the xdg-shell implementation isn't the most straightforward part of wlroots sadly.

[emersion]

Forgot to push?

[emersion]

Thanks for fixing this!