Clean up failed userscripts correctly

qutebrowser/commands/userscripts.py

@@ -82,6 +82,7 @@ class _BaseUserscriptRunner(QObject):
         _filepath: The path of the file/FIFO which is being read.
         _proc: The GUIProcess which is being executed.
         _win_id: The window ID this runner is associated with.
+        _cleaned_up: Whether temporary files were cleaned up.
 
     Signals:
         got_cmd: Emitted when a new command arrived and should be executed.
@@ -114,10 +115,14 @@ class _BaseUserscriptRunner(QObject):
                                            additional_env=self._env,
                                            verbose=verbose, parent=self)
         self._proc.finished.connect(self.on_proc_finished)
+        self._proc.error.connect(self.on_proc_error)
         self._proc.start(cmd, args)
 
     def _cleanup(self):
         """Clean up temporary files."""
+        if self._cleaned_up:
+            return
+        self._cleaned_up = True
         tempfiles = [self._filepath]
         if 'QUTE_HTML' in self._env:
             tempfiles.append(self._env['QUTE_HTML'])
@@ -150,6 +155,7 @@ class _BaseUserscriptRunner(QObject):
         """
         raise NotImplementedError
 
+    @pyqtSlot()
     def on_proc_finished(self):
         """Called when the process has finished.
 
@@ -157,6 +163,13 @@ class _BaseUserscriptRunner(QObject):
         """
         raise NotImplementedError
 
+    @pyqtSlot()
+    def on_proc_error(self):
+        """Called when the process encountered an error.
+
+        Needs to be overridden by subclasses.
+        """
+        raise NotImplementedError
 
 class _POSIXUserscriptRunner(_BaseUserscriptRunner):
 
@@ -193,12 +206,18 @@ class _POSIXUserscriptRunner(_BaseUserscriptRunner):
 
         self._run_process(cmd, *args, env=env, verbose=verbose)
 
+    @pyqtSlot()
     def on_proc_finished(self):
-        """Interrupt the reader when the process finished."""
-        self.finish()
+        self._cleanup()
 
-    def finish(self):
-        """Quit the thread and clean up when the reader finished."""
+    @pyqtSlot()
+    def on_proc_error(self):
+        self._cleanup()
+
+    def _cleanup(self):
+        """Clean up reader and temorary files."""
+        if self._cleaned_up:
+            return
         log.procs.debug("Cleaning up")
         self._reader.cleanup()
         self._reader.deleteLater()
@@ -229,13 +248,21 @@ class _WindowsUserscriptRunner(_BaseUserscriptRunner):
 
     def _cleanup(self):
         """Clean up temporary files after the userscript finished."""
+        if self._cleaned_up:
+            return
         try:
             os.close(self._oshandle)
         except OSError:
             log.procs.exception("Failed to close file handle!")
         super()._cleanup()
         self._oshandle = None
+        self.finished.emit()
 
+    @pyqtSlot()
+    def on_proc_error(self):
+        self._cleanup()
+
+    @pyqtSlot()
     def on_proc_finished(self):
         """Read back the commands when the process finished."""
         try:
@@ -245,7 +272,6 @@ class _WindowsUserscriptRunner(_BaseUserscriptRunner):
         except OSError:
             log.procs.exception("Failed to read command file!")
         self._cleanup()
-        self.finished.emit()
 
     def run(self, cmd, *args, env=None, verbose=False):
         try:

tests/integration/features/test_userscripts_bdd.py

@@ -0,0 +1,22 @@
+# vim: ft=python fileencoding=utf-8 sts=4 sw=4 et:
+
+# Copyright 2016 Florian Bruhin (The Compiler) <mail@qutebrowser.org>
+#
+# This file is part of qutebrowser.
+#
+# qutebrowser is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# qutebrowser is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with qutebrowser.  If not, see <http://www.gnu.org/licenses/>.
+
+import pytest_bdd as bdd
+
+bdd.scenarios('userscripts.feature')

tests/integration/features/userscripts.feature

@@ -0,0 +1,3 @@
+Scenario: Starting an userscript which doesn't exist
+    When I run :spawn -u this_does_not_exist
+    Then the error "Error while spawning userscript: The process failed to start." should be shown