Removing FileDialog in plugin causing a Segfault

[donn-xx]

Godot version

v4.2.beta.custom_build [46cb7f9]

System information

Godot v4.2.beta (46cb7f9) - Ubuntu 22.04.3 LTS 22.04 - X11 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 2060 (nvidia; 535.113.01) - Intel(R) Core(TM) i5-9400F CPU @ 2.90GHz (6 Threads)

Issue description

Was getting segfaults on each exit whilst my plugin was enabled. Eventually compiled Godot and followed some guides to get gdb going and got enough info to hopefully help you boffins fix it.

Nutshell:
In the demo project, look at main.gd

@tool
extends MarginContainer

@onready var file_dialog: FileDialog = $FileDialog

# This fixes the crash. Remark to see the segfault.
func _exit_tree() -> void:
	remove_child($FileDialog)

Remark out the _exit_tree func and then enable the plugin. Open it (from the bottom container) and then Ctrl+Q quit Godot. It should segfault.

Replacing the _exit_tree stops the crash.

Steps to reproduce

As above.

Minimal reproduction project

Still can't drop a zip file in here. So, made a repo:

https://gitlab.com/dbat/godot-filedialog-segfault

[donn-xx]

Oh, addons/death/debug_notes is where I pasted my gdb output and where I fingered the FileDialog as the culprit.

[akien-mga]

Stacktrace:

(gdb) bt
#0  0x00005555593be246 in HashSet<FileDialog*, HashMapHasherDefault, HashMapComparatorDefault<FileDialog*> >::_lookup_pos (this=0xc60, p_key=@0x7fffffffd628: 0x555579bd6580, r_pos=@0x7fffffffd5d8: 0)
    at ./core/templates/hash_set.h:83
#1  0x00005555593b0d5b in HashSet<FileDialog*, HashMapHasherDefault, HashMapComparatorDefault<FileDialog*> >::erase (this=0xc60, p_key=@0x7fffffffd628: 0x555579bd6580) at ./core/templates/hash_set.h:261
#2  0x000055555937fb7b in EditorNode::_file_dialog_unregister (p_dialog=0x555579bd6580)
    at editor/editor_node.cpp:4607
#3  0x000055555a5f010c in FileDialog::~FileDialog (this=0x555579bd6580, __in_chrg=<optimized out>)
    at scene/gui/file_dialog.cpp:1246
#4  0x00005555584ce314 in memdelete<Node> (p_class=0x555579bd6580) at ./core/os/memory.h:109
#5  0x000055555a3d1dc8 in Node::_notification (this=0x555579bd3360, p_notification=1)
    at scene/main/node.cpp:212
#6  0x000055555822ee98 in Node::_notificationv (this=0x555579bd3360, p_notification=1, p_reversed=true)
    at scene/main/node.h:49
#7  0x0000555558796401 in CanvasItem::_notificationv (this=0x555579bd3360, p_notification=1, 
    p_reversed=true) at ./scene/main/canvas_item.h:45
#8  0x0000555558796d7b in Control::_notificationv (this=0x555579bd3360, p_notification=1, 
    p_reversed=true) at ./scene/gui/control.h:48
#9  0x0000555558797715 in Container::_notificationv (this=0x555579bd3360, p_notification=1, 
    p_reversed=true) at ./scene/gui/container.h:37
#10 0x00005555592d2ff5 in MarginContainer::_notificationv (this=0x555579bd3360, p_notification=1, 
    p_reversed=true) at ./scene/gui/margin_container.h:37
#11 0x000055555c65bb85 in Object::notification (this=0x555579bd3360, p_notification=1, p_reversed=true)
    at core/object/object.cpp:850
#12 0x000055555c659557 in Object::_predelete (this=0x555579bd3360) at core/object/object.cpp:198
#13 0x000055555c665251 in predelete_handler (p_object=0x555579bd3360) at core/object/object.cpp:2050
#14 0x000055555803736c in memdelete<Object> (p_class=0x555579bd3360) at ./core/os/memory.h:105
#15 0x000055555a41bfd4 in SceneTree::_flush_delete_queue (this=0x555564595f60)
    at scene/main/scene_tree.cpp:1345
#16 0x000055555a418c76 in SceneTree::finalize (this=0x555564595f60) at scene/main/scene_tree.cpp:633
#17 0x0000555557f9b15c in OS_LinuxBSD::run (this=0x7fffffffd9e0)
    at platform/linuxbsd/os_linuxbsd.cpp:938
#18 0x0000555557f932a8 in main (argc=2, argv=0x7fffffffdfa8) at platform/linuxbsd/godot_linuxbsd.cpp:74

I can reproduce the crash with the described steps.

[akien-mga]

It seems to be a regression, introduced in 4.2-dev3 (4.1-stable, 4.1.2-stable, 4.2-dev1 and 4.2-dev2 don't reproduce it, any later dev or beta does).

So it should be one of these PRs: https://godotengine.github.io/godot-interactive-changelog/#4.2-dev3

I suspect changes in notification order caused by #67791 maybe? Just a wild guess, would need bisecting. @Sauermann

[AThousandShips]

Possibly related to:

• Godot 4.1 crashes on simple plugin print statement #79379

Might be worth checking if that error has popped back up itself

[YuriSizov]

This is technically a regression from #80161, at least from the user perspective. From the code perspective this PR simply uncovered a fault in logic.

What happens is that we register all file dialogs with the editor node. When those dialogs are destroyed, they are unregistered. The registration happens via callback methods on the editor node, triggered from the constructor and the destructor. The faulty thing that happens is that EditorNode is freed before the nodes added by plugins are. So the unregistration method is called after ~EditorNode.

The linked PR coincidentally intervenes with this, because it nullptrs the singleton reference on destruction. So the unregistration method has a broken reference in it. The same happens in 4.1.2, but just because the reference is not nulled, it works. I have confirmed that commenting out singleton = nullptr; in the master branch fixes the crash on exit as well.

I'm not entirely sure what this reference is after the destructor, but it somehow works... I think the proper way to address the crash would be to add some guards to the unregistration callback. Edit: Or rather we should just remove the method references, since this is all a part of EditorNode setup anyway.