Skip to content

Commit

Permalink
[monodroid] Return first entry in binary_search
Browse files Browse the repository at this point in the history
Fixes: #4596

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

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 `EmbeddedAssemblies::binary_search()` so that instead of
returning the "first" entry that the binary search happens to land on,
probe *previous* entries in the type mappings to see if they *also*
contain the same key.  `binary_search()` can instead return the
*first entry* within the mapping, not just the first it happens to
land upon while binary searching.

This should allow for more *consistent* behaviors, ensuring that
Java-to-managed typemap lookup obtains a *non*-generic type, thus
avoiding the `MemberAccessException`.

Additionally, 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…)

[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
  • Loading branch information
jonpryor committed May 1, 2020
1 parent 61f09bf commit 7a317f8
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 3 deletions.
1 change: 1 addition & 0 deletions src/Mono.Android/Test/Android.OS/BundleTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public void TestBundleIntegerArrayList2 ()
b.PutIntegerArrayList ("key", new List<Java.Lang.Integer> () { Java.Lang.Integer.ValueOf (1) });
var obj = b.Get ("key");
Assert.NotNull (obj, "Missing 'key' in bundle");
Assert.IsTrue (obj is global::Android.Runtime.JavaList, "`obj` should be a JavaList!");
try {
var list = b.GetIntegerArrayList ("key");
Assert.NotNull (list, "'key' doesn't refer to a list of integers");
Expand Down
24 changes: 21 additions & 3 deletions src/monodroid/jni/embedded-assemblies.cc
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,8 @@ EmbeddedAssemblies::binary_search (const Key *key, const Entry *base, size_t nme
}

constexpr size_t size = sizeof(Entry);
const Entry *ret = nullptr;
while (nmemb > 0) {
const Entry *ret;
if constexpr (use_extra_size) {
ret = reinterpret_cast<const Entry*>(reinterpret_cast<const uint8_t*>(base) + ((size + extra_size) * (nmemb / 2)));
} else {
Expand All @@ -168,11 +168,29 @@ EmbeddedAssemblies::binary_search (const Key *key, const Entry *base, size_t nme
}
nmemb -= nmemb / 2 + 1;
} else {
return ret;
break;
}
}

return nullptr;
if (ret == nullptr) {
return ret;
}

// `base` may contain duplicate keys. Return the *first* entry for a given key.
while (ret > base) {
const Entry *prev_ret;
if constexpr (use_extra_size) {
prev_ret = reinterpret_cast<const Entry*>(reinterpret_cast<const uint8_t*>(ret) - (size + extra_size));
} else {
prev_ret = reinterpret_cast<const Entry*>(reinterpret_cast<const uint8_t*>(ret) - size);
}
int result = compare (key, prev_ret);
if (result != 0) {
break;
}
ret = prev_ret;
}
return ret;
}

#if defined (DEBUG) || !defined (ANDROID)
Expand Down

0 comments on commit 7a317f8

Please sign in to comment.