Skip to content
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

[generator] Use proper syntax for nested classes for default interface method invokers. #662

Merged
merged 1 commit into from
Jun 11, 2020

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Jun 11, 2020

Fixes #661

When we [Register] the invoker for a default method in a nested interface, we are using . to denote a nested interface, when it should be a /.

For example, in:

[Register ("onActivityPostCreated", "(Landroid/app/Activity;Landroid/os/Bundle;)V", "GetOnActivityPostCreated_Landroid_app_Activity_Landroid_os_Bundle_Handler:Android.App.Application.IActivityLifecycleCallbacks, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null", ApiSince = 29)]

Android.App.Application.IActivityLifecycleCallbacks should be Android.App.Application/IActivityLifecycleCallbacks.

This is causing the nested type to not be found.

Fixed by using AssemblyQualifiedName instead of FullName, which is explicitly defined to support the correct notation.

@jonpryor
Copy link
Member

jonpryor commented Jun 11, 2020

Squash-and-merge:

Summary:

[generator] Use proper syntax for nested classes for DIM invokers (#662)

Body:

Fixes: https://github.com/xamarin/java.interop/issues/661

App crashes with a `TypeLoadException` when instantiating a class
which implements an interface containing default interface members:

 1. Create a new app within Visual Studio 16.6 or 16.7.

 2. Set the `$(TargetFrameworkVersion)` to v10.0.99.  This pulls in a
    `Mono.Android.dll` which uses default interface methods.

 3. Add the following class to the project.  The
    `Android.App.Application.IActivityLifecycleCallbacks` interface
    is (1) a nested type, and (2) contains default interface methods.

        class ActivityLifecycleContextListener : Java.Lang.Object, Android.App.Application.IActivityLifecycleCallbacks
        {
            public void OnActivityCreated (Activity activity, Bundle savedInstanceState)    => throw new NotImplementedException ();
            public void OnActivityDestroyed (Activity activity)                             => throw new NotImplementedException ();
            public void OnActivityPaused (Activity activity)                                => throw new NotImplementedException ();
            public void OnActivityResumed (Activity activity)                               => throw new NotImplementedException ();
            public void OnActivitySaveInstanceState (Activity activity, Bundle outState)    => throw new NotImplementedException ();
            public void OnActivityStarted (Activity activity)                               => throw new NotImplementedException ();
            public void OnActivityStopped (Activity activity)                               => throw new NotImplementedException ();
        }

 4. Instantiate `ActivityLifecycleContextListener` within the app's
    `OnCreate()` method:

        var a = new ActivityLifecycleContextListener ();

Expected results: it works!

Actual results:

	System.TypeLoadException: Could not load type 'Android.App.Application.IActivityLifecycleCallbacks' from assembly 'Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.
	  at System.RuntimeType.GetType (System.String typeName, System.Boolean throwOnError, System.Boolean ignoreCase, System.Boolean reflectionOnly, Sys06-08 19:19:38.543 I/MonoDroid( 5187):   at (wrapper managed-to-native) System.RuntimeTypeHandle.internal_from_name(string,System.Threading.StackCrawlMark&,System.Reflection.Assembly,bool,bool,bool)
	  at System.RuntimeTypeHandle.GetTypeByName (System.String typeName, System.Boolean throwOnError, System.Boolean ignoreCase, System.Boolean reflectionOnly, System.Threading.StackCrawlMark& stackMark, System.Boolean loadTypeFromPartialName)
	  at (wrapper managed-to-native) Java.Interop.NativeM06-08
	  at System.Type.GetType (System.String typeName, System.Boolean throwOnError)
	  at Android.Runtime.AndroidTypeManager.RegisterNativeMembers (Java.Interop.JniType nativeClass, System.Type type, System.String methods)
	  at Android.Runtime.JNIEnv.RegisterJniNatives (System.IntPtr typeName_ptr, System.Int32 typeName_len, System.IntPtr jniClass, System.IntPtr methods_ptr, System.Int32 methods_len)
	  …

The cause of the bug is that Xamarin.Android attempts to call:

	Type.GetType ("Android.App.Application.IActivityLifecycleCallbacks, Mono.Android");

which fails, because this is the wrong syntax for looking up nested
types via Reflection.  Xamarin.Android should instead be doing:

	Type.GetType ("Android.App.Application/IActivityLifecycleCallbacks, Mono.Android");

Note `/` instead of `.` in the type name.

Why was Xamarin.Android trying to load the wrong type?  Because the
*Java Callable Wrapper* contained the wrong type, and Xamarin.Android
used the string as-is:

	public /* partial */ class ActivityLifecycleContextListener extends java.lang.Object
		implements mono.android.IGCUserPeer, android.app.Application.ActivityLifecycleCallbacks
	{
	/** @hide */
	    public static final String __md_methods;
	    static {
	        __md_methods = 
	            "n_onActivityCreated:(Landroid/app/Activity;Landroid/os/Bundle;)V:GetOnActivityCreated_Landroid_app_Activity_Landroid_os_Bundle_Handler:Android.App.Application/IActivityLifecycleCallbacksInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
	            …
	            "n_onActivityPreStopped:(Landroid/app/Activity;)V:GetOnActivityPreStopped_Landroid_app_Activity_Handler:Android.App.Application.IActivityLifecycleCallbacks, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
	            "";
	        …
	    }
	}

"Interesting" here is the difference between the first and the last
entry: the first entry is for a "normal" interface method, which uses
the required `/` nested type separator, while the last entry is for a
Java default interface method.

Aside/TODO: Why is `n_onActivityPreStopped()` being registered when
`Application.IActivityLifecycleCallbacks.OnActivityPreStopped()` is
not implemented in `ActivityLifecycleContextListener`?

Where do the `__md_methods` entries come from?  From the binding
assembly; from `generator` output!

	public partial interface IActivityLifecycleCallbacks : IJavaObject, IJavaPeerable {
	    // Metadata.xml XPath method reference: path="/api/package[@name='android.app']/interface[@name='Application.ActivityLifecycleCallbacks']/method[@name='onActivityPreStopped' and count(parameter)=1 and parameter[1][@type='android.app.Activity']]"
	    [Register ("onActivityPreStopped", "(Landroid/app/Activity;)V", "GetOnActivityPreStopped_Landroid_app_Activity_Handler:Android.App.Application.IActivityLifecycleCallbacks, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null", ApiSince = 29)]
	    virtual unsafe void OnActivityPreStopped (Android.App.Activity activity) {…}
	}

The 3rd `RegisterAttribute` parameter, which becomes the
`RegisterAttribute.Connector` property, is where the offending string
comes from:

	…:Android.App.Application.IActivityLifecycleCallbacks, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null"

The fix is to update `Method.GetConectorNameFull()` to use
`DeclaringType.AssemblyQualifiedName`, *not* `DeclaringType.FullName`,
as the former is what's needed for reflection compatibility, while the
latter is a "C#" name, generally more useful for error messages.

@jonpryor jonpryor merged commit 9a56465 into master Jun 11, 2020
@jonpryor jonpryor deleted the nested-dim-invoker branch June 11, 2020 21:57
jonpryor pushed a commit that referenced this pull request Jun 11, 2020
Fixes: #661

App crashes with a `TypeLoadException` when instantiating a class
which implements an interface containing default interface members:

 1. Create a new app within Visual Studio 16.6 or 16.7.

 2. Set the `$(TargetFrameworkVersion)` to v10.0.99.  This pulls in a
    `Mono.Android.dll` which uses default interface methods.

 3. Add the following class to the project.  The
    `Android.App.Application.IActivityLifecycleCallbacks` interface
    is (1) a nested type, and (2) contains default interface methods.

        class ActivityLifecycleContextListener : Java.Lang.Object, Android.App.Application.IActivityLifecycleCallbacks
        {
            public void OnActivityCreated (Activity activity, Bundle savedInstanceState)    => throw new NotImplementedException ();
            public void OnActivityDestroyed (Activity activity)                             => throw new NotImplementedException ();
            public void OnActivityPaused (Activity activity)                                => throw new NotImplementedException ();
            public void OnActivityResumed (Activity activity)                               => throw new NotImplementedException ();
            public void OnActivitySaveInstanceState (Activity activity, Bundle outState)    => throw new NotImplementedException ();
            public void OnActivityStarted (Activity activity)                               => throw new NotImplementedException ();
            public void OnActivityStopped (Activity activity)                               => throw new NotImplementedException ();
        }

 4. Instantiate `ActivityLifecycleContextListener` within the app's
    `OnCreate()` method:

        var a = new ActivityLifecycleContextListener ();

Expected results: it works!

Actual results:

	System.TypeLoadException: Could not load type 'Android.App.Application.IActivityLifecycleCallbacks' from assembly 'Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.
	  at System.RuntimeType.GetType (System.String typeName, System.Boolean throwOnError, System.Boolean ignoreCase, System.Boolean reflectionOnly, Sys06-08 19:19:38.543 I/MonoDroid( 5187):   at (wrapper managed-to-native) System.RuntimeTypeHandle.internal_from_name(string,System.Threading.StackCrawlMark&,System.Reflection.Assembly,bool,bool,bool)
	  at System.RuntimeTypeHandle.GetTypeByName (System.String typeName, System.Boolean throwOnError, System.Boolean ignoreCase, System.Boolean reflectionOnly, System.Threading.StackCrawlMark& stackMark, System.Boolean loadTypeFromPartialName)
	  at (wrapper managed-to-native) Java.Interop.NativeM06-08
	  at System.Type.GetType (System.String typeName, System.Boolean throwOnError)
	  at Android.Runtime.AndroidTypeManager.RegisterNativeMembers (Java.Interop.JniType nativeClass, System.Type type, System.String methods)
	  at Android.Runtime.JNIEnv.RegisterJniNatives (System.IntPtr typeName_ptr, System.Int32 typeName_len, System.IntPtr jniClass, System.IntPtr methods_ptr, System.Int32 methods_len)
	  …

The cause of the bug is that Xamarin.Android attempts to call:

	Type.GetType ("Android.App.Application.IActivityLifecycleCallbacks, Mono.Android");

which fails, because this is the wrong syntax for looking up nested
types via Reflection.  Xamarin.Android should instead be doing:

	Type.GetType ("Android.App.Application/IActivityLifecycleCallbacks, Mono.Android");

Note `/` instead of `.` in the type name.

Why was Xamarin.Android trying to load the wrong type?  Because the
*Java Callable Wrapper* contained the wrong type, and Xamarin.Android
used the string as-is:

	public /* partial */ class ActivityLifecycleContextListener extends java.lang.Object
		implements mono.android.IGCUserPeer, android.app.Application.ActivityLifecycleCallbacks
	{
	/** @hide */
	    public static final String __md_methods;
	    static {
	        __md_methods = 
	            "n_onActivityCreated:(Landroid/app/Activity;Landroid/os/Bundle;)V:GetOnActivityCreated_Landroid_app_Activity_Landroid_os_Bundle_Handler:Android.App.Application/IActivityLifecycleCallbacksInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
	            …
	            "n_onActivityPreStopped:(Landroid/app/Activity;)V:GetOnActivityPreStopped_Landroid_app_Activity_Handler:Android.App.Application.IActivityLifecycleCallbacks, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
	            "";
	        …
	    }
	}

"Interesting" here is the difference between the first and the last
entry: the first entry is for a "normal" interface method, which uses
the required `/` nested type separator, while the last entry is for a
Java default interface method.

Aside/TODO: Why is `n_onActivityPreStopped()` being registered when
`Application.IActivityLifecycleCallbacks.OnActivityPreStopped()` is
not implemented in `ActivityLifecycleContextListener`?

Where do the `__md_methods` entries come from?  From the binding
assembly; from `generator` output!

	public partial interface IActivityLifecycleCallbacks : IJavaObject, IJavaPeerable {
	    // Metadata.xml XPath method reference: path="/api/package[@name='android.app']/interface[@name='Application.ActivityLifecycleCallbacks']/method[@name='onActivityPreStopped' and count(parameter)=1 and parameter[1][@type='android.app.Activity']]"
	    [Register ("onActivityPreStopped", "(Landroid/app/Activity;)V", "GetOnActivityPreStopped_Landroid_app_Activity_Handler:Android.App.Application.IActivityLifecycleCallbacks, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null", ApiSince = 29)]
	    virtual unsafe void OnActivityPreStopped (Android.App.Activity activity) {…}
	}

The 3rd `RegisterAttribute` parameter, which becomes the
`RegisterAttribute.Connector` property, is where the offending string
comes from:

	…:Android.App.Application.IActivityLifecycleCallbacks, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null"

The fix is to update `Method.GetConectorNameFull()` to use
`DeclaringType.AssemblyQualifiedName`, *not* `DeclaringType.FullName`,
as the former is what's needed for reflection compatibility, while the
latter is a "C#" name, generally more useful for error messages.
@jpobst jpobst added this to the 10.4 (16.7 / 8.7) milestone Jun 15, 2020
@jpobst
Copy link
Contributor Author

jpobst commented Jun 15, 2020

Release Notes

- [Java.Interop GitHub PR 662](https://github.com/xamarin/java.interop/pull/662):
  Fix `Could not load type ...` crash caused by trying to use a default interface method on
  a nested interface.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid [Register] for nested default interface method invokers
2 participants