[2020-02] [cominterop] Add coop handle enter/return on native CCW methods (#21366)mono-6.12.0.163

* [tests] Add CCW GetIUnknownForObject leak test

* [cominterop] Add coop handle enter/return on native CCW methods

The CCW methods for IUnknown (and in principle IDispatch - except they all have
trivial bodies) are native C code in the runtime that may allocate coop
handles.  Add a coop handle frame around the entire call in order to make sure
they're cleaned up and don't retain a reference.

This helps fix managed object leaks with code like:

```
   var o  = new SomeClass();
   var pUnk = Marshal.GetIUnknownForObject(o);
   int c = Marshal.Release(pUnk);
   o = null;
```

Which retains a reference to the `SomeClass` instance that won't be collected
until the thread dies, despite cleaning up the IUnknown refcount. The underling
ccw addref/release methods leak coop handles on the thread.

This is not an issue when the CCW calls some managed method because there are
no coop handles there until some icall (at which point it will set up the
coop handle stack properly).

Co-authored-by: Aleksey Kliger <alklig@microsoft.com>

2 files changed, 85 insertions, 20 deletions

diff --git a/mono/metadata/cominterop.c b/mono/metadata/cominterop.c
index 9ec1d0f10c3..d0e2241063c 100644
--- a/mono/metadata/cominterop.c
+++ b/mono/metadata/cominterop.c
@@ -173,6 +173,31 @@ typedef struct {
 	MonoCCW* ccw;
 } MonoCCWInterface;
 
+/*
+ * COM Callable Wrappers
+ *
+ * CCWs may be called on threads that aren't attached to the runtime, they can
+ * then run managed code or the method implementations may use coop handles.
+ * Use the macros below to setup the thread state.
+ *
+ * For managed methods, the runtime marshaling wrappers handle attaching and
+ * coop state switching.
+ */
+
+#define MONO_CCW_CALL_ENTER do {					\
+	gpointer dummy;							\
+	gpointer orig_domain = mono_threads_attach_coop (mono_domain_get (), &dummy); \
+	MONO_ENTER_GC_UNSAFE;						\
+	HANDLE_FUNCTION_ENTER ();					\
+	do {} while (0)
+
+#define MONO_CCW_CALL_EXIT				\
+	HANDLE_FUNCTION_RETURN ();			\
+	MONO_EXIT_GC_UNSAFE;				\
+	mono_threads_detach_coop (orig_domain, &dummy); \
+	} while (0)
+
+
 /* IUnknown */
 static int STDCALL cominterop_ccw_addref (MonoCCWInterface* ccwe);
 
@@ -2568,12 +2593,9 @@ static int STDCALL
 cominterop_ccw_addref (MonoCCWInterface* ccwe)
 {
 	int result;
-	gpointer dummy;
-	gpointer orig_domain = mono_threads_attach_coop (mono_domain_get (), &dummy);
-	MONO_ENTER_GC_UNSAFE;
+	MONO_CCW_CALL_ENTER;
 	result = cominterop_ccw_addref_impl (ccwe);
-	MONO_EXIT_GC_UNSAFE;
-	mono_threads_detach_coop (orig_domain, &dummy);
+	MONO_CCW_CALL_EXIT;
 	return result;
 }
 
@@ -2602,12 +2624,9 @@ static int STDCALL
 cominterop_ccw_release (MonoCCWInterface* ccwe)
 {
 	int result;
-	gpointer dummy;
-	gpointer orig_domain = mono_threads_attach_coop (mono_domain_get (), &dummy);
-	MONO_ENTER_GC_UNSAFE;
+	MONO_CCW_CALL_ENTER;
 	result = cominterop_ccw_release_impl (ccwe);
-	MONO_EXIT_GC_UNSAFE;
-	mono_threads_detach_coop (orig_domain, &dummy);
+	MONO_CCW_CALL_EXIT;
 	return result;
 }
 
@@ -2654,12 +2673,9 @@ static int STDCALL
 cominterop_ccw_queryinterface (MonoCCWInterface* ccwe, const guint8* riid, gpointer* ppv)
 {
 	int result;
-	gpointer dummy;
-	gpointer orig_domain = mono_threads_attach_coop (mono_domain_get (), &dummy);
-	MONO_ENTER_GC_UNSAFE;
+	MONO_CCW_CALL_ENTER;
 	result = cominterop_ccw_queryinterface_impl (ccwe, riid, ppv);
-	MONO_EXIT_GC_UNSAFE;
-	mono_threads_detach_coop (orig_domain, &dummy);
+	MONO_CCW_CALL_EXIT;
 	return result;
 }
 
@@ -2777,12 +2793,9 @@ cominterop_ccw_get_ids_of_names (MonoCCWInterface* ccwe, gpointer riid,
 											 guint32 lcid, gint32 *rgDispId)
 {
 	int result;
-	gpointer dummy;
-	gpointer orig_domain = mono_threads_attach_coop (mono_domain_get(), &dummy);
-	MONO_ENTER_GC_UNSAFE;
+	MONO_CCW_CALL_ENTER;
 	result = cominterop_ccw_get_ids_of_names_impl (ccwe, riid, rgszNames, cNames, lcid, rgDispId);
-	MONO_EXIT_GC_UNSAFE;
-	mono_threads_detach_coop (orig_domain, &dummy);
+	MONO_CCW_CALL_EXIT;
 	return result;
 }
 
diff --git a/mono/tests/cominterop.cs b/mono/tests/cominterop.cs
index 11cb68bcdbc..3ff018efa71 100644
--- a/mono/tests/cominterop.cs
+++ b/mono/tests/cominterop.cs
@@ -659,6 +659,9 @@ public class Tests
 				}
 			}
 
+			if (CallableWrapperLeakTest () != 0)
+			    return 209;
+
 			#endregion // COM Callable Wrapper Tests
 
 			#region SAFEARRAY tests
@@ -1630,6 +1633,55 @@ public class Tests
                 return 2;
             return 0;
         }
+
+	// Doesn't matter what this is
+	internal class CallableWrapperLeakTestClass {
+	}
+
+	public static int CallableWrapperLeakTest () {
+		bool ok = true;
+		GCHandle h = HideFromGC (() => {
+			var o = new CallableWrapperLeakTestClass();
+			var weakHandle = GCHandle.Alloc (o, GCHandleType.Weak);
+			var pUnk = Marshal.GetIUnknownForObject (o);
+			int c = Marshal.Release(pUnk);
+			o = null;
+			if (c != 0) {
+				Console.Error.WriteLine ("Expected IUnknown refcount on a CCW to be 0 after Release, was {0}", c);
+				ok = false;
+			}
+			return weakHandle;
+		});
+		if (!ok)
+			return 1;
+		GC.Collect();
+		GC.WaitForPendingFinalizers();
+		GC.Collect();
+		GC.WaitForPendingFinalizers();
+		HideFromGC(() => {
+			var o = h.Target;
+			if (o != null) {
+				Console.Error.WriteLine ("Expected weak handle to be null after GC, but the object (of type {0}) was retained", o.GetType());
+				ok = false;
+			}
+			return "done"; // doesn't matter
+		});
+		if (!ok)
+			return 2;
+		return 0;
+	}
+
+	[MethodImpl (MethodImplOptions.NoInlining)]
+	private static T HideFromGC<T> (Func<T> f) => HideFromGC (20, f);
+
+	[MethodImpl (MethodImplOptions.NoInlining)]
+	private static T HideFromGC<T> (int rec, Func<T> f) {
+		if (rec <= 0)
+			return f ();
+		else
+			return HideFromGC (rec - 1, f);
+	}
+
 }
 
 public class TestVisible