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

Catch otherwise uncaught exceptions, allow the user to report them #3628

Closed
wants to merge 3 commits into from
Closed

Catch otherwise uncaught exceptions, allow the user to report them #3628

wants to merge 3 commits into from

Conversation

nikclayton
Copy link
Contributor

Record the 5 most recent API requests and responses with RecordResponseInterceptor

Catch and report exceptions

  • Run the Android loop with a custom exception handler
  • Display otherwise uncaught exceptions in CrashReportActivity
  • Allow the user to share / copy the crash data, and restart the app
  • Include the most recent API requests/responses in the crash data

All of this only happens in debug mode, and the user has to take specific action to share the data, nothing is shared automatically.

Record the 5 most recent API requests and responses with RecordResponseInterceptor

Catch and report exceptions
- Run the Android loop with a custom exception handler
- Display otherwise uncaught exceptions in CrashReportActivity
- Allow the user to share / copy the crash data, and restart the app
- Include the most recent API requests/responses in the crash data

All of this only happens in debug mode, and the user has to take specific action to share the data, nothing is shared automatically.
@nikclayton
Copy link
Contributor Author

Draft because I haven't done any of the localisation work yet, and I'm not even sure if we want this, and/or want to clean up the UI.

Speaking of, here's what it looks like (I've redacted some stuff).

Clicking "Share" opens a normal Android share sheet, clicking "Copy" copies all the JSON to the clipboard, and "Restart" restarts Tusky from scratch (on the assumption that the app is now in a bad state, so continuing on would not be good).

@Lakoja
Copy link
Collaborator

Lakoja commented May 6, 2023

This would be the parallel issue / PR: #3516

@connyduck
Copy link
Collaborator

I don't think we need this for Google Play releases as we will see crash reports there anyway. For F-Droid releases it would be useful, BUT we would need to have different code on F-Droid and Google Play and that could make things more complicated than it is helpful.

@Lakoja
Copy link
Collaborator

Lakoja commented May 7, 2023

I also wouldn't prohibit the actual crash.
My idea in the earlier PR was and is that it should be easy to access some information after the fact - i. e. after a restart.
All of our current "something crashes" problems could probably be advanced / debugged further when we could tell the user where to look (after the crash).
The few with more complex problems could then be solved by "well, then dump the info in that screen and send it to us".

@Lakoja
Copy link
Collaborator

Lakoja commented May 7, 2023

Quick idea just now: Some code could be even in a normal release - provided that it is leightweight (small = easy to keep in mind or overview).

It could be normally disabled with a setting. Once activated it could then even be timed and switch off after some time.
(If we suspect our recording code might be harmful it would return to normal after say an hour.)

@nikclayton
Copy link
Contributor Author

For all of that it's probably easier to embed https://github.com/ACRA/acra, and have it display something like the CrashReport activity that's in here.

To be clear, I'm not suggesting that we should use ACRA to silently send crash reports, or run infrastructure to catch crash reports.

Re some of the points:

I don't think we need this for Google Play releases as we will see crash reports there anyway

That doesn't include anything about what happened in the run up to the crash, or the request/response bodies.

For example, for 22 beta 2, right now the crash that's occured the most has the following stack trace according to the Google Play Console.

Exception com.google.gson.o:
  at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$Adapter.read (ReflectiveTypeAdapterFactory.java:397)
  at com.google.gson.internal.bind.TypeAdapterRuntimeTypeWrapper.read (TypeAdapterRuntimeTypeWrapper.java)
  at com.google.gson.internal.bind.CollectionTypeAdapterFactory$Adapter.read (CollectionTypeAdapterFactory.java:82)
  at com.google.gson.internal.bind.CollectionTypeAdapterFactory$Adapter.read (CollectionTypeAdapterFactory.java:61)
  at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$1.readIntoField (ReflectiveTypeAdapterFactory.java:212)
  at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$FieldReflectionAdapter.readField (ReflectiveTypeAdapterFactory.java:433)
  at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$Adapter.read (ReflectiveTypeAdapterFactory.java:393)
  at com.google.gson.internal.bind.TypeAdapterRuntimeTypeWrapper.read (TypeAdapterRuntimeTypeWrapper.java)
  at com.google.gson.internal.bind.CollectionTypeAdapterFactory$Adapter.read (CollectionTypeAdapterFactory.java:82)
  at com.google.gson.internal.bind.CollectionTypeAdapterFactory$Adapter.read (CollectionTypeAdapterFactory.java:61)
  at retrofit2.converter.gson.GsonResponseBodyConverter.convert (GsonResponseBodyConverter.java:40)
  at retrofit2.converter.gson.GsonResponseBodyConverter.convert (GsonResponseBodyConverter.java:27)
  at retrofit2.OkHttpCall.parseResponse (OkHttpCall.java:243)
  at retrofit2.OkHttpCall$1.onResponse (OkHttpCall.java:153)
  at okhttp3.internal.connection.RealCall$AsyncCall.run (RealCall.kt:519)
  at java.util.concurrent.ThreadPoolExecutor.runWorker (ThreadPoolExecutor.java:1137)
  at java.util.concurrent.ThreadPoolExecutor$Worker.run (ThreadPoolExecutor.java:637)
  at java.lang.Thread.run (Thread.java:1012)
Caused by java.lang.IllegalStateException:
  at com.google.gson.stream.JsonReader.nextString (JsonReader.java:836)
  at com.google.gson.internal.bind.TypeAdapters$15.read (TypeAdapters.java:421)
  at com.google.gson.internal.bind.TypeAdapters$15.read (TypeAdapters.java:409)
  at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$1.readIntoField (ReflectiveTypeAdapterFactory.java:212)
  at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$FieldReflectionAdapter.readField (ReflectiveTypeAdapterFactory.java:433)
  at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$Adapter.read (ReflectiveTypeAdapterFactory.java:393)

That's not helpful -- JSON failed to parse somewhere, but without knowing what the request was and what the response JSON looked like there's no way to debug this.

As an aside, if the user has Google Play installed, but they installed Tusky from F-Droid, I think we might still get crash reports in the Google Play console. One of the available filters in the console is "Installed from: Google Play", so if you remove that filter I assume you see Tusky crashes irrespective of where it was installed from.

My idea in the earlier PR was and is that it should be easy to access some information after the fact - i. e. after a restart.

That's too late. Once you've crashed the app is, by definition, in an unknown state. That's why the code in this PR doesn't touch the database (so there's no risk of database corruption), doesn't need to write to the filesystem, and if it does only writes to the cache directory, which is, by definition, expendable.

Showing the user a "Hey, Tusky crashed, do you want to do something about?" message when they start Tusky is also the wrong thing to do.

At that point in time we know what the user wants to do -- they want to check their Mastodon feed. Putting a roadblock in front of them that prevents them from accomplishing their task is poor UX.

Handling the crash at the time when it happens also allows us to prompt the user to enter any additional information they think might be relevant about what they were doing at the time of the crash.

This PR doesn't do that -- I wrote it very quickly because I needed something to debug the JSON crash in v22 beta 2, but any variant of this idea might want to do that

Nik Clayton added 2 commits July 11, 2023 15:24
# Conflicts:
#	app/src/main/java/com/keylesspalace/tusky/TuskyApplication.kt
@nikclayton nikclayton closed this by deleting the head repository Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants