Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unloading plugin results into crash on macOS and Linux at Godot exit #92727

Closed
MikeSchulze opened this issue Jun 3, 2024 · 9 comments · Fixed by #93238
Closed

Unloading plugin results into crash on macOS and Linux at Godot exit #92727

MikeSchulze opened this issue Jun 3, 2024 · 9 comments · Fixed by #93238

Comments

@MikeSchulze
Copy link

MikeSchulze commented Jun 3, 2024

Tested versions

v4.2.2.stable.official [15073af] macOS

System information

macOS Sonoma 14.1.1

Issue description

My plugin is crashing at Godot exit on macOS and Linux, it works without any issues on Windows and Linux systems.

There is an issue when using free at __exit_tree inside the EditorPlugin, using queue_free helps but ends up with a lot of orphan resources at exit, so I prefer to use free.
Using queue_free with afterward await get_tree().process_frame do also crash in my plugin.

Steps to reproduce

Use the attached example project (a strictly reduced version of my plugin, includes only the inspector)

  • start Godot do enable it
  • exit
    sometimes the Godot editor is hangs and needs manually to kill, sometimes it ends with signal 11

Minimal reproduction project (MRP)

plugin_free.zip

@MikeSchulze MikeSchulze changed the title Unloading plugin results into crash on macOS at Godot exit Unloading plugin results into crash on macOS and Linux at Godot exit Jun 5, 2024
@coppolaemilio
Copy link
Member

After consulting on the issue, it appears that the problem lies with the plugin cleanup logic, not an engine bug. The plugin attempts to remove a node from the tree and free control in _exit_tree while the scene tree is in the process of being deleted. This causes the remove call to fail with a "Parent node is busy adding/removing children" error, and the free call to delete a node that is still referenced in the scene tree.

The entire cleanup logic should be moved to _disable_plugin (to remove the dock when the plugin is disabled from the settings). There's no need to call this from _exit_tree since it's part of the tree and will be deleted recursively with it.

If you think something else should be done feel free to comment and I can reopen the issue. Thanks!

@MikeSchulze
Copy link
Author

The entire cleanup logic should be moved to _disable_plugin (to remove the dock when the plugin is disabled from the settings). There's no need to call this from _exit_tree since it's part of the tree and will be deleted recursively with it.

@coppolaemilio That not work because _disable_plugin is never called when closing the editor.
So I have to use _exit_tree this is a bug, please reopen this issue

@bruvzg
Copy link
Member

bruvzg commented Jun 16, 2024

So I have to use _exit_tree this is a bug, please reopen this issue

Why are you trying to free it? There's no reason to free any of the plugin nodes from _exit_tree at all, nodes are part of the editor scene tree and will be freed automatically.

@MikeSchulze
Copy link
Author

So I have to use _exit_tree this is a bug, please reopen this issue

Why are you trying to free it? There's no reason to free any of the plugin nodes from _exit_tree at all, nodes are part of the editor scene tree and will be freed automatically.

From the plugin documentation, it is recommended.
https://docs.godotengine.org/de/4.x/tutorials/plugins/editor/making_plugins.html
image

And if I'm not freeing the resource, the Editor closes with a lot of memory leaks.

@bruvzg
Copy link
Member

bruvzg commented Jun 16, 2024

For custom resources yes, but it should not be necessary for nodes.

I guess if it's documented like this we probably shouldn't add an extra step to unload all plugins before quitting the editor. I'll take another look.

@MikeSchulze
Copy link
Author

For custom resources yes, but it should not be necessary for nodes.

From the example, this is a scene added to a dock, and I guessing it is a Control Node.

I guess if it's documented like this we probably shouldn't add an extra step to unload all plugins before quitting the editor. I'll take another look.

Yes, thanks please check this

@bruvzg
Copy link
Member

bruvzg commented Jun 16, 2024

And if I'm not freeing the resource, the Editor closes with a lot of memory leaks.

I can't reproduce any leaks when node removal is missing (at least with MRP).

I'll take another look.

Opened PR that should fix it - #93238, will fully test it tomorrow.

@MikeSchulze
Copy link
Author

One more interesting part is why using free and queue_free has so different runtime effects when using under Windows or macOS.
Actually under windows using queue_free() on my inspector at _exit_tree results in a segmentation fault at Godot exit, but using free() works fine, no orphan nodes and normal exit.
And under macOS the opposite is the case, it ends with a segmentation fault at Godot exit when using free()

@MikeSchulze
Copy link
Author

update

I was able to fix now my segmentation faults and memory leaks at exit, it works for Windows and macOS, Linux needs still to be tested.

@akien-mga akien-mga added this to the 4.3 milestone Jun 18, 2024
@akien-mga akien-mga added bug and removed archived labels Jun 18, 2024
MikeSchulze added a commit to MikeSchulze/gdUnit4 that referenced this issue Jun 19, 2024
# Why
The plugin has some issues to release resources at Godot exit and result
sometimes in a segmentation fault under Linux

# What
- fix `remove_child` on `free_instance` to be called deferred now
- removed the temporary `free_fix` workaround
- fix orphan nodes in `GdUnitTestDiscoverer`
- introduce a new flag to call deferred on `free_instance` at plugin
exit


# Godot bug related
godotengine/godot#92727
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants