Skip to content

Commit e88cfbc

Browse files
brendanzagaeskibrendanzagaeski
authored
[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
1 parent 136353a commit e88cfbc

File tree

13 files changed

+327
-94
lines changed

13 files changed

+327
-94
lines changed

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,16 @@ processing Android assets and resources.
5050

5151
Added in Xamarin.Android 9.1.
5252

53+
## AndroidAddKeepAlives
54+
55+
A boolean property that controls whether the linker will insert
56+
`GC.KeepAlive()` invocations within binding projects to prevent premature
57+
object collection.
58+
59+
Defaults to `True` for Release configuration builds.
60+
61+
Added in Xamarin.Android 11.2.
62+
5363
## AndroidAotCustomProfilePath
5464

5565
The file that `aprofutil` should create to hold profiler data.

‎Documentation/release-notes/5278.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
#### Application and library build and deployment
2+
3+
* [GitHub PR 5278](https://github.com/xamarin/xamarin-android/pull/5278):
4+
Update the linker in Release builds to insert `GC.KeepAlive()` invocations within
5+
JNI marshal methods to prevent premature instance collection.

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
<Compile Remove="..\Xamarin.Android.Build.Tasks\Linker\MonoDroid.Tuner\LinkerOptions.cs" />
2929

3030
<!--Steps that are upstreamed, these are OK to remove-->
31+
<Compile Remove="..\Xamarin.Android.Build.Tasks\Linker\MonoDroid.Tuner\AddKeepAlivesStep.cs" />
3132
<Compile Remove="..\Xamarin.Android.Build.Tasks\Linker\MonoDroid.Tuner\ApplyPreserveAttribute.cs" />
3233
<Compile Remove="..\Xamarin.Android.Build.Tasks\Linker\MonoDroid.Tuner\OutputStepWithTimestamps.cs" />
3334
<Compile Remove="..\Xamarin.Android.Build.Tasks\Linker\MonoDroid.Tuner\PreserveCode.cs" />

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

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
using System;
2+
using System.Collections.Generic;
3+
using System.Linq;
4+
5+
using Mono.Cecil;
6+
7+
using Java.Interop.Tools.Cecil;
8+
9+
using Mono.Linker;
10+
using Mono.Linker.Steps;
11+
12+
using Mono.Cecil.Cil;
13+
14+
namespace MonoDroid.Tuner
15+
{
16+
public class AddKeepAlivesStep : BaseStep
17+
{
18+
readonly TypeDefinitionCache cache;
19+
20+
public AddKeepAlivesStep (TypeDefinitionCache cache)
21+
{
22+
this.cache = cache;
23+
}
24+
25+
protected override void ProcessAssembly (AssemblyDefinition assembly)
26+
{
27+
var action = Annotations.HasAction (assembly) ? Annotations.GetAction (assembly) : AssemblyAction.Skip;
28+
if (action == AssemblyAction.Delete)
29+
return;
30+
31+
if (AddKeepAlives (assembly)) {
32+
if (action == AssemblyAction.Skip || action == AssemblyAction.Copy)
33+
Annotations.SetAction (assembly, AssemblyAction.Save);
34+
}
35+
}
36+
37+
internal bool AddKeepAlives (AssemblyDefinition assembly)
38+
{
39+
if (!assembly.MainModule.HasTypeReference ("Java.Lang.Object"))
40+
return false;
41+
42+
bool changed = false;
43+
List<TypeDefinition> types = assembly.MainModule.Types.ToList ();
44+
foreach (TypeDefinition type in assembly.MainModule.Types)
45+
AddNestedTypes (types, type);
46+
47+
foreach (TypeDefinition type in types)
48+
if (MightNeedFix (type))
49+
changed |= AddKeepAlives (type);
50+
51+
return changed;
52+
}
53+
54+
// Adapted from `MarkJavaObjects`
55+
static void AddNestedTypes (List<TypeDefinition> types, TypeDefinition type)
56+
{
57+
if (!type.HasNestedTypes)
58+
return;
59+
60+
foreach (var t in type.NestedTypes) {
61+
types.Add (t);
62+
AddNestedTypes (types, t);
63+
}
64+
}
65+
66+
bool MightNeedFix (TypeDefinition type)
67+
{
68+
return !type.IsAbstract && type.IsSubclassOf ("Java.Lang.Object", cache);
69+
}
70+
71+
MethodDefinition methodKeepAlive = null;
72+
73+
bool AddKeepAlives (TypeDefinition type)
74+
{
75+
bool changed = false;
76+
foreach (MethodDefinition method in type.Methods) {
77+
if (!method.CustomAttributes.Any (a => a.AttributeType.FullName == "Android.Runtime.RegisterAttribute"))
78+
continue;
79+
80+
if (method.Parameters.Count == 0)
81+
continue;
82+
83+
var processor = method.Body.GetILProcessor ();
84+
var module = method.DeclaringType.Module;
85+
var instructions = method.Body.Instructions;
86+
var end = instructions.Last ();
87+
if (end.Previous.OpCode == OpCodes.Endfinally)
88+
end = end.Previous;
89+
90+
var found = false;
91+
for (int off = Math.Max (0, instructions.Count - 6); off < instructions.Count; off++) {
92+
var current = instructions [off];
93+
if (current.OpCode == OpCodes.Call && current.Operand.ToString ().Contains ("System.GC::KeepAlive")) {
94+
found = true;
95+
break;
96+
}
97+
}
98+
99+
if (found)
100+
continue;
101+
102+
for (int i = 0; i < method.Parameters.Count; i++) {
103+
if (method.Parameters [i].ParameterType.IsValueType || method.Parameters [i].ParameterType.FullName == "System.String")
104+
continue;
105+
106+
changed = true;
107+
108+
if (methodKeepAlive == null)
109+
methodKeepAlive = Context.GetMethod ("mscorlib", "System.GC", "KeepAlive", new string [] { "System.Object" });
110+
111+
processor.InsertBefore (end, GetLoadArgumentInstruction (method.IsStatic ? i : i + 1, method.Parameters [i]));
112+
processor.InsertBefore (end, Instruction.Create (OpCodes.Call, module.ImportReference (methodKeepAlive)));
113+
}
114+
}
115+
return changed;
116+
}
117+
118+
// Adapted from src/Mono.Android.Export/Mono.CodeGeneration/CodeArgumentReference.cs
119+
static Instruction GetLoadArgumentInstruction (int argNum, ParameterDefinition parameter)
120+
{
121+
switch (argNum) {
122+
case 0: return Instruction.Create (OpCodes.Ldarg_0);
123+
case 1: return Instruction.Create (OpCodes.Ldarg_1);
124+
case 2: return Instruction.Create (OpCodes.Ldarg_2);
125+
case 3: return Instruction.Create (OpCodes.Ldarg_3);
126+
default: return Instruction.Create (OpCodes.Ldarg, parameter);
127+
}
128+
}
129+
}
130+
}

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

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,76 @@ public static object GetSettableValue (this CustomAttributeArgument arg)
4646
return td != null ? td.FullName + "," + td.Module.Assembly.FullName : arg.Value;
4747
}
4848

49+
#if !NETCOREAPP
50+
public static AssemblyDefinition GetAssembly (this LinkContext context, string assemblyName)
51+
{
52+
AssemblyDefinition ad;
53+
context.TryGetLinkedAssembly (assemblyName, out ad);
54+
return ad;
55+
}
56+
57+
public static TypeDefinition GetType (this LinkContext context, string assemblyName, string typeName)
58+
{
59+
AssemblyDefinition ad = context.GetAssembly (assemblyName);
60+
return ad == null ? null : GetType (ad, typeName);
61+
}
62+
63+
public static MethodDefinition GetMethod (this LinkContext context, string ns, string typeName, string name, string [] parameters)
64+
{
65+
var type = context.GetType (ns, typeName);
66+
if (type == null)
67+
return null;
68+
69+
return GetMethod (type, name, parameters);
70+
}
71+
#endif
72+
73+
public static MethodDefinition GetMethod (TypeDefinition td, string name)
74+
{
75+
MethodDefinition method = null;
76+
foreach (var md in td.Methods) {
77+
if (md.Name == name) {
78+
method = md;
79+
break;
80+
}
81+
}
82+
83+
return method;
84+
}
85+
86+
public static MethodDefinition GetMethod (TypeDefinition type, string name, string [] parameters)
87+
{
88+
MethodDefinition method = null;
89+
foreach (var md in type.Methods) {
90+
if (md.Name != name)
91+
continue;
92+
93+
if (md.Parameters.Count != parameters.Length)
94+
continue;
95+
96+
var equal = true;
97+
for (int i = 0; i < parameters.Length; i++) {
98+
if (md.Parameters [i].ParameterType.FullName != parameters [i]) {
99+
equal = false;
100+
break;
101+
}
102+
}
103+
104+
if (!equal)
105+
continue;
106+
107+
method = md;
108+
break;
109+
}
110+
111+
return method;
112+
}
113+
114+
public static TypeDefinition GetType (AssemblyDefinition assembly, string typeName)
115+
{
116+
return assembly.MainModule.GetType (typeName);
117+
}
118+
49119
public static bool Implements (this TypeReference self, string interfaceName)
50120
{
51121
if (interfaceName == null)

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@ static Pipeline CreatePipeline (LinkerOptions options)
6565

6666
if (options.LinkNone) {
6767
pipeline.AppendStep (new FixAbstractMethodsStep (cache));
68+
if (options.AddKeepAlives)
69+
pipeline.AppendStep (new AddKeepAlivesStep (cache));
6870
pipeline.AppendStep (new OutputStepWithTimestamps ());
6971
return pipeline;
7072
}
@@ -118,6 +120,8 @@ static Pipeline CreatePipeline (LinkerOptions options)
118120
if (!string.IsNullOrWhiteSpace (options.ProguardConfiguration))
119121
pipeline.AppendStep (new GenerateProguardConfiguration (options.ProguardConfiguration));
120122
pipeline.AppendStep (new StripEmbeddedLibraries ());
123+
if (options.AddKeepAlives)
124+
pipeline.AppendStep (new AddKeepAlivesStep (cache));
121125
// end monodroid specific
122126
pipeline.AppendStep (new RegenerateGuidStep ());
123127
pipeline.AppendStep (new OutputStepWithTimestamps ());

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ class LinkerOptions
2222
public bool DumpDependencies { get; set; }
2323
public string HttpClientHandlerType { get; set; }
2424
public string TlsProvider { get; set; }
25+
public bool AddKeepAlives { get; set; }
2526
public bool PreserveJniMarshalMethods { get; set; }
2627
public bool DeterministicOutput { get; set; }
2728
}

0 commit comments

Comments
 (0)