Fix crash when quitting Nagstamon on macOS

Nagstamon/config.py

@@ -609,9 +609,6 @@ def save_config(self):
             config = configparser.ConfigParser(allow_no_value=True, interpolation=None)
             # general section for Nagstamon
             config.add_section('Nagstamon')
-            for option in self.__dict__:
-                if option not in ['servers', 'actions', 'configfile', 'configdir', 'cli_args']:
-                    config.set('Nagstamon', option, str(self.__dict__[option]))
 
             # because the switch from Nagstamon 1.0 to 1.0.1 brings the use_system_keyring property
             # and all the thousands 1.0 installations do not know it yet it will be more comfortable
@@ -634,6 +631,12 @@ def save_config(self):
             # save actions dict
             self.save_multiple_config('actions', 'action')
 
+            # fill the general section only now because saving the servers might have
+            # switched off use_system_keyring after the keyring refused to store a password
+            for option in self.__dict__:
+                if option not in ['servers', 'actions', 'configfile', 'configdir', 'cli_args']:
+                    config.set('Nagstamon', option, str(self.__dict__[option]))
+
             # open, save and close config file
             with open(os.path.normpath(self.configfile), 'w') as file:
                 config.write(file)
@@ -695,35 +698,24 @@ def save_multiple_config(self, settingsdir, setting):
                                 value = ''
                             elif self.keyring_available and self.use_system_keyring:
                                 if self.__dict__[settingsdir][s].password != '':
-                                    # provoke crash if password saving does not work - this is the case
-                                    # on newer Ubuntu releases
-                                    try:
-                                        keyring.set_password('Nagstamon',
-                                                             '@'.join((self.__dict__[settingsdir][s].username,
-                                                                       self.__dict__[settingsdir][s].monitor_url)),
-                                                             self.__dict__[settingsdir][s].password)
-                                    except Exception:
-                                        import traceback
-                                        traceback.print_exc(file=sys.stdout)
-                                        sys.exit(1)
-                                value = ''
+                                    if self.store_password_in_keyring(
+                                            '@'.join((self.__dict__[settingsdir][s].username,
+                                                      self.__dict__[settingsdir][s].monitor_url)),
+                                            self.__dict__[settingsdir][s].password):
+                                        value = ''
+                                else:
+                                    value = ''
                         if option == 'proxy_password':
                             if self.keyring_available and self.use_system_keyring:
                                 if self.__dict__[settingsdir][s].proxy_password != '':
-                                    # provoke crash if password saving does not work - this is the case
-                                    # on newer Ubuntu releases
-                                    try:
-                                        keyring.set_password('Nagstamon',
-                                                             '@'.join(('proxy',
-                                                                       self.__dict__[settingsdir][s].proxy_username,
-                                                                       self.__dict__[settingsdir][s].proxy_address)),
-                                                             self.__dict__[settingsdir][s].proxy_password)
-                                    except Exception:
-                                        import traceback
-                                        traceback.print_exc(file=sys.stdout)
-                                        sys.exit(1)
-
-                                value = ''
+                                    if self.store_password_in_keyring(
+                                            '@'.join(('proxy',
+                                                      self.__dict__[settingsdir][s].proxy_username,
+                                                      self.__dict__[settingsdir][s].proxy_address)),
+                                            self.__dict__[settingsdir][s].proxy_password):
+                                        value = ''
+                                else:
+                                    value = ''
                         config.set(setting + '_' + s, option, str(value))
                     else:
                         config.set(setting + '_' + s, option, str(self.__dict__[settingsdir][s].__dict__[option]))
@@ -750,6 +742,50 @@ def save_multiple_config(self, settingsdir, setting):
             # ##        if not f.split(setting + "_")[1].split(".conf")[0] in self.__dict__[settingsdir]:
             # ##            os.unlink(self.configdir + os.sep + settingsdir + os.sep + f)
 
+    def store_password_in_keyring(self, account, password):
+        """
+        Stores a password for the given account in the system keyring.
+
+        Returns:
+            bool: True if the password could be stored, False otherwise.
+
+        Special Considerations:
+            - On macOS the ACL of an existing keychain entry may reject a freshly built
+              binary with error -25299, so the entry gets deleted and written again before
+              giving up - see https://github.com/HenriWahl/Nagstamon/issues/1159
+            - If the keyring stays unusable on macOS the keyring gets switched off and the
+              caller falls back to storing the obfuscated password in the config file.
+              Killing the application in the middle of saving the configuration, as it was
+              done before, leaves the user with an unusable installation.
+            - On Linux and Windows the previous behaviour is kept: a keyring which accepts
+              no password is a broken system which should be noticed loudly.
+        """
+        try:
+            keyring.set_password('Nagstamon', account, password)
+            return True
+        except Exception:
+            # an existing entry might carry an ACL which does not accept the current binary
+            # deleting and recreating it repairs exactly that case
+            try:
+                keyring.delete_password('Nagstamon', account)
+                keyring.set_password('Nagstamon', account, password)
+                return True
+            except Exception:
+                import traceback
+                traceback.print_exc(file=sys.stdout)
+
+        if OS != OS_MACOS:
+            # provoke crash if password saving does not work - this is the case
+            # on newer Ubuntu releases
+            sys.exit(1)
+
+        # do not bother the keyring again for the remaining passwords of this run
+        self.keyring_available = False
+        self.use_system_keyring = False
+        print('Storing passwords in the system keyring failed - '
+              'falling back to storing them in the configuration file.')
+        return False
+
     def is_keyring_available(self):
         """
         Determines if the keyring module and a suitable backend are available for secure password storage.

Nagstamon/qui/__init__.py

@@ -68,14 +68,7 @@
 
 from Nagstamon.config import (conf,
                               OS_NON_LINUX,
-                              OS,
-                              OS_MACOS)
-
-# make icon status in macOS dock accessible via NSApp, used by set_macos_dock_icon_visible()
-if OS == OS_MACOS:
-    from AppKit import (NSApp,
-                        NSApplicationPresentationDefault,
-                        NSApplicationPresentationHideDock)
+                              OS)
 
 
 # check for updates
@@ -157,6 +150,12 @@
 # connect application exit with server missing dialog
 dialogs.server_missing.window.button_exit.clicked.connect(statuswindow.exit)
 
+# stop all worker threads no matter how the application is quit - on macOS Cmd-Q, the
+# application menu and the dock quit the application directly via QApplication, bypassing
+# statuswindow.exit() and leaving running QThreads to be destroyed at interpreter shutdown,
+# which makes Qt call qFatal() - see https://github.com/HenriWahl/Nagstamon/issues/1055
+app.aboutToQuit.connect(statuswindow.shutdown_workers)
+
 # connect weblogin browser
 #dialogs.weblogin.page_loaded.connect(statuswindow.refresh)
 

Nagstamon/qui/dialogs/__init__.py

@@ -184,7 +184,5 @@ def hide_macos_dock_icon_if_necessary(self):
 dialogs.action.check_macos_dock_icon_fix_hide.connect(dialogs.hide_macos_dock_icon_if_necessary)
 dialogs.authentication.check_macos_dock_icon_fix_show.connect(dialogs.show_macos_dock_icon_if_necessary)
 dialogs.authentication.check_macos_dock_icon_fix_hide.connect(dialogs.hide_macos_dock_icon_if_necessary)
-dialogs.authentication.check_macos_dock_icon_fix_show.connect(dialogs.show_macos_dock_icon_if_necessary)
-dialogs.authentication.check_macos_dock_icon_fix_hide.connect(dialogs.hide_macos_dock_icon_if_necessary)
 dialogs.server.check_macos_dock_icon_fix_show.connect(dialogs.show_macos_dock_icon_if_necessary)
 dialogs.server.check_macos_dock_icon_fix_hide.connect(dialogs.hide_macos_dock_icon_if_necessary)

Nagstamon/qui/helpers.py

@@ -25,11 +25,11 @@
 from Nagstamon.qui.widgets.app import app
 from Nagstamon.servers import servers
 
-# make icon status in macOS dock accessible via NSApp, used by set_macos_dock_icon_visible()
+# make icon status in macOS dock accessible via NSApp, used by hide_macos_dock_icon()
 if OS == OS_MACOS:
     from AppKit import (NSApp,
-                        NSApplicationPresentationDefault,
-                        NSApplicationPresentationHideDock)
+                        NSApplicationActivationPolicyAccessory,
+                        NSApplicationActivationPolicyRegular)
 
 
 class CheckServers(QObject):
@@ -58,11 +58,14 @@ def hide_macos_dock_icon(hide=False):
     """
     small helper to make dock icon visible or not in macOS
     inspired by https://stackoverflow.com/questions/6796028/start-a-gui-process-in-mac-os-x-without-dock-icon
+    Accessory hides the dock icon but still allows the application to be activated and to
+    receive keyboard input - Prohibited, which the previously used and wrongly named
+    NSApplicationPresentationHideDock happens to be equal to, does not
     """
     if hide:
-        NSApp.setActivationPolicy_(NSApplicationPresentationHideDock)
+        NSApp.setActivationPolicy_(NSApplicationActivationPolicyAccessory)
     else:
-        NSApp.setActivationPolicy_(NSApplicationPresentationDefault)
+        NSApp.setActivationPolicy_(NSApplicationActivationPolicyRegular)
 
 
 def get_screen_name(x, y):

Nagstamon/qui/widgets/statuswindow.py

@@ -40,10 +40,8 @@
 from Nagstamon.qui.helpers import (get_screen_geometry,
                                    get_screen_name,
                                    hide_macos_dock_icon)
-from Nagstamon.qui.qt import (QAction,
-                              QCursor,
+from Nagstamon.qui.qt import (QCursor,
                               QIcon,
-                              QMenuBar,
                               QMessageBox,
                               QObject,
                               QVBoxLayout,
@@ -63,6 +61,8 @@
 from Nagstamon.qui.widgets.server_vbox import ServerVBox
 from Nagstamon.qui.widgets.statusbar import StatusBar
 from Nagstamon.qui.widgets.toparea import TopArea
+from Nagstamon.qui.widgets.treeview import (stop_worker_thread,
+                                            treeviews)
 from Nagstamon.servers import (get_enabled_servers,
                                get_status_count,
                                servers)
@@ -127,6 +127,10 @@ def __init__(self, dialogs=None, systrayicon=None):
         """
         QWidget.__init__(self)
 
+        # flag to make shutdown_workers() idempotent - it may be called by exit() and
+        # by the QApplication.aboutToQuit signal
+        self.workers_shut_down = False
+
         # immediately hide to avoid flicker on Windows and OSX
         self.hide()
 
@@ -180,14 +184,6 @@ def __init__(self, dialogs=None, systrayicon=None):
         self.label_all_ok.hide()
         self.servers_vbox.addWidget(self.label_all_ok)
 
-        # test with OSX top menubar
-        if OS == OS_MACOS:
-            self.menubar = QMenuBar()
-            action_exit = QAction('exit', self.menubar)
-            action_settings = QAction('settings', self.menubar)
-            self.menubar.addAction(action_settings)
-            self.menubar.addAction(action_exit)
-
         # stored x y values for systemtray icon
         statuswindow_properties.icon_x = 0
         statuswindow_properties.icon_y = 0
@@ -1356,20 +1352,14 @@ def finish_worker_thread(self):
         """
         # stop debugging
         statuswindow_properties.worker_debug_loop_looping = False
-        # tell thread to quit
-        self.worker_thread.quit()
-        # wait until thread is really stopped
-        self.worker_thread.wait()
+        stop_worker_thread(self.worker, self.worker_thread)
 
     @Slot()
     def finish_worker_notification_thread(self):
         """
         attempt to shut down thread cleanly
         """
-        # tell thread to quit
-        self.worker_notification_thread.quit()
-        # wait until thread is really stopped
-        self.worker_notification_thread.wait()
+        stop_worker_thread(self.worker_notification, self.worker_notification_thread)
 
     @Slot(str)
     def remove_previous_server_vbox(self, previous_server_name):
@@ -1394,6 +1384,41 @@ def decrease_shown_timestamp(self):
         """
         statuswindow_properties.is_shown_timestamp -= 1
 
+    @Slot()
+    def shutdown_workers(self):
+        """
+        stop all worker threads
+        has to be idempotent because it is called both by exit() and by the
+        QApplication.aboutToQuit signal - the latter is the only way to catch quitting
+        via the macOS application menu, the dock or Cmd-Q, which all bypass exit()
+        leaving running QThreads to be destroyed at interpreter shutdown, which makes
+        Qt call qFatal() - see https://github.com/HenriWahl/Nagstamon/issues/1055
+        """
+        if self.workers_shut_down:
+            return
+        self.workers_shut_down = True
+
+        # stop statuswindow workers - the running flag has to be cleared first, otherwise
+        # the workers just schedule themselves again
+        if hasattr(self, 'worker'):
+            self.worker.running = False
+            self.worker.finish.emit()
+        if hasattr(self, 'worker_notification'):
+            self.worker_notification.running = False
+            self.worker_notification.finish.emit()
+
+        # tell all treeview threads to stop - iterating over the registry instead of the
+        # layout because sort_server_vboxes() may have dropped ServerVBoxes which still
+        # own a running worker thread
+        for treeview in list(treeviews):
+            try:
+                treeview.worker.running = False
+                treeview.worker.finish.emit()
+            except RuntimeError:
+                # underlying C++ object might be gone already
+                if treeview in treeviews:
+                    treeviews.remove(treeview)
+
     @Slot()
     def exit(self):
         """
@@ -1408,13 +1433,7 @@ def exit(self):
         # hide statuswindow first to avoid lag when waiting for finished threads
         self.hide()
 
-        # stop statuswindow workers
-        self.worker.finish.emit()
-        self.worker_notification.finish.emit()
-
-        # tell all treeview threads to stop
-        for server_vbox in self.servers_vbox.children():
-            server_vbox.table.worker.finish.emit()
+        self.shutdown_workers()
 
         app.exit()
 
@@ -1504,12 +1523,18 @@ class WorkerNotification(QObject):
         def __init__(self, statuswindow_properties=None):
             QObject.__init__(self)
             self.statuswindow_properties = statuswindow_properties
+            # flag to decide if the thread has to run or to be stopped
+            self.running = True
 
         @Slot(str, str, str)
         def start(self, server_name, worst_status_diff, worst_status_current):
             """
             start notification
             """
+            # while shutting down no notification should be started anymore - a sound or a
+            # notification action would only delay the quit
+            if not self.running:
+                return
             if conf.notification:
                 # only if not notifying yet or the current state is worse than the prior AND
                 # only when the current state is configured to be honking about