From 0c59583b323c52c5dcf33fb48b3c3e238aee3f4b Mon Sep 17 00:00:00 2001
From: James Turner <zakalawe@mac.com>
Date: Wed, 7 Jun 2017 16:25:19 +0100
Subject: [PATCH] Fix intermittent crash on exit with Qt+XCB
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We can still crash on exit(-1) code paths since we can’t run this from
atexit, but at least the non-error paths are ok.
---
 src/GUI/QtLauncher.cxx | 23 ++++++++++++++++++-----
 src/GUI/QtLauncher.hxx |  5 +++++
 src/Main/bootstrap.cxx | 13 ++++++++++++-
 3 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/src/GUI/QtLauncher.cxx b/src/GUI/QtLauncher.cxx
index a06e7e020..ea7e44217 100644
--- a/src/GUI/QtLauncher.cxx
+++ b/src/GUI/QtLauncher.cxx
@@ -232,6 +232,14 @@ static void simgearMessageOutput(QtMsgType type, const QMessageLogContext &conte
 namespace flightgear
 {
 
+// making this a unique ptr ensures the QApplication will be deleted
+// event if we forget to call shutdownQtApp. Cleanly destroying this is
+// important so QPA resources, in particular the XCB thread, are exited
+// cleanly on quit. However, at present, the official policy is that static
+// destruction is too late to call this, hence why we have shutdownQtApp()
+
+std::unique_ptr<QApplication> static_qApp;
+
 // Only requires FGGlobals to be initialized if 'doInitQSettings' is true.
 // Safe to call several times.
 void initApp(int& argc, char** argv, bool doInitQSettings)
@@ -256,13 +264,13 @@ void initApp(int& argc, char** argv, bool doInitQSettings)
 #if QT_VERSION >= QT_VERSION_CHECK(5, 6, 0)
         QCoreApplication::setAttribute(Qt::AA_EnableHighDpiScaling);
 #endif
-        QApplication* app = new QApplication(s_argc, argv);
-        app->setOrganizationName("FlightGear");
-        app->setApplicationName("FlightGear");
-        app->setOrganizationDomain("flightgear.org");
+        static_qApp.reset(new QApplication(s_argc, argv));
+        static_qApp->setOrganizationName("FlightGear");
+        static_qApp->setApplicationName("FlightGear");
+        static_qApp->setOrganizationDomain("flightgear.org");
 
 #if QT_VERSION >= QT_VERSION_CHECK(5, 7, 0)
-        app->setDesktopFileName(
+        static_qApp->setDesktopFileName(
           QStringLiteral("org.flightgear.FlightGear.desktop"));
 #endif
 
@@ -277,6 +285,11 @@ void initApp(int& argc, char** argv, bool doInitQSettings)
     }
 }
 
+void shutdownQtApp()
+{
+    static_qApp.reset();
+}
+
 // Requires FGGlobals to be initialized. Safe to call several times.
 void initQSettings()
 {
diff --git a/src/GUI/QtLauncher.hxx b/src/GUI/QtLauncher.hxx
index 1b414043f..22742be3c 100644
--- a/src/GUI/QtLauncher.hxx
+++ b/src/GUI/QtLauncher.hxx
@@ -26,6 +26,11 @@ namespace flightgear
   // Only requires FGGlobals to be initialized if 'doInitQSettings' is true.
   // Safe to call several times.
   void initApp(int& argc, char** argv, bool doInitQSettings = true);
+
+  // ensures Qt-related resources are cleaned up. Avoids crashes on shutdown
+  // if QPA assets are hanging around. (With the XCB QPA plugin especially)
+  void shutdownQtApp();
+
   // Requires FGGlobals to be initialized. Safe to call several times.
   void initQSettings();
 
diff --git a/src/Main/bootstrap.cxx b/src/Main/bootstrap.cxx
index 61c4089df..e1648b2ce 100644
--- a/src/Main/bootstrap.cxx
+++ b/src/Main/bootstrap.cxx
@@ -69,6 +69,10 @@
 
 #include "fg_os.hxx"
 
+#if defined(HAVE_QT)
+#include <GUI/QtLauncher.hxx>
+#endif
+
 #if defined(HAVE_CRASHRPT)
 	#include <CrashRpt.h>
 
@@ -351,6 +355,10 @@ int main ( int argc, char **argv )
             perror("Possible cause");
     }
 
+#if defined(HAVE_QT)
+    flightgear::shutdownQtApp();
+#endif
+
 #if defined(HAVE_CRASHRPT)
 	crUninstall();
 #endif
@@ -364,10 +372,13 @@ void fgExitCleanup() {
 
     if (_bootstrap_OSInit != 0) {
         fgSetMouseCursor(MOUSE_CURSOR_POINTER);
-
         fgOSCloseWindow();
     }
 
+    // you might imagine we'd call shutdownQtApp here, but it's not safe to do
+    // so in an atexit handler, and crashes on Mac. Thiago states this explicitly:
+    // https://bugreports.qt.io/browse/QTBUG-48709
+
     // on the common exit path globals is already deleted, and NULL,
     // so this only happens on error paths.
     delete globals;