Merge pull request #38482 from RandomShaper/improve_yield_3.2

Fix object leaks caused by unfulfilled yields (3.2)

core/self_list.h

@@ -120,6 +120,9 @@ private:
 
 public:
 	_FORCE_INLINE_ bool in_list() const { return _root; }
+	_FORCE_INLINE_ void remove_from_list() {
+		if (_root) _root->remove(this);
+	}
 	_FORCE_INLINE_ SelfList<T> *next() { return _next; }
 	_FORCE_INLINE_ SelfList<T> *prev() { return _prev; }
 	_FORCE_INLINE_ const SelfList<T> *next() const { return _next; }

modules/gdscript/gdscript.cpp

@@ -947,6 +947,18 @@ void GDScript::_save_orphaned_subclasses() {
 }
 
 GDScript::~GDScript() {
+
+	if (GDScriptLanguage::get_singleton()->lock) {
+		GDScriptLanguage::get_singleton()->lock->lock();
+	}
+	while (SelfList<GDScriptFunctionState> *E = pending_func_states.first()) {
+		E->self()->_clear_stack();
+		pending_func_states.remove(E);
+	}
+	if (GDScriptLanguage::get_singleton()->lock) {
+		GDScriptLanguage::get_singleton()->lock->unlock();
+	}
+
 	for (Map<StringName, GDScriptFunction *>::Element *E = member_functions.front(); E; E = E->next()) {
 		memdelete(E->get());
 	}
@@ -1364,16 +1376,22 @@ GDScriptInstance::GDScriptInstance() {
 }
 
 GDScriptInstance::~GDScriptInstance() {
-	if (script.is_valid() && owner) {
 #ifndef NO_THREADS
-		GDScriptLanguage::singleton->lock->lock();
+	GDScriptLanguage::singleton->lock->lock();
 #endif
 
+	while (SelfList<GDScriptFunctionState> *E = pending_func_states.first()) {
+		E->self()->_clear_stack();
+		pending_func_states.remove(E);
+	}
+
+	if (script.is_valid() && owner) {
 		script->instances.erase(owner);
+	}
+
 #ifndef NO_THREADS
-		GDScriptLanguage::singleton->lock->unlock();
+	GDScriptLanguage::singleton->lock->unlock();
 #endif
-	}
 }
 
 /************* SCRIPT LANGUAGE **************/

modules/gdscript/gdscript.h

@@ -114,6 +114,8 @@ class GDScript : public Script {
 	String fully_qualified_name;
 	SelfList<GDScript> script_list;
 
+	SelfList<GDScriptFunctionState>::List pending_func_states;
+
 	GDScriptInstance *_create_instance(const Variant **p_args, int p_argcount, Object *p_owner, bool p_isref, Variant::CallError &r_error);
 
 	void _set_subclass_path(Ref<GDScript> &p_sc, const String &p_path);
@@ -235,6 +237,8 @@ class GDScriptInstance : public ScriptInstance {
 	Vector<Variant> members;
 	bool base_ref;
 
+	SelfList<GDScriptFunctionState>::List pending_func_states;
+
 	void _ml_call_reversed(GDScript *sptr, const StringName &p_method, const Variant **p_args, int p_argcount);
 
 public:
@@ -319,6 +323,8 @@ struct GDScriptWarning {
 
 class GDScriptLanguage : public ScriptLanguage {
 
+	friend class GDScriptFunctionState;
+
 	static GDScriptLanguage *singleton;
 
 	Variant *_global_array;

modules/gdscript/gdscript_function.cpp

@@ -1274,12 +1274,24 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a
 				gdfs->state.ip = ip + ipofs;
 				gdfs->state.line = line;
 				gdfs->state.script = _script;
-				gdfs->state.script_id = _script->get_instance_id();
+#ifndef NO_THREADS
+				GDScriptLanguage::singleton->lock->lock();
+#endif
+
+				_script->pending_func_states.add(&gdfs->scripts_list);
+				if (p_instance) {
+					gdfs->state.instance = p_instance;
+					p_instance->pending_func_states.add(&gdfs->instances_list);
+				} else {
+					gdfs->state.instance = NULL;
+				}
+#ifndef NO_THREADS
+				GDScriptLanguage::singleton->lock->unlock();
+#endif
 #ifdef DEBUG_ENABLED
+				gdfs->state.function_name = name;
 				gdfs->state.script_path = _script->get_path();
 #endif
-				gdfs->state.instance = p_instance;
-				gdfs->state.instance_id = (p_instance && p_instance->get_owner()) ? p_instance->get_owner()->get_instance_id() : 0;
 				//gdfs->state.result_pos=ip+ipofs-1;
 				gdfs->state.defarg = defarg;
 				gdfs->function = this;
@@ -1832,16 +1844,16 @@ bool GDScriptFunctionState::is_valid(bool p_extended_check) const {
 		return false;
 
 	if (p_extended_check) {
-		if (state.instance_id) {
-			// Class instance gone? (Otherwise script is valid for sure, because the instance has a ref to the script)
-			if (!ObjectDB::get_instance(state.instance_id)) {
-				return false;
-			}
-		} else {
-			// Script gone? (Static method, so there's no instance whose ref to the script can ensure it's valid)
-			if (!ObjectDB::get_instance(state.script_id)) {
-				return false;
-			}
+#ifndef NO_THREADS
+		MutexLock lock(GDScriptLanguage::get_singleton()->lock);
+#endif
+		// Script gone?
+		if (!scripts_list.in_list()) {
+			return false;
+		}
+		// Class instance gone? (if not static function)
+		if (state.instance && !instances_list.in_list()) {
+			return false;
 		}
 	}
 
@@ -1851,22 +1863,27 @@ bool GDScriptFunctionState::is_valid(bool p_extended_check) const {
 Variant GDScriptFunctionState::resume(const Variant &p_arg) {
 
 	ERR_FAIL_COND_V(!function, Variant());
-	if (state.instance_id) {
-		if (!ObjectDB::get_instance(state.instance_id)) {
+	{
+#ifndef NO_THREADS
+		MutexLock lock(GDScriptLanguage::singleton->lock);
+#endif
+		if (!scripts_list.in_list()) {
 #ifdef DEBUG_ENABLED
-			ERR_FAIL_V_MSG(Variant(), "Resumed function '" + String(function->get_name()) + "()' after yield, but class instance is gone. At script: " + state.script_path + ":" + itos(state.line));
+			ERR_FAIL_V_MSG(Variant(), "Resumed function '" + state.function_name + "()' after yield, but script is gone. At script: " + state.script_path + ":" + itos(state.line));
 #else
 			return Variant();
 #endif
 		}
-	} else {
-		if (!ObjectDB::get_instance(state.script_id)) {
+		if (state.instance && !instances_list.in_list()) {
 #ifdef DEBUG_ENABLED
-			ERR_FAIL_V_MSG(Variant(), "Resumed function '" + String(function->get_name()) + "()' after yield, but script is gone. At script: " + state.script_path + ":" + itos(state.line));
+			ERR_FAIL_V_MSG(Variant(), "Resumed function '" + state.function_name + "()' after yield, but class instance is gone. At script: " + state.script_path + ":" + itos(state.line));
 #else
 			return Variant();
 #endif
 		}
+		// Do these now to avoid locking again after the call
+		scripts_list.remove_from_list();
+		instances_list.remove_from_list();
 	}
 
 	state.result = p_arg;
@@ -1889,6 +1906,8 @@ Variant GDScriptFunctionState::resume(const Variant &p_arg) {
 	state.result = Variant();
 
 	if (completed) {
+		_clear_stack();
+
 		if (first_state.is_valid()) {
 			first_state->emit_signal("completed", ret);
 		} else {
@@ -1898,18 +1917,22 @@ Variant GDScriptFunctionState::resume(const Variant &p_arg) {
 #ifdef DEBUG_ENABLED
 		if (ScriptDebugger::get_singleton())
 			GDScriptLanguage::get_singleton()->exit_function();
-		if (state.stack_size) {
-			//free stack
-			Variant *stack = (Variant *)state.stack.ptr();
-			for (int i = 0; i < state.stack_size; i++)
-				stack[i].~Variant();
-		}
 #endif
 	}
 
 	return ret;
 }
 
+void GDScriptFunctionState::_clear_stack() {
+
+	if (state.stack_size) {
+		Variant *stack = (Variant *)state.stack.ptr();
+		for (int i = 0; i < state.stack_size; i++)
+			stack[i].~Variant();
+		state.stack_size = 0;
+	}
+}
+
 void GDScriptFunctionState::_bind_methods() {
 
 	ClassDB::bind_method(D_METHOD("resume", "arg"), &GDScriptFunctionState::resume, DEFVAL(Variant()));
@@ -1919,18 +1942,22 @@ void GDScriptFunctionState::_bind_methods() {
 	ADD_SIGNAL(MethodInfo("completed", PropertyInfo(Variant::NIL, "result", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_NIL_IS_VARIANT)));
 }
 
-GDScriptFunctionState::GDScriptFunctionState() {
+GDScriptFunctionState::GDScriptFunctionState() :
+		scripts_list(this),
+		instances_list(this) {
 
 	function = NULL;
 }
 
 GDScriptFunctionState::~GDScriptFunctionState() {
 
-	if (function != NULL) {
-		//never called, deinitialize stack
-		for (int i = 0; i < state.stack_size; i++) {
-			Variant *v = (Variant *)&state.stack[sizeof(Variant) * i];
-			v->~Variant();
-		}
-	}
+	_clear_stack();
+#ifndef NO_THREADS
+	GDScriptLanguage::singleton->lock->lock();
+#endif
+	scripts_list.remove_from_list();
+	instances_list.remove_from_list();
+#ifndef NO_THREADS
+	GDScriptLanguage::singleton->lock->unlock();
+#endif
 }

modules/gdscript/gdscript_function.h

@@ -294,12 +294,11 @@ public:
 	struct CallState {
 
 		GDScript *script;
-		ObjectID script_id;
+		GDScriptInstance *instance;
 #ifdef DEBUG_ENABLED
+		StringName function_name;
 		String script_path;
 #endif
-		GDScriptInstance *instance;
-		ObjectID instance_id;
 		Vector<uint8_t> stack;
 		int stack_size;
 		Variant self;
@@ -359,12 +358,18 @@ class GDScriptFunctionState : public Reference {
 	Variant _signal_callback(const Variant **p_args, int p_argcount, Variant::CallError &r_error);
 	Ref<GDScriptFunctionState> first_state;
 
+	SelfList<GDScriptFunctionState> scripts_list;
+	SelfList<GDScriptFunctionState> instances_list;
+
 protected:
 	static void _bind_methods();
 
 public:
 	bool is_valid(bool p_extended_check = false) const;
 	Variant resume(const Variant &p_arg = Variant());
+
+	void _clear_stack();
+
 	GDScriptFunctionState();
 	~GDScriptFunctionState();
 };