C# Editor Plugin crashes Godot 4.3 when GraphNode is added to GraphEdit

Meythulhu commented 2 months ago

Tested versions

Reproducible in v4.3.beta1 (a4f2ea91a), Godot v4.3-stable (77dcf97d8), and v4.3.1-rc
Tested in Godot v4.2.2-stable (15073afe3), and this is not an issue

System information

Windows 11 - v4.3.stable.mono.official.77dcf97d8 - Vulkan 1.3.278 - Forward+ - NVIDIA GeForce RTX 4060 Ti

Issue description

Through the process of using the Dialogue Trees plugin, I noticed that an exception is thrown whenever trying to edit a DialogueTree node. The exception thrown is a AccessViolationException, which shortly crashes Godot 4.3 with no further information other than the stack trace.

System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at Godot.NativeInterop.NativeFuncs.godotsharp_method_bind_ptrcall(IntPtr, IntPtr, Void**, Void*)
   at Godot.NativeCalls.godot_icall_3_767(IntPtr, IntPtr, IntPtr, Godot.NativeInterop.godot_bool, Int32)
   at Godot.Node.AddChild(Godot.Node, Boolean, InternalMode)
   at Ardot.DialogueTrees.DialogueGraph.<LoadTree>g__AddDialogueNode|26_0(Godot.Node, Int32, <>c__DisplayClass26_0 ByRef)
   at Ardot.DialogueTrees.DialogueGraph.LoadTree(Ardot.DialogueTrees.DialogueTreeData, Boolean, Godot.Collections.Array`1<Godot.Node>)
   at Ardot.DialogueTrees.DialogueTreeDock.LoadTree(Ardot.DialogueTrees.DialogueTree)
   at Ardot.DialogueTrees.DialogueTreesPlugin._Edit(Godot.GodotObject)
   at Godot.EditorPlugin.InvokeGodotClassMethod(Godot.NativeInterop.godot_string_name ByRef, Godot.NativeInterop.NativeVariantPtrArgs, Godot.NativeInterop.godot_variant ByRef)
   at Ardot.DialogueTrees.DialogueTreesPlugin.InvokeGodotClassMethod(Godot.NativeInterop.godot_string_name ByRef, Godot.NativeInterop.NativeVariantPtrArgs, Godot.NativeInterop.godot_variant ByRef)
   at Godot.Bridge.CSharpInstanceBridge.Call(IntPtr, Godot.NativeInterop.godot_string_name*, Godot.NativeInterop.godot_variant**, Int32, Godot.NativeInterop.godot_variant_call_error*, Godot.NativeInterop.godot_variant*)

It seems to occur when trying to add or remove a GraphNode from the DialogueGraph class (which extends GraphEdit). I tried passing brand new GraphNode, I tried deferring the calls, and I tried removing the optional parameters, as well as playing with the values of the different AddChild parameters. All of these seem to have no effect on the exception occurring and a crash following after.

It's also important to note that this does not occur on DialogueTree nodes which are recently added to the scene, only ones loaded from the file system. This is only impacting the DialogueGraph added to the dock; the DialogueTree node functions properly when running in-game.

I tried running memory dumps and attaching the godot process to a debugger, and there is no breakpoint hit or any sort of trace in the memory dump outside of the AccessViolationException being thrown only in the managed assembly. It just silently closes Godot with exit code 139 (SIGSEGV signal - Memory Access Violation).

Steps to reproduce

With the Minimal Reproduction Project:

Open up TestScene.tscn in the Godot Editor
If the crash hasn't happened yet, select the DialogueTree node in the scene.

Without the Minimal Reproduction Project:

Install Dialogue Trees - C# from the Asset Library
Create new scene with DialogueTree node
In the DialogueTree editor in the bottom dock, add an output node and hook it up to the start node (you'll need to expand the window)
Save the scene and close it
Re-open the scene and try selecting the DialogueTree node

Minimal reproduction project (MRP)

DialogueTest.zip

raulsntos commented 2 months ago

Looks like it may be a regression from https://github.com/godotengine/godot/pull/88014 cc @Geometror

It seems connections_layer is null when this Callable is called:

https://github.com/godotengine/godot/blob/fd7239cfab228c50777cd54a8bf6eb279a02ef81/scene/gui/graph_edit.cpp#L626

Geometror commented 2 months ago

Well, I'm surprised it took so long until we see the non-internal connection layer causing problems ;)

It looks like you remove all children including the connection layer at some point (haven't checked the addon code). To fix this crash you need to check whether each node you remove is a GraphElement. The connections layer has to be kept non-internal for now because internal nodes at arbitrary indices (not just front or back) are currently not possible and it has to be drawn between frames and graph nodes.

I'm going to open a PR which adds a warning when you accidentally remove the connection layer as well as checks everywhere a crash could occur (not ideal, but at least it's safe).

Meythulhu commented 2 months ago

It looks like you remove all children including the connection layer at some point (haven't checked the addon code). To fix this crash you need to check whether each node you remove is a GraphElement. The connections layer has to be kept non-internal for now because internal nodes at arbitrary indices (not just front or back) are currently not possible and it has to be drawn between frames and graph nodes.

Yep! It seems to be caused by DialogueGraph.ClearTree(), which just calls GetChildren() and frees all children. Adding a filter for any node named _connection_layer seems to prevent it from being removed.

This seems like an issue that can be easily ran into; Is it possible for _connection_layer to be made internal to prevent it from being treated as a non-internal node?

Geometror commented 2 months ago

Unfortunately not, as I said:

The connections layer has to be kept non-internal for now because internal nodes at arbitrary indices (not just front or back) are currently not possible and it has to be drawn between frames and graph nodes.

The best we can do is warn the user about it and prevent the crash, hence https://github.com/godotengine/godot/pull/96309

godotengine / godot