From d68b9c20d69e037cff60260fe2e1fb198c62bba4 Mon Sep 17 00:00:00 2001
From: Ignacio Etcheverry <ignalfonsore@gmail.com>
Date: Thu, 16 Jan 2020 17:11:13 +0100
Subject: [PATCH] Mono/C#: Script interface calls now attach the current thread

Added guards to all C# script interface calls to attach the current thread
for the current scope if the thread is not already attached.
This is far from ideal, as attaching the thread is not cheap and all managed
thread local storage is lost when we detach the thread at the end of the calls.
However, it's the best we can do for now to avoid crashing
when an unattached thread tries to interact with C# code.
---
 modules/mono/csharp_script.cpp         | 112 +++++++++++++++++--------
 modules/mono/mono_gd/gd_mono_utils.cpp |  35 +++++++-
 modules/mono/mono_gd/gd_mono_utils.h   |  23 ++++-
 3 files changed, 128 insertions(+), 42 deletions(-)

diff --git a/modules/mono/csharp_script.cpp b/modules/mono/csharp_script.cpp
index 8a024eef131..c68404fdf51 100644
--- a/modules/mono/csharp_script.cpp
+++ b/modules/mono/csharp_script.cpp
@@ -559,6 +559,7 @@ Vector<ScriptLanguage::StackInfo> CSharpLanguage::debug_get_current_stack_info()
 
 #ifdef DEBUG_ENABLED
 	_TLS_RECURSION_GUARD_V_(Vector<StackInfo>());
+	GD_MONO_SCOPE_THREAD_ATTACH;
 
 	if (!gdmono->is_runtime_initialized() || !GDMono::get_singleton()->get_core_api_assembly() || !GDMonoCache::cached_data.corlib_cache_updated)
 		return Vector<StackInfo>();
@@ -583,6 +584,7 @@ Vector<ScriptLanguage::StackInfo> CSharpLanguage::debug_get_current_stack_info()
 Vector<ScriptLanguage::StackInfo> CSharpLanguage::stack_trace_get_info(MonoObject *p_stack_trace) {
 
 	_TLS_RECURSION_GUARD_V_(Vector<StackInfo>());
+	GD_MONO_SCOPE_THREAD_ATTACH;
 
 	MonoException *exc = NULL;
 
@@ -689,6 +691,7 @@ void CSharpLanguage::reload_all_scripts() {
 
 #ifdef GD_MONO_HOT_RELOAD
 	if (is_assembly_reloading_needed()) {
+		GD_MONO_SCOPE_THREAD_ATTACH;
 		reload_assemblies(false);
 	}
 #endif
@@ -706,6 +709,7 @@ void CSharpLanguage::reload_tool_script(const Ref<Script> &p_script, bool p_soft
 
 #ifdef GD_MONO_HOT_RELOAD
 	if (is_assembly_reloading_needed()) {
+		GD_MONO_SCOPE_THREAD_ATTACH;
 		reload_assemblies(p_soft_reload);
 	}
 #endif
@@ -1356,6 +1360,8 @@ void CSharpLanguage::free_instance_binding_data(void *p_data) {
 	if (finalizing)
 		return; // inside CSharpLanguage::finish(), all the gchandle bindings are released there
 
+	GD_MONO_ASSERT_THREAD_ATTACHED;
+
 	{
 		SCOPED_MUTEX_LOCK(language_bind_mutex);
 
@@ -1382,6 +1388,7 @@ void CSharpLanguage::refcount_incremented_instance_binding(Object *p_object) {
 
 #ifdef DEBUG_ENABLED
 	CRASH_COND(!ref_owner);
+	CRASH_COND(!p_object->has_script_instance_binding(get_language_index()));
 #endif
 
 	void *data = p_object->get_script_instance_binding(get_language_index());
@@ -1394,6 +1401,8 @@ void CSharpLanguage::refcount_incremented_instance_binding(Object *p_object) {
 		return;
 
 	if (ref_owner->reference_get_count() > 1 && gchandle->is_weak()) { // The managed side also holds a reference, hence 1 instead of 0
+		GD_MONO_SCOPE_THREAD_ATTACH;
+
 		// The reference count was increased after the managed side was the only one referencing our owner.
 		// This means the owner is being referenced again by the unmanaged side,
 		// so the owner must hold the managed side alive again to avoid it from being GCed.
@@ -1415,6 +1424,7 @@ bool CSharpLanguage::refcount_decremented_instance_binding(Object *p_object) {
 
 #ifdef DEBUG_ENABLED
 	CRASH_COND(!ref_owner);
+	CRASH_COND(!p_object->has_script_instance_binding(get_language_index()));
 #endif
 
 	void *data = p_object->get_script_instance_binding(get_language_index());
@@ -1429,6 +1439,8 @@ bool CSharpLanguage::refcount_decremented_instance_binding(Object *p_object) {
 		return refcount == 0;
 
 	if (refcount == 1 && gchandle.is_valid() && !gchandle->is_weak()) { // The managed side also holds a reference, hence 1 instead of 0
+		GD_MONO_SCOPE_THREAD_ATTACH;
+
 		// If owner owner is no longer referenced by the unmanaged side,
 		// the managed instance takes responsibility of deleting the owner when GCed.
 
@@ -1480,6 +1492,8 @@ bool CSharpInstance::set(const StringName &p_name, const Variant &p_value) {
 
 	ERR_FAIL_COND_V(!script.is_valid(), false);
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	MonoObject *mono_object = get_mono_object();
 	ERR_FAIL_NULL_V(mono_object, false);
 
@@ -1532,6 +1546,8 @@ bool CSharpInstance::get(const StringName &p_name, Variant &r_ret) const {
 
 	ERR_FAIL_COND_V(!script.is_valid(), false);
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	MonoObject *mono_object = get_mono_object();
 	ERR_FAIL_NULL_V(mono_object, false);
 
@@ -1625,6 +1641,8 @@ void CSharpInstance::get_property_list(List<PropertyInfo> *p_properties) const {
 
 	ERR_FAIL_COND(!script.is_valid());
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	MonoObject *mono_object = get_mono_object();
 	ERR_FAIL_NULL(mono_object);
 
@@ -1669,6 +1687,8 @@ bool CSharpInstance::has_method(const StringName &p_method) const {
 	if (!script.is_valid())
 		return false;
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	GDMonoClass *top = script->script_class;
 
 	while (top && top != script->native) {
@@ -1684,6 +1704,11 @@ bool CSharpInstance::has_method(const StringName &p_method) const {
 
 Variant CSharpInstance::call(const StringName &p_method, const Variant **p_args, int p_argcount, Variant::CallError &r_error) {
 
+	if (!script.is_valid())
+		ERR_FAIL_V(Variant());
+
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	MonoObject *mono_object = get_mono_object();
 
 	if (!mono_object) {
@@ -1691,9 +1716,6 @@ Variant CSharpInstance::call(const StringName &p_method, const Variant **p_args,
 		ERR_FAIL_V(Variant());
 	}
 
-	if (!script.is_valid())
-		ERR_FAIL_V(Variant());
-
 	GDMonoClass *top = script->script_class;
 
 	while (top && top != script->native) {
@@ -1721,6 +1743,8 @@ Variant CSharpInstance::call(const StringName &p_method, const Variant **p_args,
 
 void CSharpInstance::call_multilevel(const StringName &p_method, const Variant **p_args, int p_argcount) {
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	if (script.is_valid()) {
 		MonoObject *mono_object = get_mono_object();
 
@@ -1732,6 +1756,8 @@ void CSharpInstance::call_multilevel(const StringName &p_method, const Variant *
 
 void CSharpInstance::_call_multilevel(MonoObject *p_mono_object, const StringName &p_method, const Variant **p_args, int p_argcount) {
 
+	GD_MONO_ASSERT_THREAD_ATTACHED;
+
 	GDMonoClass *top = script->script_class;
 
 	while (top && top != script->native) {
@@ -1894,6 +1920,8 @@ void CSharpInstance::refcount_incremented() {
 	Reference *ref_owner = Object::cast_to<Reference>(owner);
 
 	if (ref_owner->reference_get_count() > 1 && gchandle->is_weak()) { // The managed side also holds a reference, hence 1 instead of 0
+		GD_MONO_SCOPE_THREAD_ATTACH;
+
 		// The reference count was increased after the managed side was the only one referencing our owner.
 		// This means the owner is being referenced again by the unmanaged side,
 		// so the owner must hold the managed side alive again to avoid it from being GCed.
@@ -1917,6 +1945,8 @@ bool CSharpInstance::refcount_decremented() {
 	int refcount = ref_owner->reference_get_count();
 
 	if (refcount == 1 && !gchandle->is_weak()) { // The managed side also holds a reference, hence 1 instead of 0
+		GD_MONO_SCOPE_THREAD_ATTACH;
+
 		// If owner owner is no longer referenced by the unmanaged side,
 		// the managed instance takes responsibility of deleting the owner when GCed.
 
@@ -1957,6 +1987,8 @@ MultiplayerAPI::RPCMode CSharpInstance::_member_get_rpc_mode(IMonoClassMember *p
 
 MultiplayerAPI::RPCMode CSharpInstance::get_rpc_mode(const StringName &p_method) const {
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	GDMonoClass *top = script->script_class;
 
 	while (top && top != script->native) {
@@ -1973,6 +2005,8 @@ MultiplayerAPI::RPCMode CSharpInstance::get_rpc_mode(const StringName &p_method)
 
 MultiplayerAPI::RPCMode CSharpInstance::get_rset_mode(const StringName &p_variable) const {
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	GDMonoClass *top = script->script_class;
 
 	while (top && top != script->native) {
@@ -1994,6 +2028,8 @@ MultiplayerAPI::RPCMode CSharpInstance::get_rset_mode(const StringName &p_variab
 
 void CSharpInstance::notification(int p_notification) {
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	if (p_notification == Object::NOTIFICATION_PREDELETE) {
 		// When NOTIFICATION_PREDELETE is sent, we also take the chance to call Dispose().
 		// It's safe to call Dispose() multiple times and NOTIFICATION_PREDELETE is guaranteed
@@ -2031,6 +2067,8 @@ void CSharpInstance::notification(int p_notification) {
 
 void CSharpInstance::_call_notification(int p_notification) {
 
+	GD_MONO_ASSERT_THREAD_ATTACHED;
+
 	MonoObject *mono_object = get_mono_object();
 	ERR_FAIL_NULL(mono_object);
 
@@ -2055,6 +2093,8 @@ void CSharpInstance::_call_notification(int p_notification) {
 }
 
 String CSharpInstance::to_string(bool *r_valid) {
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	MonoObject *mono_object = get_mono_object();
 
 	if (mono_object == NULL) {
@@ -2103,6 +2143,8 @@ CSharpInstance::CSharpInstance() :
 
 CSharpInstance::~CSharpInstance() {
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	destructing_script_instance = true;
 
 	if (gchandle.is_valid()) {
@@ -2193,6 +2235,8 @@ void CSharpScript::_update_exports_values(Map<StringName, Variant> &values, List
 void CSharpScript::_update_member_info_no_exports() {
 
 	if (exports_invalidated) {
+		GD_MONO_ASSERT_THREAD_ATTACHED;
+
 		exports_invalidated = false;
 
 		member_info.clear();
@@ -2251,6 +2295,8 @@ bool CSharpScript::_update_exports() {
 	bool changed = false;
 
 	if (exports_invalidated) {
+		GD_MONO_SCOPE_THREAD_ATTACH;
+
 		exports_invalidated = false;
 
 		changed = true;
@@ -2400,6 +2446,8 @@ void CSharpScript::load_script_signals(GDMonoClass *p_class, GDMonoClass *p_nati
 	// make sure this classes signals are empty when loading for the first time
 	_signals.clear();
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	GDMonoClass *top = p_class;
 	while (top && top != p_native_class) {
 		const Vector<GDMonoClass *> &delegates = top->get_all_delegates();
@@ -2420,6 +2468,8 @@ void CSharpScript::load_script_signals(GDMonoClass *p_class, GDMonoClass *p_nati
 }
 
 bool CSharpScript::_get_signal(GDMonoClass *p_class, GDMonoClass *p_delegate, Vector<Argument> &params) {
+	GD_MONO_ASSERT_THREAD_ATTACHED;
+
 	if (p_delegate->has_attribute(CACHED_CLASS(SignalAttribute))) {
 		MonoType *raw_type = p_delegate->get_mono_type();
 
@@ -2461,6 +2511,8 @@ bool CSharpScript::_get_signal(GDMonoClass *p_class, GDMonoClass *p_delegate, Ve
  */
 bool CSharpScript::_get_member_export(IMonoClassMember *p_member, bool p_inspect_export, PropertyInfo &r_prop_info, bool &r_exported) {
 
+	GD_MONO_ASSERT_THREAD_ATTACHED;
+
 	// Goddammit, C++. All I wanted was some nested functions.
 #define MEMBER_FULL_QUALIFIED_NAME(m_member) \
 	(m_member->get_enclosing_class()->get_full_name() + "." + (String)m_member->get_name())
@@ -2539,6 +2591,8 @@ bool CSharpScript::_get_member_export(IMonoClassMember *p_member, bool p_inspect
 
 int CSharpScript::_try_get_member_export_hint(IMonoClassMember *p_member, ManagedType p_type, Variant::Type p_variant_type, bool p_allow_generics, PropertyHint &r_hint, String &r_hint_string) {
 
+	GD_MONO_ASSERT_THREAD_ATTACHED;
+
 	if (p_variant_type == Variant::INT && p_type.type_encoding == MONO_TYPE_VALUETYPE && mono_class_is_enum(p_type.type_class->get_mono_ptr())) {
 		r_hint = PROPERTY_HINT_ENUM;
 
@@ -2648,6 +2702,8 @@ Variant CSharpScript::call(const StringName &p_method, const Variant **p_args, i
 		return Variant();
 	}
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	GDMonoClass *top = script_class;
 
 	while (top && top != native) {
@@ -2840,6 +2896,8 @@ StringName CSharpScript::get_instance_base_type() const {
 
 CSharpInstance *CSharpScript::_create_instance(const Variant **p_args, int p_argcount, Object *p_owner, bool p_isref, Variant::CallError &r_error) {
 
+	GD_MONO_ASSERT_THREAD_ATTACHED;
+
 	/* STEP 1, CREATE */
 
 	// Search the constructor first, to fail with an error if it's not found before allocating anything else.
@@ -2934,12 +2992,14 @@ Variant CSharpScript::_new(const Variant **p_args, int p_argcount, Variant::Call
 	}
 
 	r_error.error = Variant::CallError::CALL_OK;
-	REF ref;
 
 	ERR_FAIL_NULL_V(native, Variant());
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	Object *owner = ClassDB::instance(NATIVE_GDMONOCLASS_NAME(native));
 
+	REF ref;
 	Reference *r = Object::cast_to<Reference>(owner);
 	if (r) {
 		ref = REF(r);
@@ -2977,6 +3037,8 @@ ScriptInstance *CSharpScript::instance_create(Object *p_this) {
 		}
 	}
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	Variant::CallError unchecked_error;
 	return _create_instance(NULL, 0, p_this, Object::cast_to<Reference>(p_this) != NULL, unchecked_error);
 }
@@ -3024,6 +3086,8 @@ void CSharpScript::get_script_method_list(List<MethodInfo> *p_list) const {
 	if (!script_class)
 		return;
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	// TODO: Filter out things unsuitable for explicit calls, like constructors.
 	const Vector<GDMonoMethod *> &methods = script_class->get_all_methods();
 	for (int i = 0; i < methods.size(); ++i) {
@@ -3036,6 +3100,8 @@ bool CSharpScript::has_method(const StringName &p_method) const {
 	if (!script_class)
 		return false;
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	return script_class->has_fetched_method_unknown_params(p_method);
 }
 
@@ -3044,6 +3110,8 @@ MethodInfo CSharpScript::get_method_info(const StringName &p_method) const {
 	if (!script_class)
 		return MethodInfo();
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	GDMonoClass *top = script_class;
 
 	while (top && top != native) {
@@ -3068,6 +3136,8 @@ Error CSharpScript::reload(bool p_keep_state) {
 
 	ERR_FAIL_COND_V(!p_keep_state && has_instances, ERR_ALREADY_IN_USE);
 
+	GD_MONO_SCOPE_THREAD_ATTACH;
+
 	GDMonoAssembly *project_assembly = GDMono::get_singleton()->get_project_assembly();
 
 	if (project_assembly) {
@@ -3295,39 +3365,7 @@ RES ResourceFormatLoaderCSharpScript::load(const String &p_path, const String &p
 
 	script->set_path(p_original_path);
 
-#ifndef TOOLS_ENABLED
-
-#ifdef DEBUG_ENABLED
-	// User is responsible for thread attach/detach
-	CRASH_COND_MSG(mono_domain_get() == NULL, "Thread is not attached.");
-#endif
-
-#endif
-
-#ifdef TOOLS_ENABLED
-	MonoDomain *domain = mono_domain_get();
-	if (Engine::get_singleton()->is_editor_hint() && domain == NULL) {
-
-		CRASH_COND(Thread::get_caller_id() == Thread::get_main_id());
-
-		// Thread is not attached, but we will make an exception in this case
-		// because this may be called by one of the editor's worker threads.
-		// Attach this thread temporarily to reload the script.
-
-		if (domain) {
-			MonoThread *mono_thread = mono_thread_attach(domain);
-			CRASH_COND(mono_thread == NULL);
-			script->reload();
-			mono_thread_detach(mono_thread);
-		}
-
-	} else { // just reload it normally
-#endif
-		script->reload();
-
-#ifdef TOOLS_ENABLED
-	}
-#endif
+	script->reload();
 
 	if (r_error)
 		*r_error = OK;
diff --git a/modules/mono/mono_gd/gd_mono_utils.cpp b/modules/mono/mono_gd/gd_mono_utils.cpp
index 6bb9f28a32e..4e7f590a698 100644
--- a/modules/mono/mono_gd/gd_mono_utils.cpp
+++ b/modules/mono/mono_gd/gd_mono_utils.cpp
@@ -125,10 +125,12 @@ void set_main_thread(MonoThread *p_thread) {
 	mono_thread_set_main(p_thread);
 }
 
-void attach_current_thread() {
-	ERR_FAIL_COND(!GDMono::get_singleton()->is_runtime_initialized());
-	MonoThread *mono_thread = mono_thread_attach(mono_get_root_domain());
-	ERR_FAIL_NULL(mono_thread);
+MonoThread *attach_current_thread() {
+	ERR_FAIL_COND_V(!GDMono::get_singleton()->is_runtime_initialized(), NULL);
+	MonoDomain *scripts_domain = GDMono::get_singleton()->get_scripts_domain();
+	MonoThread *mono_thread = mono_thread_attach(scripts_domain ? scripts_domain : mono_get_root_domain());
+	ERR_FAIL_NULL_V(mono_thread, NULL);
+	return mono_thread;
 }
 
 void detach_current_thread() {
@@ -138,10 +140,20 @@ void detach_current_thread() {
 	mono_thread_detach(mono_thread);
 }
 
+void detach_current_thread(MonoThread *p_mono_thread) {
+	ERR_FAIL_COND(!GDMono::get_singleton()->is_runtime_initialized());
+	ERR_FAIL_NULL(p_mono_thread);
+	mono_thread_detach(p_mono_thread);
+}
+
 MonoThread *get_current_thread() {
 	return mono_thread_current();
 }
 
+bool is_thread_attached() {
+	return mono_domain_get() != NULL;
+}
+
 void runtime_object_init(MonoObject *p_this_obj, GDMonoClass *p_class, MonoException **r_exc) {
 	GDMonoMethod *ctor = p_class->get_method(".ctor", 0);
 	ERR_FAIL_NULL(ctor);
@@ -617,4 +629,19 @@ GDMonoClass *make_generic_dictionary_type(MonoReflectionType *p_key_reftype, Mon
 
 } // namespace Marshal
 
+ScopeThreadAttach::ScopeThreadAttach() :
+		mono_thread(NULL) {
+	if (likely(GDMono::get_singleton()->is_runtime_initialized()) && unlikely(!mono_domain_get())) {
+		mono_thread = GDMonoUtils::attach_current_thread();
+	}
+}
+
+ScopeThreadAttach::~ScopeThreadAttach() {
+	if (unlikely(mono_thread)) {
+		GDMonoUtils::detach_current_thread(mono_thread);
+	}
+}
+
+// namespace Marshal
+
 } // namespace GDMonoUtils
diff --git a/modules/mono/mono_gd/gd_mono_utils.h b/modules/mono/mono_gd/gd_mono_utils.h
index d9eb912cd73..db9f99bfdc6 100644
--- a/modules/mono/mono_gd/gd_mono_utils.h
+++ b/modules/mono/mono_gd/gd_mono_utils.h
@@ -83,9 +83,11 @@ _FORCE_INLINE_ void hash_combine(uint32_t &p_hash, const uint32_t &p_with_hash)
 MonoObject *unmanaged_get_managed(Object *unmanaged);
 
 void set_main_thread(MonoThread *p_thread);
-void attach_current_thread();
+MonoThread *attach_current_thread();
 void detach_current_thread();
+void detach_current_thread(MonoThread *p_mono_thread);
 MonoThread *get_current_thread();
+bool is_thread_attached();
 
 _FORCE_INLINE_ bool is_main_thread() {
 	return mono_domain_get() != NULL && mono_thread_get_main() == mono_thread_current();
@@ -142,6 +144,14 @@ uint64_t unbox_enum_value(MonoObject *p_boxed, MonoType *p_enum_basetype, bool &
 
 void dispose(MonoObject *p_mono_object, MonoException **r_exc);
 
+struct ScopeThreadAttach {
+	ScopeThreadAttach();
+	~ScopeThreadAttach();
+
+private:
+	MonoThread *mono_thread;
+};
+
 } // namespace GDMonoUtils
 
 #define NATIVE_GDMONOCLASS_NAME(m_class) (GDMonoMarshal::mono_string_to_godot((MonoString *)m_class->get_field(BINDINGS_NATIVE_NAME_FIELD)->get_value(NULL)))
@@ -153,4 +163,15 @@ void dispose(MonoObject *p_mono_object, MonoException **r_exc);
 #define GD_MONO_END_RUNTIME_INVOKE \
 	_runtime_invoke_count_ref -= 1;
 
+#define GD_MONO_SCOPE_THREAD_ATTACH                                   \
+	GDMonoUtils::ScopeThreadAttach __gdmono__scope__thread__attach__; \
+	(void)__gdmono__scope__thread__attach__;
+
+#ifdef DEBUG_ENABLED
+#define GD_MONO_ASSERT_THREAD_ATTACHED \
+	{ CRASH_COND(!GDMonoUtils::is_thread_attached()); }
+#else
+#define GD_MONO_ASSERT_THREAD_ATTACHED
+#endif
+
 #endif // GD_MONOUTILS_H