Mono: Script lifetime fixes

- alloc_language_binding: Use strong GC handle as well for references. Fixes #15138
- Set the native instance field of Godot.Object to IntPtr.Zero when it's freed.
- Create weak handles without tracking resurrection (that was causing trouble). This means we have to call notification predelete before queueing a native Object for deletion, and use the MonoObject* passed by the finalizer because the weak GC handle target will return NULL at this point.

modules/mono/csharp_script.cpp

@@ -122,6 +122,8 @@ void CSharpLanguage::init() {
 
 void CSharpLanguage::finish() {
 
+	finalizing = true;
+
 	// Release gchandle bindings before finalizing mono runtime
 	gchandle_bindings.clear();
 
@@ -129,6 +131,8 @@ void CSharpLanguage::finish() {
 		memdelete(gdmono);
 		gdmono = NULL;
 	}
+
+	finalizing = false;
 }
 
 void CSharpLanguage::get_reserved_words(List<String> *p_words) const {
@@ -742,6 +746,8 @@ CSharpLanguage::CSharpLanguage() {
 	ERR_FAIL_COND(singleton);
 	singleton = this;
 
+	finalizing = false;
+
 	gdmono = NULL;
 
 #ifdef NO_THREADS
@@ -798,12 +804,9 @@ void *CSharpLanguage::alloc_instance_binding_data(Object *p_object) {
 	ERR_FAIL_NULL_V(mono_object, NULL);
 
 	// Tie managed to unmanaged
-	bool strong_handle = true;
 	Reference *ref = Object::cast_to<Reference>(p_object);
 
 	if (ref) {
-		strong_handle = false;
-
 		// 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.
@@ -812,8 +815,7 @@ void *CSharpLanguage::alloc_instance_binding_data(Object *p_object) {
 		ref->reference();
 	}
 
-	Ref<MonoGCHandle> gchandle = strong_handle ? MonoGCHandle::create_strong(mono_object) :
-												 MonoGCHandle::create_weak(mono_object);
+	Ref<MonoGCHandle> gchandle = MonoGCHandle::create_strong(mono_object);
 
 #ifndef NO_THREADS
 	script_bind_lock->lock();
@@ -838,27 +840,38 @@ void CSharpLanguage::free_instance_binding_data(void *p_data) {
 		return;
 	}
 
+	if (finalizing)
+		return; // inside CSharpLanguage::finish(), all the gchandle bindings are released there
+
 #ifndef NO_THREADS
 	script_bind_lock->lock();
 #endif
 
-	gchandle_bindings.erase((Map<Object *, Ref<MonoGCHandle> >::Element *)p_data);
+	Map<Object *, Ref<MonoGCHandle> >::Element *data = (Map<Object *, Ref<MonoGCHandle> >::Element *)p_data;
+
+	// Set the native instance field to IntPtr.Zero, if not yet garbage collected
+	MonoObject *mono_object = data->value()->get_target();
+	if (mono_object) {
+		CACHED_FIELD(GodotObject, ptr)->set_value_raw(mono_object, NULL);
+	}
+
+	gchandle_bindings.erase(data);
 
 #ifndef NO_THREADS
 	script_bind_lock->unlock();
 #endif
 }
 
-void CSharpInstance::_ml_call_reversed(GDMonoClass *klass, const StringName &p_method, const Variant **p_args, int p_argcount) {
+void CSharpInstance::_ml_call_reversed(MonoObject *p_mono_object, GDMonoClass *p_klass, const StringName &p_method, const Variant **p_args, int p_argcount) {
 
-	GDMonoClass *base = klass->get_parent_class();
+	GDMonoClass *base = p_klass->get_parent_class();
 	if (base && base != script->native)
-		_ml_call_reversed(base, p_method, p_args, p_argcount);
+		_ml_call_reversed(p_mono_object, base, p_method, p_args, p_argcount);
 
-	GDMonoMethod *method = klass->get_method(p_method, p_argcount);
+	GDMonoMethod *method = p_klass->get_method(p_method, p_argcount);
 
 	if (method) {
-		method->invoke(get_mono_object(), p_args);
+		method->invoke(p_mono_object, p_args);
 	}
 }
 
@@ -1020,7 +1033,6 @@ Variant CSharpInstance::call(const StringName &p_method, const Variant **p_args,
 
 	MonoObject *mono_object = get_mono_object();
 
-	ERR_EXPLAIN("Reference has been garbage collected?");
 	ERR_FAIL_NULL_V(mono_object, Variant());
 
 	if (!script.is_valid())
@@ -1054,23 +1066,34 @@ void CSharpInstance::call_multilevel(const StringName &p_method, const Variant *
 	if (script.is_valid()) {
 		MonoObject *mono_object = get_mono_object();
 
+		ERR_FAIL_NULL(mono_object);
+
+		_call_multilevel(mono_object, p_method, p_args, p_argcount);
+	}
+}
+
+void CSharpInstance::_call_multilevel(MonoObject *p_mono_object, const StringName &p_method, const Variant **p_args, int p_argcount) {
+
 	GDMonoClass *top = script->script_class;
 
 	while (top && top != script->native) {
 		GDMonoMethod *method = top->get_method(p_method, p_argcount);
 
 		if (method)
-				method->invoke(mono_object, p_args);
+			method->invoke(p_mono_object, p_args);
 
 		top = top->get_parent_class();
 	}
 }
-}
 
 void CSharpInstance::call_multilevel_reversed(const StringName &p_method, const Variant **p_args, int p_argcount) {
 
 	if (script.is_valid()) {
-		_ml_call_reversed(script->script_class, p_method, p_args, p_argcount);
+		MonoObject *mono_object = get_mono_object();
+
+		ERR_FAIL_NULL(mono_object);
+
+		_ml_call_reversed(mono_object, script->script_class, p_method, p_args, p_argcount);
 	}
 }
 
@@ -1118,7 +1141,7 @@ void CSharpInstance::refcount_incremented() {
 
 	Reference *ref_owner = Object::cast_to<Reference>(owner);
 
-	if (ref_owner->reference_get_count() > 1) { // Remember the managed side holds a reference, hence 1 instead of 0 here
+	if (ref_owner->reference_get_count() > 1) { // The managed side also holds a reference, hence 1 instead of 0
 		// 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.
@@ -1138,7 +1161,7 @@ bool CSharpInstance::refcount_decremented() {
 
 	int refcount = ref_owner->reference_get_count();
 
-	if (refcount == 1) { // Remember the managed side holds a reference, hence 1 instead of 0 here
+	if (refcount == 1) { // The managed side also holds a reference, hence 1 instead of 0
 		// If owner owner is no longer referenced by the unmanaged side,
 		// the managed instance takes responsibility of deleting the owner when GCed.
 
@@ -1207,10 +1230,25 @@ ScriptInstance::RPCMode CSharpInstance::get_rset_mode(const StringName &p_variab
 
 void CSharpInstance::notification(int p_notification) {
 
+	MonoObject *mono_object = get_mono_object();
+
+	if (p_notification == Object::NOTIFICATION_PREDELETE) {
+		if (mono_object != NULL) { // otherwise it was collected, and the finalizer already called NOTIFICATION_PREDELETE
+			call_notification_no_check(mono_object, p_notification);
+			// Set the native instance field to IntPtr.Zero
+			CACHED_FIELD(GodotObject, ptr)->set_value_raw(mono_object, NULL);
+		}
+		return;
+	}
+
+	call_notification_no_check(mono_object, p_notification);
+}
+
+void CSharpInstance::call_notification_no_check(MonoObject *p_mono_object, int p_notification) {
 	Variant value = p_notification;
 	const Variant *args[1] = { &value };
 
-	call_multilevel(CACHED_STRING_NAME(_notification), args, 1);
+	_call_multilevel(p_mono_object, CACHED_STRING_NAME(_notification), args, 1);
 }
 
 Ref<Script> CSharpInstance::get_script() const {

modules/mono/csharp_script.h

@@ -167,7 +167,7 @@ class CSharpInstance : public ScriptInstance {
 	bool base_ref;
 	bool ref_dying;
 
-	void _ml_call_reversed(GDMonoClass *klass, const StringName &p_method, const Variant **p_args, int p_argcount);
+	void _ml_call_reversed(MonoObject *p_mono_object, GDMonoClass *klass, const StringName &p_method, const Variant **p_args, int p_argcount);
 
 	void _reference_owner_unsafe();
 	void _unreference_owner_unsafe();
@@ -176,6 +176,8 @@ class CSharpInstance : public ScriptInstance {
 	friend void GDMonoInternals::tie_managed_to_unmanaged(MonoObject *, Object *);
 	static CSharpInstance *create_for_managed_type(Object *p_owner, CSharpScript *p_script, const Ref<MonoGCHandle> &p_gchandle);
 
+	void _call_multilevel(MonoObject *p_mono_object, const StringName &p_method, const Variant **p_args, int p_argcount);
+
 public:
 	MonoObject *get_mono_object() const;
 
@@ -192,13 +194,14 @@ public:
 
 	void mono_object_disposed();
 
-	void refcount_incremented();
-	bool refcount_decremented();
+	virtual void refcount_incremented();
+	virtual bool refcount_decremented();
 
-	RPCMode get_rpc_mode(const StringName &p_method) const;
-	RPCMode get_rset_mode(const StringName &p_variable) const;
+	virtual RPCMode get_rpc_mode(const StringName &p_method) const;
+	virtual RPCMode get_rset_mode(const StringName &p_variable) const;
 
 	virtual void notification(int p_notification);
+	void call_notification_no_check(MonoObject *p_mono_object, int p_notification);
 
 	virtual Ref<Script> get_script() const;
 
@@ -215,6 +218,8 @@ class CSharpLanguage : public ScriptLanguage {
 
 	static CSharpLanguage *singleton;
 
+	bool finalizing;
+
 	GDMono *gdmono;
 	SelfList<CSharpScript>::List script_list;
 

modules/mono/editor/bindings_generator.cpp

@@ -200,7 +200,7 @@ void BindingsGenerator::_generate_header_icalls() {
 	core_custom_icalls.clear();
 
 	core_custom_icalls.push_back(InternalCall(ICALL_GET_METHODBIND, "IntPtr", "string type, string method"));
-	core_custom_icalls.push_back(InternalCall(ICALL_OBJECT_DTOR, "void", "IntPtr ptr"));
+	core_custom_icalls.push_back(InternalCall(ICALL_OBJECT_DTOR, "void", "object obj, IntPtr ptr"));
 
 	core_custom_icalls.push_back(InternalCall(ICALL_CONNECT_SIGNAL_AWAITER, "Error",
 			"IntPtr source, string signal, IntPtr target, " CS_CLASS_SIGNALAWAITER " awaiter"));
@@ -931,8 +931,8 @@ Error BindingsGenerator::_generate_cs_type(const TypeInterface &itype, const Str
 										  "if (" BINDINGS_PTR_FIELD " != IntPtr.Zero)\n" OPEN_BLOCK_L3
 										  "if (" CS_FIELD_MEMORYOWN ")\n" OPEN_BLOCK_L4 CS_FIELD_MEMORYOWN
 										  " = false;\n" INDENT5 CS_CLASS_NATIVECALLS "." ICALL_OBJECT_DTOR
-										  "(" BINDINGS_PTR_FIELD ");\n" INDENT5 BINDINGS_PTR_FIELD
-										  " = IntPtr.Zero;\n" CLOSE_BLOCK_L4 CLOSE_BLOCK_L3 INDENT3
+										  "(this, " BINDINGS_PTR_FIELD ");\n" CLOSE_BLOCK_L4 CLOSE_BLOCK_L3 INDENT3
+										  "this." BINDINGS_PTR_FIELD " = IntPtr.Zero;\n" INDENT3
 										  "GC.SuppressFinalize(this);\n" INDENT3 "disposed = true;\n" CLOSE_BLOCK_L2);
 
 			Map<StringName, TypeInterface>::Element *array_itype = builtin_types.find(name_cache.type_Array);

modules/mono/glue/glue_header.h

@@ -53,9 +53,11 @@
 	}                                                  \
 	Object *m_instance = ci->creation_func();
 
-void godot_icall_Object_Dtor(Object *ptr) {
-	ERR_FAIL_NULL(ptr);
-	_GodotSharp::get_singleton()->queue_dispose(ptr);
+void godot_icall_Object_Dtor(MonoObject *obj, Object *ptr) {
+#ifdef DEBUG_ENABLED
+	CRASH_COND(ptr == NULL);
+#endif
+	_GodotSharp::get_singleton()->queue_dispose(obj, ptr);
 }
 
 // -- ClassDB --

modules/mono/mono_gc_handle.cpp

@@ -41,10 +41,7 @@ uint32_t MonoGCHandle::make_strong_handle(MonoObject *p_object) {
 
 uint32_t MonoGCHandle::make_weak_handle(MonoObject *p_object) {
 
-	return mono_gchandle_new_weakref(
-			p_object,
-			true /* track_resurrection: allows us to invoke _notification(NOTIFICATION_PREDELETE) while disposing */
-	);
+	return mono_gchandle_new_weakref(p_object, false);
 }
 
 Ref<MonoGCHandle> MonoGCHandle::create_strong(MonoObject *p_object) {

modules/mono/mono_gd/gd_mono.cpp

@@ -703,7 +703,7 @@ bool _GodotSharp::is_domain_loaded() {
 		call_deferred("_dispose_callback");   \
 	}
 
-void _GodotSharp::queue_dispose(Object *p_object) {
+void _GodotSharp::queue_dispose(MonoObject *p_mono_object, Object *p_object) {
 
 	if (GDMonoUtils::is_main_thread() && !GDMono::get_singleton()->is_finalizing_scripts_domain()) {
 		_dispose_object(p_object);
@@ -712,6 +712,13 @@ void _GodotSharp::queue_dispose(Object *p_object) {
 		queue_mutex->lock();
 #endif
 
+		// 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);
+		}
+
 		ENQUEUE_FOR_DISPOSAL(obj_delete_queue, p_object);
 
 #ifndef NO_THREADS

modules/mono/mono_gd/gd_mono.h

@@ -215,7 +215,7 @@ public:
 	bool is_finalizing_domain();
 	bool is_domain_loaded();
 
-	void queue_dispose(Object *p_object);
+	void queue_dispose(MonoObject *p_mono_object, Object *p_object);
 	void queue_dispose(NodePath *p_node_path);
 	void queue_dispose(RID *p_rid);