Crash when closing popups

[ehopperdietzel]

Hi, this is the bug we discussed on Reddit. As you can see in the video, it only happens with menus and not with tooltips. In this case, only Gimp crashes, but sometimes xwayland-xdg-shell crashes too.

The problem is that the surface is being destroyed before its role which is a protocol error. The same happens in other compositors like Sway.

debug.log

Btw I am using a scale factor of 2.

[maxhbooth]

Thanks. I just tried this and it didn't reproduce for me (tested with GNOME, though), are you using Sway? If you can get another debug.log with WAYLAND_DEBUG=1 set on xwayland-xdg-shell that would be helpful, so I can see what the actual messages being sent are.

I'm guessing we just need to add some handling to the PopupHandler in xwayland-xdg-shell/client.rs to destroy the role:

fn done(&mut self, _conn: &Connection, _qh: &QueueHandle<Self>, popup: &Popup) {
        popup.xdg_popup().destroy();
}

[ehopperdietzel]

I tested it on Sway and on Louvre's built-in examples. The log clearly indicates that the wl_surface of the popup is being destroyed before the xdg_popup and xdg_surface. While this behavior is not explicitly described as a client error in the protocols, it is considered bad practice. This can lead to problems on the compositor side, such as inefficient cleanup of surfaces and roles (resulting in unnecessary client resources lingering and wasting memory), improper fade-out animations, and so on. That's why some compositors treat it as a client error and simply close the connection with the client. This could be easily fixed by destroying the resources in this order xdg_popup -> xdg_surface -> wl_surface.

If you can get another debug.log with WAYLAND_DEBUG=1 set on xwayland-xdg-shell that would be helpful

Okay, I'll upload the log again. I had tried the built-in xwayland-xdg-shell logging options, but it generated logs larger than 70 MB.

[maxhbooth]

Sorry, the in-built logs are definitely a little verbose :sweat:. Thanks, it does seem like we have a bug there then. I ran WAYLAND_DEBUG=1 WAYLAND_DISPLAY=wayland-0 cargo run --bin xwayland-xdg-shell --profile=release-with-symbols 2> >(grep "surface\|xdg") | tee debug.log (debug.log) and I can see we get:

[ 258112.814][rs] -> wl_surface@46.destroy()
[ 258113.151][rs] -> xdg_popup@44.destroy()
[ 258113.176][rs] -> xdg_surface@43.destroy()

[ehopperdietzel]

Nice, if you decide to change it, let me know and I'll test it again.

[maxhbooth]

thanks :) https://github.com/wayland-transpositor/wprs/pull/51 should fix it

[ehopperdietzel]

Hmm, it seems it's still not working :( I tested it on Louvre, Sway, and Wayfire (bug/destroy-role-before-surface branch).

While this behavior is not explicitly described as a client error in the protocols

I was wrong, it's actually a protocol error. For some reason Gnome and Weston don't care about it.

[maxhbooth]

Did you see the protocol error again, or is there some other bug?

[ehopperdietzel]

Did you see the protocol error again?

Yes, the wl_surface is still being destroyed first. Filtering the logs, it seems you're using subsurfaces for topbar menus. So maybe the same problem needs to be addressed for the wl_subsurface role.

[ehopperdietzel]

I think the type for those kind of windows is NET_WM_WINDOW_TYPE_MENU according to this.

[maxhbooth]

Thanks. I think you're right. I don't actually see any subsurface destroys in the logs.

[maxhbooth]

I think the latest change will fix it; haven't tested it yet, though

[maxhbooth]

alright, looks like subsurfaces are being destroyed now: debug.log

[ehopperdietzel]

Hooray! Now it's working on Sway, Louvre, and Wayfire (and probably any compositor). Also, the GIMP splash screen looks better with the recent changes made on the main branch. I'll continue testing it periodically to catch any potential issues.

[maxhbooth]

Nice! 🥳 Thanks for opening a bug about it :)