GDScript: Fix stack manipulation for `await`

The stack now contains three special addresses that should no be copied
to the state, since it contains references that creates cycles. They can
be recreated when the function is resumed.

This commit also removes the clearing of stack from the
GDScriptFunctionState destructor, since it should be cleared when the
function exits. The state stack should only be cleared manually if the
instance is freed before the state resumes (which is already being
done). Otherwise this would destruct the stack twice, causing crashes.

2 files changed, 28 insertions, 32 deletions

diff --git a/modules/gdscript/gdscript_function.cpp b/modules/gdscript/gdscript_function.cpp
index 3d708955ed..a40fc6ca6d 100644
--- a/modules/gdscript/gdscript_function.cpp
+++ b/modules/gdscript/gdscript_function.cpp
@@ -279,7 +279,8 @@ Variant GDScriptFunctionState::resume(const Variant &p_arg) {
 void GDScriptFunctionState::_clear_stack() {
 	if (state.stack_size) {
 		Variant *stack = (Variant *)state.stack.ptr();
-		for (int i = 0; i < state.stack_size; i++) {
+		// The first 3 are special addresses and not copied to the state, so we skip them here.
+		for (int i = 3; i < state.stack_size; i++) {
 			stack[i].~Variant();
 		}
 		state.stack_size = 0;
@@ -300,8 +301,6 @@ GDScriptFunctionState::GDScriptFunctionState() :
 }
 
 GDScriptFunctionState::~GDScriptFunctionState() {
-	_clear_stack();
-
 	{
 		MutexLock lock(GDScriptLanguage::singleton->lock);
 		scripts_list.remove_from_list();
diff --git a/modules/gdscript/gdscript_vm.cpp b/modules/gdscript/gdscript_vm.cpp
index e28dd26c28..4d8655764f 100644
--- a/modules/gdscript/gdscript_vm.cpp
+++ b/modules/gdscript/gdscript_vm.cpp
@@ -546,33 +546,32 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a
 			memnew_placement(&stack[i], Variant);
 		}
 
-		memnew_placement(&stack[ADDR_STACK_NIL], Variant);
-
 		if (_instruction_args_size) {
 			instruction_args = (Variant **)&aptr[sizeof(Variant) * _stack_size];
 		} else {
 			instruction_args = nullptr;
 		}
 
-		if (p_instance) {
-			memnew_placement(&stack[ADDR_STACK_SELF], Variant(p_instance->owner));
-			script = p_instance->script.ptr();
-		} else {
-			memnew_placement(&stack[ADDR_STACK_SELF], Variant);
-			script = _script;
+		for (const KeyValue<int, Variant::Type> &E : temporary_slots) {
+			type_init_function_table[E.value](&stack[E.key]);
 		}
 	}
+
 	if (_ptrcall_args_size) {
 		call_args_ptr = (const void **)alloca(_ptrcall_args_size * sizeof(void *));
 	} else {
 		call_args_ptr = nullptr;
 	}
 
-	memnew_placement(&stack[ADDR_STACK_CLASS], Variant(script));
-
-	for (const KeyValue<int, Variant::Type> &E : temporary_slots) {
-		type_init_function_table[E.value](&stack[E.key]);
+	if (p_instance) {
+		memnew_placement(&stack[ADDR_STACK_SELF], Variant(p_instance->owner));
+		script = p_instance->script.ptr();
+	} else {
+		memnew_placement(&stack[ADDR_STACK_SELF], Variant);
+		script = _script;
 	}
+	memnew_placement(&stack[ADDR_STACK_CLASS], Variant(script));
+	memnew_placement(&stack[ADDR_STACK_NIL], Variant);
 
 	String err_text;
 
@@ -2171,8 +2170,7 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a
 						// Is this even possible to be null at this point?
 						if (obj) {
 							if (obj->is_class_ptr(GDScriptFunctionState::get_class_ptr_static())) {
-								static StringName completed = _scs_create("completed");
-								result = Signal(obj, completed);
+								result = Signal(obj, "completed");
 							}
 						}
 					}
@@ -2193,8 +2191,9 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a
 					gdfs->function = this;
 
 					gdfs->state.stack.resize(alloca_size);
-					//copy variant stack
-					for (int i = 0; i < _stack_size; i++) {
+
+					// First 3 stack addresses are special, so we just skip them here.
+					for (int i = 3; i < _stack_size; i++) {
 						memnew_placement(&gdfs->state.stack.write[sizeof(Variant) * i], Variant(stack[i]));
 					}
 					gdfs->state.stack_size = _stack_size;
@@ -3451,26 +3450,24 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a
 		GDScriptLanguage::get_singleton()->script_frame_time += time_taken - function_call_time;
 	}
 
-	// Check if this is the last time the function is resuming from await
-	// Will be true if never awaited as well
-	// When it's the last resume it will postpone the exit from stack,
-	// so the debugger knows which function triggered the resume of the next function (if any)
-	if (!p_state || awaited) {
+	// Check if this function has been interrupted by `await`.
+	// If that is the case we want to keep it in the debugger until it actually exits.
+	// This ensures the call stack can be properly shown when using `await`, showing what resumed the function.
+	if (!awaited) {
 		if (EngineDebugger::is_active()) {
 			GDScriptLanguage::get_singleton()->exit_function();
 		}
+	}
 #endif
 
-		if (_stack_size) {
-			//free stack
-			for (int i = 0; i < _stack_size; i++) {
-				stack[i].~Variant();
-			}
+	// Clear the stack even if there was an `await`.
+	// The stack saved in the state is a copy, so this needs to be destructed to avoid leaks.
+	if (_stack_size) {
+		// Free stack.
+		for (int i = 0; i < _stack_size; i++) {
+			stack[i].~Variant();
 		}
-
-#ifdef DEBUG_ENABLED
 	}
-#endif
 
 	return retvalue;
 }