[tests] rework JavaObjectTest, use FinalizerHelper from mono/mono (#899)

jonathanpeppers · web-flow · commit 220b87fbbb8d · 2021-10-26T12:58:18.000-04:00
Context: dotnet/android#6363 Context: dotnet/runtime#60638 (comment) Context: d1d64c1 Context: dotnet/android@c2c9ed4 We were seeing `JavaObjectTest.Dispose_Finalized()` fail in a .NET 6 bump in xamarin-android with `$(UseInterpreter)` set to `true`: Expected: True But was: False at Java.InteropTests.JavaObjectTest.Dispose_Finalized() at System.Reflection.RuntimeMethodInfo.Invoke(Object , BindingFlags , Binder , Object[] , CultureInfo ) The first recommendation was to use a helper method from mono/mono's unit tests: * https://github.com/mono/mono/blob/8266c5604b8c03882f2b06af27fdea46b142d6b9/mono/mini/TestHelpers.cs#L12 I removed usage of all `System.Threading` in `JavaObjectTest` in favor of this helper method, which internally uses a `Thread`. This did not solve this issue; we need to fix up the test to wait for two GCs to complete, on two separate threads: FinalizerHelpers.PerformNoPinAction (() => { FinalizerHelpers.PerformNoPinAction (() => { var v = new JavaDisposedObject (() => d = true, () => f = true); GC.KeepAlive (v); }); // Thread 2 JniEnvironment.Runtime.ValueManager.CollectPeers (); }); // Thread 1 JniEnvironment.Runtime.ValueManager.CollectPeers (); `ValueManager.CollectPeers()` uses `GC.Collect()`, and `GC.Collect()` needs to be called from two separate threads because, as per @BrzVlad: > if an object is alive then the GC will scan it and, by doing so, it > will probably store it in the stack somewhere. In very unfortunate > scenarios, at the second collection, the pinning step will find this > reference existing on the stack and will pin the object (that might > have now been dead otherwise). Because the `JavaDisposedObject` is > alive during the first collection, we do this collection on a > separate thread, so the second GC doesn't get to find this left over > references on the stack. With this change in place, the test now passes. We can also remove the category added in d1d64c1.
diff --git a/tests/Java.Interop-Tests/Java.Interop/FinalizerHelpers.cs b/tests/Java.Interop-Tests/Java.Interop/FinalizerHelpers.cs
@@ -0,0 +1,47 @@
+// Originally from: https://github.com/mono/mono/blob/8266c5604b8c03882f2b06af27fdea46b142d6b9/mono/mini/TestHelpers.cs#L12
+
+using System;
+using System.Threading;
+
+namespace Java.InteropTests
+{
+	// False pinning cases are still possible. For example the thread can die
+	// and its stack reused by another thread. It also seems that a thread that
+	// does a GC can keep on the stack references to objects it encountered
+	// during the collection which are never released afterwards. This would
+	// be more likely to happen with the interpreter which reuses more stack.
+	static class FinalizerHelpers
+	{
+		private static IntPtr aptr;
+
+		private static unsafe void NoPinActionHelper (int depth, Action act)
+		{
+			// Avoid tail calls
+			int* values = stackalloc int [20];
+			aptr = new IntPtr (values);
+
+			if (depth <= 0) {
+				//
+				// When the action is called, this new thread might have not allocated
+				// anything yet in the nursery. This means that the address of the first
+				// object that would be allocated would be at the start of the tlab and
+				// implicitly the end of the previous tlab (address which can be in use
+				// when allocating on another thread, at checking if an object fits in
+				// this other tlab). We allocate a new dummy object to avoid this type
+				// of false pinning for most common cases.
+				//
+				new object ();
+				act ();
+			} else {
+				NoPinActionHelper (depth - 1, act);
+			}
+		}
+
+		public static void PerformNoPinAction (Action act)
+		{
+			Thread thr = new Thread (() => NoPinActionHelper (128, act));
+			thr.Start ();
+			thr.Join ();
+		}
+	}
+}
diff --git a/tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs b/tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs
@@ -1,5 +1,4 @@
 using System;
-using System.Threading;
 
 using Java.Interop;
 
@@ -18,13 +17,11 @@ public void JavaReferencedInstanceSurvivesCollection ()
 			using (var t = new JniType ("java/lang/Object")) {
 				var oldHandle = IntPtr.Zero;
 				var array     = new JavaObjectArray<JavaObject> (1);
-				var w = new Thread (() => {
+				FinalizerHelpers.PerformNoPinAction (() => {
 						var v       = new JavaObject ();
 						oldHandle   = v.PeerReference.Handle;
 						array [0] = v;
 				});
-				w.Start ();
-				w.Join ();
 				JniEnvironment.Runtime.ValueManager.CollectPeers ();
 				GC.WaitForPendingFinalizers ();
 				GC.WaitForPendingFinalizers ();
@@ -80,13 +77,11 @@ public void UnreferencedInstanceIsCollected ()
 		{
 			JniObjectReference  oldHandle = new JniObjectReference ();
 			WeakReference r = null;
-			var t = new Thread (() => {
+			FinalizerHelpers.PerformNoPinAction (() => {
 					var v     = new JavaObject ();
 					oldHandle = v.PeerReference.NewWeakGlobalRef ();
 					r         = new WeakReference (v);
 			});
-			t.Start ();
-			t.Join ();
 			JniEnvironment.Runtime.ValueManager.CollectPeers ();
 			GC.WaitForPendingFinalizers ();
 			GC.WaitForPendingFinalizers ();
@@ -110,20 +105,17 @@ public void Dispose ()
 
 #if !NO_GC_BRIDGE_SUPPORT
 		[Test]
-		// See: https://github.com/dotnet/runtime/issues/60638
-		[Category ("IgnoreInterpreter")]
 		public void Dispose_Finalized ()
 		{
 			var d = false;
 			var f = false;
-			var t = new Thread (() => {
-				var v     = new JavaDisposedObject (() => d = true, () => f = true);
-				GC.KeepAlive (v);
+			FinalizerHelpers.PerformNoPinAction (() => {
+				FinalizerHelpers.PerformNoPinAction (() => {
+					var v     = new JavaDisposedObject (() => d = true, () => f = true);
+					GC.KeepAlive (v);
+				});
+				JniEnvironment.Runtime.ValueManager.CollectPeers ();
 			});
-			t.Start ();
-			t.Join ();
-			JniEnvironment.Runtime.ValueManager.CollectPeers ();
-			GC.WaitForPendingFinalizers ();
 			JniEnvironment.Runtime.ValueManager.CollectPeers ();
 			GC.WaitForPendingFinalizers ();
 			Assert.IsFalse (d);
@@ -185,11 +177,9 @@ public void Ctor_Exceptions ()
 		public void CrossThreadSharingRequresRegistration ()
 		{
 			JavaObject o = null;
-			var t = new Thread (() => {
+			FinalizerHelpers.PerformNoPinAction (() => {
 					o = new JavaObject ();
 			});
-			t.Start ();
-			t.Join ();
 			o.ToString ();
 			o.Dispose ();
 		}
