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

"Send crashes" option should reflect if that is even possible #1484

Closed
Penguin-Guru opened this issue Nov 28, 2021 · 17 comments
Closed

"Send crashes" option should reflect if that is even possible #1484

Penguin-Guru opened this issue Nov 28, 2021 · 17 comments
Labels
bug Something isn't working dev-client-specific Severity: Low No important functionality is affected stale Issue / PR has not had activity

Comments

@Penguin-Guru
Copy link

Following up on conversation in #1464.

I will run the AppImage, trigger a crash, and send the whole terminal output in a few minutes.

@Penguin-Guru
Copy link
Author

@JulianGro Here you go. I ran Vircadia-x86_64_v2021.1.3-build2-Eos.AppImage, enabled crash reporting, restarted the application, confirmed crash reporting was still on, cancelled that menu, then selected "New fault".

vircadia.log

@JulianGro
Copy link
Contributor

JulianGro commented Nov 29, 2021

That one actually made it to Sentry. It also looks normal. It's being authentication but just for reference: https://sentry.vircadia.dev/organizations/vircadia/issues/477/events/308aea87db5c404fb1ded57e1384a6c3/
Weird that the ones before didn't make it. Maybe it only works if run in terminal?

That seems to be the only one that has made it up from your client. I am identifying you by your kernel version since you seemingly weren't logged in. The Vircadia version also matches.

@JulianGro JulianGro added the linux This is related to Linux. label Nov 29, 2021
@JulianGro
Copy link
Contributor

Also are you sure you ran v2021.1.3-build2 and not v2021.1.3-rc1? If so then I messed the build up a little and gave it the wrong identification.

@Penguin-Guru
Copy link
Author

Penguin-Guru commented Nov 29, 2021

That one actually made it to Sentry. It also looks normal. It's being authentication but just for reference: https://sentry.vircadia.dev/organizations/vircadia/issues/477/events/308aea87db5c404fb1ded57e1384a6c3/ Weird that the ones before didn't make it. Maybe it only works if run in terminal?

That seems to be the only one that has made it up from your client. I am identifying you by your kernel version since you seemingly weren't logged in. The Vircadia version also matches.

I use i3, so everything I run is from a termianl. My previous attempts were with various builds I've compiled. I'm pretty sure the last one that didn't work should have been the very recent master branch without any modifications.

Fair warning, I do change my kernel occasionally. Three times today (don't ask). I'm going to try submitting another "new fault" from the build I just compiled. I happen to be running a generic kernel configuration right now so it's a good baseline. Let me know if you see it. vircadia.log

Also are you sure you ran v2021.1.3-build2 and not v2021.1.3-rc1? If so then I messed the build up a little and gave it the wrong identification.

Yup, that's what the file is named. I got it here: #1483

@JulianGro
Copy link
Contributor

JulianGro commented Nov 29, 2021

My previous attempts were with various builds I've compiled. I'm pretty sure the last one that didn't work should have been the very recent master branch without any modifications.

Fair warning, I do change my kernel occasionally. Three times today (don't ask). I'm going to try submitting another "new fault" from the build I just compiled.

On Linux, crash reporting only works on AppImages built by me (and potentially anyone else from the build team).
The logs will also indicate that you haven't enabled a crash handler.
From your log:

[11/28 23:39:24] [DEBUG] [default] Crash handler logger is enabled: true
[11/28 23:39:24] [WARNING] [vircadia.crash_handler] No crash handler available.
[11/28 23:39:24] [DEBUG] [default] Crash handler started: false

So yeah, this has been your problem all along.

@Penguin-Guru
Copy link
Author

This is definitely still an issue... The U.I. needs to disable that option if it's not supported, or at least include a warning with the option in the settings.

@Penguin-Guru
Copy link
Author

Why does it only work in the AppImage?

@Penguin-Guru Penguin-Guru changed the title Crash reporting not working. Linux crash reporting only works on AppImage Nov 29, 2021
@JulianGro
Copy link
Contributor

Yeah I guess the UI should show that crash reporting isn't configured.

It only works on builds generated by specific people because it needs semi private data to send crash reports.
For one we don't want people killing the server by sending fake crash dumps, etc..
We also don't want people to spam the system with crap and make it impossible to sort through the issues.
If you are trying to debug your own builds, you should use gdb instead. Maybe @daleglass has some other thoughts about it, but I feel like we would get a lot of invalid crash reports if people report from their personal builds.

@JulianGro JulianGro reopened this Nov 29, 2021
@JulianGro JulianGro changed the title Linux crash reporting only works on AppImage "Send crashes" option should reflect if that is even possible Nov 29, 2021
@JulianGro JulianGro removed the linux This is related to Linux. label Nov 29, 2021
@Penguin-Guru
Copy link
Author

I think the best solution would be to save crash reports locally on the client. Not everyone wants to send some unknown data to an unknown server. Even if they do, they would probably want access to view it. Disabling the checkbox in Qt would be an easy fix though, if there is a value to indicate functionality.

@daleglass
Copy link
Contributor

Yup, crash reports are saved on the client. The main issue is that crashpad isn't very controllable and doesn't seem to have any obvious place where we could plug in a dialog asking "Hey, send this?". It could possibly be hacked in, but it'd take some work.

@Penguin-Guru
Copy link
Author

Penguin-Guru commented Nov 30, 2021

I guess the questions then are:

  1. Should crashes be saved on the client even when the setting to send crash reports is disabled? I think yes if only a small number are saved and rotated.
  2. Are crash reports currently saved on the client even when the setting to send crash reports is disabled?
  3. How should we inform the user of this behaviour? I think disabling the checkbox when Crashpad is not functional is important but the user still needs to know that the reports are saved locally, and ideally where to find them. Maybe this could be added to the description test for that checkbox and mentioned somewhere in the docs (I don't think it currently is but maybe I missed it).
  4. Is there a convenient way to query the Crashpad functionality status from "PreferencesDialog.cpp" (or I guess "GeneralPreferencesDialog.qml")? If not, should a bool be added to "CrashHandler.h" and that file imported? It seems like there is a preprocessor conditional "HAS_CRASHPAD" and a command-line parameter to forceCrashReporting. I'm not sure what Breakpad is used for.

@Penguin-Guru
Copy link
Author

Penguin-Guru commented Nov 30, 2021

I suppose another option would be completely removing that setting from the main repository and applying it only to the build repository. Cleaner code but a bit more workflow complexity and U.I. inconsistency. I don't like that option personally.

@daleglass
Copy link
Contributor

  1. This could be done, but some rework would be needed. Currently nothing is saved if crash reporting is disabled.
  2. No.
  3. I think there should be a dialog that pops up on crash, and ideally allows you to fill in some extra info to add an user account of what they were doing. But crashpad doesn't make this easy. Crashpad runs an external process that doesn't seem to have any hooks for this.
  4. You can query the status of the menu item

Overall I have thoughts for how to improve this, but it'll take time as I've got a bunch of other plans that seem more urgent right now. You're welcome to give it a try of course.

@Penguin-Guru
Copy link
Author

Ok, I don't think I will try to add the dialogue on crash but it is a nice idea. I may add logic to disable the current setting and change the text a little-- we'll see. Right now I'm stalled on two other problems so I probably won't do anything with this until at least one of those is resolved.

The problem with querying the menu item is that it should reflect the state of the QtSetting, which is controlled by the settings menu/app, which is not aware of whether the setting will actually work. That's the issue at hand. Maybe I'm misunderstanding something. I will probably take a look at some point and see if I can figure out how Crashpad/Breakpad are used.

@daleglass
Copy link
Contributor

Ok. For posteriority, here's the overview of how it works:

Crashpad runs a standalone binary, a sort of mini debugger. It talks to the main application, and can receive metadata from it, which allows attaching stuff like usernames and the location to a crash report. Once the app crashes, crashpad pokes into the application's memory, builds a crash dump, writes it to disk, and uploads it.

It accepts commandline arguments, but the functionality available is fairly limited.

The Vircadia code does this:

database->GetSettings()->SetUploadsEnabled(true);

which makes crashpad send crash reports on its own. The long term plan may be to disable this, and either ask the user and then enable it (hopefully it uploads the backlog), or go all the way to making a custom application that deals with submission and possibly can parse crash dumps to allow choosing what to submit and allow attaching extra data.

@Penguin-Guru
Copy link
Author

It seems like Breakpad is a predecessor to Crashpad and Vircadia's code may only use it for Android?

@daleglass daleglass added bug Something isn't working Severity: Low No important functionality is affected labels Dec 4, 2021
@stale
Copy link

stale bot commented Jun 2, 2022

Hello! Is this still an issue?

@stale stale bot added the stale Issue / PR has not had activity label Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dev-client-specific Severity: Low No important functionality is affected stale Issue / PR has not had activity
Projects
None yet
Development

No branches or pull requests

4 participants