Cleanup function state connections when destroying instance

[KoBeWi]

Closes #24311

It can't be that simple, can it? 🤔

[KoBeWi]

I rebased and the code is not valid anymore :/
It works, but sanitizer is not happy about it.

[adamscott]

I suspect that the same GDScriptInstance::~GDScriptInstance() is ran twice. Try adding a destructing variable, like I did in GDScript::~GDScript().

[anvilfolk]

I suspect that the same GDScriptInstance::~GDScriptInstance() is ran twice. Try adding a destructing variable, like I did in GDScript::~GDScript().

Curious to know how that would happen. I figured destructors would only ever run once!

[anvilfolk]

I just tested adding destructing to GDScriptInstance but the issues persist unfortunately.

[adamscott]

This patch makes the build pass the sanitizer.

diff --git a/modules/gdscript/gdscript.cpp b/modules/gdscript/gdscript.cpp
index 72d5bb23c5..dc87560fdd 100644
--- a/modules/gdscript/gdscript.cpp
+++ b/modules/gdscript/gdscript.cpp
@@ -1925,10 +1925,14 @@ GDScriptInstance::~GDScriptInstance() {
 
 	while (SelfList<GDScriptFunctionState> *E = pending_func_states.first()) {
 		// Order matters since clearing the stack may already cause
-		// the GDSCriptFunctionState to be destroyed and thus removed from the list.
+		// the GDScriptFunctionState to be destroyed and thus removed from the list.
 		pending_func_states.remove(E);
-		E->self()->_clear_connections();
-		E->self()->_clear_stack();
+		GDScriptFunctionState *state = E->self();
+		state->_clear_connections();
+		state = Object::cast_to<GDScriptFunctionState>(state);
+		if (state != nullptr) {
+			state->_clear_stack();
+		}
 	}
 
 	if (script.is_valid() && owner) {

[adamscott]

Weird. My local build pass the clang test sanitizers.

[adamscott]

My hunch says that this is more sane than casting AND it should fix the macos SIGSEGV.

diff --git a/modules/gdscript/gdscript.cpp b/modules/gdscript/gdscript.cpp
index df57749f76..fc3d42b1b0 100644
--- a/modules/gdscript/gdscript.cpp
+++ b/modules/gdscript/gdscript.cpp
@@ -1484,9 +1484,9 @@ GDScript::~GDScript() {
 			// the GDScriptFunctionState to be destroyed and thus removed from the list.
 			pending_func_states.remove(E);
 			GDScriptFunctionState *state = E->self();
+			ObjectID state_id = state->get_instance_id();
 			state->_clear_connections();
-			state = Object::cast_to<GDScriptFunctionState>(state);
-			if (state != nullptr) {
+			if (ObjectDB::get_instance(state_id) != nullptr) {
 				state->_clear_stack();
 			}
 		}
@@ -1932,9 +1932,9 @@ GDScriptInstance::~GDScriptInstance() {
 		// the GDSCriptFunctionState to be destroyed and thus removed from the list.
 		pending_func_states.remove(E);
 		GDScriptFunctionState *state = E->self();
+		ObjectID state_id = state->get_instance_id();
 		state->_clear_connections();
-		state = Object::cast_to<GDScriptFunctionState>(state);
-		if (state != nullptr) {
+		if (ObjectDB::get_instance(state_id) != nullptr) {
 			state->_clear_stack();
 		}
 	}

[adamscott]

Let's merge this!

[vnen]

I think this is okay.

[akien-mga]

Thanks!