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

Modifying certain Array type through Inspector will cause Editor crash #78578

Closed
sepTN opened this issue Jun 22, 2023 · 13 comments · Fixed by #78591
Closed

Modifying certain Array type through Inspector will cause Editor crash #78578

sepTN opened this issue Jun 22, 2023 · 13 comments · Fixed by #78591

Comments

@sepTN
Copy link
Contributor

sepTN commented Jun 22, 2023

Godot version

e74bf83

System information

Windows 11, Godot v4.1.beta

Issue description

There's nasty bug going on with the latest master branch build of Godot. If you have an exported variable of type PackedVector2Array, and then proceed to modify its value through the Inspector, it will crash the IDE for no reason.

I reopened the same project on 4.0.3 stable version on Steam, and the bug did not happen and I was able to modify my PackedVector2Array freely through the Inspector.

Steps to reproduce

  • Make a new scene, call it 'main.tscn'
  • Attach a script to it, call it 'main.gd'
  • Create an export variable, something like this will do
extends Node2D
@export var mypacked : PackedVector2Array
  • Go to that scene, click "Add Elements" on that exported variable several times. Then try to modify some of its value to something like [1,2] [2,3] [3,3]

image

  • The IDE will crash

Minimal reproduction project

empty.zip

@AThousandShips
Copy link
Member

My stack trace for the crash:

================================================================
CrashHandlerException: Program crashed
Engine version: Godot Engine v4.1.beta.custom_build (522a2ea3f41fef616980b4d3287f470dd3af9109)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] Node::is_inside_tree (dev\godot\scene\main\node.h:416)
[1] Node::can_process (dev\godot\scene\main\node.cpp:751)
[2] Viewport::_gui_input_event (dev\godot\scene\main\viewport.cpp:1774)
[3] Viewport::push_input (dev\godot\scene\main\viewport.cpp:2978)
[4] Window::_window_input (dev\godot\scene\main\window.cpp:1476)
[5] call_with_variant_args_helper<Window,Ref<InputEvent> const &,0> (dev\godot\core\variant\binder_common.h:303)
[6] call_with_variant_args<Window,Ref<InputEvent> const &> (dev\godot\core\variant\binder_common.h:418)
[7] CallableCustomMethodPointer<Window,Ref<InputEvent> const &>::call (dev\godot\core\object\callable_method_pointer.h:105)
[8] Callable::callp (dev\godot\core\variant\callable.cpp:51)
[9] DisplayServerWindows::_dispatch_input_event (dev\godot\platform\windows\display_server_windows.cpp:2381)
[10] DisplayServerWindows::_dispatch_input_events (dev\godot\platform\windows\display_server_windows.cpp:2346)
[11] Input::_parse_input_event_impl (dev\godot\core\input\input.cpp:719)
[12] Input::flush_buffered_events (dev\godot\core\input\input.cpp:976)
[13] DisplayServerWindows::process_events (dev\godot\platform\windows\display_server_windows.cpp:2075)
[14] OS_Windows::run (dev\godot\platform\windows\os_windows.cpp:1479)
[15] widechar_main (dev\godot\platform\windows\godot_windows.cpp:182)
[16] _main (dev\godot\platform\windows\godot_windows.cpp:204)
[17] main (dev\godot\platform\windows\godot_windows.cpp:218)
[18] WinMain (dev\godot\platform\windows\godot_windows.cpp:232)
[19] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[20] <couldn't map PC to fn name>
-- END OF BACKTRACE --
================================================================

@sepTN
Copy link
Contributor Author

sepTN commented Jun 22, 2023

Doesn't seems to be specific to PackedVector2Array. It also happened to some Array[T], such as

  • Array[int]
  • Array[Vector2i]
  • Array[Vector2]

Those will cause crashes when you try to edit it via Inspector.

But Array[String], Array[StringName] doesn't cause crash, however it behaves weirdly. Array[float] is working fine as intended.

@sepTN sepTN changed the title Modifying PackedVector2Array through Inspector will cause Editor crash Modifying certain Array type through Inspector will cause Editor crash Jun 22, 2023
@bitsawer
Copy link
Member

I debugged this a little and looks like the crash happens because gui.mouse_focus is null on this line:

bool stopped = gui.mouse_focus->can_process() && _gui_call_input(gui.mouse_focus, mb);

It then calls that null pointer's can_process() method which will then blow up soon in is_inside_tree(), as the stack trace posted by AThousandShips shows.

@AThousandShips
Copy link
Member

Should be straight forward to add a null check like so:

bool stopped = gui.mouse_focus && gui.mouse_focus->can_process() && _gui_call_input(gui.mouse_focus, mb);

Will make a PR tomorrow unless someone else snatches it up before then

@bitsawer
Copy link
Member

There are a few other places in that function that access gui.mouse_focus without null checks, although some are checked after making sure MouseButton::LEFT is pressed etc. Some other branches do check for null. Kind of hard to say if there is something more fundamental going on, for example viewport state being inconsistent or something like that or if the code just needs a few more null checks. Maybe someone from the input / editor teams can comment if they have any idea.

@AThousandShips
Copy link
Member

Good points, will see where it leads

@kleonc
Copy link
Member

kleonc commented Jun 22, 2023

Seems like regression from #78005, cc @Sauermann.

@kleonc kleonc added this to the 4.1 milestone Jun 22, 2023
@Sauermann
Copy link
Contributor

Thanks, I will investigate.

@Sauermann
Copy link
Contributor

The following thing happens in

godot/scene/main/viewport.cpp

Lines 1755 to 1757 in cb73a6e

if (control != gui.key_focus) {
control->grab_focus();
}

The EditorSpinSlider relinquishes its focus during its grab_focus call. That is why the above mentioned crash happens, when I switch to a different entry-field by a mouse-click.

I find it questionable, that a Control relinquishes its focus during its grab_focus call, and would ask, if that is intended behavior.
However the `grab_focus´-call may also execute user-defined code, where nothing should lead to a crash. So the null-check, that #78005 removed, needs to be re-added again.
I will create a PR for this.

Also when I edit a number and switch to a different entry-field via TAB-key, I get a different crash, which seems to be more complex:

handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.1.beta.custom_build (cb73a6e9f9046229a16b6b298c9086f03548e186)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x3bf90) [0x7f2462bfdf90] (??:0)
[2] Node::is_inside_tree() const (/srv/docker/data/godot-compile/godot/./scene/main/node.h:416)
[3] Node::get_path() const (/srv/docker/data/godot-compile/godot/scene/main/node.cpp:2046)
[4] EditorSpinSlider::_focus_entered() (/srv/docker/data/godot-compile/godot/editor/gui/editor_spin_slider.cpp:648)
[5] EditorSpinSlider::_notification(int) (/srv/docker/data/godot-compile/godot/editor/gui/editor_spin_slider.cpp:490)
[6] EditorSpinSlider::_notificationv(int, bool) (/srv/docker/data/godot-compile/godot/editor/gui/editor_spin_slider.h:39)
[7] Object::notification(int, bool) (/srv/docker/data/godot-compile/godot/core/object/object.cpp:796)
[8] Viewport::_gui_control_grab_focus(Control*) (/srv/docker/data/godot-compile/godot/scene/main/viewport.cpp:2433)
[9] Control::grab_focus() (/srv/docker/data/godot-compile/godot/scene/gui/control.cpp:2003)
[10] Viewport::_gui_input_event(Ref<InputEvent>) (/srv/docker/data/godot-compile/godot/scene/main/viewport.cpp:2256)
[11] Viewport::push_input(Ref<InputEvent> const&, bool) (/srv/docker/data/godot-compile/godot/scene/main/viewport.cpp:2976)
[12] Window::_window_input(Ref<InputEvent> const&) (/srv/docker/data/godot-compile/godot/scene/main/window.cpp:1476)
[13] void call_with_variant_args_helper<Window, Ref<InputEvent> const&, 0ul>(Window*, void (Window::*)(Ref<InputEvent> const&), Variant const**, Callable::CallError&, IndexSequence<0ul>) (/srv/docker/data/godot-compile/godot/./core/variant/binder_common.h:303)
[14] void call_with_variant_args<Window, Ref<InputEvent> const&>(Window*, void (Window::*)(Ref<InputEvent> const&), Variant const**, int, Callable::CallError&) (/srv/docker/data/godot-compile/godot/./core/variant/binder_common.h:418)
[15] CallableCustomMethodPointer<Window, Ref<InputEvent> const&>::call(Variant const**, int, Variant&, Callable::CallError&) const (/srv/docker/data/godot-compile/godot/./core/object/callable_method_pointer.h:105)
[16] Callable::callp(Variant const**, int, Variant&, Callable::CallError&) const (/srv/docker/data/godot-compile/godot/core/variant/callable.cpp:51)
[17] DisplayServerX11::_dispatch_input_event(Ref<InputEvent> const&) (/srv/docker/data/godot-compile/godot/platform/linuxbsd/x11/display_server_x11.cpp:3721)
[18] DisplayServerX11::_dispatch_input_events(Ref<InputEvent> const&) (/srv/docker/data/godot-compile/godot/platform/linuxbsd/x11/display_server_x11.cpp:3693)
[19] Input::_parse_input_event_impl(Ref<InputEvent> const&, bool) (/srv/docker/data/godot-compile/godot/core/input/input.cpp:718)
[20] Input::flush_buffered_events() (/srv/docker/data/godot-compile/godot/core/input/input.cpp:976)
[21] DisplayServerX11::process_events() (/srv/docker/data/godot-compile/godot/platform/linuxbsd/x11/display_server_x11.cpp:4789)
[22] OS_LinuxBSD::run() (/srv/docker/data/godot-compile/godot/platform/linuxbsd/os_linuxbsd.cpp:908)
[23] ./bin/godot.linuxbsd.editor.dev.x86_64.llvm(main+0x1fe) [0x55f8b5c8b54e] (/srv/docker/data/godot-compile/godot/platform/linuxbsd/godot_linuxbsd.cpp:74)
[24] /lib/x86_64-linux-gnu/libc.so.6(+0x2718a) [0x7f2462be918a] (??:0)
[25] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x85) [0x7f2462be9245] (??:0)
[26] ./bin/godot.linuxbsd.editor.dev.x86_64.llvm(_start+0x21) [0x55f8b5c8b281] (??:?)
-- END OF BACKTRACE --

@Sauermann
Copy link
Contributor

I have bisected that second crash to #76711 and verified, that reverting fixes this crash. cc @ajreckof

@ajreckof
Copy link
Member

I have bisected that second crash to #76711 and verified, that reverting fixes this crash. cc @ajreckof

I'm looking into it. I was able to reproduce the crash locally but I get a different trace. Hoping those two backtrace have the same reasons for crashing.

@ajreckof
Copy link
Member

So after some really harduous debugging time where I couldn't understand what was done wrong I realised that EditorPropertyArray was removing child early and that release focus was called on exit tree. Therefore the node was removed from tree before being focused which means it was never unfocused. This can be reproduced with a simple godot project which will also crash. See #78590

@Rindbee
Copy link
Contributor

Rindbee commented Jun 23, 2023

Modifying the value of an element causes EditorPropertyArray::update_property() to be called. This results in parts of the tree being rebuilt. Somewhat similar to #76985. Rebuild should not happen because the structure (size) has not changed then.

Variant array = object->get_array().duplicate();
array.set(index, p_value);
object->set_array(array);
emit_changed(get_edited_property(), array, "", true);
if (!p_changing) {
update_property();
}

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