Open kromenak opened 2 months ago
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:
SceneManager
class is getting unregistered, and so the object on the godot-cpp side is being freedRef<GameSettings>
destructor (on the godot-cpp side), and calling into RefCounted::unreference()
on the Godot sideI 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.
@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:
Object::clear_internal_extension
to gd_extension_object_method_bind_ptrcall
. And at that point, p_instance appears to be null.RefCounted::unreference
on line 77, "this" (RefCounted*) appears to be null.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.
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.
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.
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:
Minimal reproduction project
ResourceBug.zip