Merge pull request #35097 from neikeq/issue-34954

Mono/C#: Fix _update_exports() leaking temporary Object/Node instances

modules/mono/csharp_script.cpp

@@ -159,6 +159,19 @@ void CSharpLanguage::finish() {
 	// Clear here, after finalizing all domains to make sure there is nothing else referencing the elements.
 	script_bindings.clear();
 
+#ifdef DEBUG_ENABLED
+	for (List<ObjectID>::Element *E = unsafely_referenced_objects.front(); E; E = E->next()) {
+		const ObjectID &id = E->get();
+		Object *obj = ObjectDB::get_instance(id);
+
+		if (obj) {
+			ERR_PRINTS("Leaked unsafe reference to object: " + obj->get_class() + ":" + itos(id));
+		} else {
+			ERR_PRINTS("Leaked unsafe reference to deleted object: " + itos(id));
+		}
+	}
+#endif
+
 	finalizing = false;
 }
 
@@ -615,6 +628,23 @@ Vector<ScriptLanguage::StackInfo> CSharpLanguage::stack_trace_get_info(MonoObjec
 }
 #endif
 
+void CSharpLanguage::post_unsafe_reference(Object *p_obj) {
+#ifdef DEBUG_ENABLED
+	ObjectID id = p_obj->get_instance_id();
+	ERR_FAIL_COND_MSG(unsafely_referenced_objects.find(id), "Multiple unsafe references for object: " + p_obj->get_class() + ":" + itos(id));
+	unsafely_referenced_objects.push_back(id);
+#endif
+}
+
+void CSharpLanguage::pre_unsafe_unreference(Object *p_obj) {
+#ifdef DEBUG_ENABLED
+	ObjectID id = p_obj->get_instance_id();
+	List<ObjectID>::Element *elem = unsafely_referenced_objects.find(id);
+	ERR_FAIL_NULL(elem);
+	unsafely_referenced_objects.erase(elem);
+#endif
+}
+
 void CSharpLanguage::frame() {
 
 	if (gdmono && gdmono->is_runtime_initialized() && gdmono->get_core_api_assembly() != NULL) {
@@ -1286,6 +1316,7 @@ bool CSharpLanguage::setup_csharp_script_binding(CSharpScriptBinding &r_script_b
 		// See: godot_icall_Reference_Dtor(MonoObject *p_obj, Object *p_ptr)
 
 		ref->reference();
+		CSharpLanguage::get_singleton()->post_unsafe_reference(ref);
 	}
 
 	return true;
@@ -1736,9 +1767,12 @@ bool CSharpInstance::_reference_owner_unsafe() {
 	// See: _unreference_owner_unsafe()
 
 	// May not me referenced yet, so we must use init_ref() instead of reference()
-	bool success = Object::cast_to<Reference>(owner)->init_ref();
-	unsafe_referenced = success;
-	return success;
+	if (static_cast<Reference *>(owner)->init_ref()) {
+		CSharpLanguage::get_singleton()->post_unsafe_reference(owner);
+		unsafe_referenced = true;
+	}
+
+	return unsafe_referenced;
 }
 
 bool CSharpInstance::_unreference_owner_unsafe() {
@@ -1759,6 +1793,7 @@ bool CSharpInstance::_unreference_owner_unsafe() {
 	// See: _reference_owner_unsafe()
 
 	// Destroying the owner here means self destructing, so we defer the owner destruction to the caller.
+	CSharpLanguage::get_singleton()->pre_unsafe_unreference(owner);
 	return static_cast<Reference *>(owner)->unreference();
 }
 
@@ -2243,7 +2278,11 @@ bool CSharpScript::_update_exports() {
 		MonoException *ctor_exc = NULL;
 		ctor->invoke(tmp_object, NULL, &ctor_exc);
 
+		Object *tmp_native = GDMonoMarshal::unbox<Object *>(CACHED_FIELD(GodotObject, ptr)->get_value(tmp_object));
+
 		if (ctor_exc) {
+			// TODO: Should we free 'tmp_native' if the exception was thrown after its creation?
+
 			MonoGCHandle::free_handle(tmp_pinned_gchandle);
 			tmp_object = NULL;
 
@@ -2322,6 +2361,15 @@ bool CSharpScript::_update_exports() {
 
 		MonoGCHandle::free_handle(tmp_pinned_gchandle);
 		tmp_object = NULL;
+
+		if (tmp_native && !Object::cast_to<Reference>(tmp_native)) {
+			Node *node = Object::cast_to<Node>(tmp_native);
+			if (node && node->is_inside_tree()) {
+				ERR_PRINTS("Temporary instance was added to the scene tree.");
+			} else {
+				memdelete(tmp_native);
+			}
+		}
 	}
 
 	placeholder_fallback_enabled = false;

modules/mono/csharp_script.h

@@ -307,6 +307,11 @@ class CSharpLanguage : public ScriptLanguage {
 
 	Map<Object *, CSharpScriptBinding> script_bindings;
 
+#ifdef DEBUG_ENABLED
+	// List of unsafely referenced objects
+	List<ObjectID> unsafely_referenced_objects;
+#endif
+
 	struct StringNameCache {
 
 		StringName _signal_callback;
@@ -458,6 +463,9 @@ public:
 	Vector<StackInfo> stack_trace_get_info(MonoObject *p_stack_trace);
 #endif
 
+	void post_unsafe_reference(Object *p_obj);
+	void pre_unsafe_unreference(Object *p_obj);
+
 	CSharpLanguage();
 	~CSharpLanguage();
 };

modules/mono/glue/base_object_glue.cpp

@@ -108,6 +108,7 @@ void godot_icall_Reference_Disposed(MonoObject *p_obj, Object *p_ptr, MonoBoolea
 
 	// Unsafe refcount decrement. The managed instance also counts as a reference.
 	// See: CSharpLanguage::alloc_instance_binding_data(Object *p_object)
+	CSharpLanguage::get_singleton()->pre_unsafe_unreference(ref);
 	if (ref->unreference()) {
 		memdelete(ref);
 	} else {

modules/mono/mono_gd/gd_mono_internals.cpp

@@ -83,7 +83,9 @@ void tie_managed_to_unmanaged(MonoObject *managed, Object *unmanaged) {
 			// See: godot_icall_Reference_Dtor(MonoObject *p_obj, Object *p_ptr)
 
 			// May not me referenced yet, so we must use init_ref() instead of reference()
-			ref->init_ref();
+			if (ref->init_ref()) {
+				CSharpLanguage::get_singleton()->post_unsafe_reference(ref);
+			}
 		}
 
 		// The object was just created, no script instance binding should have been attached

modules/mono/mono_gd/gd_mono_utils.cpp

@@ -115,6 +115,7 @@ MonoObject *unmanaged_get_managed(Object *unmanaged) {
 		// but the managed instance is alive, the refcount will be 1 instead of 0.
 		// See: godot_icall_Reference_Dtor(MonoObject *p_obj, Object *p_ptr)
 		ref->reference();
+		CSharpLanguage::get_singleton()->post_unsafe_reference(ref);
 	}
 
 	return mono_object;