ThorVG regression causing potentially corrupted SVGs

[EIREXE]

Tested versions

• Master (b4e2a24)

System information

Arch Linux

Issue description

When rendering SVGs using ImageLoaderSVG::create_image_from_utf8_buffer (I'm doing it from C++, but I can't see why it wouldn't affect godot proper too) the rendered images sometimes come out corrupted, an example:

I believe it may be a ThorVG bug, since if I downgrade to thorvg 0.12.1 it works fine, but the issue is introduced in 0.12.2

Steps to reproduce

• Delete .godot folder
• open the project
• svgs may be corrupt, or not, since it's probably some kind of data race it might be unreliable, but at least one of the svgs comes out corrupted for me

Minimal reproduction project (MRP)

mrp.zip

[capnm]

Steps to reproduce

* Delete .godot folder

* open the project

* svgs may be corrupt, or not, since it's probably some kind of data race it might be unreliable, but at least one of the svgs comes out corrupted for me

Done this and rescaled many times to different sizes, can't reproduce (Debian 12, Ubuntu...).
You could perhaps post some reproducible C++ mrp.

[EIREXE]

Steps to reproduce

* Delete .godot folder

* open the project

* svgs may be corrupt, or not, since it's probably some kind of data race it might be unreliable, but at least one of the svgs comes out corrupted for me

Done this and rescaled many times to different sizes, can't reproduce (Debian 12, Ubuntu...). You could perhaps post some reproducible C++ mrp.

How strange, I'm just using the editor as normal, I made sure that my c++ code wasn't the culprit 🤔

I'll try a few things and let you know

[capnm]

• Segmentation fault upon custom resource re-import after removing .godot #84364 ?
• A large collection of svg images for testing: svg.zip
• Try to use a thread sanitizer, see the scons compile options in CI Linux / Editor with * sanitizers in Actions GHA: https://github.com/godotengine/godot/actions .

[Rubonnek]

I'm able to reproduce this issue and disabling multiple threads for the import process will correctly import the SVGs. The issue is not ThorVG (I think) but a race condition in the import process. Like capnm previously mentioned, this issue ties back to #84364.

[capnm]

I'm able to reproduce this issue and disabling multiple threads for the import process will correctly import the SVGs. The issue is not ThorVG but a race condition in the import process. Like capnm previously mentioned, this issue ties back to #84364.

Since I am not able to reproduce the problem myself, could you try to find a latest version of Godot4 where this issue does not occur? Thank you!

[akien-mga]

Might be the same bug as #87788 which was bisected as caused by #87612. That update has threading changes in ThorVG.

[capnm]

Might be the same bug as #87788 which was bisected as caused by #87612. That update has threading changes in ThorVG.

Read the last comment: "I'm able to reproduce the crash but I don't think it's related to ThorVG -- the crash happens at different places and due to double frees or some memory corruption. The root cause is most likely a race condition."

[akien-mga]

Well the fact is that reverting #87612 appears to solve the bug according to @RevoluPowered. So it is related to the ThorVG change, even if the actual crash doesn't happen in ThorVG. That change is triggering something which may be a pre-existing Godot issue but understanding why ThorVG's new threading changes trigger it would be helpful.

[Rubonnek]

could you try to find a latest version of Godot4 where this issue does not occur?

I can try but it may be hard for me to pinpoint the version -- it was likely introduced before 4.1.1 and some versions before that Godot didn't work on my system for some reason. I'll try to bisect later.

For now I finally got ThreadSanitizer to work on my system -- here's the log that includes the thread issues it detected during the import process of the SVGs in this issue.

Note that the first data race it detects is in EditorFileSystem::_reimport_thread:

SUMMARY: ThreadSanitizer: data race /opt/godot/editor/editor_file_system.cpp:2301:93 in EditorFileSystem::_reimport_thread(unsigned int, EditorFileSystem::ImportThreadData*)

I didn't check everything, but most, if not all, the other data races ThreadSanitizer detected have EditorFileSystem::_reimport_thread in their stack. I suppose that's the root cause that ties #84364, #83520, #87788, and this issue together but I need to test to confirm.

The thread sanitizer log was created with a build from 63d6bda and by running the following at the project path:

TSAN_OPTIONS=second_deadlock_stack=1 godot.linuxbsd.editor.dev.x86_64.llvm.san -e                                                      

The output is very verbose and it generates about 598MB of text before reaching the import process. I'll be opening a new issue to cover those other issues detected by ThreadSanitizer.

[hermet]

Hello, here are my idea.

1. Strange but currently, ThorVG should not initiate any threads nor use any of it in Godot, as confirmed upon inspection. The 'THORVG_THREAD_SUPPORT' define has not been enabled, which means it should operate on the nested thread(or main thread?) as intended. It appears that this option-flag has been introduced in the course of ThorVG updates, Godot could re-enable threading support as it previously did by adding the following: It is worth trying to see the results.

diff --git a/thirdparty/thorvg/inc/config.h b/thirdparty/thorvg/inc/config.h
index e50971e297..05b0275e66 100644
--- a/thirdparty/thorvg/inc/config.h
+++ b/thirdparty/thorvg/inc/config.h
@@ -5,6 +5,7 @@
#define THORVG_SVG_LOADER_SUPPORT
#define THORVG_PNG_LOADER_SUPPORT
#define THORVG_JPG_LOADER_SUPPORT
+#define THORVG_THREAD_SUPPORT

// For internal debugging:
//#define THORVG_LOG_ENABLED

2. However, upon reviewing the ThorVG implementation in 'modules/svg/image_loader_svg.cpp', I noticed that it initiates a synchronous call immediately. Actually, it might not significantly benefit from threading. In this scenario, it may be advisable to disable threading.

3. If we can identify specific resources related to this issue for debugging, it would be helpful. Currently ThorVG doesn't encounter any threading issues according to the thread sanitizer.

Thanks.

[akien-mga]

I can reproduce the issue on Linux, and as @hermet identified, enabling THORVG_THREAD_SUPPORT fixes it.

The changes in #87612 indeed made it opt-in via a define, when it used to be always enabled. So we need to make sure to set that define (that's good to keep in mind for future updates @capnm, when new defines are added upstream we need to make sure to evaluate if we need them too).

@adamscott This might be useful for the no-threads web build btw. For now I'll make a PR restoring the previous behavior where threads are always enabled, but this would be worth looking into further for web, and assing points 2 from @hermet's comment.

[adamscott]

@adamscott This might be useful for the no-threads web build btw. For now I'll make a PR restoring the previous behavior where threads are always enabled, but this would be worth looking into further for web, and assing points 2 from @hermet's comment.

@akien-mga Threads are already disabled for the no-threads builds already.

godot/modules/svg/register_types.cpp

Lines 37 to 54 in 36e943b

	#ifdef THREADS_ENABLED
	#define TVG_THREADS 1
	#else
	#define TVG_THREADS 0
	#endif
	static Ref<ImageLoaderSVG> image_loader_svg;
	void initialize_svg_module(ModuleInitializationLevel p_level) {
	if (p_level != MODULE_INITIALIZATION_LEVEL_SCENE) {
	return;
	}
	tvg::CanvasEngine tvgEngine = tvg::CanvasEngine::Sw;
	if (tvg::Initializer::init(tvgEngine, TVG_THREADS) != tvg::Result::Success) {
	return;
	}

I imagine that we could just turn off the threads for every build.

[capnm]

I imagine that we could just turn off the threads for every build.

Somebody needs first to fix the real issue described by @Rubonnek that it triggers.