[linker] Add AddKeepAlivesStep (#5278)

Context: dotnet/java-interop#719
Context: dotnet/java-interop@1f21f38
Context: https://docs.microsoft.com/archive/blogs/cbrumme/lifetime-gc-keepalive-handle-recycling

We discovered the cause of a latent potential crash within
Xamarin.Android apps:

	JNI DETECTED ERROR IN APPLICATION: use of deleted global reference 0x3d86
	    …

The cause of the "use of deleted global reference" was that the GC
collected an instance after it was provided to Java, but before Java
could keep the instance alive in a manner which would cause our GC
bridge to keep it alive, akin to:

	CallIntoJava (new JavaLangObjectSubclass ().Handle);

In the above example code, if the `JavaLangObjectSubclass` instance
is collected by the GC *after* the `.Handle` property is accessed
but *before* `CallIntoJava()` is executed, then the `.Handle` value
will refer to a "deleted global reference" and cause the app crash.

The appropriate fix is to ensure that the GC *won't* collect it,
usually by calling [`GC.KeepAlive()`][0]:

	var value = new JavaLangObjectSubclass ();
	CallIntoJava (value.Handle);
	GC.KeepAlive (value);

An important aspect of the problem is that much of the relevant code
introducing this scenario is within *binding assemblies*:

	partial class Activity {
	    public virtual unsafe Android.App.PendingIntent? CreatePendingResult (
	            int requestCode,
	            Android.Content.Intent data,
	            [global::Android.Runtime.GeneratedEnum] Android.App.PendingIntentFlags flags)
	    {
	        const string __id = "createPendingResult.(ILandroid/content/Intent;I)Landroid/app/PendingIntent;";
	        try {
	            JniArgumentValue* __args = stackalloc JniArgumentValue [3];
	            __args [0] = new JniArgumentValue (requestCode);
	            __args [1] = new JniArgumentValue ((data == null) ? IntPtr.Zero : ((global::Java.Lang.Object) data).Handle);
	            __args [2] = new JniArgumentValue ((int) flags);
	            var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, __args);
	            return global::Java.Lang.Object.GetObject<Android.App.PendingIntent> (__rm.Handle, JniHandleOwnership.TransferLocalRef);
	        } finally {
	        }    
	    }
	}

Here, the issue is that `data.Handle` is passed into Java code, but
it's possible that `data` may be collected *after* `data.Handle` is
accessed but before
`_members.InstanceMethods.InvokeVirtualObjectMethod()` is invoked.

dotnet/java-interop@1f21f38c introduced a `generator` fix for this
this problem, inserting an appropriate `GC.KeepAlive()`:

	partial class Activity {
	    public virtual unsafe Android.App.PendingIntent? CreatePendingResult (
	            int requestCode, Android.Content.Intent data,
	            [global::Android.Runtime.GeneratedEnum] Android.App.PendingIntentFlags flags)
	    {
	        const string __id = "createPendingResult.(ILandroid/content/Intent;I)Landroid/app/PendingIntent;";
	        try {
	            JniArgumentValue* __args = stackalloc JniArgumentValue [3];
	            __args [0] = new JniArgumentValue (requestCode);
	            __args [1] = new JniArgumentValue ((data == null) ? IntPtr.Zero : ((global::Java.Lang.Object) data).Handle);
	            __args [2] = new JniArgumentValue ((int) flags);
	            var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, __args);
	            return global::Java.Lang.Object.GetObject<Android.App.PendingIntent> (__rm.Handle, JniHandleOwnership.TransferLocalRef);
	        } finally {
	            GC.KeepAlive (data);
	        }    
	    }
	}

The problem is that a fix within *generator* is meaningless until all
relevant binding assemblies are *also* updated: fixing the issue
within `Mono.Android.dll` only fixes `Mono.Android.dll`!

We don't -- and can't! -- expect the entire ecosystem to rebuild and
republish binding assemblies, which means a `generator` fix isn't a
complete fix!

To help fix the *ecosystem*, update the *linker* to insert appropriate
`GC.KeepAlive()` invocations into marshal methods.  This allows fixing
the "problem domain" -- premature instance collection -- without
requiring that the entire ecosystem be rebuilt.

Instead, it "merely" requires that all *applications* be rebuilt.

Add a new `$(AndroidAddKeepAlives)` MSBuild property which controls
whether or not the linker inserts the `GC.KeepAlive()` calls.
`$(AndroidAddKeepAlives)` defaults to True for Release configuration
apps, and is False by default for "fast deployment" Debug builds.
`$(AndroidAddKeepAlives)` can be overridden to explicitly enable or
disable the new linker steps.

The impact on release build of XA forms template (flyout variety) is
cca 137ms on @radekdoulik's machine.

  * Disabled:

        12079 ms  LinkAssemblies                             1 calls

  * Enabled:

        12216 ms  LinkAssemblies                             1 calls

Co-authored-by: Radek Doulik <[email protected]>

[0]: https://docs.microsoft.com/dotnet/api/system.gc.keepalive

Documentation/guides/building-apps/build-properties.md

@@ -50,6 +50,16 @@ processing Android assets and resources.
 
 Added in Xamarin.Android 9.1.
 
+## AndroidAddKeepAlives
+
+A boolean property that controls whether the linker will insert
+`GC.KeepAlive()` invocations within binding projects to prevent premature
+object collection.
+
+Defaults to `True` for Release configuration builds.
+
+Added in Xamarin.Android 11.2.
+
 ## AndroidAotCustomProfilePath
 
 The file that `aprofutil` should create to hold profiler data.

Documentation/release-notes/5278.md

@@ -0,0 +1,5 @@
+#### Application and library build and deployment
+
+  * [GitHub PR 5278](https://github.com/xamarin/xamarin-android/pull/5278):
+    Update the linker in Release builds to insert `GC.KeepAlive()` invocations within
+    JNI marshal methods to prevent premature instance collection.

src/Microsoft.Android.Sdk.ILLink/Microsoft.Android.Sdk.ILLink.csproj

@@ -28,6 +28,7 @@
     <Compile Remove="..\Xamarin.Android.Build.Tasks\Linker\MonoDroid.Tuner\LinkerOptions.cs" />
 
     <!--Steps that are upstreamed, these are OK to remove-->
+    <Compile Remove="..\Xamarin.Android.Build.Tasks\Linker\MonoDroid.Tuner\AddKeepAlivesStep.cs" />
     <Compile Remove="..\Xamarin.Android.Build.Tasks\Linker\MonoDroid.Tuner\ApplyPreserveAttribute.cs" />
     <Compile Remove="..\Xamarin.Android.Build.Tasks\Linker\MonoDroid.Tuner\OutputStepWithTimestamps.cs" />
     <Compile Remove="..\Xamarin.Android.Build.Tasks\Linker\MonoDroid.Tuner\PreserveCode.cs" />

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/AddKeepAlivesStep.cs

@@ -0,0 +1,130 @@
+using System;
+using System.Collections.Generic;
+using System.Linq;
+
+using Mono.Cecil;
+
+using Java.Interop.Tools.Cecil;
+
+using Mono.Linker;
+using Mono.Linker.Steps;
+
+using Mono.Cecil.Cil;
+
+namespace MonoDroid.Tuner
+{
+	public class AddKeepAlivesStep : BaseStep
+	{
+		readonly TypeDefinitionCache cache;
+
+		public AddKeepAlivesStep (TypeDefinitionCache cache)
+		{
+			this.cache = cache;
+		}
+
+		protected override void ProcessAssembly (AssemblyDefinition assembly)
+		{
+			var action = Annotations.HasAction (assembly) ? Annotations.GetAction (assembly) : AssemblyAction.Skip;
+			if (action == AssemblyAction.Delete)
+				return;
+
+			if (AddKeepAlives (assembly)) {
+				if (action == AssemblyAction.Skip || action == AssemblyAction.Copy)
+					Annotations.SetAction (assembly, AssemblyAction.Save);
+			}
+		}
+
+		internal bool AddKeepAlives (AssemblyDefinition assembly)
+		{
+			if (!assembly.MainModule.HasTypeReference ("Java.Lang.Object"))
+				return false;
+
+			bool changed = false;
+			List<TypeDefinition> types = assembly.MainModule.Types.ToList ();
+			foreach (TypeDefinition type in assembly.MainModule.Types)
+				AddNestedTypes (types, type);
+
+			foreach (TypeDefinition type in types)
+				if (MightNeedFix (type))
+					changed |= AddKeepAlives (type);
+
+			return changed;
+		}
+
+		// Adapted from `MarkJavaObjects`
+		static void AddNestedTypes (List<TypeDefinition> types, TypeDefinition type)
+		{
+			if (!type.HasNestedTypes)
+				return;
+
+			foreach (var t in type.NestedTypes) {
+				types.Add (t);
+				AddNestedTypes (types, t);
+			}
+		}
+
+		bool MightNeedFix (TypeDefinition type)
+		{
+			return !type.IsAbstract && type.IsSubclassOf ("Java.Lang.Object", cache);
+		}
+
+		MethodDefinition methodKeepAlive = null;
+
+		bool AddKeepAlives (TypeDefinition type)
+		{
+			bool changed = false;
+			foreach (MethodDefinition method in type.Methods) {
+				if (!method.CustomAttributes.Any (a => a.AttributeType.FullName == "Android.Runtime.RegisterAttribute"))
+					continue;
+
+				if (method.Parameters.Count == 0)
+					continue;
+
+				var processor = method.Body.GetILProcessor ();
+				var module = method.DeclaringType.Module;
+				var instructions = method.Body.Instructions;
+				var end = instructions.Last ();
+				if (end.Previous.OpCode == OpCodes.Endfinally)
+					end = end.Previous;
+
+				var found = false;
+				for (int off = Math.Max (0, instructions.Count - 6); off < instructions.Count; off++) {
+					var current = instructions [off];
+					if (current.OpCode == OpCodes.Call && current.Operand.ToString ().Contains ("System.GC::KeepAlive")) {
+						found = true;
+						break;
+					}
+				}
+
+				if (found)
+					continue;
+
+				for (int i = 0; i < method.Parameters.Count; i++) {
+					if (method.Parameters [i].ParameterType.IsValueType || method.Parameters [i].ParameterType.FullName == "System.String")
+						continue;
+
+					changed = true;
+
+					if (methodKeepAlive == null)
+						methodKeepAlive = Context.GetMethod ("mscorlib", "System.GC", "KeepAlive", new string [] { "System.Object" });
+
+					processor.InsertBefore (end, GetLoadArgumentInstruction (method.IsStatic ? i : i + 1, method.Parameters [i]));
+					processor.InsertBefore (end, Instruction.Create (OpCodes.Call, module.ImportReference (methodKeepAlive)));
+				}
+			}
+			return changed;
+		}
+
+		// Adapted from src/Mono.Android.Export/Mono.CodeGeneration/CodeArgumentReference.cs
+		static Instruction GetLoadArgumentInstruction (int argNum, ParameterDefinition parameter)
+		{
+			switch (argNum) {
+				case 0: return Instruction.Create (OpCodes.Ldarg_0);
+				case 1: return Instruction.Create (OpCodes.Ldarg_1);
+				case 2: return Instruction.Create (OpCodes.Ldarg_2);
+				case 3: return Instruction.Create (OpCodes.Ldarg_3);
+				default: return Instruction.Create (OpCodes.Ldarg, parameter);
+			}
+		}
+	}
+}

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/Extensions.cs

@@ -46,6 +46,76 @@ public static object GetSettableValue (this CustomAttributeArgument arg)
 			return td != null ? td.FullName + "," + td.Module.Assembly.FullName : arg.Value;
 		}
 
+#if !NETCOREAPP
+		public static AssemblyDefinition GetAssembly (this LinkContext context, string assemblyName)
+		{
+			AssemblyDefinition ad;
+			context.TryGetLinkedAssembly (assemblyName, out ad);
+			return ad;
+		}
+
+		public static TypeDefinition GetType (this LinkContext context, string assemblyName, string typeName)
+		{
+			AssemblyDefinition ad = context.GetAssembly (assemblyName);
+			return ad == null ? null : GetType (ad, typeName);
+		}
+
+		public static MethodDefinition GetMethod (this LinkContext context, string ns, string typeName, string name, string [] parameters)
+		{
+			var type = context.GetType (ns, typeName);
+			if (type == null)
+				return null;
+
+			return GetMethod (type, name, parameters);
+		}
+#endif
+
+		public static MethodDefinition GetMethod (TypeDefinition td, string name)
+		{
+			MethodDefinition method = null;
+			foreach (var md in td.Methods) {
+				if (md.Name == name) {
+					method = md;
+					break;
+				}
+			}
+
+			return method;
+		}
+
+		public static MethodDefinition GetMethod (TypeDefinition type, string name, string [] parameters)
+		{
+			MethodDefinition method = null;
+			foreach (var md in type.Methods) {
+				if (md.Name != name)
+					continue;
+
+				if (md.Parameters.Count != parameters.Length)
+					continue;
+
+				var equal = true;
+				for (int i = 0; i < parameters.Length; i++) {
+					if (md.Parameters [i].ParameterType.FullName != parameters [i]) {
+						equal = false;
+						break;
+					}
+				}
+
+				if (!equal)
+					continue;
+
+				method = md;
+				break;
+			}
+
+			return method;
+		}
+
+		public static TypeDefinition GetType (AssemblyDefinition assembly, string typeName)
+		{
+			return assembly.MainModule.GetType (typeName);
+		}
+
 		public static bool Implements (this TypeReference self, string interfaceName)
 		{
 			if (interfaceName == null)

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/Linker.cs

@@ -65,6 +65,8 @@ static Pipeline CreatePipeline (LinkerOptions options)
 
 			if (options.LinkNone) {
 				pipeline.AppendStep (new FixAbstractMethodsStep (cache));
+				if (options.AddKeepAlives)
+					pipeline.AppendStep (new AddKeepAlivesStep (cache));
 				pipeline.AppendStep (new OutputStepWithTimestamps ());
 				return pipeline;
 			}
@@ -118,6 +120,8 @@ static Pipeline CreatePipeline (LinkerOptions options)
 			if (!string.IsNullOrWhiteSpace (options.ProguardConfiguration))
 				pipeline.AppendStep (new GenerateProguardConfiguration (options.ProguardConfiguration));
 			pipeline.AppendStep (new StripEmbeddedLibraries ());
+			if (options.AddKeepAlives)
+				pipeline.AppendStep (new AddKeepAlivesStep (cache));
 			// end monodroid specific
 			pipeline.AppendStep (new RegenerateGuidStep ());
 			pipeline.AppendStep (new OutputStepWithTimestamps ());

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/LinkerOptions.cs

@@ -22,6 +22,7 @@ class LinkerOptions
 		public bool DumpDependencies { get; set; }
 		public string HttpClientHandlerType { get; set; }
 		public string TlsProvider { get; set; }
+		public bool AddKeepAlives { get; set; }
 		public bool PreserveJniMarshalMethods { get; set; }
 		public bool DeterministicOutput { get; set; }
 	}