Improved out of process crash handler #5543

NathanW2 · 2017-11-06T01:02:15Z

Description

This PR moves the crash handler work out into a new process called qgiscrashhandler.exe (only Windows at the moment). As part of this it will also generate a crash report folder inside %APPDATA%\QGIS\QGIS3\crashes. In future this lets us look back over the data from inside QGIS with a crash reporting tool. Report folder includes the users report in markdown format as well as the full stack trace with more info. In future, we can include a memory dump here if we want.

Main reasons for doing this:

Loading a dialog inside the crashed app can lock/crash the dialog which defeats the purpose of a crash dialog ;)
You can't reliability dump stack information from inside the crashed process (just like people shouldn't do operations on themselves)
Once we are out of process we can do a lot more e.g write the report, open web browsers, etc.

All arguments that the crash handler needs are written to a temp file on crash and then read back into the crash handler process, this includes stuff like process id, thread id, the pointer to expectation stack.

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

Commit messages are descriptive and explain the rationale for changes
Commits which fix bugs include fixes #11111 in the commit message next to the description
Commits which add new features are tagged with [FEATURE] in the commit message
Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and containt sufficient information in the commit message to be documented
I have read the QGIS Coding Standards and this PR complies with them
This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
New unit tests have been added for core changes
I have run the scripts/prepare-commit.sh script before each commit

nyalldawson · 2017-11-06T01:29:43Z

src/app/qgscrashhandler.h

+     * This class doesn't need to be created by anyone as is only used to handle
+     * crashes in the application.
+     */
+    QgsCrashHandler() {}


nyalldawson · 2017-11-06T01:32:23Z

src/crashhandler/main.cpp

+  QString extraInfoFile = QString( argv[1] );
+  std::cout << "Extra Info File: " << extraInfoFile.toUtf8().data() << std::endl;
+
+  QFile file( extraInfoFile );


Bomb out if this doesn't exist?

nyalldawson · 2017-11-06T01:32:43Z

src/crashhandler/main.cpp

+    // bit gross but :)
+    QString info = file.readAll();
+    versionInfo = info.split("\n");
+  }


Graceful exit if we can't open the file?

I might still show the dialog but with no info.

although this shouldn't really happen anyway but dialog might be better then just crashing out.

With no info this is what you get:

I think that is ok to leave like that.

Looks perfect! Just needs a . between "information Your"

Also "Stack trace unable to be generated" should be "Stack trace could not be generated"

nyalldawson · 2017-11-06T01:33:38Z

src/crashhandler/main.cpp

+  std::cout << "Exception Pointer: " << exceptionPointersString.toLocal8Bit().constData() << std::endl;
+  std::cout << "Symbol Path :" << symbolPaths.toUtf8().data() << std::endl;
+
+  QgsStackTrace *stackTrace = QgsStackTrace::trace( processId, threadId, exception, symbolPaths );


std::unique_ptr

nyalldawson · 2017-11-06T01:36:39Z

src/crashhandler/qgscrashreport.cpp

-    reportData.append( QStringLiteral( "Compiled against GDAL: %1" ).arg( GDAL_RELEASE_NAME ) );
-    reportData.append( QStringLiteral( "Running against GDAL: %1" ).arg( GDALVersionInfo( "RELEASE_NAME" ) ) );
+    reportData.append( mVersionInfo );
+//    reportData.append( QStringLiteral( "QGIS Version: %1" ).arg( Qgis::QGIS_VERSION ) );


What's the go here?

nyalldawson · 2017-11-06T01:36:45Z

src/crashhandler/qgscrashreport.cpp

@@ -117,13 +129,13 @@ const QString QgsCrashReport::toHtml() const

 const QString QgsCrashReport::crashID() const
 {
-  if ( mStackTrace.isEmpty() )
+  if (!mStackTrace->symbolsLoaded || mStackTrace->lines.isEmpty() )


nyalldawson · 2017-11-06T01:38:02Z

src/crashhandler/qgscrashreport.cpp

+  fileName = folder + "/report.txt";
+
+  file.setFileName(fileName);
+  if ( file.open( QIODevice::WriteOnly | QIODevice::Text ) )


Pretty sure you'll need QIODevice::Truncate here too

It's a new file each time so should be at the start already.

good point!

nyalldawson · 2017-11-06T01:38:45Z

src/crashhandler/qgscrashreport.cpp

+
+QString QgsCrashReport::crashReportFolder()
+{
+  return QStandardPaths::standardLocations( QStandardPaths::AppDataLocation ).value( 0 ) +


AppLocalDataLocation ?

nyalldawson · 2017-11-06T01:39:33Z

src/crashhandler/qgsstacktrace.cpp

+  getStackTrace( stackTrace, symbolPath, trace );
+  trace->fullStack = QString::fromWCharArray( stackTrace->message );
+
+//  wchar_t dumpFilename[MAX_PATH + 1];


What's with all this? Better #if 0 it or kill it.

nyalldawson · 2017-11-06T01:41:49Z

You can't reliability dump stack information from inside the crashed process (just like people shouldn't do operations on themselves)

Speak for yourself! I've saved $1978 by following Youtube DIY surgical operation guides.

nyalldawson · 2017-11-06T01:42:27Z

Great move! This looks like a much better approach to me.

nyalldawson

Looks good! Let's merge and test

NathanW2 · 2017-11-06T05:19:12Z

@jef-n let me know if you see anything out of place here and I will fix it up. qgiscrashhandler doesn't need to go into osgeo4w\bin like qgis.exe as I'm loading it from the prefix path.

NathanW2 · 2017-11-06T05:34:19Z

Thanks for the review @nyalldawson

NathanW2 requested review from jef-n and nyalldawson November 6, 2017 01:02

NathanW2 self-assigned this Nov 6, 2017

NathanW2 added this to the QGIS 3 milestone Nov 6, 2017

NathanW2 added Bugfix labels Nov 6, 2017

nyalldawson reviewed Nov 6, 2017

View reviewed changes

Add out of process crash handler

e2cbc5c

NathanW2 force-pushed the crashhandler branch from bf24747 to e2cbc5c Compare November 6, 2017 05:06

nyalldawson approved these changes Nov 6, 2017

View reviewed changes

Astyle

e333ef1

NathanW2 merged commit ee59abf into qgis:master Nov 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved out of process crash handler #5543

Improved out of process crash handler #5543

NathanW2 commented Nov 6, 2017 •

edited

Loading

nyalldawson Nov 6, 2017

nyalldawson Nov 6, 2017

nyalldawson Nov 6, 2017

NathanW2 Nov 6, 2017

NathanW2 Nov 6, 2017

NathanW2 Nov 6, 2017

nyalldawson Nov 6, 2017

nyalldawson Nov 6, 2017

nyalldawson Nov 6, 2017

nyalldawson Nov 6, 2017

nyalldawson Nov 6, 2017

NathanW2 Nov 6, 2017

nyalldawson Nov 6, 2017

nyalldawson Nov 6, 2017

nyalldawson Nov 6, 2017

nyalldawson commented Nov 6, 2017

nyalldawson commented Nov 6, 2017

nyalldawson left a comment

NathanW2 commented Nov 6, 2017

NathanW2 commented Nov 6, 2017

Improved out of process crash handler #5543

Improved out of process crash handler #5543

Conversation

NathanW2 commented Nov 6, 2017 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyalldawson commented Nov 6, 2017

nyalldawson commented Nov 6, 2017

nyalldawson left a comment

Choose a reason for hiding this comment

NathanW2 commented Nov 6, 2017

NathanW2 commented Nov 6, 2017

NathanW2 commented Nov 6, 2017 •

edited

Loading