Prevent NREX when instantiating Java.Lang.StackTraceElement

[grendello]

Context: #8788 (comment)
Context: https://developer.android.com/reference/java/lang/StackTraceElement?hl=en#StackTraceElement(java.lang.String,%20java.lang.String,%20java.lang.String,%20int)

Java.Lang.StackTraceElement constructor requires the declaringClass
and methodName parameters to never be null. Pass the string
Unknown whenever any of this information is missing from the managed
stack frame.

Additionally, the lineNumber parameter is now set to -2 if we think
a stack frame points to native code.

[grendello]

@jonpryor our Java.Lang.StackTraceElement MCW generates the following signature which is, IMO, incorrect according to the Java API docs:

// Metadata.xml XPath constructor reference: path="/api/package[@name='java.lang']/class[@name='StackTraceElement']/constructor[@name='StackTraceElement' and count(parameter)=4 and parameter[1][@type='java.lang.String'] and parameter[2][@type='java.lang.String'] and parameter[3][@type='java.lang.String'] and parameter[4][@type='int']]"
[Register (".ctor", "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;I)V", "")]
public unsafe StackTraceElement (string? declaringClass, string? methodName, string? fileName, int lineNumber) : base (IntPtr.Zero, JniHandleOwnership.DoNotTransfer)

Shouldn't declaringClass and methodName be declared as non-nullable string?

[grendello]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[grendello]

/azp run

[jpobst]

Shouldn't declaringClass and methodName be declared as non-nullable string?

We do not manually assign nullability, we simply pass along what Google has annotated, and it appears this method has no @NonNull annotations declared in android.jar:

public final class StackTraceElement implements Serializable {
  public StackTraceElement(String declaringClass, String methodName, String fileName, int lineNumber) {
    throw new RuntimeException("Stub!");
  }
  ...
}

[jonpryor]

@grendello asked:

Shouldn't declaringClass and methodName be declared as non-nullable string?

We only flag a parameter or return type as non-null if it contains one of these annotations: https://github.com/xamarin/java.interop/blob/e557044f81917b61031f2ee887700ac43a526475/src/Xamarin.Android.Tools.Bytecode/XmlClassDeclarationBuilder.cs#L503-L512

By default, then, everything is allowed to be null, which is the behavior you're seeing.

We can use Metadata to update these parameters so that null is not allowed.

@jpobst would such an update be a good idea?

  <attr
      path="/api/package[@name='java.lang']/class[@name='StackTraceElement']/constructor/parameter[@type='java.lang.String']"
      name="not-null">true</attr>

It should be noted that (1) the above metadata is an oversimplication -- the filename String parameter should be allowed to be null -- and (2) the above Metadata results in a build break:

error RS0016: Symbol 'StackTraceElement' is not part of the declared public API
error RS0017: Symbol 'Java.Lang.StackTraceElement.StackTraceElement(string? declaringClass, string? methodName, string? fileName, int lineNumber) -> void' is part of the declared API, but is either not public or could not be found

[jpobst]

@jpobst would such an update be a good idea?

There are likely thousands of Android APIs that are missing nullability information. Thus our stated policy is that we will not manually fix them: https://learn.microsoft.com/en-us/xamarin/android/release-notes/11/11.0#monoandroid-nullable-reference-types-compatibility.

[grendello]

@jpobst would such an update be a good idea?

There are likely thousands of Android APIs that are missing nullability information. Thus our stated policy is that we will not manually fix them: https://learn.microsoft.com/en-us/xamarin/android/release-notes/11/11.0#monoandroid-nullable-reference-types-compatibility.

While it was fine before nullable support was added to .NET, I think now it would be good to change that policy... I realize it's a lot of work, but this is where the compiler (or the IDE) can help avoid mistakes like this one.

[jonpryor]

I'm reminded of: https://github.com/xamarin/xamarin-android/pull/8758/files#r1504920023

wherein StackFrame.GetMethod() may return null, while StackFrame.ToString() contains useful information.

I suspect that we should instead "lean in" to using StackFrame.ToString(), as we know managedFrame is not null (if it were, we'd get a NullReferenceException from here!).

[grendello]

I don't want to use ToString() output, as it's unwieldy to handle in a presentable manner. However, the information mentioned above can be obtained using the StackFrame extension methods (which this PR already uses in a small capacity). We can get this kind of output:

MainActivity.OnCreate() + 0x37 at offset 55 in file:line:column <filename unknown>:0:0

Only I'm not sure how useful it really is.

[jonpryor]

@grendello: parsing MainActivity.OnCreate() is more useful than providing Unknown.Unknown:-1 as the stack frame information to Java. Is it perfect? No. But it's better than the alternative.