Fix toplevel disappearing after window gets unmapped and mapped

[iexavl]

Currently, when a window is unmapped and at some point mapped again, it doesn't reappear as a toplevel.
This happens, because new_toplevel only inserts a toplevel state in the userdata of the window if it doesn't already exist, which is just the first time the window is mapped.
So when it gets unmapped, and its foreign handle gets removed
it doesn't get it's foreign_handle reassigned.

Also, I am not entirely sure about this, but shouldn't the state_inner.instances get drained in remove_toplevel while sending out the closed events? If they aren't and for some reason the ZcosmicToplevelHandleV1 objects there aren't destroyed by the time the surface is mapped again, aren't events are going to be sent for a presumably closed handle?

[Drakulix]

Currently, when a window is unmapped and at some point mapped again, it doesn't reappear as a toplevel. This happens, because new_toplevel only inserts a toplevel state in the userdata of the window if it doesn't already exist, which is just the first time the window is mapped. So when it gets unmapped, and its foreign handle gets removed it doesn't get it's foreign_handle reassigned.

Thanks!

Also, I am not entirely sure about this, but shouldn't the state_inner.instances get drained in remove_toplevel while sending out the closed events? If they aren't and for some reason the ZcosmicToplevelHandleV1 objects there aren't destroyed by the time the surface is mapped again, aren't events are going to be sent for a presumably closed handle?

closed is not a destructor event: https://wayland.app/protocols/ext-foreign-toplevel-list-v1#ext_foreign_toplevel_handle_v1:event:closed

This we can't simply destroy the wayland object. But if we can end up still sending events to these objects, that would be an error on our side.

[Drakulix]

@iexavl Did you test this with a specific client/application, that does unmap/map it's window? Just so @pop-os/quality-assurance knows what to look for, when testing the PR.

[iexavl]

Thanks!

My pleasure, it is a pretty annoying bug.

closed is not a destructor event: https://wayland.app/protocols/ext-foreign-toplevel-list-v1#ext_foreign_toplevel_handle_v1:event:closed

This we can't simply destroy the wayland object. But if we can end up still sending events to these objects, that would be an error on our side.

Right, there was that. I think it is a real possibility though, that if the handles aren't destroyed by the next time the surface is mapped, events are sent. I haven't found any evidence that would prevent this, but I also haven't tested it. At some point I will and see what happens.

[iexavl]

@iexavl Did you test this with a specific client/application, that does unmap/map it's window? Just so @pop-os/quality-assurance knows what to look for, when testing the PR.

It happens in warframe, though any application that unmaps itself and maps itself again will do the job.
To get it to happen in warframe, just minimize the window (in fullscreen) and unminimize it.

Edit: note that while running warframe, you may also experience this unreachable code crash. I also discovered the cause of this and will probably open a pr for that too.