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

Freeing a node on macOS + Godot Mono from _GuiInput causes a crash #100864

Open
AdamLearns opened this issue Dec 27, 2024 · 4 comments
Open

Freeing a node on macOS + Godot Mono from _GuiInput causes a crash #100864

AdamLearns opened this issue Dec 27, 2024 · 4 comments

Comments

@AdamLearns
Copy link
Contributor

AdamLearns commented Dec 27, 2024

Tested versions

  • Reproducible in v4.3.stable.mono.official [77dcf97d8]
  • Reproducible in v4.4.dev.mono.custom_build.c602c168b

System information

Sonoma 14.6.1

Issue description

Freeing a node right when Godot calls the GridMapEditorPlugin destructor causes a crash. This only seems to happen on macOS; I had several people test on Windows for a combined total of 50+ times and it didn't repro there. I also couldn't get this to repro from GDScript alone.

Stack trace for the crash:

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.3.stable.mono.official (77dcf97d82cbfe4e4615475fa52ca03da645dbd8)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] invoke_previous_action(sigaction*, int, __siginfo*, void*, bool)
[2] 2   libsystem_platform.dylib            0x0000000194666584 _sigtramp + 56
[3] 3   libc++abi.dylib                     0x00000001945ed7b0 __dynamic_cast + 56
[4] Viewport::push_input(Ref<InputEvent> const&, bool) (in Godot) + 720
[5] Window::_window_input(Ref<InputEvent> const&) (in Godot) + 756
[6] GridMapEditorPlugin::~GridMapEditorPlugin()
[7] DisplayServerMacOS::_dispatch_input_event(Ref<InputEvent> const&)
[8] DisplayServerMacOS::_dispatch_input_event(Ref<InputEvent> const&) (in Godot) + 572
[9] Input::_parse_input_event_impl(Ref<InputEvent> const&, bool) (in Godot) + 2828
[10] Input::flush_buffered_events() (in Godot) + 136
[11] Input::release_pressed_events() (in Godot) + 20
[12] DisplayServerMacOS::release_pressed_events() (in Godot) + 40
[13] RendererCompositorRD::_create_current()
[14] 14  CoreFoundation                      0x000000019470a144 __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 148
[15] 15  CoreFoundation                      0x000000019479e3d8 ___CFXRegistrationPost_block_invoke + 88
[16] 16  CoreFoundation                      0x000000019479e320 _CFXRegistrationPost + 440
[17] 17  CoreFoundation                      0x00000001946d8678 _CFXNotificationPost + 768
[18] 18  Foundation                          0x00000001957f52c4 -[NSNotificationCenter postNotificationName:object:userInfo:] + 88
[19] 19  AppKit                              0x00000001980ce7f4 -[NSWindow resignKeyWindow] + 640
[20] 20  AppKit                              0x00000001980ce4dc _NXEndKeyAndMain + 128
[21] 21  AppKit                              0x00000001980cd5e4 -[NSApplication _handleDeactivateEvent:] + 724
[22] 22  AppKit                              0x0000000198768380 -[NSApplication(NSEventRouting) sendEvent:] + 1236
[23] Ref<DirAccess> DirAccess::_create_builtin<DirAccessMacOS>()
[24] DisplayServerMacOS::process_events() (in Godot) + 220
[25] OS_MacOS::run() (in Godot) + 156
[26] main (in Godot) + 392
[27] 27  dyld                                0x00000001942ab154 start + 2476
-- END OF BACKTRACE --
================================================================
The program '[47492] Godot' has exited with code 0 (0x0).

As a workaround, calling QueueFree instead of Free should fix the issue.

Steps to reproduce

  • Load the minimal repro from the attached .zip
  • Hover your mouse rapidly around where the sprites show up. The chance of repro is quite high if you do this as soon as the program starts, so try to memorize where they'll show up and start hovering there.

Quick video repro here.

The most relevant code is this snippet, which simply frees one of the icons as soon as it receives mouse input:

public override void _GuiInput(InputEvent @event)
{
    AcceptEvent();
    Free();
}

Minimal reproduction project (MRP)

crashrepro.zip

@AdamLearns AdamLearns changed the title Freeing a node on macOS + Godot Mono causes a crash (possibly related to GridMapEditorPlugin) Freeing a node on macOS + Godot Mono from _GuiInput causes a crash (possibly related to GridMapEditorPlugin) Dec 27, 2024
@AdamLearns AdamLearns changed the title Freeing a node on macOS + Godot Mono from _GuiInput causes a crash (possibly related to GridMapEditorPlugin) Freeing a node on macOS + Godot Mono from _GuiInput causes a crash Dec 27, 2024
@AThousandShips
Copy link
Member

Does using QueueFree work? This isn't safe use of Free AFAIK

@kOyCHFqLJYDV
Copy link

Does using QueueFree work? This isn't safe use of Free AFAIK

I concur. This is not safe use of Free. One would always want to use QueueFree instead of Free there.

@AdamLearns
Copy link
Contributor Author

Does using QueueFree work? This isn't safe use of Free AFAIK

Yes, QueueFree() works, and I'll switch my code to that.

I think it may be worth adding some context here. I only recognized that this was unsafe usage after witnessing a crash. However, it still took me a few hours to create the minimal repro. It was difficult to pinpoint because:

  • It wasn't my code directly accessing the freed node.
    • To add to this, I'm not super familiar with the Godot code itself, so despite looking through it and trying to log from various points, the dylib stuff threw me off.
  • When it is my code accessing the freed node, I get ERROR: System.ObjectDisposedException: Cannot access a disposed object.
  • The crash only happens every so often.

I'm not trying to argue that this should result in a code fix, but perhaps this documentation could be clearer? Maybe an extra line like "When Godot tries to access a freed node, it may even result in a crash." Then again, this issue will now exist as a reference point for anyone searching for this in the future.

Anyway, thank you for the prompt response!

P.S. out of curiosity, why is GridMapEditorPlugin::~GridMapEditorPlugin() in the middle of that stack trace? The existence of that in input-related code perplexed me.

@kOyCHFqLJYDV
Copy link

I'm not trying to argue that this should result in a code fix, but perhaps this documentation could be clearer? Maybe an extra line like "When Godot tries to access a freed node, it may even result in a crash." Then again, this issue will now exist as a reference point for anyone searching for this in the future.

Heck, I'd be in favor of just removing API access to Free. As in, making it an internal thing only. There is no real world scenario where a game/application designer using Godot would need to use a direct Free instead of a QueueFree. C# is managed code, GDScript is interpreted. Both consequently not in a position to have as much free control over freeing a node as a direct Free offers.

So, yeah, documentation clarification is the least we should consider. Fully agreeing with you there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants