[generator] Use GC.KeepAlive for reference type method parameters. (#722)

Context: #719

@brendanzagaeski has been investigating a Xamarin.Android app crash:

	JNI DETECTED ERROR IN APPLICATION: use of deleted global reference 0x3d86
	    from android.view.View crc64720bb2db43a66fe9.FragmentContainer.n_onCreateView(android.view.LayoutInflater, android.view.ViewGroup, android.os.Bundle)
	…
	  at crc64720bb2db43a66fe9.FragmentContainer.n_onCreateView(Native method)
	  at crc64720bb2db43a66fe9.FragmentContainer.onCreateView(FragmentContainer.java:41)

This had been a head-scratcher, but we had a GREF log, so all should
be clear, right?

	09-10 17:56:48.280 10123 10123 I monodroid-gref: +g+ grefc 1141 gwrefc 0 obj-handle 0x79/I -> new-handle 0x3d86/G from thread '(null)'(1)
	09-10 17:56:48.294 10123 10123 I monodroid-gref: +w+ grefc 1140 gwrefc 2 obj-handle 0x3d86/G -> new-handle 0x1e3/W from thread 'finalizer'(10123)
	09-10 17:56:48.294 10123 10123 I monodroid-gref: -g- grefc 1139 gwrefc 2 handle 0x3d86/G from thread 'finalizer'(10123)

The GREF log *wasn't* immediately clear: sure, the GREF was turned
into a Weak-GREF, and the Weak-GREF was then collected, but none of
this explained *how* were were using this deleted GREF.

(We were at this state of affairs for months: we "know" we're using
a deleted GREF, but we don't know *how* or *why*.  It was a very hard
to hit bug.)

Eventually we had a ["that's funny"][0] event: *sure*, the GREF is
deleted, but:

 1. Why is it being deleted by the finalizer?
 2. …14ms *after construction*?

A [Garbage Collection][1] refresher may be in order, but in short:

 1. All `Java.Lang.Object` subclasses are "bridged" objects.

 2. During a GC, all "collectable" bridged objects are gathered.
    A collectable object is one in which nothing in the managed
    GC references the object.

 3. Once the GC is complete, all gathered collectable bridged objects
    are passed to a "cross references" callback.

    The callback is called *outside* the "scope" of a GC; "the world"
    is *not* frozen, other threads may be executing.

 4. The "cross references" callback is the
    `MonoGCBridgeCallbacks::cross_references` field provided provided
    to [`mono_gc_register_bridge_callbacks()`][2].
    
    In a Xamarin.Android app, the "cross references" callback will
    "toggle" a JNI Global Reference to a JNI Weak Global Reference,
    invoke a Java-side GC, and then try to obtain a
    JNI Global Reference from the JNI Weak Global Reference.
    If a non-`NULL` pointer is returned, the bridged object is kept
    alive.  If a `NULL` pointer is returned, the bridged object will
    be considered dead, and will be added to the Finalization Queue
    (as `Java.Lang.Object` has a finalizer).

Thus, it seemed "funny" that within 14ms an instance was created,
GC'd, and determined to be garbage, *especially* when we *knew* that
this instance was being passed to Java, which we expected to retain
the instance.  (Yet wasn't…?)

After much banging of heads, and the yeoman's work of creating a
simplified and consistently reproducible test case, we *think* we
know the cause of the crash.

Consider our normal Managed-to-Java marshaling code, e.g.

	partial class MaterialButton  {
	    public unsafe MaterialButton (global::Android.Content.Context context)
	    	: base (IntPtr.Zero, JniHandleOwnership.DoNotTransfer)
	    {
	        const string __id = "(Landroid/content/Context;)V";

	        if (((global::Java.Lang.Object) this).Handle != IntPtr.Zero)
	            return;

	        try {
	            JniArgumentValue* __args = stackalloc JniArgumentValue [1];
	/* 1 */     __args [0] = new JniArgumentValue ((context == null) ? IntPtr.Zero : ((global::Java.Lang.Object) context).Handle);
	/* 2 */     var __r = _members.InstanceMethods.StartCreateInstance (__id, ((object) this).GetType (), __args);
	            SetHandle (__r.Handle, JniHandleOwnership.TransferLocalRef);
	/* 3 */     _members.InstanceMethods.FinishCreateInstance (__id, this, __args);
	        } finally {
	        }
	    }
	}

At (1), `context.Handle` is -- a JNI GREF -- is stored into a
`JniArgumentValue* __args` value, and at (3) `__args` is passed into
JNI, which will presumably "Do Something" with that handle.

However, nothing ties `context.Handle` to `context`, so from
(2) onward, `context` *may* be eligible for garbage collection.

See also Chris Brumme's [Lifetime, GC.KeepAlive, handle recycling][3]
blog article.  It's about .NET Framework, but the same fundamental
multithreading concepts apply.

The fix is to *ensure* that `context` is kept alive
*for as long as* `context.Handle` will be used, i.e. across the
JNI `_members.InstanceMethods.FinishCreateInstance()` call:

	partial class MaterialButton  {
	    public unsafe MaterialButton (global::Android.Content.Context context)
	    	: base (IntPtr.Zero, JniHandleOwnership.DoNotTransfer)
	    {
	        const string __id = "(Landroid/content/Context;)V";

	        if (((global::Java.Lang.Object) this).Handle != IntPtr.Zero)
	            return;

	        try {
	            JniArgumentValue* __args = stackalloc JniArgumentValue [1];
	/* 1 */     __args [0] = new JniArgumentValue ((context == null) ? IntPtr.Zero : ((global::Java.Lang.Object) context).Handle);
	/* 2 */     var __r = _members.InstanceMethods.StartCreateInstance (__id, ((object) this).GetType (), __args);
	            SetHandle (__r.Handle, JniHandleOwnership.TransferLocalRef);
	/* 3 */     _members.InstanceMethods.FinishCreateInstance (__id, this, __args);
	        } finally {
	/* 4 */     global::System.GC.KeepAlive (context);
	        }
	    }
	}

This should prevent e.g. `context` from being prematurely GC'd,
which in turn should prevent the `JNI DETECTED ERROR` message.

Update `tools/generator` to emit `GC.KeepAlive()` statements for
every parameter type which isn't a value type (`enum`, `int`, `string`,
etc.).  `string` is considered a value type because we always send a
"deep copy" of the string contents, so it won't matter if the `string`
instance is GC'd immediately.

[0]: https://quoteinvestigator.com/2015/03/02/eureka-funny/
[1]: https://docs.microsoft.com/xamarin/android/internals/garbage-collection
[2]: http://docs.go-mono.com/?link=xhtml%3adeploy%2fmono-api-gc.html
[3]: https://docs.microsoft.com/archive/blogs/cbrumme/lifetime-gc-keepalive-handle-recycling

Author: jpobst

tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/JavaInterop1/WriteCtorWithStringOverload.txt

@@ -17,6 +17,7 @@
 		_members.InstanceMethods.FinishCreateInstance (__id, this, __args);
 	} finally {
 		JNIEnv.DeleteLocalRef (native_mystring);
+		global::System.GC.KeepAlive (mystring);
 	}
 }
 
@@ -38,6 +39,7 @@
 		_members.InstanceMethods.FinishCreateInstance (__id, this, __args);
 	} finally {
 		JNIEnv.DeleteLocalRef (native_mystring);
+		global::System.GC.KeepAlive (mystring);
 	}
 }
 

tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/JavaInterop1/WriteKotlinUnsignedArrayTypeMethodsClass.txt

@@ -27,6 +27,7 @@ public partial class MyClass  {
 				JNIEnv.CopyArray (native_value, value);
 				JNIEnv.DeleteLocalRef (native_value);
 			}
+			global::System.GC.KeepAlive (value);
 		}
 	}
 
@@ -46,6 +47,7 @@ public partial class MyClass  {
 				JNIEnv.CopyArray (native_value, value);
 				JNIEnv.DeleteLocalRef (native_value);
 			}
+			global::System.GC.KeepAlive (value);
 		}
 	}
 
@@ -65,6 +67,7 @@ public partial class MyClass  {
 				JNIEnv.CopyArray (native_value, value);
 				JNIEnv.DeleteLocalRef (native_value);
 			}
+			global::System.GC.KeepAlive (value);
 		}
 	}
 
@@ -84,6 +87,7 @@ public partial class MyClass  {
 				JNIEnv.CopyArray (native_value, value);
 				JNIEnv.DeleteLocalRef (native_value);
 			}
+			global::System.GC.KeepAlive (value);
 		}
 	}
 

tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/JavaInterop1/WriteKotlinUnsignedArrayTypePropertiesClass.txt

@@ -36,6 +36,7 @@ public partial class MyClass  {
 					JNIEnv.CopyArray (native_value, value);
 					JNIEnv.DeleteLocalRef (native_value);
 				}
+				global::System.GC.KeepAlive (value);
 			}
 		}
 	}
@@ -65,6 +66,7 @@ public partial class MyClass  {
 					JNIEnv.CopyArray (native_value, value);
 					JNIEnv.DeleteLocalRef (native_value);
 				}
+				global::System.GC.KeepAlive (value);
 			}
 		}
 	}
@@ -94,6 +96,7 @@ public partial class MyClass  {
 					JNIEnv.CopyArray (native_value, value);
 					JNIEnv.DeleteLocalRef (native_value);
 				}
+				global::System.GC.KeepAlive (value);
 			}
 		}
 	}
@@ -123,6 +126,7 @@ public partial class MyClass  {
 					JNIEnv.CopyArray (native_value, value);
 					JNIEnv.DeleteLocalRef (native_value);
 				}
+				global::System.GC.KeepAlive (value);
 			}
 		}
 	}

tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/XAJavaInterop1-NRT/WriteCtorWithStringOverload.txt

@@ -17,6 +17,7 @@
 		_members.InstanceMethods.FinishCreateInstance (__id, this, __args);
 	} finally {
 		JNIEnv.DeleteLocalRef (native_mystring);
+		global::System.GC.KeepAlive (mystring);
 	}
 }
 
@@ -38,6 +39,7 @@
 		_members.InstanceMethods.FinishCreateInstance (__id, this, __args);
 	} finally {
 		JNIEnv.DeleteLocalRef (native_mystring);
+		global::System.GC.KeepAlive (mystring);
 	}
 }
 

tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/XAJavaInterop1/WriteCtorWithStringOverload.txt

@@ -17,6 +17,7 @@
 		_members.InstanceMethods.FinishCreateInstance (__id, this, __args);
 	} finally {
 		JNIEnv.DeleteLocalRef (native_mystring);
+		global::System.GC.KeepAlive (mystring);
 	}
 }
 
@@ -38,6 +39,7 @@
 		_members.InstanceMethods.FinishCreateInstance (__id, this, __args);
 	} finally {
 		JNIEnv.DeleteLocalRef (native_mystring);
+		global::System.GC.KeepAlive (mystring);
 	}
 }
 

tests/generator-Tests/expected.ji/Adapters/Xamarin.Test.AbsSpinner.cs

@@ -83,6 +83,7 @@ static void n_SetAdapter_Lxamarin_test_SpinnerAdapter_ (IntPtr jnienv, IntPtr na
 					__args [0] = new JniArgumentValue ((value == null) ? IntPtr.Zero : ((global::Java.Lang.Object) value).Handle);
 					_members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
 				} finally {
+					global::System.GC.KeepAlive (value);
 				}
 			}
 		}
@@ -126,6 +127,7 @@ public AbsSpinnerInvoker (IntPtr handle, JniHandleOwnership transfer) : base (ha
 					_members.InstanceMethods.InvokeAbstractVoidMethod (__id, this, __args);
 				} finally {
 					JNIEnv.DeleteLocalRef (native_value);
+					global::System.GC.KeepAlive (value);
 				}
 			}
 		}

tests/generator-Tests/expected.ji/Adapters/Xamarin.Test.AdapterView.cs

@@ -110,6 +110,7 @@ public AdapterViewInvoker (IntPtr handle, JniHandleOwnership transfer) : base (h
 					_members.InstanceMethods.InvokeAbstractVoidMethod (__id, this, __args);
 				} finally {
 					JNIEnv.DeleteLocalRef (native_value);
+					global::System.GC.KeepAlive (value);
 				}
 			}
 		}

tests/generator-Tests/expected.ji/Core_Jar2Xml/Android.Text.SpannableString.cs

@@ -49,6 +49,7 @@ public unsafe SpannableString (global::Java.Lang.ICharSequence source)
 				_members.InstanceMethods.FinishCreateInstance (__id, this, __args);
 			} finally {
 				JNIEnv.DeleteLocalRef (native_source);
+				global::System.GC.KeepAlive (source);
 			}
 		}
 
@@ -70,6 +71,7 @@ public unsafe SpannableString (string source)
 				_members.InstanceMethods.FinishCreateInstance (__id, this, __args);
 			} finally {
 				JNIEnv.DeleteLocalRef (native_source);
+				global::System.GC.KeepAlive (source);
 			}
 		}
 
@@ -102,6 +104,7 @@ static int n_GetSpanFlags_Ljava_lang_Object_ (IntPtr jnienv, IntPtr native__this
 				var __rm = _members.InstanceMethods.InvokeVirtualInt32Method (__id, this, __args);
 				return (global::Android.Text.SpanTypes) __rm;
 			} finally {
+				global::System.GC.KeepAlive (what);
 			}
 		}
 

tests/generator-Tests/expected.ji/Core_Jar2Xml/Android.Text.SpannableStringInternal.cs

@@ -60,6 +60,7 @@ static int n_GetSpanFlags_Ljava_lang_Object_ (IntPtr jnienv, IntPtr native__this
 				var __rm = _members.InstanceMethods.InvokeVirtualInt32Method (__id, this, __args);
 				return (global::Android.Text.SpanTypes) __rm;
 			} finally {
+				global::System.GC.KeepAlive (p0);
 			}
 		}
 

tests/generator-Tests/expected.ji/Core_Jar2Xml/Android.Views.View.cs

@@ -176,6 +176,7 @@ public virtual unsafe void SetOnClickListener (global::Android.Views.View.IOnCli
 				__args [0] = new JniArgumentValue ((l == null) ? IntPtr.Zero : ((global::Java.Lang.Object) l).Handle);
 				_members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
 			} finally {
+				global::System.GC.KeepAlive (l);
 			}
 		}
 
@@ -206,6 +207,7 @@ public virtual unsafe void SetOn123Listener (global::Android.Views.View.IOnClick
 				__args [0] = new JniArgumentValue ((l == null) ? IntPtr.Zero : ((global::Java.Lang.Object) l).Handle);
 				_members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
 			} finally {
+				global::System.GC.KeepAlive (l);
 			}
 		}
 
@@ -238,6 +240,7 @@ public virtual unsafe void AddTouchables (global::System.Collections.Generic.ILi
 				_members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
 			} finally {
 				JNIEnv.DeleteLocalRef (native_views);
+				global::System.GC.KeepAlive (views);
 			}
 		}
 

tests/generator-Tests/expected.ji/GenericArguments/Com.Google.Android.Exoplayer.Drm.FrameworkMediaDrm.cs

@@ -58,6 +58,7 @@ public unsafe void SetOnEventListener (global::Com.Google.Android.Exoplayer.Drm.
 				__args [0] = new JniArgumentValue ((p0 == null) ? IntPtr.Zero : ((global::Java.Lang.Object) p0).Handle);
 				_members.InstanceMethods.InvokeAbstractVoidMethod (__id, this, __args);
 			} finally {
+				global::System.GC.KeepAlive (p0);
 			}
 		}
 

tests/generator-Tests/expected.ji/NestedTypes/Xamarin.Test.NotificationCompatBase.cs

@@ -186,6 +186,7 @@ public unsafe InstanceInner (global::Xamarin.Test.NotificationCompatBase __self)
 					SetHandle (__r.Handle, JniHandleOwnership.TransferLocalRef);
 					_members.InstanceMethods.FinishCreateInstance (__id, this, __args);
 				} finally {
+					global::System.GC.KeepAlive (__self);
 				}
 			}
 

tests/generator-Tests/expected.ji/NormalMethods/Xamarin.Test.SomeObject.cs

@@ -47,6 +47,7 @@ public unsafe SomeObject (global::Java.Lang.Class c)
 				SetHandle (__r.Handle, JniHandleOwnership.TransferLocalRef);
 				_members.InstanceMethods.FinishCreateInstance (__id, this, __args);
 			} finally {
+				global::System.GC.KeepAlive (c);
 			}
 		}
 
@@ -109,6 +110,8 @@ static int n_Handle_Ljava_lang_Object_Ljava_lang_Throwable_ (IntPtr jnienv, IntP
 				var __rm = _members.InstanceMethods.InvokeVirtualInt32Method (__id, this, __args);
 				return __rm;
 			} finally {
+				global::System.GC.KeepAlive (o);
+				global::System.GC.KeepAlive (t);
 			}
 		}
 
@@ -255,6 +258,7 @@ public virtual unsafe void VoidMethodWithParams (string astring, int anint, glob
 				_members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
 			} finally {
 				JNIEnv.DeleteLocalRef (native_astring);
+				global::System.GC.KeepAlive (anObject);
 			}
 		}
 
@@ -318,6 +322,7 @@ public virtual unsafe void ArrayListTest (global::System.Collections.Generic.ILi
 				_members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
 			} finally {
 				JNIEnv.DeleteLocalRef (native_p0);
+				global::System.GC.KeepAlive (p0);
 			}
 		}
 

tests/generator-Tests/expected.ji/NormalProperties/Xamarin.Test.SomeObject.cs

@@ -210,6 +210,7 @@ public override unsafe int SomeInteger {
 					__args [0] = new JniArgumentValue ((value == null) ? IntPtr.Zero : ((global::Java.Lang.Object) value).Handle);
 					_members.InstanceMethods.InvokeAbstractVoidMethod (__id, this, __args);
 				} finally {
+					global::System.GC.KeepAlive (value);
 				}
 			}
 		}

tests/generator-Tests/expected.ji/ParameterXPath/Xamarin.Test.A.cs

@@ -60,6 +60,7 @@ public virtual unsafe void SetA (global::Java.Lang.Object adapter)
 				_members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
 			} finally {
 				JNIEnv.DeleteLocalRef (native_adapter);
+				global::System.GC.KeepAlive (adapter);
 			}
 		}
 
@@ -92,6 +93,7 @@ public virtual unsafe void ListTest (global::System.Collections.Generic.IList<gl
 				_members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
 			} finally {
 				JNIEnv.DeleteLocalRef (native_p0);
+				global::System.GC.KeepAlive (p0);
 			}
 		}
 

tests/generator-Tests/expected.ji/StaticProperties/Xamarin.Test.SomeObject.cs

@@ -102,6 +102,7 @@ public static unsafe void SetSomeObject (global::Java.Lang.Object newvalue)
 				__args [0] = new JniArgumentValue ((newvalue == null) ? IntPtr.Zero : ((global::Java.Lang.Object) newvalue).Handle);
 				_members.StaticMethods.InvokeVoidMethod (__id, __args);
 			} finally {
+				global::System.GC.KeepAlive (newvalue);
 			}
 		}
 

tests/generator-Tests/expected.ji/Streams/Java.IO.FilterOutputStream.cs

@@ -49,6 +49,7 @@ public unsafe FilterOutputStream (global::System.IO.Stream @out)
 				_members.InstanceMethods.FinishCreateInstance (__id, this, __args);
 			} finally {
 				JNIEnv.DeleteLocalRef (native__out);
+				global::System.GC.KeepAlive (@out);
 			}
 		}
 

tests/generator-Tests/expected.ji/Streams/Java.IO.InputStream.cs

@@ -216,6 +216,7 @@ public virtual unsafe int Read (byte[] buffer)
 					JNIEnv.CopyArray (native_buffer, buffer);
 					JNIEnv.DeleteLocalRef (native_buffer);
 				}
+				global::System.GC.KeepAlive (buffer);
 			}
 		}
 
@@ -257,6 +258,7 @@ public virtual unsafe int Read (byte[] buffer, int byteOffset, int byteCount)
 					JNIEnv.CopyArray (native_buffer, buffer);
 					JNIEnv.DeleteLocalRef (native_buffer);
 				}
+				global::System.GC.KeepAlive (buffer);
 			}
 		}
 

tests/generator-Tests/expected.ji/Streams/Java.IO.OutputStream.cs

@@ -136,6 +136,7 @@ public virtual unsafe void Write (byte[] buffer)
 					JNIEnv.CopyArray (native_buffer, buffer);
 					JNIEnv.DeleteLocalRef (native_buffer);
 				}
+				global::System.GC.KeepAlive (buffer);
 			}
 		}
 
@@ -175,6 +176,7 @@ public virtual unsafe void Write (byte[] buffer, int offset, int count)
 					JNIEnv.CopyArray (native_buffer, buffer);
 					JNIEnv.DeleteLocalRef (native_buffer);
 				}
+				global::System.GC.KeepAlive (buffer);
 			}
 		}
 

tests/generator-Tests/expected.ji/TestInterface/Test.ME.GenericImplementation.cs

@@ -82,6 +82,7 @@ public virtual unsafe void SetObject (byte[] value)
 					JNIEnv.CopyArray (native_value, value);
 					JNIEnv.DeleteLocalRef (native_value);
 				}
+				global::System.GC.KeepAlive (value);
 			}
 		}
 

tests/generator-Tests/expected.ji/TestInterface/Test.ME.GenericObjectPropertyImplementation.cs

@@ -101,6 +101,7 @@ static void n_SetObject_Ljava_lang_Object_ (IntPtr jnienv, IntPtr native__this,
 					__args [0] = new JniArgumentValue ((value == null) ? IntPtr.Zero : ((global::Java.Lang.Object) value).Handle);
 					_members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
 				} finally {
+					global::System.GC.KeepAlive (value);
 				}
 			}
 		}

tests/generator-Tests/expected.ji/TestInterface/Test.ME.GenericStringImplementation.cs

@@ -82,6 +82,7 @@ public virtual unsafe void SetObject (string[] value)
 					JNIEnv.CopyArray (native_value, value);
 					JNIEnv.DeleteLocalRef (native_value);
 				}
+				global::System.GC.KeepAlive (value);
 			}
 		}
 

tests/generator-Tests/expected.ji/TestInterface/Test.ME.TestInterfaceImplementation.cs

@@ -179,6 +179,7 @@ public override unsafe int GetSpanFlags (global::Java.Lang.Object tag)
 				var __rm = _members.InstanceMethods.InvokeAbstractInt32Method (__id, this, __args);
 				return __rm;
 			} finally {
+				global::System.GC.KeepAlive (tag);
 			}
 		}
 
@@ -194,6 +195,7 @@ public override unsafe void Append (global::Java.Lang.ICharSequence value)
 				_members.InstanceMethods.InvokeAbstractVoidMethod (__id, this, __args);
 			} finally {
 				JNIEnv.DeleteLocalRef (native_value);
+				global::System.GC.KeepAlive (value);
 			}
 		}
 
@@ -210,6 +212,7 @@ public override unsafe void Append (global::Java.Lang.ICharSequence value)
 				return global::Java.Lang.Object.GetObject<Java.Lang.ICharSequence> (__rm.Handle, JniHandleOwnership.TransferLocalRef);
 			} finally {
 				JNIEnv.DeleteLocalRef (native_value);
+				global::System.GC.KeepAlive (value);
 			}
 		}
 

tests/generator-Tests/expected.ji/java.lang.Enum/Java.Lang.Enum.cs

@@ -44,6 +44,7 @@ public unsafe int CompareTo (global::Java.Lang.Object o)
 				return __rm;
 			} finally {
 				JNIEnv.DeleteLocalRef (native_o);
+				global::System.GC.KeepAlive (o);
 			}
 		}