ryzom / ryzomcore

Ryzom Core is the open-source project related to the Ryzom game. This community repository is synchronized with the Ryzom Forge repository, based on the Core branch.
https://wiki.ryzom.dev
GNU Affero General Public License v3.0
330 stars 89 forks source link

Exiting or Crashing (including Crash Reporting) is Windows-only #153

Closed ryzom-pipeline closed 9 years ago

ryzom-pipeline commented 10 years ago

Original report by Matt Raykowski (Bitbucket: [Matt Raykowski](https://bitbucket.org/Matt Raykowski), ).


There's a lot of Windows-only failed exit and crash handling code in client.cpp and init.cpp (code/ryzom/client/src). Bug Report or whatever is MFC. Maybe we consider having a cross-platform (Qt) crash reporter and gut out the rest of this Windows-specific code?

ryzom-pipeline commented 10 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


Do we really need to pull that giant Qt thing into game dependencies just for a window that sends crash reports to /dev/null? :)

ryzom-pipeline commented 10 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


All we really need is a bog standard message box with an Ignore, Always Ignore and Exit button, a simple dump to a txt file, and run a separate crash report exe tool on exit.

ryzom-pipeline commented 10 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


And send the crash reports to some database somewhere, rather than using a non-existant smtp server that's blocked anyways, because most isp's around here block direct connections to outside smtp server to block spam networks.

Oh, and I want a web app with an ingame map that shows the locations where people crash. Something fancy, CSI style.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


Removing version: 1.0 (automated comment)

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


We could make an external application in Qt for reporting crashes. That way it's not a dependency in the Nel / Ryzom code, and at the same time it's a Qt app like Matt would like it. I'm volunteering for such a feat!

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


Yup, an external app. Just make the game dump to some text file in temp directory, and launch an error reporter with that as parameter. Like when you crash 3ds Max.

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


...or World of Warcraft, that's where I got the idea from...

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


I think it should post it to some database like you suggested. With HTTP post method for example. I could also whip up a sample PHP application for it.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


And collect user e-mail addresses and send them to me :D

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


And yeah, just a really simple HTTP post to some dumb php script.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


Can I get an optional checkbox in the crash report app to subscribe to a newsletter? xD

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


Sure! Along with the creditcard and verification field.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


Heheh, just something along the lines of "If you want to receive updates about this crash report, enter your mail adress here".

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


Sounds fair. Also the AMS has bug tracking feature already, correct? Because then the same bug database could be used.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


Well, AMS is really just a customer support system that thinks it owns the world. It can probably be used for that too, but I'd really like something that's very much aware of crash report ingame locations, etcetera.

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


Yes I know, I've read your previous comments. Alright then it will use a separate database.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


I don't mind if it integrates with AMS, but... I just want to have the report data somewhere flexible so it can be batch analyzed.

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


So apparently there are 3 of these platform specific error / bug report things:

1.) NLMISC has a bug report window that is opened when an assertion is triggered.

2.) Ryzom client has some code that displays error, like wrong desktop color depth, but that doesn't actually send any reports, just informs the user.

3.) There's the MFC tool called bug_report that is launched by the launch_bug_report action handler in the GUI code of the Ryzom client. However there are no references to this in the open client's xml files, so it's unused.

I am not really sure that 1 should even be there, because NLMISC is a library, there could be many uses, many games that use it. 2 should be left intact for now, longer term plan could be to get rid of the platform specific code and move it all to Qt, so that all windowing would be done by Qt. 3 is a big question, as it's not used.

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


After further examination, case 1 ofc makes sense, since it supports user callbacks so the application that triggers an asser can submit it's own data.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


Something like 1 even exists in SDL, but normally it's only for developer builds, and it's only a messagebox, not a fancy text box thing. The report button should take you straight into the bug report tool, though.

Maybe move 2 into NLMISC and just provide a simple MessageBox facility? Platform specific code is fine for this.

Tool 3 was used during beta when you pushed the bug report button ingame. It made a screenshot and launched the tool, and automatically attached the screenshot.

Also investigate how to catch regular application crashes (platform specific) if this isn't done yet (it hooks into windows error report thing, I think), and trigger the bug report tool launch on that as well.

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


Catching crashes is quite tricky, just take a look at breakpad's source code... Maybe we could re-use breakpad's code to generate the crash reports and then use our own custom application to send them along with user messages.

I don't think NLMISC needs messageboxes at all. What's the use case for it? I mean simple users won't understand the message either, and we know where to look for it in the log file :D So I think it should just be removed.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


For simple things like "Your graphics card is not supported." etc. It's nicer than crashing.

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


That sounds like something the application should do, not the library.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


And platform specific code belongs in the library.

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


Implemented and merged in the error reporter that replaces case 1.

20150221_000002149.jpg

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


Please do not put features in the hotfix branch.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


I am going to revert this out of the hotfix branch.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


You're also not allowed to put this into develop yet. There are several serious issues that need to be fixed first.

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


I see. Could you by any chance elaborate on that? What exactly are those several serious issues?

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


1) Please move the tool into tools/misc/.

2) Don't Ryzom Core it, it's part of NeL. Rename to "crash_report". Default title NeL Error Report. Command arguments that launches the crash report should be able to set parameters such as title and crash reporting target.

3) Don't use a fixed filename for the crash report log. Use a temporary name provided by the OS. Pass the filepath to the crash report to the tool using the command arguments.

4) No hardcoding the report target. These settings should be set by the application at launch, and passed over command arguments to the report tool. There may be defaults in NeL.

5) Expected behaviour for nlassert / nlerror with FINAL_VERSION turned off (so the developers behaviour), is to first show a messagebox which asks whether to ignore, always ignore, break, or abort. Compare with SDL behaviour. This is necessary, as asserts are used to break into the debugger. We cannot have the application exit on a crash during debugging.

6) Class names in NeL start with a C.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


Also if you want to test things with compatibility, please just create your own dfighter/blahsomething branch, and merge compatibility + develop + whatever branch you're working on into that. Simply branch your features into a separate feature branch, which you can base on develop/hotfix/default depending on what your requirements are. I've moved your commits into feature-crashreport based off hotfix.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


5) That's why I said NLMISC needs a messagebox facility that uses OS calls to show a simple messagebox. Again, see SDL2. Under develop mode we cannot have the application exit immediately on a nlerror or nlassert.

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


1) I didn't put it there for a reason: I think it really belongs to Nel, it's not just a separate tool that's based on it.

2) Unfortunately the command arguments are broken in Qt 4.8.5. The first argument that is actually passed to the application is either the 0th ( executable name ) or the 1st, depending on whether you have a 1st argument. Arg count also reflects this. That's why I hardcoded the report name. However the other parameters you want to pass could be passed using the generated file as well. See this commit: https://bitbucket.org/ryzom/ryzomcore/commits/ee495ddae9e67924bcfd3734bab8e118bfa9f7f1

3) See 2.

4) See 2.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


1) nel/tools/misc that is

no exceptions

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


2) qt5 pls

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


argv[0] is always the application name

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


Yes, normally it is. However you forget that in a Qt application it's not the OS that passes the arguments to the main function, but some Qt code...

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


And make sure to cleanup the args before passing them to QApplication (for example do (argc + 3) and &argv[3] and fix that first argv to the exe)

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


Qt shouldn't mess with argv and argc :|

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


Yes, it shouldn't. Yet this is what happens:

First image, starting the application with 0 arguments:

20150222_000002153.jpg

Second image, starting with 1 parameter:

20150222_000002152.jpg

In both cases, argc contains 1, and argv contains either the 0th or the 1st parameter, depending on whether you provided a parameter or not. Just like I said....

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


Maybe just do a named parameter thingy then like

-title blah -log bsfjlkdj.log -host lkfjjslfjsd/lfjksdf.php

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


I was wrong about 1 thing:

It's not Qt causing this, but Nel somehow. When I pass the arguments from the command line, or from VS they show up.

When I pass the arguments from NLMISC:launchProgram() the issue appears.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


Eh. Strange. Someone should fix that.

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


Doesn't matter, I've already implemented a parser for the command line parameters you suggested.

ryzom-pipeline commented 9 years ago

Original comment by Cédric Ochs (Bitbucket: [Cédric OCHS](https://bitbucket.org/Cédric OCHS), ).


If you're interested I'm using QCommandLineParser in a tool I wrote some weeks ago :

http://hg.kervala.net/redmineuploader/file/17587b23d7e1/src/redmine.cpp

It's useful to not reinvent the wheel :)

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


Already have CCmdArgs in NLMISC for this purpose!

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


See further direction on this feature here https://bitbucket.org/ryzom/ryzomcore/pull-request/123/crash-reporter/diff

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


Cédric OCHS: It was faster to implement my own than to look for an existing one, but yes generally I agree.

Jan Boon: The tool doesn't use Nel at all, so I'd definitely go with the Qt version.

ryzom-pipeline commented 9 years ago

Original comment by Jan Boon (Bitbucket: [Jan Boon](https://bitbucket.org/Jan Boon), ).


dfighter: Can you do the additions to the tool that I listed? I'll do the nlmisc stuff.

ryzom-pipeline commented 9 years ago

Original comment by dfighter1985 (Bitbucket: dfighter1985, GitHub: dfighter1985).


Sure, I just thought it could wait for later today.