Reentrency crash when calling EditorFileSystem::scan() while import in progress.

[lyuma]

Godot version

4.0.dev 7211012

System information

Windows 10, Vulkan

Issue description

While Godot is importing files, it may call the Main::iteration() any number of times. This will trigger GDScript code to execute. If this GDScript code adds any files into the project and calls EditorFilesystem::scan() during this time, there is a chance that Godot will crash.

================================================================
CrashHandlerException: Program crashed
Engine version: Godot Engine v4.0.dev.custom_build (7211012c4f1a737a78436a55fd1c66d7e323b668)
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[0] CowData<char32_t>::size (S:\repo\godot-pr-pr\core\templates\cowdata.h:141)
[1] CowData<char32_t>::size (S:\repo\godot-pr-pr\core\templates\cowdata.h:141)
[2] String::size (S:\repo\godot-pr-pr\core\string\ustring.h:218)
[3] String::length (S:\repo\godot-pr-pr\core\string\ustring.h:272)
[4] String::operator== (S:\repo\godot-pr-pr\core\string\ustring.cpp:658)
[5] EditorFileSystemDirectory::find_file_index (S:\repo\godot-pr-pr\editor\editor_file_system.cpp:55)
[6] EditorFileSystem::_update_scan_actions (S:\repo\godot-pr-pr\editor\editor_file_system.cpp:586)
[7] EditorFileSystem::_notification (S:\repo\godot-pr-pr\editor\editor_file_system.cpp:1215)
[8] EditorFileSystem::_notificationv (S:\repo\godot-pr-pr\editor\editor_file_system.h:113)
[9] Object::notification (S:\repo\godot-pr-pr\core\object\object.cpp:843)
[10] SceneTree::_notify_group_pause (S:\repo\godot-pr-pr\scene\main\scene_tree.cpp:857)
[11] SceneTree::process (S:\repo\godot-pr-pr\scene\main\scene_tree.cpp:455)
[12] Main::iteration (S:\repo\godot-pr-pr\main\main.cpp:2649)
[13] ProgressDialog::task_step (S:\repo\godot-pr-pr\editor\progress_dialog.cpp:211)
[14] EditorNode::progress_task_step (S:\repo\godot-pr-pr\editor\editor_node.cpp:4109)
[15] EditorProgress::step (S:\repo\godot-pr-pr\editor\editor_node.h:911)
[16] EditorFileSystem::reimport_files (S:\repo\godot-pr-pr\editor\editor_file_system.cpp:2126)
[17] EditorFileSystem::_update_scan_actions (S:\repo\godot-pr-pr\editor\editor_file_system.cpp:627)
[18] EditorFileSystem::_notification (S:\repo\godot-pr-pr\editor\editor_file_system.cpp:1215)
[19] EditorFileSystem::_notificationv (S:\repo\godot-pr-pr\editor\editor_file_system.h:113)
[20] Object::notification (S:\repo\godot-pr-pr\core\object\object.cpp:843)
[21] SceneTree::_notify_group_pause (S:\repo\godot-pr-pr\scene\main\scene_tree.cpp:857)
[22] SceneTree::process (S:\repo\godot-pr-pr\scene\main\scene_tree.cpp:455)
[23] Main::iteration (S:\repo\godot-pr-pr\main\main.cpp:2649)
[24] ProgressDialog::task_step (S:\repo\godot-pr-pr\editor\progress_dialog.cpp:211)
[25] EditorNode::progress_task_step (S:\repo\godot-pr-pr\editor\editor_node.cpp:4109)
[26] EditorProgress::step (S:\repo\godot-pr-pr\editor\editor_node.h:911)
[27] EditorFileSystem::reimport_files (S:\repo\godot-pr-pr\editor\editor_file_system.cpp:2126)
[28] EditorFileSystem::_update_scan_actions (S:\repo\godot-pr-pr\editor\editor_file_system.cpp:627)
[29] EditorFileSystem::_notification (S:\repo\godot-pr-pr\editor\editor_file_system.cpp:1215)
[30] EditorFileSystem::_notificationv (S:\repo\godot-pr-pr\editor\editor_file_system.h:113)
[31] Object::notification (S:\repo\godot-pr-pr\core\object\object.cpp:843)
[32] SceneTree::_notify_group_pause (S:\repo\godot-pr-pr\scene\main\scene_tree.cpp:857)
[33] SceneTree::process (S:\repo\godot-pr-pr\scene\main\scene_tree.cpp:455)
[34] Main::iteration (S:\repo\godot-pr-pr\main\main.cpp:2649)
[35] OS_Windows::run (S:\repo\godot-pr-pr\platform\windows\os_windows.cpp:643)
[36] widechar_main (S:\repo\godot-pr-pr\platform\windows\godot_windows.cpp:163)
[37] _main (S:\repo\godot-pr-pr\platform\windows\godot_windows.cpp:185)
[38] main (S:\repo\godot-pr-pr\platform\windows\godot_windows.cpp:199)
[39] __scrt_common_main_seh (D:\agent\_work\9\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[40] BaseThreadInitThunk
-- END OF BACKTRACE --

I know someone's going to tell me well if it crashes, don't do that. But the point of this issue is it's never safe to call EditorFileSystem::scan in its current state, since there's some chance it is currently inside a nested Main::iteration call.

However, from what I can tell so far, there is no way for GDScript to know if this Main::iteration() call is reentrant or not. Waiting for the next frame isn't good enough. Waiting 1 second also isn't good enough, since the current import could take longer than that.

The following change works around the issue, but also prevents the ProgressDialog from showing up during all imports. Ideally there would be a better solution.

diff --git a/editor/progress_dialog.cpp b/editor/progress_dialog.cpp
index 95a5646013..8588999c35 100644
--- a/editor/progress_dialog.cpp
+++ b/editor/progress_dialog.cpp
@@ -207,7 +207,7 @@ bool ProgressDialog::task_step(const String &p_task, const String &p_state, int
 		DisplayServer::get_singleton()->process_events();
 	}
 
-	Main::iteration(); // this will not work on a lot of platforms, so it's only meant for the editor
+	// Main::iteration(); // this will not work on a lot of platforms, so it's only meant for the editor
 	return cancelled;
 }

Steps to reproduce

1. Open the attached project.
2. Select Project -> Tools... -> Stress Test Import...
3. If it's stuck on _temp0.png or _temp0_retry.png then click out and back into the window once or twice.

Commenting out the calls to scan() avoids the crash, but require that the user alt-tab in and out of the editor repeatedly to complete the test.

Minimal reproduction project

Updated for 4.0 beta2 (FileAccess change) import_reentrancy4b2.zip
import_reentrancy4.zip

[fire]

@lyuma Did we work around it? How important is this to be solved?

[lyuma]

Had to update the test project for FileAccess. The problem still happens.

@fire In my project, we worked around it using the reimport_files api added in #61004 which is safe if no scan is in progress.
As for how important, it is currently very easy to cause editor crashes by using EditorFileSystem APIs.... so this represents a stability problem for the editor (currently only when scripts are involved).

I think this crash doesn't currently happen in normal editor use because the operating system trigger for window focus and scanning acts as a sort of synchronization point, and it doesn't process operating system events within a reentrant main loop.

See also: PR #54918 which is addressing some part of this issue, but I'm not sure if it's a complete fix.

[akien-mga]

#73214 sounds related, could you see if it solved (or more likely worked around) this issue?

[lyuma]

I was able to reproduce the crash in RC1, but not in RC2 or in my own build (with #73214 applied). So the crash is fixed.

To reproduce, I had to make a couple fixes to MRP. Edited async_importer.gd line 47:

	timer.process_callback = Timer.TIMER_PROCESS_IDLE

I also had to change the if not dres.file_exists(generate_sentinel_png_filename() + ".import"): to not return because it was not detecting the .import file. Commenting out the return here allowed the imports to run without triggering the crash.

So I think the original issue got fixed somehow and can no longer be reproduced.

That said, the files are failing to generate ".import" files and hence failing to import properly, so it's debatable whether or not this is a fix, but they do import successfully if the editor later loses and regains focus. It's certainly better than crashing.

Okay I found that calling EditorFileSystem,update_file(f) after creating that file seemed to make the MRP import all files successfully in RC2.

There seems to be some nuance in this part of the code:

	if not dres.file_exists(generate_sentinel_png_filename() + ".import"):
		print(generate_sentinel_png_filename() + ".import" + " not created yet...")
		editor_filesystem.scan()
		editor_filesystem.update_file(generate_sentinel_png_filename())
		return

Even though the .import file was already created and update_file was called once already, #73214 may cause these to be ignored in some cases if there is already an import in progress.

Putting the scan() after the update_file seems to make it work a bit more often but now it randomly gets stuck on some file or other. In the cases it gets stuck, not calling scan() seems to get it unstuck, but there's no way to know if the race was hit, so I ended up diong:

	if not dres.file_exists(generate_sentinel_png_filename() + ".import"):
		print(generate_sentinel_png_filename() + ".import" + " not created yet...")
		editor_filesystem.update_file(generate_sentinel_png_filename())
		if randf()<0.7:
			editor_filesystem.scan()
		return

and oddly enough with this code, it seems to import successfully every time. My theory is the randomness gives it a way out of the reentrant main loop.

Anyway clearly this is a pathological case. I've since found that calling update_file and reimport_files methodically allows writing reliable import code without relying on scan() and waiting arbitrary time. But it's good to see that writing code in the way the MRP did it no longer crashes the engine.