Skip to content

Prevent NREX when instantiating Java.Lang.StackTraceElement #8795

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Mar 17, 2024
Merged

Prevent NREX when instantiating Java.Lang.StackTraceElement #8795

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
de9a903
Prevent NREX when instantiating Java.Lang.StackTraceElement
grendello Mar 7, 2024
2a9acb4
Merge branch 'main' into dev/grendel/fix-proxy-throwable
grendello Mar 7, 2024
6b9b064
Merge branch 'main' into dev/grendel/fix-proxy-throwable
grendello Mar 8, 2024
695304c
Update unit test
grendello Mar 8, 2024
7971f77
Merge branch 'main' into dev/grendel/fix-proxy-throwable
grendello Mar 13, 2024
705a6ec
More comprehensive frame reporting
grendello Mar 13, 2024
15da2c7
Merge branch 'main' into dev/grendel/fix-proxy-throwable
grendello Mar 15, 2024
e13935d
Fix oops(1) and oops(2)
grendello Mar 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions src/Mono.Android/Android.Runtime/JavaProxyThrowable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,28 @@ void TranslateStackTrace ()
int nElements = frames.Length + (javaTrace?.Length ?? 0);
StackTraceElement[] elements = new StackTraceElement[nElements];

const string Unknown = "Unknown";
for (int i = 0; i < frames.Length; i++) {
StackFrame managedFrame = frames[i];
MethodBase? managedMethod = StackFrameGetMethod (managedFrame);

// https://developer.android.com/reference/java/lang/StackTraceElement?hl=en#StackTraceElement(java.lang.String,%20java.lang.String,%20java.lang.String,%20int)
int lineNumber;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!).

Copy link
Contributor Author

@grendello grendello Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

if (managedFrame != null) {
lineNumber = managedFrame.GetFileLineNumber ();
if (lineNumber == 0) {
// -2 means it's a native frame
lineNumber = managedFrame.HasNativeImage () ? -2 : -1;
}
} else {
lineNumber = -1;
}

var throwableFrame = new StackTraceElement (
declaringClass: managedMethod?.DeclaringType?.FullName,
methodName: managedMethod?.Name,
declaringClass: managedMethod?.DeclaringType?.FullName ?? Unknown,
methodName: managedMethod?.Name ?? Unknown,
fileName: managedFrame?.GetFileName (),
lineNumber: managedFrame?.GetFileLineNumber () ?? -1
lineNumber: lineNumber
);

elements[i] = throwableFrame;
Expand Down
22 changes: 14 additions & 8 deletions tests/Mono.Android-Tests/System/ExceptionTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,13 @@ public void InnerExceptionIsSet ()
ex = e;
}

using (Java.Lang.Throwable proxy = CreateJavaProxyThrowable (ex))
using (var source = new Java.Lang.Throwable ("detailMessage", proxy))
using (var alias = new Java.Lang.Throwable (source.Handle, JniHandleOwnership.DoNotTransfer)) {
CompareStackTraces (ex, proxy);
Assert.AreEqual ("detailMessage", alias.Message);
Assert.AreSame (ex, alias.InnerException);
}
using Java.Lang.Throwable proxy = CreateJavaProxyThrowable (ex);
using var source = new Java.Lang.Throwable ("detailMessage", proxy);
using var alias = new Java.Lang.Throwable (source.Handle, JniHandleOwnership.DoNotTransfer);

CompareStackTraces (ex, proxy);
Assert.AreEqual ("detailMessage", alias.Message);
Assert.AreSame (ex, alias.InnerException);
}

void CompareStackTraces (Exception ex, Java.Lang.Throwable throwable)
Expand All @@ -61,10 +61,16 @@ void CompareStackTraces (Exception ex, Java.Lang.Throwable throwable)
var mf = managedFrames[i];
var jf = javaFrames[i];

// Unknown line locations are -1 on the Java side
int managedLine = mf.GetFileLineNumber ();
if (managedLine == 0) {
managedLine = -1;
}

Assert.AreEqual (mf.GetMethod ()?.Name, jf.MethodName, $"Frame {i}: method names differ");
Assert.AreEqual (mf.GetMethod ()?.DeclaringType.FullName, jf.ClassName, $"Frame {i}: class names differ");
Assert.AreEqual (mf.GetFileName (), jf.FileName, $"Frame {i}: file names differ");
Assert.AreEqual (mf.GetFileLineNumber (), jf.LineNumber, $"Frame {i}: line numbers differ");
Assert.AreEqual (managedLine, jf.LineNumber, $"Frame {i}: line numbers differ");
}
}
}
Expand Down