From e3ca891235609663785d70aa1bbc544232070321 Mon Sep 17 00:00:00 2001
From: Vlad Brezae <brezaevlad@gmail.com>
Date: Thu, 1 Aug 2019 16:06:05 +0300
Subject: [PATCH] [marshal] Free delegates with target that are passed to
 native code. (#15935)

* [marshal] Always use gchandles in delegate_hash_table

Makes the code easier to follow and it also fixes race from https://github.com/mono/mono/commit/caa4a753ca8e15d43baaa01adb0f56f374b74a2b with boehm.

* [marshal] Free delegates with target that are passed to native code.

For static method delegates, we have a unique delegate_trampoline that is shared among all delegates. We always keep alive the first static method delegate passed to native, by creating a normal gchandle to it and storing it in delegate_hash_table. For instance methods, each delegate will create a separate wrapper and these wrappers are never shared (which was wrongly assumed in https://github.com/mono/mono/commit/caa4a753ca8e15d43baaa01adb0f56f374b74a2b). We shuldn't keep the delegate alive and this commit reverts the behavior for delegate with instance methods introduced in the mentioned commit.

Fixes https://github.com/mono/mono/issues/15751
---
 mono/metadata/marshal.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/mono/metadata/marshal.c b/mono/metadata/marshal.c
index 5b3fdfd02b4..97f090bee7c 100644
--- a/mono/metadata/marshal.c
+++ b/mono/metadata/marshal.c
@@ -363,14 +363,19 @@ delegate_hash_table_new (void) {
 static void 
 delegate_hash_table_remove (MonoDelegate *d)
 {
-	if (mono_gc_is_moving ())
+	guint32 gchandle = 0;
+
+	if (!d->target)
 		return;
 
 	mono_marshal_lock ();
 	if (delegate_hash_table == NULL)
 		delegate_hash_table = delegate_hash_table_new ();
+	gchandle = GPOINTER_TO_UINT (g_hash_table_lookup (delegate_hash_table, d->delegate_trampoline));
 	g_hash_table_remove (delegate_hash_table, d->delegate_trampoline);
 	mono_marshal_unlock ();
+	if (gchandle)
+		mono_gchandle_free_internal (gchandle);
 }
 
 static void
@@ -380,7 +385,19 @@ delegate_hash_table_add (MonoDelegateHandle d)
 	if (delegate_hash_table == NULL)
 		delegate_hash_table = delegate_hash_table_new ();
 	gpointer delegate_trampoline = MONO_HANDLE_GETVAL (d, delegate_trampoline);
-	if (mono_gc_is_moving ()) {
+	gboolean has_target = MONO_HANDLE_GETVAL (d, target) != NULL;
+	if (has_target) {
+		// If the delegate has an instance method there is 1 to 1 mapping between
+		// the delegate object and the delegate_trampoline
+		guint32 gchandle = GPOINTER_TO_UINT (g_hash_table_lookup (delegate_hash_table, delegate_trampoline));
+		if (gchandle) {
+			// Somehow, some other thread beat us to it ?
+			g_assert (mono_gchandle_target_equal (gchandle, MONO_HANDLE_CAST (MonoObject, d)));
+		} else {
+			gchandle = mono_gchandle_new_weakref_from_handle (MONO_HANDLE_CAST (MonoObject, d));
+			g_hash_table_insert (delegate_hash_table, delegate_trampoline, GUINT_TO_POINTER (gchandle));
+		}
+	} else {
 		if (g_hash_table_lookup (delegate_hash_table, delegate_trampoline) == NULL) {
 			guint32 gchandle = mono_gchandle_from_handle (MONO_HANDLE_CAST (MonoObject, d), FALSE);
 			// This delegate will always be associated with its delegate_trampoline in the table.
@@ -388,8 +405,6 @@ delegate_hash_table_add (MonoDelegateHandle d)
 			// pairs and avoid races with the delegate finalization.
 			g_hash_table_insert (delegate_hash_table, delegate_trampoline, GUINT_TO_POINTER (gchandle));
 		}
-	} else {
-		g_hash_table_insert (delegate_hash_table, delegate_trampoline, MONO_HANDLE_RAW (d));
 	}
 	mono_marshal_unlock ();
 }
@@ -452,16 +467,11 @@ mono_ftnptr_to_delegate_impl (MonoClass *klass, gpointer ftn, MonoError *error)
 	mono_marshal_lock ();
 	if (delegate_hash_table == NULL)
 		delegate_hash_table = delegate_hash_table_new ();
+	gchandle = GPOINTER_TO_UINT (g_hash_table_lookup (delegate_hash_table, ftn));
+	mono_marshal_unlock ();
+	if (gchandle)
+		MONO_HANDLE_ASSIGN (d, MONO_HANDLE_CAST (MonoDelegate, mono_gchandle_get_target_handle (gchandle)));
 
-	if (mono_gc_is_moving ()) {
-		gchandle = GPOINTER_TO_UINT (g_hash_table_lookup (delegate_hash_table, ftn));
-		mono_marshal_unlock ();
-		if (gchandle)
-			MONO_HANDLE_ASSIGN (d, MONO_HANDLE_CAST (MonoDelegate, mono_gchandle_get_target_handle (gchandle)));
-	} else {
-		MONO_HANDLE_ASSIGN (d, MONO_HANDLE_NEW (MonoDelegate, (MonoDelegate*)g_hash_table_lookup (delegate_hash_table, ftn)));
-		mono_marshal_unlock ();
-	}
 	if (MONO_HANDLE_IS_NULL (d)) {
 		/* This is a native function, so construct a delegate for it */
 		MonoMethodSignature *sig;
-- 
2.11.4.GIT