Godot 3.2.4rc3 mono crashes on exit with specific combination of packed scene and dynamic font #46679
Comments
|
Thanks for the detailed bug report! I can't reproduce the crash on Linux, but it might be related to #46411. Can you confirm if it's a regression in 3.2.4 RC 3 or do you also experience the issue with previous releases? |
|
If it's a regression in 3.2.4 RC 3, could you try those two builds to confirm if the "before-pr45618" one fixes the issue? That's just a hunch, a proper Git bisect would be better but time consuming. |
|
Might actually be the same issue as #46550 which also happens on Ubuntu, so might not be macOS-specific. I still can't reproduce either issue on Mageia 8 or Ubuntu 20.04 myself :( |
I can confirm that this bug happens in the pr45618 build but not the before-pr45618 build. |
|
Thanks, that confirms that it's a regression from #45618. I see that there's some changes to |
|
I can confirm the crash on macOS. I do not see anything related to Stack trace: It crashes at the Edit: crash at editor exit, with empty Mono project opened. |
|
With the test project from #46550 (comment) it crashes on Again OpenGL call from the thread. |
|
Just FYI, error does not occur on Windows 10 on 3.23 stable or 3.24 beta6, 3.2.4rc2 or 3.2.4. rc3 |
|
For the reference in the pre-#45618 build, |
|
Probably reproducibility depends on OpenGL driver implementation, not the platform. |
|
The commit causing the issue is It's not caused by difference between |
|
It probably has something to do with Edit: Yep, it's ID collision between Mono and Godot threads. Mono thread causing crash do have the same it as the main thread (which is also server thread). godot/servers/server_wrap_mt_common.h Line 58 in 36bec66 This check fails, and it's calling OpenGL API in the thread, instead of pushing it to command query. |
|
Indeed, see #46411 (comment) where @neikeq has come to the same conclusion (he's preparing a fix). |
In case it's useful, here's my patch, which fixes crash on macOS (I have not tested it on any other platforms). diff --git a/core/os/thread.cpp b/core/os/thread.cpp
index 58b29b9802..61cc31e154 100644
--- a/core/os/thread.cpp
+++ b/core/os/thread.cpp
@@ -41,9 +41,13 @@ void (*Thread::set_priority_func)(Thread::Priority) = nullptr;
void (*Thread::init_func)() = nullptr;
void (*Thread::term_func)() = nullptr;
-Thread::ID Thread::main_thread_id = 1;
-SafeNumeric<Thread::ID> Thread::last_thread_id{ 1 };
-thread_local Thread::ID Thread::caller_id = 1;
+uint64_t _thread_id_hash(const std::thread::id &p_t) {
+ static std::hash<std::thread::id> hasher;
+ return hasher(p_t);
+}
+
+Thread::ID Thread::main_thread_id = _thread_id_hash(std::this_thread::get_id());
+thread_local Thread::ID Thread::caller_id = _thread_id_hash(std::this_thread::get_id());
void Thread::_set_platform_funcs(
Error (*p_set_name_func)(const String &),
@@ -57,7 +61,7 @@ void Thread::_set_platform_funcs(
}
void Thread::callback(Thread *p_self, const Settings &p_settings, Callback p_callback, void *p_userdata) {
- Thread::caller_id = p_self->id;
+ Thread::caller_id = _thread_id_hash(p_self->thread.get_id());
if (set_priority_func) {
set_priority_func(p_settings.priority);
}
@@ -73,7 +77,7 @@ void Thread::callback(Thread *p_self, const Settings &p_settings, Callback p_cal
}
void Thread::start(Thread::Callback p_callback, void *p_user, const Settings &p_settings) {
- if (id != 0) {
+ if (id != _thread_id_hash(std::thread::id())) {
#ifdef DEBUG_ENABLED
WARN_PRINT("A Thread object has been re-started without wait_to_finish() having been called on it. Please do so to ensure correct cleanup of the thread.");
#endif
@@ -81,22 +85,22 @@ void Thread::start(Thread::Callback p_callback, void *p_user, const Settings &p_
std::thread empty_thread;
thread.swap(empty_thread);
}
- id = last_thread_id.increment();
std::thread new_thread(&Thread::callback, this, p_settings, p_callback, p_user);
thread.swap(new_thread);
+ id = _thread_id_hash(thread.get_id());
}
bool Thread::is_started() const {
- return id != 0;
+ return id != _thread_id_hash(std::thread::id());
}
void Thread::wait_to_finish() {
- if (id != 0) {
+ if (id != _thread_id_hash(std::thread::id())) {
ERR_FAIL_COND_MSG(id == get_caller_id(), "A Thread can't wait for itself to finish.");
thread.join();
std::thread empty_thread;
thread.swap(empty_thread);
- id = 0;
+ id = _thread_id_hash(std::thread::id());
}
}
@@ -109,10 +113,10 @@ Error Thread::set_name(const String &p_name) {
}
Thread::Thread() :
- id(0) {}
+ id(_thread_id_hash(std::thread::id())) {}
Thread::~Thread() {
- if (id != 0) {
+ if (id != _thread_id_hash(std::thread::id())) {
#ifdef DEBUG_ENABLED
WARN_PRINT("A Thread object has been destroyed without wait_to_finish() having been called on it. Please do so to ensure correct cleanup of the thread.");
#endif
diff --git a/core/os/thread.h b/core/os/thread.h
index 837f9323c3..38763e2ebc 100644
--- a/core/os/thread.h
+++ b/core/os/thread.h
@@ -62,7 +62,6 @@ private:
friend class Main;
static ID main_thread_id;
- static SafeNumeric<ID> last_thread_id;
ID id;
static thread_local ID caller_id;Edit: Added default |
|
Can confirm. Patch is working under windows to. Needs to be implement to rc4 |
|
@neikeq is also working on it. The only (minor) downside of this approach is that we lose the human-friendly, serial thread ids, but I don't think that will really be a problem in practice. If at some point the debugger lets you see the list of threads so the serial numbering is desired, we can always revisit that. Getting the multiple issues related to this fixed is the top priority now, to unblock the release. That said, let's see what we comes up with. |
|
@RandomShaper My idea was something like this: struct ThreadID {
ID id;
bool assigned = false;
};
static ThreadID ID caller_id;
static ID get_caller_id() { if (!caller_id.assigned) { assign_id(); } return caller_id.id; }Godot threads should already have an id pre-assigned in This solution has the benefit of human-friendly ids. However, maybe your solution is better for performance as there's no if condition in |
|
The only thing I can say is that performance beats human-friendly ids, so, if I had to choose, I'd go @bruvzg's way. |
Here's the same patch as PR - #46742 |
I meant the Godot debugger, if it some day it gets features to debug GDScript threads. But, again, that's not relevant now. If that happens at some point, we will just decide how to deal with it then. |
For Godot debugger we can add |
Godot version:
v3.2.4.rc3.mono.official
OS/device including version:
MacOS 11.2.2
OpenGL ES 3.0 Renderer: Intel(R) Iris(TM) Plus Graphics 650
OpenGL ES Batching: ON
Issue description:
Sorry the title is such a mess. I couldn't figure out a way to summarize this succinctly.
Godot segfaults on exit if the following sequence occurs:
PackedScenevariableDynamicFontGodot will now segfault when quitting producing a traceback such as the following.TRACEBACK.txt
Concretely if I have a button with this script
and the scene
Othercontains a dynamic font, then the game crashes on exit if the scene Other has been loaded.Steps to reproduce:
The conditions for this are somewhat specific but I've been able to reproduce them reliably with the following.
AScene.tscnAt this point the game crashes on exit and dumps a stacktrace.
Note that no crash occurs if:
DynamicFontMinimal reproduction project:
SampleCrash.zip
The text was updated successfully, but these errors were encountered: