Mac Editor Crash When Exposing Resource in Inspector from GDExtension

[kromenak]

Godot version

4.2.1.stable

godot-cpp version

godot-4.2.1-stable

System information

macOS 14.4.1 - Vulkan (Mobile) - integrated Apple M3 Max - Apple M3 Max (16 Threads)

Issue description

I am writing some game code in C++ using GDExtension. As part of this, I have a custom manager class (set up as an Autoload) that exposes a custom Resource in the Inspector.

On Mac, every time I recompile the dylib and the editor does the hot reload process, the editor crashes with this stack trace:
CrashReport.txt

The crash report appears to point to destructing a Resource inside my Autoload manager class as the culprit for the crash. In my testing, I found that if this field is set to an object (either on-disk or embedded in the manager scene), I get the crash. If the field is null/empty, I do not get a crash. If I remove the _bind_methods code to expose the field in the Inspector, I do not get a crash.

I see this behavior when compiling on Mac via Xcode or CLion. I haven't yet seen this issue on Windows.

I believe the method I'm using to expose the Resource is correct, but there isn't a huge amount of GDExtension documentation, so I'm not 100% sure. My expectation would be that the current code doesn't crash the editor when hot reloading.

Steps to reproduce

In the provided project, I believe you can reproduce the crash using these steps:

1. Open the provided C++ project (in "Cpp" folder) in either CLion or Xcode on Mac. It is configured using CMake. Build in debug mode.
2. Open the Godot project in the Res folder. Note that there is a "SceneManager.tscn" with a GameSettings Resource that is embedded in the scene directly. Also note that this SceneManager scene is hooked up as an Autoload in the project. You should also see a log output about the Resource.
3. Make a small change to the C++ code and rebuild the library in debug mode.
4. Focus the Godot Editor, which will then try to hot reload the GDExtension's updated dylib. However, the Editor will beach-ball and then crash.

Minimal reproduction project

ResourceBug.zip

[dsnopek]

Thanks for the report!

Unfortunately, I haven't been successful in reproducing this. I'm on Linux (not MacOS), but it seems weird that the issue would be MacOS only.

Above you wrote:

I haven't yet seen this issue on Windows.

Is that because you haven't tried it on Windows? Or, have you tried it on Windows, but the issue wasn't reproducible there?

Looking at the stack trace, it looks like something like this is happening:

1. The SceneManager class is getting unregistered, and so the object on the godot-cpp side is being freed
2. This leads to the Ref<GameSettings> destructor (on the godot-cpp side), and calling into RefCounted::unreference() on the Godot side
3. After two more stack frames on the Godot side, we crash -- it sounds like for accessing an invalid address

I wish you were using a Godot build with debug symbols so we could get a little more information about what Godot was trying to do when it crashed.

[kromenak]

@dsnopek, thanks for taking a look!

I have actually been working on both Mac and Windows, but have not seen the issue on Windows at all.

I've also now seen this issue not only with an Autoload manager class, but also when an object in an open scene has a custom resource on it. For example, I have a level scene that contains treasure chests, and each treasure chest has a custom "Weighted List" embedded resource. On hot reload, this seems to cause the same problem if the level scene is open.

Per your note, I did grab the 4.2 source code, compiled and debug mode, and reproduced the issue. Here's a more complete crash report as a result!
CrashReport_GodotDebug.txt

Also, running in debug mode myself and capturing the crash, here's what I can gather:

1. When deinitializing the extension during the hot reload, the SceneManager object is deinitialized (via call to clear_internal_extension).
2. Though the SceneManager free and destructor appear in the crash report, they do not appear when debugging in CLion. It goes directly from Object::clear_internal_extension to gd_extension_object_method_bind_ptrcall. And at that point, p_instance appears to be null.
3. When I get down to RefCounted::unreference on line 77, "this" (RefCounted*) appears to be null.
4. I suspect the crash happens because refcount.unrefval is called on a null object.

Just as a quick test, it appears that if I add a null check inside gdextension_object_method_bind_ptrcall, I wasn't able to get the crash any longer:

static void gdextension_object_method_bind_ptrcall(GDExtensionMethodBindPtr p_method_bind, GDExtensionObjectPtr p_instance, const GDExtensionConstTypePtr *p_args, GDExtensionTypePtr p_ret) {
	const MethodBind *mb = reinterpret_cast<const MethodBind *>(p_method_bind);
	Object *o = (Object *)p_instance;
	if(o != nullptr) { // null check added here
		mb->ptrcall(o, (const void **)p_args, p_ret);
	}
}

However, I'm not familiar enough with Godot overall to understand whether that is the root cause, or a good solution, or if doing a null check in that function would be undesirable. Perhaps a null check there just masks some other more serious issue.

[dsnopek]

Thanks for all the extra debugging info!

I have theory about what's happening here. During reload, we go class-by-class, unhooking the Godot objects from the godot-cpp objects, and then deleting the godot-cpp objects. I think we're deleting the GameSettings objects before deleting the SceneManager objects, and so when the SceneManager is deleted it's trying to unreference the GameSettings that has already been deleted.

(I suspect the reason this only crashes on MacOS has to do with how it's implementing the SafeNumeric.)

This won't necessarily prove it, but what happens if you change the order of ClassDB::register_class<T>() in your RegisterTypes.cpp? Right now, SceneManager is registered before GameSettings, but what happens if you reverse that?

Anyway, if I'm right, then this is a pretty serious problem with reload. :-/ We don't know the dependencies between the different classes, and can't know the right order to clean them up in. I'm not sure how to handle this.

[kromenak]

Yes, I think you're right - I was able to reproduce the crash very consistently, but reversing the registration order in RegisterTypes.cpp does seem to stop the crash from occurring.

For a workaround, I can simply adjust the ordering to avoid this problem. However, it can get more and more difficult to do this as the project gets more complex.

I think storing dependency data would probably be too difficult to do manually or automatically, especially since those dependencies shift over time based on what variables exist in what classes. Also, it would be a complex global solution to a pretty minor problem that only occurs in development.

One saving grace is that this is only a problem when hot reloading in editor (and only on Mac apparently). If this was an end-user runtime issue, I'd think that an elegant and solid solution is needed. However, for editor and debug libraries, I'd argue that some kind of band-aid fix (like gdextension_object_method_bind_ptrcall performing null checks only in debug mode to avoid this issue when hot reloading) could be an OK workaround.