Editor crashes when saving built-in script with a non empty last line

[Mohamed-Kr]

Godot version

4.2.rc1

System information

Godot v4.2.rc1 - Windows 10.0.22621 - GLES3 (Compatibility) - Intel(R) Iris(R) Plus Graphics (Intel Corporation; 27.20.100.9621) - Intel(R) Core(TM) i7-1065G7 CPU @ 1.30GHz (8 Threads)

Issue description

The editor crashes when saving a scene that contains a built-in script with a non empty last line.
The save always seem to take place anyway with an empty line added to the problematic script.

Steps to reproduce

• Add a built-in script to a node
• Give it the script below
• Save the scene

extends Node

func _ready() -> void:
	pass

Minimal reproduction project

N/A

[akien-mga]

I can't reproduce the issue on Linux, for me it saves properly, adding a newline at the end of the script (as Godot follows the Unix convention of terminating files with a newline character). Might behave differently on Windows.

Can you reproduce the bug systematically, and does this happen with 4.1.3-stable?

[YuriSizov]

Might behave differently on Windows.

Can't reproduce it on Windows. The new line is added and the script is saved. Not entirely sure if it would be related, since all it does is add a line break to the string edited by the text editor. But we also can't disable this behavior via settings, so we can't test if it's a factor or not. But I would guess that the crash happens for some other reason.

[Mohamed-Kr]

I'm sorry, I should have done more testing before writing the issue, it is consistent, it happens every single time on 4.2.rc1 with the project I was working on but actually only in certain scenes. And I do not have it in newly created scenes.

Also I ran the console version and when crashing it printed

ERROR: FATAL: Index p_index = 0 is out of bounds (size() = 0).
   at: get (./core/templates/cowdata.h:158)

And I think I ended up cornering the bug, it seems an AnimationPlayer Node is causing the crash (which was in a lot of the files I was working, which made me think it was always happening), maybe because it references deleted nodes. But it is somehow linked with the ending line of built-in scripts being empty or not.

The scene was originally created using 4.1-stable, the conversions may also have introduced the problem.

Here is the scene file :
MinimalReproduction.zip

[Mohamed-Kr]

Actually I think this time I really got it, It works even in new projects.
But not for 4.1.3.stable.
I don't know if I should update the issue but here is the actual reproduction steps :

• Create a Scene
• Give it an AnimationPlayer child
• Create an animation and add a keyframe for a property of the root (it also seems to work for a property of any Node eventually added in the scene)
• Give the exact same built-in script as below (without additional empty line) to any Node and save the scene
  and it should crash

extends Node

func _ready() -> void:
	pass

[YuriSizov]

@Mohamed-Kr This is perfect, I can reproduce it now!

I also noticed that when you first try to keyframe a property, you get this logged:

And here's the crash stack:

000001ea39c889b0() (Unknown Source:0)
memdelete<AnimationMixer::TrackCache>(AnimationMixer::TrackCache * p_class) Line 112 (c:\Projects\godot-engine\master\core\os\memory.h:112)
AnimatedValuesBackup::~AnimatedValuesBackup() Line 398 (c:\Projects\godot-engine\master\scene\animation\animation_mixer.h:398)
AnimatedValuesBackup::`scalar deleting destructor'(unsigned int) (Unknown Source:0)
memdelete<AnimatedValuesBackup>(AnimatedValuesBackup * p_class) Line 112 (c:\Projects\godot-engine\master\core\os\memory.h:112)
Ref<AnimatedValuesBackup>::unref() Line 212 (c:\Projects\godot-engine\master\core\object\ref_counted.h:212)
Ref<AnimatedValuesBackup>::~Ref<AnimatedValuesBackup>() Line 223 (c:\Projects\godot-engine\master\core\object\ref_counted.h:223)
Pair<AnimationMixer *,Ref<AnimatedValuesBackup>>::~Pair<AnimationMixer *,Ref<AnimatedValuesBackup>>() (Unknown Source:0)
List<Pair<AnimationMixer *,Ref<AnimatedValuesBackup>>,DefaultAllocator>::Element::~Element() (Unknown Source:0)
List<Pair<AnimationMixer *,Ref<AnimatedValuesBackup>>,DefaultAllocator>::Element::`scalar deleting destructor'(unsigned int) (Unknown Source:0)
memdelete_allocator<List<Pair<AnimationMixer *,Ref<AnimatedValuesBackup>>,DefaultAllocator>::Element,DefaultAllocator>(List<Pair<AnimationMixer *,Ref<AnimatedValuesBackup>>,DefaultAllocator>::Element * p_class) Line 124 (c:\Projects\godot-engine\master\core\os\memory.h:124)
List<Pair<AnimationMixer *,Ref<AnimatedValuesBackup>>,DefaultAllocator>::_Data::erase(const List<Pair<AnimationMixer *,Ref<AnimatedValuesBackup>>,DefaultAllocator>::Element * p_I) Line 242 (c:\Projects\godot-engine\master\core\templates\list.h:242)
List<Pair<AnimationMixer *,Ref<AnimatedValuesBackup>>,DefaultAllocator>::erase(const List<Pair<AnimationMixer *,Ref<AnimatedValuesBackup>>,DefaultAllocator>::Element * p_I) Line 429 (c:\Projects\godot-engine\master\core\templates\list.h:429)
List<Pair<AnimationMixer *,Ref<AnimatedValuesBackup>>,DefaultAllocator>::clear() Line 463 (c:\Projects\godot-engine\master\core\templates\list.h:463)
List<Pair<AnimationMixer *,Ref<AnimatedValuesBackup>>,DefaultAllocator>::~List<Pair<AnimationMixer *,Ref<AnimatedValuesBackup>>,DefaultAllocator>() Line 758 (c:\Projects\godot-engine\master\core\templates\list.h:758)
EditorNode::_save_scene(String p_file, int idx) Line 1840 (c:\Projects\godot-engine\master\editor\editor_node.cpp:1840)
EditorNode::_save_scene_with_preview(String p_file, int p_idx) Line 1676 (c:\Projects\godot-engine\master\editor\editor_node.cpp:1676)
EditorNode::_menu_option_confirm(int p_option, bool p_confirmed) Line 2604 (c:\Projects\godot-engine\master\editor\editor_node.cpp:2604)
EditorNode::_menu_option(int p_option) Line 1386 (c:\Projects\godot-engine\master\editor\editor_node.cpp:1386)
call_with_variant_args_helper<EditorNode,int,0>(EditorNode * p_instance, void(EditorNode::*)(int) p_method, const Variant * * p_args, Callable::CallError & r_error, IndexSequence<0> __formal) Line 308 (c:\Projects\godot-engine\master\core\variant\binder_common.h:308)
call_with_variant_args<EditorNode,int>(EditorNode * p_instance, void(EditorNode::*)(int) p_method, const Variant * * p_args, int p_argcount, Callable::CallError & r_error) Line 418 (c:\Projects\godot-engine\master\core\variant\binder_common.h:418)
CallableCustomMethodPointer<EditorNode,int>::call(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 105 (c:\Projects\godot-engine\master\core\object\callable_method_pointer.h:105)
Callable::callp(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 58 (c:\Projects\godot-engine\master\core\variant\callable.cpp:58)
Object::emit_signalp(const StringName & p_name, const Variant * * p_args, int p_argcount) Line 1128 (c:\Projects\godot-engine\master\core\object\object.cpp:1128)
Node::emit_signalp(const StringName & p_name, const Variant * * p_args, int p_argcount) Line 3607 (c:\Projects\godot-engine\master\scene\main\node.cpp:3607)
Object::emit_signal<int>(const StringName & p_name, int <p_args_0>) Line 922 (c:\Projects\godot-engine\master\core\object\object.h:922)
PopupMenu::activate_item(int p_idx) Line 2312 (c:\Projects\godot-engine\master\scene\gui\popup_menu.cpp:2312)
PopupMenu::activate_item_by_event(const Ref<InputEvent> & p_event, bool p_for_global_only) Line 2224 (c:\Projects\godot-engine\master\scene\gui\popup_menu.cpp:2224)
MenuBar::shortcut_input(const Ref<InputEvent> & p_event) Line 166 (c:\Projects\godot-engine\master\scene\gui\menu_bar.cpp:166)
Node::_call_shortcut_input(const Ref<InputEvent> & p_event) Line 3132 (c:\Projects\godot-engine\master\scene\main\node.cpp:3132)
SceneTree::_call_input_pause(const StringName & p_group, SceneTree::CallInputType p_call_type, const Ref<InputEvent> & p_input, Viewport * p_viewport) Line 1233 (c:\Projects\godot-engine\master\scene\main\scene_tree.cpp:1233)
Viewport::_push_unhandled_input_internal(const Ref<InputEvent> & p_event) Line 3373 (c:\Projects\godot-engine\master\scene\main\viewport.cpp:3373)
Viewport::push_input(const Ref<InputEvent> & p_event, bool p_local_coords) Line 3334 (c:\Projects\godot-engine\master\scene\main\viewport.cpp:3334)
Window::_window_input(const Ref<InputEvent> & p_ev) Line 1553 (c:\Projects\godot-engine\master\scene\main\window.cpp:1553)
call_with_variant_args_helper<Window,Ref<InputEvent> const &,0>(Window * p_instance, void(Window::*)(const Ref<InputEvent> &) p_method, const Variant * * p_args, Callable::CallError & r_error, IndexSequence<0> __formal) Line 303 (c:\Projects\godot-engine\master\core\variant\binder_common.h:303)
call_with_variant_args<Window,Ref<InputEvent> const &>(Window * p_instance, void(Window::*)(const Ref<InputEvent> &) p_method, const Variant * * p_args, int p_argcount, Callable::CallError & r_error) Line 418 (c:\Projects\godot-engine\master\core\variant\binder_common.h:418)
CallableCustomMethodPointer<Window,Ref<InputEvent> const &>::call(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 105 (c:\Projects\godot-engine\master\core\object\callable_method_pointer.h:105)
Callable::callp(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 58 (c:\Projects\godot-engine\master\core\variant\callable.cpp:58)
Callable::call<Ref<InputEvent>>(Ref<InputEvent> <p_args_0>) Line 850 (c:\Projects\godot-engine\master\core\variant\variant.h:850)
DisplayServerWindows::_dispatch_input_event(const Ref<InputEvent> & p_event) Line 2716 (c:\Projects\godot-engine\master\platform\windows\display_server_windows.cpp:2716)
DisplayServerWindows::_dispatch_input_events(const Ref<InputEvent> & p_event) Line 2686 (c:\Projects\godot-engine\master\platform\windows\display_server_windows.cpp:2686)
Input::_parse_input_event_impl(const Ref<InputEvent> & p_event, bool p_is_emulated) Line 731 (c:\Projects\godot-engine\master\core\input\input.cpp:731)
Input::flush_buffered_events() Line 995 (c:\Projects\godot-engine\master\core\input\input.cpp:995)
DisplayServerWindows::process_events() Line 2401 (c:\Projects\godot-engine\master\platform\windows\display_server_windows.cpp:2401)
OS_Windows::run() Line 1474 (c:\Projects\godot-engine\master\platform\windows\os_windows.cpp:1474)
widechar_main(int argc, wchar_t * * argv) Line 182 (c:\Projects\godot-engine\master\platform\windows\godot_windows.cpp:182)
_main() Line 204 (c:\Projects\godot-engine\master\platform\windows\godot_windows.cpp:204)
main(int argc, char * * argv) Line 218 (c:\Projects\godot-engine\master\platform\windows\godot_windows.cpp:218)
WinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, char * lpCmdLine, int nCmdShow) Line 232 (c:\Projects\godot-engine\master\platform\windows\godot_windows.cpp:232)
[Inline Frame] invoke_main() Line 102 (d:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:102)
__scrt_common_main_seh() Line 288 (d:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
kernel32.dll!00007ff81140257d() (Unknown Source:0)
ntdll.dll!00007ff81206aa58() (Unknown Source:0)

cc @TokageItLab

[YuriSizov]

Do note that having a script without an empty line at the end does seem to be critical to reproduce the issue. Which is weird and may imply it's not purely an animation issue. Maybe some race condition?

[YuriSizov]

So I've looked further into this. There is indeed a conflict of two operations trying to do the same thing. When we call _save_scene we end up calling save_editor_external_data, which, in case of a built-in script, triggers saving of the scene all over again.

Both times we collect animation data

	List<Pair<AnimationMixer *, Ref<AnimatedValuesBackup>>> anim_backups;
	_reset_animation_mixers(scene, &anim_backups);

But the second time happens inside of the first time. So first the first pass collects it, does other things, and eventually triggers saving of external data. This triggers the second pass, which collects the animation data, does its things, end eventually successfully ends. This gives control back to the first pass, but I guess the animation data has already expired.

We don't immediately do anything with it, but it crashes when trying to free the memory.

The reason why we trigger the second pass is indeed hidden in the fact that we add a new empty line at the end of the script, which makes the script to be marked as unsaved. This doesn't happen under other circumstances.

---

We can approach this a few different ways, though it's strange to me what happens to the animation data that we collect. Guarding against this would probably be the best option.

[TokageItLab]

I don't remember touching the script itself in relation to the AnimationMixer implementation. If I had to say, I remember putting a check to not copy the script when generating the dummy player, but it probably has nothing to do with it.

But I think it might be related to the problem of trying to memdelete the null pointer that @KoBeWi and I have been discussing on ContributorsChat and #84942 (comment).

[YuriSizov]

But I think it might be related to the problem of trying to memdelete the null pointer that @KoBeWi and I have been discussing on ContributorsChat and #84942 (comment).

Yes, this seems to be it.