xamarin / GooglePlayServicesComponents

Other
316 stars 147 forks source link

Crashlytics RecordException uses Throwable instead of Exception, which leads to unusable log #423

Open zoli13 opened 3 years ago

zoli13 commented 3 years ago

So with the lastest Firebase Crashlytics (117.3.0 ), the Console / "Issue Grouping" (based on the names/types of exceptions) has completely broken.

All crashes in console now show up under JavaProxyThrowable or RunnableImplementor, instead of the actual managed exception name, so all our crashes are grouped in a few very large blobs and it's impossible to navigate.

Previously when crashes showed up in Firebase console, it'd be the proper C# exception name. The old library we were using allowed us to call RecordException and pass in our System.Exception, so it was formatted nicely (Crashes in console were grouped by exception type and message). The new Xamarin.Firebase.Crashlytics expects a Java.Lang.Throwable. I read around that we can just use Java.Lang.Throwable.FromException to convert between the two, so we tried that.

The result is that ALL our crashes we had previously, from many different C# classes, now get clumped into: Activity1.java RunnableImplementor.java android.runtime.JavaProxyThrowable

If you go through each individual crash we can eventually find all the different issues, and it eventually includes the stack trace going into the C# classes, but trying to group/understand what's happening is completely impossible. Basically it looks like with the new Crashlytics implementation, the root level of each crash report is ignoring any C# call stack and reporting only the root java entry point.

This text above is copied from Xamarin Forums: https://forums.xamarin.com/discussion/186515/firebase-crashlytics-migration-issue-names-now-useless https://forums.xamarin.com/discussion/comment/426012/#Comment_426012 -> see: Problem 1: (thread: https://forums.xamarin.com/discussion/184157/is-there-going-to-be-updated-firebase-crashlytics-support-for-xamarin-android-and-xamarin-ios)

Expected: able to pass System.Exception to FirebaseCrashlytics.Instance.RecordException, and it should log exceptions properly.

moljac commented 3 years ago

Basically it looks like with the new Crashlytics implementation, the root level of each crash report is ignoring any C# call stack and reporting only the root java entry point.

Of course Chrashlytics does not know anything about C#/.NET/Xamarin world.

Old versions could have had additional managed API for .NET Exceptions which was not added to newer versions. I will take a look, but cannot promise anything.

Cardanis commented 3 years ago

I've also been having this issue since converting. I have a few improvements over the default behavior but nothing perfect, as well as several issues I've either not solved, or have no idea how to even start solving. Just in case it helps investigation or others trying to work through it I've included what I've already looked into.

1) Since the new library requires a java exception, properly converting a C# exception to java one. I found the Throwable.FromException method is garbage and causes firebase to lump the whole managed stacktrace into a single line. To fix that I referenced a method used by the library we were using before, which does it properly, the method can be found here: https://github.com/drungrin/Fabric.Sdk.Xamarin/blob/f7e6207e5731cfc10ac0c6e96df3d68b19caf2a0/Sources/CrashlyticsKit.Droid/Crashlytics.cs#L108. This has made some improvements just with copy pasting it, though I'll need to take another stab at it to get the hash values back for symbolicate line numbers to work again. Even with that improvement, all root issues are still coming through as java.lang.Throwable.

2) The firebase library logs out the root name of the issue as the Throwable exception name, which it pulls directly from reflection. My first thought was to somehow override part of firebase logging to not directly pull from the class type, but I don't even think that's possible (even though their client code is open source on github, so maybe forking it is possible?). The followup would then be manually making java.Lang.Throwable classes to mirror any C# exception you'd expect to get, and in your "convert from C# to java exception" method, matching it properly. However that sounds like an absolute mess, so I started looking into dynamically generating the types at runtime as needed, and completely gave up at that point.

3) Even if we get 1 and 2 above working, that only works for non-fatals. Any fatal crash handling will happen automatically without using our conversion method. Looking again at the other repo listed above, as well as other threads on overriding the crashlytics behavior in native java apps, there seems to be ways to register the unmanaged crash handler after the crashlytics one gets registered, so we could hopefully intercept the crash first and do a proper conversion, but I think at that point it's already been converted to a java exception. I'm a little lost on how to handle this since I'm not sure at what level xamarin is converting our unhandled C# exceptions into java throwables, and how we would tie in there.

Cardanis commented 3 years ago

I recently got some time to take another stab on this, and have an update with some new information. In my previous post I showed a method that can convert C# exceptions to Throwables to send as NonFatals, which works a lot better than the default Throwable.FromException (though my method needs a lot of work still). I now have found an approach to intercept any fatal crashes before they hit Crashlytics, allowing us to parse the C# Exception blob out of the default Java Throwable, and apply similar processing to it so that fatals show up correctly as well.

From reading other native Crashlytics issues, it seems Crashlytics sets up a ContentProvider where it initializes crash handling, so that the crash handler is setup before the first Activity gets created. So if we do this in Activity in OnCreate: Java.Lang.Thread.IUncaughtExceptionHandler oldExceptionHandler = Java.Lang.Thread.DefaultUncaughtExceptionHandler; We'll see it's already been set to the Crashlytics uncaught exception handler. We can then register our own implementation of IUncaughExceptionHandler. This will get called with whatever Throwable is causing a fatal. This allows us to take the Throwable which, as formatted will be absolute useless trash in the Crashlytics console, and create a new properly formatted Throwable. We then pass that to crashlytics by calling oldExceptionHandler.UncaughtException (having saved off oldExceptionHandler before assigning our own in OnCreate). I have a proof of concept right now where I am intercepting a crash and sending my own nonsense information to Crashlytics, so the concept already works. (Something to note, breakpoints inside the custom exception handler seem to not get hit, as the app is already crashing at that point and I think the debugger is already detached).

Currently I'm working on a helper function that uses Regex to turn a Java Throwable that's been created by a C# exception into one that.s properly formatted. It's not working yet, but I'm going to include here some of the reverse engineering I've done to try and figure out how the data is structured currently.

First, an example of how a C# fatal will show up in Crashlytics. One of ours looks like this (with a lot of extra lines and namespaces removed for saving space): Fatal Exception: android.runtime.JavaProxyThrowable: System.NullReferenceException: Object reference not set to an instance of an object at StrayEngine.SubState.OnExit (Vector2 direction, IInputSystem input) [0x0002b] in <34933ab90bc840c58865e4e0b8fef054>:0 at StrayEngine.StateMachine.GoToState (State desiredState, IInputSystem input, Vector2 direction) [0x0000d] in <34933ab90bc840c58865e4e0b8fef054>:0 at StrayEngine.UI.UIControl.Update (GameTime time<truncated: 2662 chars> at mono.java.lang.RunnableImplementor.n_run(RunnableImplementor.java) at mono.java.lang.RunnableImplementor.run(RunnableImplementor.java:30) at android.os.Handler.handleCallback(Handler.java:883)

What it SEEMS is happening is: "Fatal Exception: " at the start is something Crashlytics puts at the start. "android.runtime.JavaProxyThrowable: " is it logging the class type of the fatal exception, and then everything from "System.NullReferenceException" to the final "Managed" line in the stackframe ("StrayEngine.UI.UIControl.Update" line) is all part of the Message of the throwable. Double checking this I found that Throwable.FromException indeed just takes the C# Exception, calls ToString on it, and passes that in as the "Message: for a java Throwable exception. The stackframe following that is the native java stackframe that originally called in to the managed code.

So the first step is to pull the "Message" out of the Throwable, parse it out, and manually set it as the stacktrace info in the java exception. I'm already manually setting the stacktrace in my "non-fatal" helper using this method: List<StackTraceElement> stackTrace = new List<StackTraceElement>(); throwable.SetStackTrace(stackTrace.ToArray()); which works. StackTraceElement takes 4 parameters, className, methodName, fileName, lineNumber, which we should be able to parse out from the C# stacktrace in the throwable message. This is what I'm working on now, I've got a working regex copied from the mono-symbolicate code on github, I'm now just trying to put it to use.

One other interesting discovery I made was how Crashlytics populates the various parts of the console (I might be wrong on some of this though)

The way it appears in the Crashes dashboard overview: CrashlyticsBreakdownTitle

And the way it populates the individual overview page CrashlyticsBreakdownBody

TLDR: We can modify the java Throwable before it gets to Crashlytics. To get it properly formatted, the steps will be: 1) Get the C# exception stacktrace from throwable.Message 2) Parse out the C# message, make new throwable using just that line as the message 3) Parse out each line of the stacktrace, create new StackTraceElement objects from each line and set them with throwable.setStackTrace 4) Pass the new throwable to the old default crash handler (the one crashlytics set up)

rjhind commented 3 years ago

I've been working on something similar as I've just started to add Crashlytics to my Xamarin projects. My regex is:

        var matcher = new Regex(@" +at (?<method>.*?) \((?<args>(.*?))\) \[0x.+\] in (?<file>.*?):(?<line>\d+)");

Following your hint on the uncaught exception handler, I've done that and can now submit my own stacks. Any idea if it's possible to find line number info anywhere?

I previously used AppCenter and it sent recorded proper managed stack traces but still no line number info so I think this is an issue in Mono that needs addressing.

Cardanis commented 3 years ago

So the line number info you'll need to pull out using mono-symbolicate, which is it's own nightmare I've found. Seems like different "versions" of it expect different formatting, I just remember having a nightmare trying to get it to work.

Either way I've finally got a solution working for us that I'm happy with, there's still a few holes/gaps but for the most part it's doing what we want. I've tidied it up and documented it, and posted it here: https://gist.github.com/Cardanis/c4316f931ad0de4c9d6c01eebf61884b

It's an absolute mess that uses 3 different Regex patterns since I'm terrible with Regex and cobbled together multiple different solution to get what I wanted. The posted version has a few tweaks that are technically not tested yet (stack trace lines were coming through as namespace.class.namespace.class.method which should hopefully be fixed) but with what's there, this is the results we're seeing in crashlytics: CrashlyticsSolutionOverview CrashlyticsSolutionStackTrace

To get proper formatting for mono-symbolicate you SHOULD be able to just remove the last chunk of parenthesis from each stack trace line (filename.cs). Hope this helps some people out!

Toine-db commented 3 years ago

@Cardanis Looks great, formatting this JavaProxyThrowable of yours.

Would you like to tell how you implemented this in your Android code? Meaning, what code did you use to make Analytics use this?

I think you are catching crash exceptions somewhere and redirect them for this parsing and then send throw the exception any further. But that's just a guess ;-)

gil-rodrigues commented 1 year ago

Hi @zoli13 and @Cardanis

Sorry to bump an old thread.

Were you ever able to overcome this? Any workaround?

A general comment. This is still marked "needs-info", but it is a real problem for anyone trying to implement Crashlytics. I don't know what else info is missing.

DavidMarquezF commented 1 week ago

I'm also struggling with this. It makes most error logs very hard or impossible to debug since a lot of information is inside the inner exception / aggregate exceptions, which are not logged for me in crashlytics.