Fix/workaround for issue #21667

When a Reference managed instance is garbage collected and its finalizer is called, it could happen that the native instance is referenced once again before the finalizer can unreference and memdelete it. The workaround is to create a new managed instance when this happens (at least for now).

4 files changed, 118 insertions, 72 deletions

diff --git a/modules/mono/mono_gd/gd_mono.cpp b/modules/mono/mono_gd/gd_mono.cpp
index f1dbb5c514..833aa23820 100644
--- a/modules/mono/mono_gd/gd_mono.cpp
+++ b/modules/mono/mono_gd/gd_mono.cpp
@@ -639,9 +639,7 @@ Error GDMono::_unload_scripts_domain() {
 
 	mono_gc_collect(mono_gc_max_generation());
 
-	finalizing_scripts_domain = true;
 	mono_domain_finalize(scripts_domain, 2000);
-	finalizing_scripts_domain = false;
 
 	mono_gc_collect(mono_gc_max_generation());
 
@@ -837,7 +835,6 @@ GDMono::GDMono() {
 	gdmono_log = memnew(GDMonoLog);
 
 	runtime_initialized = false;
-	finalizing_scripts_domain = false;
 
 	root_domain = NULL;
 	scripts_domain = NULL;
@@ -866,7 +863,7 @@ GDMono::GDMono() {
 
 GDMono::~GDMono() {
 
-	if (runtime_initialized) {
+	if (is_runtime_initialized()) {
 
 		if (scripts_domain) {
 
@@ -891,8 +888,9 @@ GDMono::~GDMono() {
 
 		print_verbose("Mono: Runtime cleanup...");
 
-		runtime_initialized = false;
 		mono_jit_cleanup(root_domain);
+
+		runtime_initialized = false;
 	}
 
 	if (gdmono_log)
@@ -903,33 +901,12 @@ GDMono::~GDMono() {
 
 _GodotSharp *_GodotSharp::singleton = NULL;
 
-void _GodotSharp::_dispose_object(Object *p_object) {
-
-	if (p_object->get_script_instance()) {
-		CSharpInstance *cs_instance = CAST_CSHARP_INSTANCE(p_object->get_script_instance());
-		if (cs_instance) {
-			cs_instance->mono_object_disposed();
-			return;
-		}
-	}
-
-	// Unsafe refcount decrement. The managed instance also counts as a reference.
-	// See: CSharpLanguage::alloc_instance_binding_data(Object *p_object)
-	if (Object::cast_to<Reference>(p_object)->unreference()) {
-		memdelete(p_object);
-	}
-}
-
 void _GodotSharp::_dispose_callback() {
 
 #ifndef NO_THREADS
 	queue_mutex->lock();
 #endif
 
-	for (List<Object *>::Element *E = obj_delete_queue.front(); E; E = E->next()) {
-		_dispose_object(E->get());
-	}
-
 	for (List<NodePath *>::Element *E = np_delete_queue.front(); E; E = E->next()) {
 		memdelete(E->get());
 	}
@@ -938,7 +915,6 @@ void _GodotSharp::_dispose_callback() {
 		memdelete(E->get());
 	}
 
-	obj_delete_queue.clear();
 	np_delete_queue.clear();
 	rid_delete_queue.clear();
 	queue_empty = true;
@@ -958,52 +934,69 @@ void _GodotSharp::detach_thread() {
 	GDMonoUtils::detach_current_thread();
 }
 
-bool _GodotSharp::is_finalizing_domain() {
+int32_t _GodotSharp::get_domain_id() {
 
-	return GDMono::get_singleton()->is_finalizing_scripts_domain();
+	MonoDomain *domain = mono_domain_get();
+	CRASH_COND(!domain); // User must check if runtime is initialized before calling this method
+	return mono_domain_get_id(domain);
 }
 
-bool _GodotSharp::is_domain_loaded() {
+int32_t _GodotSharp::get_scripts_domain_id() {
 
-	return GDMono::get_singleton()->get_scripts_domain() != NULL;
+	MonoDomain *domain = SCRIPTS_DOMAIN;
+	CRASH_COND(!domain); // User must check if scripts domain is loaded before calling this method
+	return mono_domain_get_id(domain);
 }
 
-#define ENQUEUE_FOR_DISPOSAL(m_queue, m_inst)                                   \
-	m_queue.push_back(m_inst);                                                  \
-	if (queue_empty) {                                                          \
-		queue_empty = false;                                                    \
-		if (!is_finalizing_domain()) { /* call_deferred may not be safe here */ \
-			call_deferred("_dispose_callback");                                 \
-		}                                                                       \
-	}
+bool _GodotSharp::is_scripts_domain_loaded() {
 
-void _GodotSharp::queue_dispose(MonoObject *p_mono_object, Object *p_object) {
+	return GDMono::get_singleton()->is_runtime_initialized() && SCRIPTS_DOMAIN != NULL;
+}
 
-	if (GDMonoUtils::is_main_thread() && !GDMono::get_singleton()->is_finalizing_scripts_domain()) {
-		_dispose_object(p_object);
-	} else {
-#ifndef NO_THREADS
-		queue_mutex->lock();
-#endif
+bool _GodotSharp::_is_domain_finalizing_for_unload(int32_t p_domain_id) {
 
-		// This is our last chance to invoke notification predelete (this is being called from the finalizer)
-		// We must use the MonoObject* passed by the finalizer, because the weak GC handle target returns NULL at this point
-		CSharpInstance *si = CAST_CSHARP_INSTANCE(p_object->get_script_instance());
-		if (si) {
-			si->call_notification_no_check(p_mono_object, Object::NOTIFICATION_PREDELETE);
-		}
+	return is_domain_finalizing_for_unload(p_domain_id);
+}
 
-		ENQUEUE_FOR_DISPOSAL(obj_delete_queue, p_object);
+bool _GodotSharp::is_domain_finalizing_for_unload() {
 
-#ifndef NO_THREADS
-		queue_mutex->unlock();
-#endif
-	}
+	return is_domain_finalizing_for_unload(mono_domain_get());
+}
+
+bool _GodotSharp::is_domain_finalizing_for_unload(int32_t p_domain_id) {
+
+	return is_domain_finalizing_for_unload(mono_domain_get_by_id(p_domain_id));
+}
+
+bool _GodotSharp::is_domain_finalizing_for_unload(MonoDomain *p_domain) {
+
+	if (!p_domain)
+		return true;
+	return mono_domain_is_unloading(p_domain);
+}
+
+bool _GodotSharp::is_runtime_shutting_down() {
+
+	return mono_runtime_is_shutting_down();
+}
+
+bool _GodotSharp::is_runtime_initialized() {
+
+	return GDMono::get_singleton()->is_runtime_initialized();
 }
 
+#define ENQUEUE_FOR_DISPOSAL(m_queue, m_inst)                                                            \
+	m_queue.push_back(m_inst);                                                                           \
+	if (queue_empty) {                                                                                   \
+		queue_empty = false;                                                                             \
+		if (!is_domain_finalizing_for_unload(SCRIPTS_DOMAIN)) { /* call_deferred may not be safe here */ \
+			call_deferred("_dispose_callback");                                                          \
+		}                                                                                                \
+	}
+
 void _GodotSharp::queue_dispose(NodePath *p_node_path) {
 
-	if (GDMonoUtils::is_main_thread() && !GDMono::get_singleton()->is_finalizing_scripts_domain()) {
+	if (GDMonoUtils::is_main_thread() && !is_domain_finalizing_for_unload(SCRIPTS_DOMAIN)) {
 		memdelete(p_node_path);
 	} else {
 #ifndef NO_THREADS
@@ -1020,7 +1013,7 @@ void _GodotSharp::queue_dispose(NodePath *p_node_path) {
 
 void _GodotSharp::queue_dispose(RID *p_rid) {
 
-	if (GDMonoUtils::is_main_thread() && !GDMono::get_singleton()->is_finalizing_scripts_domain()) {
+	if (GDMonoUtils::is_main_thread() && !is_domain_finalizing_for_unload(SCRIPTS_DOMAIN)) {
 		memdelete(p_rid);
 	} else {
 #ifndef NO_THREADS
@@ -1040,8 +1033,13 @@ void _GodotSharp::_bind_methods() {
 	ClassDB::bind_method(D_METHOD("attach_thread"), &_GodotSharp::attach_thread);
 	ClassDB::bind_method(D_METHOD("detach_thread"), &_GodotSharp::detach_thread);
 
-	ClassDB::bind_method(D_METHOD("is_finalizing_domain"), &_GodotSharp::is_finalizing_domain);
-	ClassDB::bind_method(D_METHOD("is_domain_loaded"), &_GodotSharp::is_domain_loaded);
+	ClassDB::bind_method(D_METHOD("get_domain_id"), &_GodotSharp::get_domain_id);
+	ClassDB::bind_method(D_METHOD("get_scripts_domain_id"), &_GodotSharp::get_scripts_domain_id);
+	ClassDB::bind_method(D_METHOD("is_scripts_domain_loaded"), &_GodotSharp::is_scripts_domain_loaded);
+	ClassDB::bind_method(D_METHOD("is_domain_finalizing_for_unload", "domain_id"), &_GodotSharp::_is_domain_finalizing_for_unload);
+
+	ClassDB::bind_method(D_METHOD("is_runtime_shutting_down"), &_GodotSharp::is_runtime_shutting_down);
+	ClassDB::bind_method(D_METHOD("is_runtime_initialized"), &_GodotSharp::is_runtime_initialized);
 
 	ClassDB::bind_method(D_METHOD("_dispose_callback"), &_GodotSharp::_dispose_callback);
 }
diff --git a/modules/mono/mono_gd/gd_mono.h b/modules/mono/mono_gd/gd_mono.h
index e0ec6ced5e..0c5503d28e 100644
--- a/modules/mono/mono_gd/gd_mono.h
+++ b/modules/mono/mono_gd/gd_mono.h
@@ -170,8 +170,7 @@ public:
 	void add_assembly(uint32_t p_domain_id, GDMonoAssembly *p_assembly);
 	GDMonoAssembly **get_loaded_assembly(const String &p_name);
 
-	_FORCE_INLINE_ bool is_runtime_initialized() const { return runtime_initialized; }
-	_FORCE_INLINE_ bool is_finalizing_scripts_domain() const { return finalizing_scripts_domain; }
+	_FORCE_INLINE_ bool is_runtime_initialized() const { return runtime_initialized && !mono_runtime_is_shutting_down() /* stays true after shutdown finished */; }
 
 	_FORCE_INLINE_ MonoDomain *get_scripts_domain() { return scripts_domain; }
 #ifdef TOOLS_ENABLED
@@ -236,11 +235,10 @@ class _GodotSharp : public Object {
 
 	friend class GDMono;
 
-	void _dispose_object(Object *p_object);
-
 	void _dispose_callback();
 
-	List<Object *> obj_delete_queue;
+	bool _is_domain_finalizing_for_unload(int32_t p_domain_id);
+
 	List<NodePath *> np_delete_queue;
 	List<RID *> rid_delete_queue;
 
@@ -260,10 +258,18 @@ public:
 	void attach_thread();
 	void detach_thread();
 
-	bool is_finalizing_domain();
-	bool is_domain_loaded();
+	int32_t get_domain_id();
+	int32_t get_scripts_domain_id();
+
+	bool is_scripts_domain_loaded();
+
+	bool is_domain_finalizing_for_unload();
+	bool is_domain_finalizing_for_unload(int32_t p_domain_id);
+	bool is_domain_finalizing_for_unload(MonoDomain *p_domain);
+
+	bool is_runtime_shutting_down();
+	bool is_runtime_initialized();
 
-	void queue_dispose(MonoObject *p_mono_object, Object *p_object);
 	void queue_dispose(NodePath *p_node_path);
 	void queue_dispose(RID *p_rid);
 
diff --git a/modules/mono/mono_gd/gd_mono_utils.cpp b/modules/mono/mono_gd/gd_mono_utils.cpp
index c1f56bc3d2..5e02ebf430 100644
--- a/modules/mono/mono_gd/gd_mono_utils.cpp
+++ b/modules/mono/mono_gd/gd_mono_utils.cpp
@@ -138,6 +138,7 @@ void MonoCache::clear_members() {
 	field_Image_ptr = NULL;
 	field_RID_ptr = NULL;
 
+	methodthunk_GodotObject_Dispose = NULL;
 	methodthunk_Array_GetPtr = NULL;
 	methodthunk_Dictionary_GetPtr = NULL;
 	methodthunk_MarshalUtils_IsArrayGenericType = NULL;
@@ -235,6 +236,7 @@ void update_godot_api_cache() {
 	CACHE_FIELD_AND_CHECK(NodePath, ptr, CACHED_CLASS(NodePath)->get_field(BINDINGS_PTR_FIELD));
 	CACHE_FIELD_AND_CHECK(RID, ptr, CACHED_CLASS(RID)->get_field(BINDINGS_PTR_FIELD));
 
+	CACHE_METHOD_THUNK_AND_CHECK(GodotObject, Dispose, (GodotObject_Dispose)CACHED_CLASS(GodotObject)->get_method_thunk("Dispose", 0));
 	CACHE_METHOD_THUNK_AND_CHECK(Array, GetPtr, (Array_GetPtr)GODOT_API_NS_CLAS(BINDINGS_NAMESPACE_COLLECTIONS, Array)->get_method_thunk("GetPtr", 0));
 	CACHE_METHOD_THUNK_AND_CHECK(Dictionary, GetPtr, (Dictionary_GetPtr)GODOT_API_NS_CLAS(BINDINGS_NAMESPACE_COLLECTIONS, Dictionary)->get_method_thunk("GetPtr", 0));
 	CACHE_METHOD_THUNK_AND_CHECK(MarshalUtils, IsArrayGenericType, (IsArrayGenericType)GODOT_API_CLASS(MarshalUtils)->get_method_thunk("IsArrayGenericType", 1));
@@ -247,7 +249,7 @@ void update_godot_api_cache() {
 	CACHE_METHOD_THUNK_AND_CHECK(DebuggingUtils, GetStackFrameInfo, (DebugUtils_StackFrameInfo)GODOT_API_CLASS(DebuggingUtils)->get_method_thunk("GetStackFrameInfo", 4));
 #endif
 
-	// TODO Move to CSharpLanguage::init()
+	// TODO Move to CSharpLanguage::init() and do handle disposal
 	MonoObject *task_scheduler = mono_object_new(SCRIPTS_DOMAIN, GODOT_API_CLASS(GodotTaskScheduler)->get_mono_ptr());
 	GDMonoUtils::runtime_object_init(task_scheduler);
 	mono_cache.task_scheduler_handle = MonoGCHandle::create_strong(task_scheduler);
@@ -270,11 +272,48 @@ MonoObject *unmanaged_get_managed(Object *unmanaged) {
 			}
 		}
 
-		// Only called if the owner does not have a CSharpInstance
+		// If the owner does not have a CSharpInstance...
+
 		void *data = unmanaged->get_script_instance_binding(CSharpLanguage::get_singleton()->get_language_index());
 
 		if (data) {
-			return ((Map<Object *, Ref<MonoGCHandle> >::Element *)data)->value()->get_target();
+			CSharpScriptBinding &script_binding = ((Map<Object *, CSharpScriptBinding>::Element *)data)->value();
+
+			Ref<MonoGCHandle> &gchandle = script_binding.gchandle;
+			ERR_FAIL_COND_V(gchandle.is_null(), NULL);
+
+			MonoObject *target = gchandle->get_target();
+
+			if (target)
+				return target;
+
+			CSharpLanguage::get_singleton()->release_script_gchandle(gchandle);
+
+			// Create a new one
+
+#ifdef DEBUG_ENABLED
+			CRASH_COND(script_binding.type_name == StringName());
+			CRASH_COND(script_binding.wrapper_class == NULL);
+#endif
+
+			MonoObject *mono_object = GDMonoUtils::create_managed_for_godot_object(script_binding.wrapper_class, script_binding.type_name, unmanaged);
+			ERR_FAIL_NULL_V(mono_object, NULL);
+
+			gchandle->set_handle(MonoGCHandle::new_strong_handle(mono_object), MonoGCHandle::STRONG_HANDLE);
+
+			// Tie managed to unmanaged
+			Reference *ref = Object::cast_to<Reference>(unmanaged);
+
+			if (ref) {
+				// Unsafe refcount increment. The managed instance also counts as a reference.
+				// This way if the unmanaged world has no references to our owner
+				// 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();
+			}
+
+			return mono_object;
 		}
 	}
 
@@ -304,6 +343,7 @@ MonoThread *get_current_thread() {
 
 void runtime_object_init(MonoObject *p_this_obj) {
 	GD_MONO_BEGIN_RUNTIME_INVOKE;
+	// FIXME: Do not use mono_runtime_object_init, it aborts if an exception is thrown
 	mono_runtime_object_init(p_this_obj);
 	GD_MONO_END_RUNTIME_INVOKE;
 }
diff --git a/modules/mono/mono_gd/gd_mono_utils.h b/modules/mono/mono_gd/gd_mono_utils.h
index bf8860c85a..c6de824d5a 100644
--- a/modules/mono/mono_gd/gd_mono_utils.h
+++ b/modules/mono/mono_gd/gd_mono_utils.h
@@ -49,6 +49,7 @@
 
 namespace GDMonoUtils {
 
+typedef void (*GodotObject_Dispose)(MonoObject *, MonoObject **);
 typedef Array *(*Array_GetPtr)(MonoObject *, MonoObject **);
 typedef Dictionary *(*Dictionary_GetPtr)(MonoObject *, MonoObject **);
 typedef MonoObject *(*SignalAwaiter_SignalCallback)(MonoObject *, MonoArray *, MonoObject **);
@@ -141,6 +142,7 @@ struct MonoCache {
 	GDMonoField *field_Image_ptr;
 	GDMonoField *field_RID_ptr;
 
+	GodotObject_Dispose methodthunk_GodotObject_Dispose;
 	Array_GetPtr methodthunk_Array_GetPtr;
 	Dictionary_GetPtr methodthunk_Dictionary_GetPtr;
 	IsArrayGenericType methodthunk_MarshalUtils_IsArrayGenericType;