make sure old input-method-popup is destroyed or they can accumulate quite a lot.

[lilydjwg]

Some days ago I asked in Matrix that my IPC call to list_views didn't work unless I straced wayfire. That was because the list was very long: hundreds of "input-method-popup" views!

This patch makes sure that there is only one "input-method-popup". I'm not very familiar with the code so I asked codex (a coding AI agent) to help. It works for me for days.

One possible side-effect is that when I restart fcitx5 (the input method), wayfire no longer crashes. (It used to crash after wayfire had been running for e.g. days; it seemed to be random memory corruption but asan didn't catch it.)

[lilydjwg]

Style check fails with:

-wayfire_input_method_v1_panel_surface* wayfire_input_method_v1_panel_surface::current_panel = nullptr;
+wayfire_input_method_v1_panel_surface*wayfire_input_method_v1_panel_surface::current_panel = nullptr;

Is this really wanted?

[soreau]

Style check fails with:

-wayfire_input_method_v1_panel_surface* wayfire_input_method_v1_panel_surface::current_panel = nullptr;
+wayfire_input_method_v1_panel_surface*wayfire_input_method_v1_panel_surface::current_panel = nullptr;

Is this really wanted?

You might try

wayfire_input_method_v1_panel_surface
    *wayfire_input_method_v1_panel_surface::current_panel = nullptr;

but I'd just do what it wants ultimately.

[ammen99]

Some days ago I asked in Matrix that my IPC call to list_views didn't work unless I straced wayfire. That was because the list was very long: hundreds of "input-method-popup" views!

This patch makes sure that there is only one "input-method-popup". I'm not very familiar with the code so I asked codex (a coding AI agent) to help. It works for me for days.

One possible side-effect is that when I restart fcitx5 (the input method), wayfire no longer crashes. (It used to crash after wayfire had been running for e.g. days; it seemed to be random memory corruption but asan didn't catch it.)

Why aren't old popups being destroyed?

[lilydjwg]

Because nothing destroys it. There is no destroy method for zwp_input_panel_surface_v1, so in handle_input_panel_get_input_panel_surface we create one every time the input method request one but never delete it.

[ammen99]

Because nothing destroys it. There is no destroy method for zwp_input_panel_surface_v1, so in handle_input_panel_get_input_panel_surface we create one every time the input method request one but never delete it.

Ah, the panel surface is tied to a wl_surface. Is it possible that the wl_surface is destroyed, but we don't clean up our own state?

[lilydjwg]

Yes, when the wl_surface is destroyed, the text_input_v3_popup is left behind. It doesn't refer to wayfire_input_method_v1_panel_surface; the wayfire_input_method_v1_panel_surface refer to it to keep it alive instead.

I think this is a good way to handle the problem, but text_input_v3_popup is shared in both input method v1 and v2. So that part needs to be touched too.

Do you want me to do that instead?

[ammen99]

Yes, when the wl_surface is destroyed, the text_input_v3_popup is left behind. It doesn't refer to wayfire_input_method_v1_panel_surface; the wayfire_input_method_v1_panel_surface refer to it to keep it alive instead.

I think this is a good way to handle the problem, but text_input_v3_popup is shared in both input method v1 and v2. So that part needs to be touched too.

Do you want me to do that instead?

Is the bug present in v2 too? I expected wlroots to do some bookkeeping for us there - if my hunch is correct, we only need to add this to the v1 protocol. I definitely think that listening for the surface destroy event is a better way to fix the issue than the current proposal ;)

[lilydjwg]

Is the bug present in v2 too? I expected wlroots to do some bookkeeping for us there - if my hunch is correct, we only need to add this to the v1 protocol. I definitely think that listening for the surface destroy event is a better way to fix the issue than the current proposal ;)

I don't think v2 needs it. I can't test it though, because it is broken: the input method can't be activated.

I have pushed a new version.

[ammen99]

Hey, I am wondering, why does the input-v3-popup need to know anything about the wl_resource? Can't we add an extra wl_listener in the im-v1 implementation which destroys the resource? This way, we don't need to touch the popup implementation at all.

[lilydjwg]

I'm not familiar with the signal system and not sure how to do it. I tried, and it crashed with corrupted memory.

[ammen99]

I'm not familiar with the signal system and not sure how to do it. I tried, and it crashed with corrupted memory.

The basic operation is like this:

• You define the struct/class which is the signal type, it needs to be unique for each signal.
• The signal can be emitted on an object, all listeners connected to the signal type on that object will get the event.
• On the signal connection side, you connect to the signals on the object(s) you want to receive the signal type from.
• There is automatic disconnection, so you can destroy the objects on which signals are emitted, as well as the signal connections, without needing manual disconnect calls.

That being said, if you share your attempt at using signals, I could maybe try to find the bug. A frequent source of crashes in my experience has been capturing by reference & in a lambda when I need to capture by value =.

[ammen99]

@lilydjwg would you mind sharing the backtrace as well?

[lilydjwg]

The backtrace before this patch wayfire crashed when fcitx5 restarted? I didn't save it anywhere, and they are no longer available.

[ammen99]

The backtrace before this patch wayfire crashed when fcitx5 restarted? I didn't save it anywhere, and they are no longer available.

You said that with the attempt to use wl_listener_wrapper, it crashes - which is why I was hoping you have the backtrace somewhere.

wl_listener_wrapper is the wayland signal wrappers, not Wayfire's own wf::signal::connection_t signal mechanism, it requires manually disconnecting all wl_listener_wrapper signals when the object is destroyed (for example, see

wayfire/src/view/xdg-shell/xdg-toplevel-view.cpp

Lines 91 to 97 in f7f5461

	void wf::xdg_toplevel_view_base_t::destroy()
	{
	on_destroy.disconnect();
	on_new_popup.disconnect();
	on_set_title.disconnect();
	on_set_app_id.disconnect();
	on_ping_timeout.disconnect();

). This may or may not be the issue here.

Also by the way, I just realized we already have the signal you want to listen to, see https://github.com/lilydjwg/wayfire/blob/6bbd068499c93f662095e85da75659c61818b10f/plugins/protocols/input-method-v1.cpp#L435-L445

Maybe it would be enough to just add the resource destroy at the end there?

[lilydjwg]

You said that with the attempt to use wl_listener_wrapper, it crashes - which is why I was hoping you have the backtrace somewhere.

Oh this one. I tried to disconnect the signal but the object had just been destroyed. I used asan to figure it out.

Maybe it would be enough to just add the resource destroy at the end there?

Yes, I didn't notice that this is the same surface.

[ammen99]

@lilydjwg The current version looks good, we can merge once you say everything works as it should.

[lilydjwg]

Yes, things are working fine for me.

[ammen99]

Alright, thanks for working on this!