Fix Container sorting not working when overriding _sort_children in gdscript

Remove _sort_children from script bindings: _sort_children is an internal method which shouldn't be exposed to scripts. Added support for non-bound methods in MessageQueue: So we can use deferred calls without exposing internal methods to scripts. Added debug checks in CallableCustomMethodPointer: Adding method pointer callables to the message queue was causing crashes in case an object was destroyed and the same memory was allocated for another one. The new object had a valid object id but the call was erroneous. Release will be fixed later, along with Variant which has the same problem and is also fixed for debug only.

Fix Container sorting not working when overriding _sort_children in gdscript
3ad69401 · PouleyKetchoupp · d999071e · 3ad69401 · 3ad69401 · 3ad69401
Commit 3ad69401 authored May 01, 2020 by PouleyKetchoupp
Showing with 51 additions and 5 deletions

callable_method_pointer.h core/callable_method_pointer.h +34 -2

message_queue.cpp core/message_queue.cpp +15 -0

message_queue.h core/message_queue.h +1 -0

container.cpp scene/gui/container.cpp +1 -3

No files found.
--- a/core/callable_method_pointer.h
+++ b/core/callable_method_pointer.h
@@ -161,19 +161,35 @@ template <class T, class... P>
 class CallableCustomMethodPointer : public CallableCustomMethodPointerBase {
 	struct Data {
 		T *instance;
+#ifdef DEBUG_ENABLED
+		uint64_t object_id;
+#endif
 		void (T::*method)(P...);
 	} data;

 public:
-	virtual ObjectID get_object() const { return data.instance->get_instance_id(); }
+	virtual ObjectID get_object() const {
+#ifdef DEBUG_ENABLED
+		if (ObjectDB::get_instance(ObjectID(data.object_id)) == nullptr) {
+			return ObjectID();
+		}
+#endif
+		return data.instance->get_instance_id();
+	}

 	virtual void call(const Variant **p_arguments, int p_argcount, Variant &r_return_value, Callable::CallError &r_call_error) const {
+#ifdef DEBUG_ENABLED
+		ERR_FAIL_COND_MSG(ObjectDB::get_instance(ObjectID(data.object_id)) == nullptr, "Invalid Object id '" + uitos(data.object_id) + "', can't call method.");
+#endif
 		call_with_variant_args(data.instance, data.method, p_arguments, p_argcount, r_call_error);
 	}

 	CallableCustomMethodPointer(T *p_instance, void (T::*p_method)(P...)) {
 		zeromem(&data, sizeof(Data)); // Clear beforehand, may have padding bytes.
 		data.instance = p_instance;
+#ifdef DEBUG_ENABLED
+		data.object_id = p_instance->get_instance_id();
+#endif
 		data.method = p_method;
 		_setup((uint32_t *)&data, sizeof(Data));
 	}
@@ -242,20 +258,36 @@ template <class T, class R, class... P>
 class CallableCustomMethodPointerRet : public CallableCustomMethodPointerBase {
 	struct Data {
 		T *instance;
+#ifdef DEBUG_ENABLED
+		uint64_t object_id;
+#endif
 		R(T::*method)
 		(P...);
 	} data;

 public:
-	virtual ObjectID get_object() const { return data.instance->get_instance_id(); }
+	virtual ObjectID get_object() const {
+#ifdef DEBUG_ENABLED
+		if (ObjectDB::get_instance(ObjectID(data.object_id)) == nullptr) {
+			return ObjectID();
+		}
+#endif
+		return data.instance->get_instance_id();
+	}

 	virtual void call(const Variant **p_arguments, int p_argcount, Variant &r_return_value, Callable::CallError &r_call_error) const {
+#ifdef DEBUG_ENABLED
+		ERR_FAIL_COND_MSG(ObjectDB::get_instance(ObjectID(data.object_id)) == nullptr, "Invalid Object id '" + uitos(data.object_id) + "', can't call method.");
+#endif
 		call_with_variant_args_ret(data.instance, data.method, p_arguments, p_argcount, r_return_value, r_call_error);
 	}

 	CallableCustomMethodPointerRet(T *p_instance, R (T::*p_method)(P...)) {
 		zeromem(&data, sizeof(Data)); // Clear beforehand, may have padding bytes.
 		data.instance = p_instance;
+#ifdef DEBUG_ENABLED
+		data.object_id = p_instance->get_instance_id();
+#endif
 		data.method = p_method;
 		_setup((uint32_t *)&data, sizeof(Data));
 	}

--- a/core/message_queue.cpp
+++ b/core/message_queue.cpp
@@ -155,6 +155,21 @@ Error MessageQueue::push_callable(const Callable &p_callable, const Variant **p_
 	return OK;
 }

+Error MessageQueue::push_callable(const Callable &p_callable, VARIANT_ARG_DECLARE) {
+	VARIANT_ARGPTRS;
+
+	int argc = 0;
+
+	for (int i = 0; i < VARIANT_ARG_MAX; i++) {
+		if (argptr[i]->get_type() == Variant::NIL) {
+			break;
+		}
+		argc++;
+	}
+
+	return push_callable(p_callable, argptr, argc);
+}
+
 void MessageQueue::statistics() {
 	Map<StringName, int> set_count;
 	Map<int, int> notify_count;

--- a/core/message_queue.h
+++ b/core/message_queue.h
@@ -79,6 +79,7 @@ public:
 	Error push_notification(ObjectID p_id, int p_notification);
 	Error push_set(ObjectID p_id, const StringName &p_prop, const Variant &p_value);
 	Error push_callable(const Callable &p_callable, const Variant **p_args, int p_argcount, bool p_show_error = false);
+	Error push_callable(const Callable &p_callable, VARIANT_ARG_LIST);

 	Error push_call(Object *p_object, const StringName &p_method, VARIANT_ARG_LIST);
 	Error push_notification(Object *p_object, int p_notification);

--- a/scene/gui/container.cpp
+++ b/scene/gui/container.cpp
@@ -140,7 +140,7 @@ void Container::queue_sort() {
 		return;
 	}

-	MessageQueue::get_singleton()->push_call(this, "_sort_children");
+	MessageQueue::get_singleton()->push_callable(callable_mp(this, &Container::_sort_children));
 	pending_sort = true;
 }

@@ -177,8 +177,6 @@ String Container::get_configuration_warning() const {
 }

 void Container::_bind_methods() {
-	ClassDB::bind_method(D_METHOD("_sort_children"), &Container::_sort_children);
-
 	ClassDB::bind_method(D_METHOD("queue_sort"), &Container::queue_sort);
 	ClassDB::bind_method(D_METHOD("fit_child_in_rect", "child", "rect"), &Container::fit_child_in_rect);