-
Notifications
You must be signed in to change notification settings - Fork 528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[typemaps] Handle Java to managed type maps properly #4656
Conversation
db35006
to
e0936b2
Compare
Commit message snatched from #4645 |
e0936b2
to
772d643
Compare
772d643
to
b0369af
Compare
var entry = new TypeMapReleaseEntry { | ||
JavaName = javaName, | ||
JavaNameLength = outputEncoding.GetByteCount (javaName), | ||
ManagedTypeName = td.FullName, | ||
Token = td.MetadataToken.ToUInt32 (), | ||
AssemblyNameIndex = knownAssemblies [assemblyName] | ||
AssemblyNameIndex = knownAssemblies [assemblyName], | ||
SkipInJavaToManaged = td.IsInterface | td.HasGenericParameters, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be ||
, not |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can be either, since it's a bool
it will work both ways, but I guess ||
would be more conventional...
fcb5d45
to
aabda15
Compare
@@ -208,6 +209,10 @@ EmbeddedAssemblies::typemap_java_to_managed (const char *java_type_name) | |||
} | |||
|
|||
const char *managed_type_name = entry->to; | |||
if (managed_type_name == nullptr) { | |||
log_debug (LOG_DEFAULT, "typemap: Java type '%s' maps either to an open generic type or an interface type."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably use LOG_ASSEMBLY
as LOG_DEFAULT
requires an adb shell setprop debug.mono.log default
to see the messages, which just looks weird.
@@ -491,6 +554,10 @@ void OutputModule (ModuleDebugData moduleData) | |||
// | |||
void OutputModule (BinaryWriter bw, ModuleDebugData moduleData) | |||
{ | |||
if ((uint)moduleData.JavaToManagedMap.Count == UInt32.MaxValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like dead/impossible code. Sure, there's "defense in depth," but this feels like it's bordering on lunacy, as List<T>
is backed by an array which can hold at most Array.MaxArrayLength
elements.
This is admittedly less than int.MaxValue
, and is an implementation detail.
Allowing List<T>.Capacity
and List<T>.Count
to exceed int.MaxValue
would be a yuge semantic break, though. Checking that int.MaxValue
is less than uint.MaxValue
is bizarre; it can't not be true.
(And if by some chance somebody wanted to allow List<T>
/Arrays to hold more than int.MaxValue elements -- as mono could do at one point in time! -- you'd have to have Count
throw an exception when Count exceeded int.MaxValue
, otherwise currently semantically correct code that checks that list.Count > 0
will fail, which is very much not good. Which means that this code would be doubly broken!)
If anything, use int.MaxValue
as the limit, not uint.MaxValue
. Assuming we need this check at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a check for an "impossible" situation, for sure. But I don't want to rely on implementation details of List<>
to check a condition that may affect us on the runtime. Also, if for some weird reason (a bug?) .Count
is set to -1
, then casting it to uint
will yield uint.MaxValue
. However, that is not the point of all this - uint.MaxValue
is a special value used by the native runtime to determine that the entry should be skipped (is considered as "invalid") and this check makes this condition more explicit while not hurting anything.
@@ -502,7 +569,7 @@ void OutputModule (BinaryWriter bw, ModuleDebugData moduleData) | |||
foreach (TypeMapDebugEntry entry in moduleData.JavaToManagedMap) { | |||
bw.Write (outputEncoding.GetBytes (entry.JavaName)); | |||
PadField (bw, entry.JavaName.Length, (int)moduleData.JavaNameWidth); | |||
bw.Write (entry.ManagedIndex); | |||
bw.Write (entry.SkipInJavaToManaged ? UInt32.MaxValue : (uint)entry.ManagedIndex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it look like we're using uint.MaxValue
as a "placeholder" value for an "invalid" mapping. If so, we should semantically name this constant:
const uint InvalidJavaToManagedMappingIndex = uint.MaxValue;
@@ -736,7 +741,12 @@ EmbeddedAssemblies::typemap_load_file (BinaryTypeMapHeader &header, const char * | |||
cur->from = reinterpret_cast<char*>(java_pos); | |||
|
|||
uint32_t idx = *(reinterpret_cast<uint32_t*>(java_pos + header.java_name_width)); | |||
cur->to = reinterpret_cast<char*>(managed_start + (managed_entry_size * idx)); | |||
if (idx < UINT32_MAX) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Ditto" here, instead of UINT32_MAX
, we should use a semantically meaningful name, e.g. INVALID_TARGET_INDEX
.
Fixes: dotnet#4596 Fixes: dotnet#4660 Context: xamarin/monodroid@192b1f1 Context: https://github.com/xamarin/java.interop/blob/fc18c54b2ccf14f444fadac0a1e7e81f837cc470/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs#L149-L186 Context: https://github.com/xamarin/java.interop/blob/010161d1ef6625b762429334f02e7b15e1f7caae/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs#L155 ~~ Debug Mode ~~ In some environments, `BundleTest.TestBundleIntegerArrayList2()` fails when using the commercial shared runtime: Test 'Xamarin.Android.RuntimeTests.BundleTest.TestBundleIntegerArrayList2' failed: System.MemberAccessException : Cannot create an instance of Android.Runtime.JavaList`1[T] because Type.ContainsGenericParameters is true. at System.Reflection.RuntimeConstructorInfo.DoInvoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) at System.Reflection.RuntimeConstructorInfo.Invoke (System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) at System.Reflection.ConstructorInfo.Invoke (System.Object[] parameters) at Java.Interop.TypeManager.CreateProxy (System.Type type, System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Java.Interop.TypeManager.CreateInstance (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type targetType) at Java.Lang.Object.GetObject (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type type) at Java.Lang.Object._GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Java.Lang.Object.GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Android.OS.Bundle.Get (System.String key) at Xamarin.Android.RuntimeTests.BundleTest.TestBundleIntegerArrayList2 () at (wrapper managed-to-native) System.Reflection.RuntimeMethodInfo.InternalInvoke(System.Reflection.RuntimeMethodInfo,object,object[],System.Exception&) at System.Reflection.RuntimeMethodInfo.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) The problem appears to be rooted in ce2bc68. In a pre-ce2bc689 world, there were two separate sets of "typemap" files that the application could use: * Java-to-managed mappings, in `typemap.jm` * Managed-to-Java mappings, in `typemap.mj`. These files did *not* need to have the same number of entries! % unzip /Library/Frameworks/Xamarin.Android.framework/Versions/10.2.0.100/lib/xamarin.android/xbuild/Xamarin/Android/Mono.Android.DebugRuntime-debug.apk typemap.jm % unzip /Library/Frameworks/Xamarin.Android.framework/Versions/10.2.0.100/lib/xamarin.android/xbuild/Xamarin/Android/Mono.Android.DebugRuntime-debug.apk typemap.mj % strings typemap.jm | wc -l 10318 % strings typemap.mj | wc -l 11956 The reason why they have a different number of entries is *aliasing*: there can be *multiple* bindings for a given Java type. The managed-to-Java mappings can contain *all* of them; the Java-to-managed mapping *must* pick Only One. For example, [`java.util.ArrayList`][0] contains (at least?) *three* bindings: * [`Android.Runtime.JavaList`][1] * [`Android.Runtime.JavaList<T>`][2] * `Java.Util.ArrayList` (generated at build time) In a pre-ce2bc689 world, this was "fine": we'd binary search each file *separately* to find type mappings. This changed in ce2bc68, as the typemap information was merged into a *single* array in order to de-duplicate information. This reduced `.apk` size. An unanticipated result of this is that the Java-to-managed mappings can now contain *duplicate* keys, "as if" the original `typemap.jm` had the contents: java/util/ArrayList Android.Runtime.JavaList, Mono.Android java/util/ArrayList Android.Runtime.JavaList`1, Mono.Android java/util/ArrayList Java.Util.ArrayLlist, Mono.Android Whereas pre-ce2bc689 there would have only been the first entry. "Sometimes" this duplication is "fine": the typemap search "Just happens" to return the first entry, or the 3rd entry, and apps continue to work as intended. "Sometimes" it isn't: the typemap binary search finds the 2nd entry, which is a generic type. This results in attempting to instantiate a generic type *without appropriate type parameters*, which results in the aforementioned `MemberAccessException`. Update typemap generation code in `Xamarin.Android.Build.Tasks` so that all the duplicate Java type names will point to the same managed type name. Additionally, make sure we select the managed type in the same fashion the old typemap generator in `Java.Interop` worked - prefer base types to the derived ones if the type is an interface or an abstract class. The effect of this change is that no matter which entry `binary_search` ends up selecting it will always return the same managed type name for all the aliased Java types. ~~ Release mode ~~ Another issue introduced in ce2bc68 is that we no longer ignore interface and generic types when generating the Java-to-managed maps. This adversely affects the `Release` mode in which it is possible that an attempt to instantiate an open generic type (or an interface) will happen, leading to error similar to: FATAL EXCEPTION: main Process: com.companyname.androidsingleviewapp1, PID: 24068 android.runtime.JavaProxyThrowable: System.MemberAccessException: Cannot create an instance of Com.Contoso.Javalibrary1.Class0`1[T] because Type.ContainsGenericParameters is true. at System.Reflection.RuntimeConstructorInfo.DoInvoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) at System.Reflection.RuntimeConstructorInfo.Invoke (System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) at System.Reflection.ConstructorInfo.Invoke (System.Object[] parameters) at Java.Interop.TypeManager.CreateProxy (System.Type type, System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Java.Interop.TypeManager.CreateInstance (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type targetType) at Java.Lang.Object.GetObject (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type type) at Java.Lang.Object._GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Java.Lang.Object.GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Com.Contoso.Javalibrary1.Class1.Create () at AndroidSingleViewApp1.MainActivity.OnCreate (Android.OS.Bundle savedInstanceState) at Android.App.Activity.n_OnCreate_Landroid_os_Bundle_ (System.IntPtr jnienv, System.IntPtr native__this, System.IntPtr native_savedInstanceState) at (wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.1(intptr,intptr,intptr) at crc647d7dcab8da8ee782.MainActivity.n_onCreate(Native Method) at crc647d7dcab8da8ee782.MainActivity.onCreate(MainActivity.java:29) at android.app.Activity.performCreate(Activity.java:7144) at android.app.Activity.performCreate(Activity.java:7135) at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1271) at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2931) at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3086) at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:78) at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:108) at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:68) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1816) at android.os.Handler.dispatchMessage(Handler.java:106) at android.os.Looper.loop(Looper.java:193) at android.app.ActivityThread.main(ActivityThread.java:6718) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858) Update typemap generation code to also ignore generic types and interfaces when generating the Java to Managed map in `Release` mode. We must not simply skip outputting entries for such types because we need to maintain element count parity between java-to-managed and managed-to-java lookup tables. Therefore, the way we skip such types is to output a type token ID of `0` instead of the actual one. This will cause the runtime code to fail to find a mapping, thus allowing the managed code to deal with such a type properly. ~~ Test Updates ~~ Update `TestBundleIntegerArrayList2()` so that it asserts the runtime *type* of `obj` returned, asserting that it is in fact a `JavaList` and not e.g. `Java.Util.ArrayList` or something. (The linker could otherwise "change" things, and this assertion will ensure that `JavaList` won't be linked away…) Add a unit test for issue dotnet#4660 (by @brendanzagaeski) It's very cute. :-) We sort the Java-to-managed type name table, so it can be binary searched to find the Managed type. Additionally, the "preferred" managed type is the *first* type in `monodis --typedef` output; the first type that we encounter when going through all types in an assembly. Brendan's cute way of provoking the bug is to add a new *generic* type which will come *before* the "normally preferred" (and correct!) type. Thus, even if/when binary search returns the first entry, that'll still be the *wrong entry*, because that'll be a generic type! Very cute. [0]: https://developer.android.com/reference/java/util/ArrayList [1]: https://github.com/xamarin/xamarin-android/blob/61f09bf2ef0c8f09550e2d77eb97f417432b315b/src/Mono.Android/Android.Runtime/JavaList.cs#L11-L13 [2]: https://github.com/xamarin/xamarin-android/blob/61f09bf2ef0c8f09550e2d77eb97f417432b315b/src/Mono.Android/Android.Runtime/JavaList.cs#L667-L668
aabda15
to
f56d333
Compare
Fixes: #4596 Fixes: #4660 Context: xamarin/monodroid@192b1f1 Context: https://github.com/xamarin/java.interop/blob/fc18c54b2ccf14f444fadac0a1e7e81f837cc470/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs#L149-L186 Context: https://github.com/xamarin/java.interop/blob/010161d1ef6625b762429334f02e7b15e1f7caae/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs#L155 ~~ Debug Mode ~~ In some environments, `BundleTest.TestBundleIntegerArrayList2()` fails when using the commercial shared runtime: Test 'Xamarin.Android.RuntimeTests.BundleTest.TestBundleIntegerArrayList2' failed: System.MemberAccessException : Cannot create an instance of Android.Runtime.JavaList`1[T] because Type.ContainsGenericParameters is true. at System.Reflection.RuntimeConstructorInfo.DoInvoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) at System.Reflection.RuntimeConstructorInfo.Invoke (System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) at System.Reflection.ConstructorInfo.Invoke (System.Object[] parameters) at Java.Interop.TypeManager.CreateProxy (System.Type type, System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Java.Interop.TypeManager.CreateInstance (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type targetType) at Java.Lang.Object.GetObject (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type type) at Java.Lang.Object._GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Java.Lang.Object.GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Android.OS.Bundle.Get (System.String key) at Xamarin.Android.RuntimeTests.BundleTest.TestBundleIntegerArrayList2 () at (wrapper managed-to-native) System.Reflection.RuntimeMethodInfo.InternalInvoke(System.Reflection.RuntimeMethodInfo,object,object[],System.Exception&) at System.Reflection.RuntimeMethodInfo.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) The problem appears to be rooted in ce2bc68. In a pre-ce2bc689 world, there were two separate sets of "typemap" files that the application could use: * Java-to-managed mappings, in `typemap.jm` * Managed-to-Java mappings, in `typemap.mj`. These files did *not* need to have the same number of entries! % unzip /Library/Frameworks/Xamarin.Android.framework/Versions/10.2.0.100/lib/xamarin.android/xbuild/Xamarin/Android/Mono.Android.DebugRuntime-debug.apk typemap.jm % unzip /Library/Frameworks/Xamarin.Android.framework/Versions/10.2.0.100/lib/xamarin.android/xbuild/Xamarin/Android/Mono.Android.DebugRuntime-debug.apk typemap.mj % strings typemap.jm | wc -l 10318 % strings typemap.mj | wc -l 11956 The reason why they have a different number of entries is *aliasing*: there can be *multiple* bindings for a given Java type. The managed-to-Java mappings can contain *all* of them; the Java-to-managed mapping *must* pick Only One. For example, [`java.util.ArrayList`][0] contains (at least?) *three* bindings: * [`Android.Runtime.JavaList`][1] * [`Android.Runtime.JavaList<T>`][2] * `Java.Util.ArrayList` (generated at build time) In a pre-ce2bc689 world, this was "fine": we'd binary search each file *separately* to find type mappings. This changed in ce2bc68, as the typemap information was merged into a *single* array in order to de-duplicate information. This reduced `.apk` size. An unanticipated result of this is that the Java-to-managed mappings can now contain *duplicate* keys, "as if" the original `typemap.jm` had the contents: java/util/ArrayList Android.Runtime.JavaList, Mono.Android java/util/ArrayList Android.Runtime.JavaList`1, Mono.Android java/util/ArrayList Java.Util.ArrayLlist, Mono.Android Whereas pre-ce2bc689 there would have only been the first entry. "Sometimes" this duplication is "fine": the typemap search "Just happens" to return the first entry, or the 3rd entry, and apps continue to work as intended. "Sometimes" it isn't: the typemap binary search finds the 2nd entry, which is a generic type. This results in attempting to instantiate a generic type *without appropriate type parameters*, which results in the aforementioned `MemberAccessException`. Update typemap generation code in `Xamarin.Android.Build.Tasks.dll` so that all the duplicate Java type names will point to the same managed type name. Additionally, make sure we select the managed type in the same fashion the old typemap generator in `Java.Interop` worked: prefer base types to the derived ones if the type is an interface or an abstract class. The effect of this change is that no matter which entry `EmbeddedAssemblies::binary_search()` ends up selecting it will always return the same managed type name for all aliased Java types. ~~ Release config apps ~~ Another issue introduced in ce2bc68 is that we no longer ignore interface and generic types when generating the Java-to-managed maps. This adversely affects the `Release` mode in which it is possible that an attempt to instantiate an open generic type (or an interface) will happen, leading to error similar to: FATAL EXCEPTION: main Process: com.companyname.androidsingleviewapp1, PID: 24068 android.runtime.JavaProxyThrowable: System.MemberAccessException: Cannot create an instance of Com.Contoso.Javalibrary1.Class0`1[T] because Type.ContainsGenericParameters is true. at System.Reflection.RuntimeConstructorInfo.DoInvoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) at System.Reflection.RuntimeConstructorInfo.Invoke (System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) at System.Reflection.ConstructorInfo.Invoke (System.Object[] parameters) at Java.Interop.TypeManager.CreateProxy (System.Type type, System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Java.Interop.TypeManager.CreateInstance (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type targetType) at Java.Lang.Object.GetObject (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type type) at Java.Lang.Object._GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Java.Lang.Object.GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Com.Contoso.Javalibrary1.Class1.Create () at AndroidSingleViewApp1.MainActivity.OnCreate (Android.OS.Bundle savedInstanceState) at Android.App.Activity.n_OnCreate_Landroid_os_Bundle_ (System.IntPtr jnienv, System.IntPtr native__this, System.IntPtr native_savedInstanceState) at (wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.1(intptr,intptr,intptr) at crc647d7dcab8da8ee782.MainActivity.n_onCreate(Native Method) at crc647d7dcab8da8ee782.MainActivity.onCreate(MainActivity.java:29) at android.app.Activity.performCreate(Activity.java:7144) at android.app.Activity.performCreate(Activity.java:7135) at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1271) at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2931) at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3086) at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:78) at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:108) at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:68) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1816) at android.os.Handler.dispatchMessage(Handler.java:106) at android.os.Looper.loop(Looper.java:193) at android.app.ActivityThread.main(ActivityThread.java:6718) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858) Update typemap generation code to also ignore generic types and interfaces when generating the Java to Managed map in `Release` mode. We must not simply skip outputting entries for such types because we need to maintain element count parity between java-to-managed and managed-to-java lookup tables. Therefore, the way we skip such types is to output a type token ID of `0` instead of the actual one. This will cause the runtime code to fail to find a mapping, thus allowing the managed code to deal with such a type properly. ~~ Test Updates ~~ Update `BundleTest.TestBundleIntegerArrayList2()` so that it asserts the runtime *type* of `obj` returned, asserting that it is in fact a `JavaList` and not e.g. `Java.Util.ArrayList` or something. (The linker could otherwise "change" things, and this assertion will ensure that `JavaList` won't be linked away…) Add a unit test for the Release config app bug in Issue #4660; thanks to @brendanzagaeski. It works by provoking the binary search bug by "ensuring" that a *generic* type comes *before* the "normally preferred" *non*-generic type. Thus, even if/when binary search returns the first entry, that would still be the *wrong entry*, because that'll be a generic type! [0]: https://developer.android.com/reference/java/util/ArrayList [1]: https://github.com/xamarin/xamarin-android/blob/61f09bf2ef0c8f09550e2d77eb97f417432b315b/src/Mono.Android/Android.Runtime/JavaList.cs#L11-L13 [2]: https://github.com/xamarin/xamarin-android/blob/61f09bf2ef0c8f09550e2d77eb97f417432b315b/src/Mono.Android/Android.Runtime/JavaList.cs#L667-L668
Fixes: #4596 Fixes: #4660 Context: xamarin/monodroid@192b1f1 Context: https://github.com/xamarin/java.interop/blob/fc18c54b2ccf14f444fadac0a1e7e81f837cc470/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs#L149-L186 Context: https://github.com/xamarin/java.interop/blob/010161d1ef6625b762429334f02e7b15e1f7caae/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs#L155 ~~ Debug Mode ~~ In some environments, `BundleTest.TestBundleIntegerArrayList2()` fails when using the commercial shared runtime: Test 'Xamarin.Android.RuntimeTests.BundleTest.TestBundleIntegerArrayList2' failed: System.MemberAccessException : Cannot create an instance of Android.Runtime.JavaList`1[T] because Type.ContainsGenericParameters is true. at System.Reflection.RuntimeConstructorInfo.DoInvoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) at System.Reflection.RuntimeConstructorInfo.Invoke (System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) at System.Reflection.ConstructorInfo.Invoke (System.Object[] parameters) at Java.Interop.TypeManager.CreateProxy (System.Type type, System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Java.Interop.TypeManager.CreateInstance (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type targetType) at Java.Lang.Object.GetObject (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type type) at Java.Lang.Object._GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Java.Lang.Object.GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Android.OS.Bundle.Get (System.String key) at Xamarin.Android.RuntimeTests.BundleTest.TestBundleIntegerArrayList2 () at (wrapper managed-to-native) System.Reflection.RuntimeMethodInfo.InternalInvoke(System.Reflection.RuntimeMethodInfo,object,object[],System.Exception&) at System.Reflection.RuntimeMethodInfo.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) The problem appears to be rooted in ce2bc68. In a pre-ce2bc689 world, there were two separate sets of "typemap" files that the application could use: * Java-to-managed mappings, in `typemap.jm` * Managed-to-Java mappings, in `typemap.mj`. These files did *not* need to have the same number of entries! % unzip /Library/Frameworks/Xamarin.Android.framework/Versions/10.2.0.100/lib/xamarin.android/xbuild/Xamarin/Android/Mono.Android.DebugRuntime-debug.apk typemap.jm % unzip /Library/Frameworks/Xamarin.Android.framework/Versions/10.2.0.100/lib/xamarin.android/xbuild/Xamarin/Android/Mono.Android.DebugRuntime-debug.apk typemap.mj % strings typemap.jm | wc -l 10318 % strings typemap.mj | wc -l 11956 The reason why they have a different number of entries is *aliasing*: there can be *multiple* bindings for a given Java type. The managed-to-Java mappings can contain *all* of them; the Java-to-managed mapping *must* pick Only One. For example, [`java.util.ArrayList`][0] contains (at least?) *three* bindings: * [`Android.Runtime.JavaList`][1] * [`Android.Runtime.JavaList<T>`][2] * `Java.Util.ArrayList` (generated at build time) In a pre-ce2bc689 world, this was "fine": we'd binary search each file *separately* to find type mappings. This changed in ce2bc68, as the typemap information was merged into a *single* array in order to de-duplicate information. This reduced `.apk` size. An unanticipated result of this is that the Java-to-managed mappings can now contain *duplicate* keys, "as if" the original `typemap.jm` had the contents: java/util/ArrayList Android.Runtime.JavaList, Mono.Android java/util/ArrayList Android.Runtime.JavaList`1, Mono.Android java/util/ArrayList Java.Util.ArrayLlist, Mono.Android Whereas pre-ce2bc689 there would have only been the first entry. "Sometimes" this duplication is "fine": the typemap search "Just happens" to return the first entry, or the 3rd entry, and apps continue to work as intended. "Sometimes" it isn't: the typemap binary search finds the 2nd entry, which is a generic type. This results in attempting to instantiate a generic type *without appropriate type parameters*, which results in the aforementioned `MemberAccessException`. Update typemap generation code in `Xamarin.Android.Build.Tasks.dll` so that all the duplicate Java type names will point to the same managed type name. Additionally, make sure we select the managed type in the same fashion the old typemap generator in `Java.Interop` worked: prefer base types to the derived ones if the type is an interface or an abstract class. The effect of this change is that no matter which entry `EmbeddedAssemblies::binary_search()` ends up selecting it will always return the same managed type name for all aliased Java types. ~~ Release config apps ~~ Another issue introduced in ce2bc68 is that we no longer ignore interface and generic types when generating the Java-to-managed maps. This adversely affects the `Release` mode in which it is possible that an attempt to instantiate an open generic type (or an interface) will happen, leading to error similar to: FATAL EXCEPTION: main Process: com.companyname.androidsingleviewapp1, PID: 24068 android.runtime.JavaProxyThrowable: System.MemberAccessException: Cannot create an instance of Com.Contoso.Javalibrary1.Class0`1[T] because Type.ContainsGenericParameters is true. at System.Reflection.RuntimeConstructorInfo.DoInvoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) at System.Reflection.RuntimeConstructorInfo.Invoke (System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) at System.Reflection.ConstructorInfo.Invoke (System.Object[] parameters) at Java.Interop.TypeManager.CreateProxy (System.Type type, System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Java.Interop.TypeManager.CreateInstance (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type targetType) at Java.Lang.Object.GetObject (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type type) at Java.Lang.Object._GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Java.Lang.Object.GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Com.Contoso.Javalibrary1.Class1.Create () at AndroidSingleViewApp1.MainActivity.OnCreate (Android.OS.Bundle savedInstanceState) at Android.App.Activity.n_OnCreate_Landroid_os_Bundle_ (System.IntPtr jnienv, System.IntPtr native__this, System.IntPtr native_savedInstanceState) at (wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.1(intptr,intptr,intptr) at crc647d7dcab8da8ee782.MainActivity.n_onCreate(Native Method) at crc647d7dcab8da8ee782.MainActivity.onCreate(MainActivity.java:29) at android.app.Activity.performCreate(Activity.java:7144) at android.app.Activity.performCreate(Activity.java:7135) at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1271) at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2931) at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3086) at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:78) at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:108) at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:68) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1816) at android.os.Handler.dispatchMessage(Handler.java:106) at android.os.Looper.loop(Looper.java:193) at android.app.ActivityThread.main(ActivityThread.java:6718) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858) Update typemap generation code to also ignore generic types and interfaces when generating the Java to Managed map in `Release` mode. We must not simply skip outputting entries for such types because we need to maintain element count parity between java-to-managed and managed-to-java lookup tables. Therefore, the way we skip such types is to output a type token ID of `0` instead of the actual one. This will cause the runtime code to fail to find a mapping, thus allowing the managed code to deal with such a type properly. ~~ Test Updates ~~ Update `BundleTest.TestBundleIntegerArrayList2()` so that it asserts the runtime *type* of `obj` returned, asserting that it is in fact a `JavaList` and not e.g. `Java.Util.ArrayList` or something. (The linker could otherwise "change" things, and this assertion will ensure that `JavaList` won't be linked away…) Add a unit test for the Release config app bug in Issue #4660; thanks to @brendanzagaeski. It works by provoking the binary search bug by "ensuring" that a *generic* type comes *before* the "normally preferred" *non*-generic type. Thus, even if/when binary search returns the first entry, that would still be the *wrong entry*, because that'll be a generic type! [0]: https://developer.android.com/reference/java/util/ArrayList [1]: https://github.com/xamarin/xamarin-android/blob/61f09bf2ef0c8f09550e2d77eb97f417432b315b/src/Mono.Android/Android.Runtime/JavaList.cs#L11-L13 [2]: https://github.com/xamarin/xamarin-android/blob/61f09bf2ef0c8f09550e2d77eb97f417432b315b/src/Mono.Android/Android.Runtime/JavaList.cs#L667-L668
Fixes: #4596
Fixes: #4660
Context: https://github.com/xamarin/monodroid/commit/192b1f1b71c98650d04bf0a4e52ee337a054ff4c
Context: https://github.com/xamarin/java.interop/blob/fc18c54b2ccf14f444fadac0a1e7e81f837cc470/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs#L149-L186
Context: https://github.com/xamarin/java.interop/blob/010161d1ef6625b762429334f02e7b15e1f7caae/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs#L155
~~ Debug Mode ~~
In some environments,
BundleTest.TestBundleIntegerArrayList2()
failswhen using the commercial shared runtime:
The problem appears to be rooted in ce2bc68. In a pre-ce2bc689 world,
there were two separate sets of "typemap" files that the application
could use:
typemap.jm
typemap.mj
.These files did not need to have the same number of entries!
The reason why they have a different number of entries is aliasing:
there can be multiple bindings for a given Java type. The
managed-to-Java mappings can contain all of them; the
Java-to-managed mapping must pick Only One.
For example,
java.util.ArrayList
contains (at least?) three bindings:Android.Runtime.JavaList
Android.Runtime.JavaList<T>
Java.Util.ArrayList
(generated at build time)In a pre-ce2bc689 world, this was "fine": we'd binary search each file
separately to find type mappings.
This changed in ce2bc68, as the typemap information was merged into a
single array in order to de-duplicate information. This reduced
.apk
size.An unanticipated result of this is that the Java-to-managed mappings
can now contain duplicate keys, "as if" the original
typemap.jm
had the contents:
java/util/ArrayList
Android.Runtime.JavaList, Mono.Android
java/util/ArrayList
Android.Runtime.JavaList`1, Mono.Android
java/util/ArrayList
Java.Util.ArrayLlist, Mono.Android
Whereas pre-ce2bc689 there would have only been the first entry.
"Sometimes" this duplication is "fine": the typemap search
"Just happens" to return the first entry, or the 3rd entry, and apps
continue to work as intended.
"Sometimes" it isn't: the typemap binary search finds the 2nd entry,
which is a generic type. This results in attempting to instantiate a
generic type without appropriate type parameters, which results in
the aforementioned
MemberAccessException
.Update typemap generation code in
Xamarin.Android.Build.Tasks
so thatall the duplicate Java type names will point to the same managed type
name. Additionally, make sure we select the managed type in the same
fashion the old typemap generator in
Java.Interop
worked - prefer basetypes to the derived ones if the type is an interface or an abstract
class. The effect of this change is that no matter which entry
binary_search
ends up selecting it will always return the same managedtype name for all the aliased Java types.
~~ Release mode ~~
Another issue introduced in ce2bc68 is that we no longer ignore
interface and generic types when generating the Java-to-managed maps.
This adversely affects the
Release
mode in which it is possible thatan attempt to instantiate an open generic type (or an interface) will
happen, leading to error similar to:
Update typemap generation code to also ignore generic types and
interfaces when generating the Java to Managed map in
Release
mode. Wemust not simply skip outputting entries for such types because we need
to maintain element count parity between java-to-managed and
managed-to-java lookup tables. Therefore, the way we skip such types is
to output a type token ID of
0
instead of the actual one. This willcause the runtime code to fail to find a mapping, thus allowing the
managed code to deal with such a type properly.
~~ Test Updates ~~
Update
TestBundleIntegerArrayList2()
so that it assertsthe runtime type of
obj
returned, asserting that it is in fact aJavaList
and not e.g.Java.Util.ArrayList
or something.(The linker could otherwise "change" things, and this assertion will
ensure that
JavaList
won't be linked away…)Add a unit test for issue #4660 (by @brendanzagaeski)
It's very cute. :-)
We sort the Java-to-managed type name table, so it can be binary
searched to find the Managed type.
Additionally, the "preferred" managed type is the first type in
monodis --typedef
output; the first type that we encounter whengoing through all types in an assembly.
Brendan's cute way of provoking the bug is to add a new generic type
which will come before the "normally preferred" (and correct!) type.
Thus, even if/when binary search returns the first entry, that'll
still be the wrong entry, because that'll be a generic type!
Very cute.