MenuButton doesn't remove buttons properly if I force it it crashes

[MarkoGrbec]

Tested versions

Reproducible 4.4 dev2

System information

Linux manjaro

Issue description

either I don't understand sub Menus or it doesn't deallocate the sub menus

I expected to remove all submenus with clear()

Steps to reproduce

1. click on 1 shape it works
2. click on 2. shape (first it uses sub menu of the 1. shape)
3. if you uncomment queue_free() it crashes on the 2. step

quick sample of code:

class_name PopUp extends MenuButton

func _ready() -> void:
	g_man.pop_up = self

var dict_name__sub_menus = {}

## multi_array [0] name of button [1] name of sub_menu [id] is number of sub menu
func pop_up(position, multi_array, call: Callable):
	var main_menu = get_popup()
	var sub_menus = dict_name__sub_menus.values()
	if sub_menus:
		for sub in sub_menus:
			for item in sub.id_pressed.get_connections():
				sub.id_pressed.disconnect(item.callable)
	
	## TODO: uncomment and it'll crash after clicking 2 times
	#for subs in dict_name__sub_menus.values():
		#subs.queue_free()
	
	dict_name__sub_menus.clear()
	main_menu.clear()
	if multi_array:
		var id = -1
		for item in multi_array:
			var sub_menu = dict_name__sub_menus.get(item[1])
			if not sub_menu:
				id += 1
				sub_menu = PopupMenu.new()
				sub_menu.name = item[1]
				dict_name__sub_menus[item[1]] = sub_menu
				main_menu.add_child(sub_menu)
				main_menu.add_submenu_item(item[1], item[1])
				sub_menu.id_pressed.connect(
					func(index_submenu):
						call.call(id, index_submenu)
				)
			sub_menu.add_item(item[0])
	set_position(position)
	show_popup()

Minimal reproduction project (MRP)

MRP mrp-rpc.zip

file was too large even on minimal.

MRP without .godot

[AThousandShips]

file was too large even on minimal.

What are you including? This shouldn't be possible if all it contains is some code, did you exclude the .godot folder?

[MarkoGrbec]

Uhm no.

Wasnt that .godot has to be included?

[AThousandShips]

No it says:

Be sure to not include the .godot folder in the archive (but keep project.godot).

[MarkoGrbec]

ok I excluded only .godot this time now it's 4.2k :)

I've misread "not" include darn and too fast reading...

[MarkoGrbec]

ok looking deep in to documentation I've fixed my bug with:

main_menu.clear(true)

added "true" in clear

it also doesn't crash this time

but the crash still exists if there's sub menus that aren't deallocated and that I want to deallocate them with force (queue_free())

[AThousandShips]

Can confirm with the following crash trace:

================================================================
CrashHandlerException: Program crashed
Engine version: Godot Engine v4.4.dev.custom_build (f606c73fa8c65326149b9a54a5dd62d6b93cdb5d)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] WorkerThreadPool::`RTTI Complete Object Locator'
[1] Window::get_contents_minimum_size (godot\scene\main\window.cpp:1958)
[2] Window::get_clamped_minimum_size (godot\scene\main\window.cpp:1967)
[3] Window::_update_window_size (godot\scene\main\window.cpp:1042)
[4] Window::set_size (godot\scene\main\window.cpp:393)
[5] Window::reset_size (godot\scene\main\window.cpp:404)
[6] PopupMenu::_activate_submenu (godot\scene\gui\popup_menu.cpp:348)
[7] PopupMenu::_input_from_window_internal (godot\scene\gui\popup_menu.cpp:630)
[8] PopupMenu::_input_from_window (godot\scene\gui\popup_menu.cpp:448)
[9] Window::_window_input (godot\scene\main\window.cpp:1675)
[10] Viewport::_sub_windows_forward_input (godot\scene\main\viewport.cpp:2898)
[11] Viewport::push_input (godot\scene\main\viewport.cpp:3159)
[12] Window::_window_input (godot\scene\main\window.cpp:1682)
[13] call_with_variant_args_helper<Window,Ref<InputEvent> const &,0> (godot\core\variant\binder_common.h:304)
[14] call_with_variant_args<Window,Ref<InputEvent> const &> (godot\core\variant\binder_common.h:419)
[15] CallableCustomMethodPointer<Window,Ref<InputEvent> const &>::call (godot\core\object\callable_method_pointer.h:104)
[16] Callable::callp (godot\core\variant\callable.cpp:58)
[17] Callable::call<Ref<InputEvent> > (godot\core\variant\variant.h:875)
[18] DisplayServerWindows::_dispatch_input_event (godot\platform\windows\display_server_windows.cpp:3733)
[19] DisplayServerWindows::_dispatch_input_events (godot\platform\windows\display_server_windows.cpp:3704)
[20] Input::_parse_input_event_impl (godot\core\input\input.cpp:803)
[21] Input::flush_buffered_events (godot\core\input\input.cpp:1084)
[22] DisplayServerWindows::process_events (godot\platform\windows\display_server_windows.cpp:3186)
[23] OS_Windows::run (godot\platform\windows\os_windows.cpp:1772)
[24] widechar_main (godot\platform\windows\godot_windows.cpp:181)
[25] _main (godot\platform\windows\godot_windows.cpp:206)
[26] main (godot\platform\windows\godot_windows.cpp:220)
[27] WinMain (godot\platform\windows\godot_windows.cpp:234)
[28] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[29] <couldn't map PC to fn name>
-- END OF BACKTRACE --
================================================================

But not sure what is exactly going wrong here or if the use is valid, but we should avoid crashing if possible

[WhalesState]

• I feel like it's related to https://github.com/godotengine/godot/issues/97200

Note: it doesn't crash on 4.3.stable release.

Using await get_tree().process_frame also fixes the issue.

	## TODO: uncomment and it'll crash after clicking 2 times
	for subs in dict_name__sub_menus.values():
		subs.queue_free()
	await get_tree().process_frame

[matheusmdx]

But not sure what is exactly going wrong here or if the use is valid

Probably what's happening here is like delete an internal node, i took a look in PopupMenu code and saw the code checks for nullptr before do some actions but the issue code uses subs.queue_free() so the node becomes invalid while PopupMenu still processing them. Using subs.free(), removing the node from tree after subs.queue_free() or awaiting a process frame (like Whales pointed) solves the problem.

but we should avoid crashing if possible

IMO we should just put a warning like the warning in ScrollContainer.get_h_scrollbar in the docs of PopupMenu.add_submenu_item: "Using this function make the node behave like an internal node from PopupMenu, don't delete this node manually otherwise this can lead to a crash" and let the PopupMenu.clear handle the remotion.

[WhalesState]

Is this the same error that you have got while debugging?

• If yes, then it's related to https://github.com/godotengine/godot/issues/97200

The error there is that the Window tries to process the mouse exit event on a freed child. is_inside_tree() returns true but get_tree() returns nullptr.

[matheusmdx]

No, the error is different and the backtrace the same AThousandShips get:

[MarkoGrbec]

in 4.4dev-4 Fixed

sorry for long reply was busy with game