Skip to content

[typemap] Handle duplicates of generic types properly #4845

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jun 22, 2020

Conversation

grendello
Copy link
Contributor

Fixes: #4809
Context: 7117414
Context: a017561

Part of the a017561 commit dealt with mappings from certain Java types
which point to the same Managed type by making sure that they all refer
to the same type definition and managed type name. However, in doing so
it failed to synchronize the property which indicates that we are
dealing with an interface or generic type, in which case such a type
should be ignored by the type mapping code in the runtime.

In this particular case, the issue was with the ArrayAdapter type
which has both a generic and non-generic definitions and it so happens
that the first type in Mono.Android assembly is the generic one,
thus it, and its duplicate, should be ignored but due to the failure to
synchronize the SkipInJavaToManaged property, the second instance of
the type was not skipped on the runtime, resulting in the following
exception:

System.MemberAccessException: Cannot create an instance of Android.Widget.ArrayAdapter`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) [0x00054] in <d772a4bbe86c4dc5b425d8ca91786b91>:0
  at System.Reflection.RuntimeConstructorInfo.Invoke (System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00000] in <d772a4bbe86c4dc5b425d8ca91786b91>:0
  at System.Reflection.ConstructorInfo.Invoke (System.Object[] parameters) [0x00000] in <d772a4bbe86c4dc5b425d8ca91786b91>:0
  at Java.Interop.TypeManager.CreateProxy (System.Type type, System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) [0x0001b] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
  at Java.Interop.TypeManager.CreateInstance (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type targetType) [0x00111] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
  at Java.Lang.Object.GetObject (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type type) [0x00023] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
  at Java.Lang.Object._GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) [0x00017] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
  at Java.Lang.Object.GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) [0x00000] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
  at Android.Widget.ArrayAdapter.CreateFromResource (Android.Content.Context context, System.Int32 textArrayResId, System.Int32 textViewResId) [0x0006d] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
  at AndroidApp1.MainActivity.OnCreate (Android.OS.Bundle savedInstanceState) [0x00031] in <51ce4ed05744482eb8a8079a399a17d2>:0
  at Android.App.Activity.n_OnCreate_Landroid_os_Bundle_ (System.IntPtr jnienv, System.IntPtr native__this, System.IntPtr native_savedInstanceState) [0x0000f] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
  at (wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.3(intptr,intptr,intptr)

This commit adds synchronization of the SkipInJavaToManaged property
which results in the sample attached to the GitHub issue to work
properly.

Note: for some weird reason the error occurred only with the Release
build of Mono.Android.dll and only in the framework v9.0 (that is
API28) - if the app targetted any other API it would not fail. I can't
see why this would be so since the relevant code was very similar
between v9.0 and other frameworks and it was identical in case of
the Debug build of the v9.0 framework.

Fixes: dotnet#4809
Context: 7117414
Context: a017561

Part of the a017561 commit dealt with mappings from certain Java types
which point to the same Managed type by making sure that they all refer
to the same type definition and managed type name.  However, in doing so
it failed to synchronize the property which indicates that we are
dealing with an interface or generic type, in which case such a type
should be ignored by the type mapping code in the runtime.

In this particular case, the issue was with the `ArrayAdapter` type
which has both a generic and non-generic definitions and it so happens
that the *first* type in `Mono.Android` assembly is the generic one,
thus it, and its duplicate, should be ignored but due to the failure to
synchronize the `SkipInJavaToManaged` property, the second instance of
the type was not skipped on the runtime, resulting in the following
exception:

    System.MemberAccessException: Cannot create an instance of Android.Widget.ArrayAdapter`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) [0x00054] in <d772a4bbe86c4dc5b425d8ca91786b91>:0
      at System.Reflection.RuntimeConstructorInfo.Invoke (System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00000] in <d772a4bbe86c4dc5b425d8ca91786b91>:0
      at System.Reflection.ConstructorInfo.Invoke (System.Object[] parameters) [0x00000] in <d772a4bbe86c4dc5b425d8ca91786b91>:0
      at Java.Interop.TypeManager.CreateProxy (System.Type type, System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) [0x0001b] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
      at Java.Interop.TypeManager.CreateInstance (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type targetType) [0x00111] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
      at Java.Lang.Object.GetObject (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type type) [0x00023] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
      at Java.Lang.Object._GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) [0x00017] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
      at Java.Lang.Object.GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) [0x00000] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
      at Android.Widget.ArrayAdapter.CreateFromResource (Android.Content.Context context, System.Int32 textArrayResId, System.Int32 textViewResId) [0x0006d] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
      at AndroidApp1.MainActivity.OnCreate (Android.OS.Bundle savedInstanceState) [0x00031] in <51ce4ed05744482eb8a8079a399a17d2>:0
      at Android.App.Activity.n_OnCreate_Landroid_os_Bundle_ (System.IntPtr jnienv, System.IntPtr native__this, System.IntPtr native_savedInstanceState) [0x0000f] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
      at (wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.3(intptr,intptr,intptr)

This commit adds synchronization of the `SkipInJavaToManaged` property
which results in the sample attached to the GitHub issue to work
properly.

Note: for some weird reason the error occurred *only* with the *Release*
build of `Mono.Android.dll` and *only* in the framework v9.0 (that is
API28) - if the app targetted any other API it would not fail.  I can't
see why this would be so since the relevant code was very similar
between v9.0 and other frameworks and it was *identical* in case of
the *Debug* build of the v9.0 framework.
@jonpryor
Copy link
Member

jonpryor commented Jun 22, 2020

My concern/thought here is:

for some weird reason the error occurred…

meaning we don't fully understand the failure condition. "For some weird reason," we got an invalid mapping, presumably because of this change.

However, this in turn suggests that we could use more "defense in depth" around this scenario: given that we require that all Java-to-managed mappings not contain generic types, yet generic types keep getting included in there, how can we assert this requirement?

(This is where the "typemap dump" utility could be handy, for manual verification…)

Should we update TypeNameMapGenerator.GetTypeMapping() to explicitly check for and skip generic types? (Probably, but would require a submodule bump.)

Should we update TypeMapGenerator.cs to explicitly check for generic types in the java-to-managed mapping and error out when it happens?

Is there anything else that we can do to help prevent a similar "silent failure" in the future?

@grendello
Copy link
Contributor Author

grendello commented Jun 22, 2020

@jonpryor I agree completely that we should do something - we need to discuss what exactly. I absolutely don't like the fact that the current typemap scanner returns types the type map generator and runtime need to work around in order to not crash on the runtime. However, the fix here is good and correct for what we have right now, even though we don't understand what happened. TypeMapGenerator already checks for generic types and marks them as, let's say, "invalid" in the generated map. But it shouldn't work like that.

@jonpryor jonpryor merged commit bb55bb0 into dotnet:master Jun 22, 2020
@grendello grendello deleted the issue4809 branch June 22, 2020 21:22
jonpryor pushed a commit that referenced this pull request Jun 23, 2020
Fixes: #4809
Context: 7117414
Context: a017561

Part of commit a017561 dealt with mappings from certain Java types
which point to the same Managed type by making sure that they all refer
to the same type definition and managed type name.  However, in doing
so it failed to synchronize the property which indicates that we are
dealing with an interface or generic type, in which case such a type
should be ignored by the type mapping code in the runtime.

In this particular case, the issue was with the `ArrayAdapter` type,
which has both generic and non-generic definitions, and it so happens
that the *first* type in `Mono.Android` assembly is the generic one,
thus it, and its duplicate, should be ignored but due to the failure to
synchronize the `SkipInJavaToManaged` property, the second instance of
the type was not skipped on the runtime, resulting in the following
exception:

	System.MemberAccessException: Cannot create an instance of Android.Widget.ArrayAdapter`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) [0x00054] in <d772a4bbe86c4dc5b425d8ca91786b91>:0
	  at System.Reflection.RuntimeConstructorInfo.Invoke (System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00000] in <d772a4bbe86c4dc5b425d8ca91786b91>:0
	  at System.Reflection.ConstructorInfo.Invoke (System.Object[] parameters) [0x00000] in <d772a4bbe86c4dc5b425d8ca91786b91>:0
	  at Java.Interop.TypeManager.CreateProxy (System.Type type, System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) [0x0001b] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
	  at Java.Interop.TypeManager.CreateInstance (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type targetType) [0x00111] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
	  at Java.Lang.Object.GetObject (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type type) [0x00023] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
	  at Java.Lang.Object._GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) [0x00017] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
	  at Java.Lang.Object.GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) [0x00000] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
	  at Android.Widget.ArrayAdapter.CreateFromResource (Android.Content.Context context, System.Int32 textArrayResId, System.Int32 textViewResId) [0x0006d] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
	  at AndroidApp1.MainActivity.OnCreate (Android.OS.Bundle savedInstanceState) [0x00031] in <51ce4ed05744482eb8a8079a399a17d2>:0
	  at Android.App.Activity.n_OnCreate_Landroid_os_Bundle_ (System.IntPtr jnienv, System.IntPtr native__this, System.IntPtr native_savedInstanceState) [0x0000f] in <43cddc76fe22432dbdbcea2397b6fcd7>:0
	  at (wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.3(intptr,intptr,intptr)

This commit adds synchronization of the `SkipInJavaToManaged` property
which results in the sample attached to the GitHub issue to work
properly.

Note: for some weird reason the error occurred *only* with the
*Release* build of `Mono.Android.dll` and *only* with
`$(TargetFrameworkVersion)`=v9.0 (API-28); if the app used any other
framework version it would not fail.  I can't see why this would be so
since the relevant code was very similar between v9.0 and other
frameworks and it was *identical* in case of the *Debug* build of the
v9.0 framework.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 26, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
5 participants