[monodroid] Return first entry in binary_search #4645
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
jonpryor
force-pushed
the
jonp-bsearch-returns-first-match
branch
from
7a317f8
to
5016dc4
Compare
brendanzagaeski
approved these changes
May 1, 2020
Comment on lines
+1
to
+5
| #### Application behavior on device and emulator | ||
|
|
||
| * [GitHub 4596](https://github.com/xamarin/xamarin-android/issues/4596): | ||
| Ensure that Java-to-managed type mappings lookup the correct non-generic | ||
| binding type, avoiding a *System.MemberAccessException*. |
There was a problem hiding this comment.
Looks good. As a heads-up, I'll plan to adjust this to the following for the final aggregation of the release notes:
#### Application behavior on device and emulator
* [GitHub 4596](https://github.com/xamarin/xamarin-android/issues/4596):
Starting in Xamarin.Android 10.3 Preview, *System.MemberAccessException*
could cause apps to abort when using certain APIs involving generic types.
I've got in the habit of using the "Starting in..." phrase to let users know the scope of the fix for regressions and to help me see which items can removed from the final GA release notes. The other adjustment is to move the user-visible message text to appear near the beginning, just after that "Starting in..." phrase, because that text is what a user would be most likely to look for to recognize the issue if they were hitting it.
Further suggestions on the note are welcome, either as comments on this PR or as changes in the committed note, but no need to change the committed note otherwise, unless you want to. Thanks!
|
I don't like this solution as it sacrifices performance doing on the runtime what should rather be done on the build time (and especially that it affects the |
grendello
approved these changes
May 1, 2020
|
Alas, crashes in the APK tests: I'm not immediately sure why this would be happening. |
Context: #4645 (comment) The first attempt crashed. Oh well, right? But I can't see what's wrong with the original fix. (Oh well, right?) Then I started losing my mind, because my local `adb logcat` had *bizarre* output such as: monodroid-timing: Typemap.managed_to_java: `Java.Interop.ManagedPeer` -> java/io/Writer … mono-stdout: Fail: ManagedPeerType <=> JniTypeName Mismatch! javaVM.GetJniTypeInfoForType(typeof(Java.Interop.ManagedPeer)).JniTypeName="java/io/Writer" != "com/xamarin/java_interop/ManagedPeer which is *bizarrely* wrong. (Also, e4f45a2 has a NULL DEREF which is fixed in this commit.) So, to check my sanity, I added a `linear_layout()` member function. `binary_search()` is rocket science; *surely* I can write a linear search that works, right? Add a `linear_search()` function, and use it alongside `binary_search()`, to check that they match. THEY DON'T MATCH: monodroid-assembly: # jonp: typemap_java_to_managed: key mismatch for `android/app/ContextImpl`! binary_search=0xca2a8f90 linear_search=0x0 monodroid: typemap: Java type 'android/app/ContextImpl' corresponds to managed type 'Android.App.DatePickerDialog, Mono.Android' Which, again, an *insane* mapping -- `ContextImpl` should have NO mapping, as it's `package-private` and not documented -- yet we're mapping it to `DatePickerDialog` of all things! So either my `linear_search()` is broken, or -- maybe?! -- `binary_search()` is broken. I'm starting to bet on `binary_search()` being broken, and that's *before* my changes to it, because I added more logging to `binary_search()` to see the "distance" between the originally found value and the "adjusted" value. The distance is *always* 0: monodroid: # jonp: binary_search: found ret=0xca2aefa8; trying previous entries! monodroid: # jonp: binary_search: new ret=0xca2aefa8; distance=0l I've *never* seen a non-zero distance value. Thus, fit of lunacy: what if I make `linear_search()` the "primary" and `binary_search()` the secondary, i.e. `linear_search()` is what *actually* makes things work, and we check the results of `binary_search()` against `linear_search()` for sanity? It works! It looks *sane*! monodroid-assembly: # jonp: typemap_java_to_managed: key mismatch for `android/app/ContextImpl`! linear_search=0x0 binary_search=0xca2b2f90 Again, ContextImpl IS NOT BOUND; they should BOTH be NULL! Yes, a linear search obviously sucks, perf-wise. That's not the point. The point is that: 1. it *really* looks like our binary_search is *broken*, 2. I don't *think* my changes are what broke it, 3. binary_search remains borderline rocket science to me.
Commit af13997 asked, "is binary_search broken?" Answer: Yes, because 5016dc4 broke it: @@ -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 { @@ -168,11 +168,29 @@ EmbeddedAssemblies::binary_search (const Key *key, const Entry *base, size_t nme } nmemb -= nmemb / 2 + 1; } else { - return ret; + break; } } The bug is that in moving `ret` up a scope, nothing *clears out* `ret` on error! Thus, it's "garbage" when the `while(nmemb>0)` loop terminates and no key has been found, which is why it *always* returned a non-null value. (Rocket science! And bad code refactoring. And PEBKAC.) The fixis to `ret=nullptr` on every loop iteration unless we have a value value.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #4596
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
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.jmtypemap.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.ArrayListcontains (at least?) three bindings:Android.Runtime.JavaListAndroid.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
.apksize.An unanticipated result of this is that the Java-to-managed mappings
can now contain duplicate keys, "as if" the original
typemap.jmhad 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 ofreturning 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 thefirst 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 assertsthe runtime type of
objreturned, asserting that it is in fact aJavaListand not e.g.Java.Util.ArrayListor something.(The linker could otherwise "change" things, and this assertion will
ensure that
JavaListwon't be linked away…)