From ff5d4d35641863aaec740c5b5923ae5d90072a35 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Sun, 13 Oct 2024 19:08:42 +0200 Subject: [PATCH] Avoid passing a parent to QProcess With the upgrade to MarkupSafe 3.0, something funny happened when trying to pass the GUIProcess object to jinja after launching a userscript: [...] File "[...]/qutebrowser/browser/qutescheme.py", line 291, in qute_process src = jinja.render('process.html', title=f'Process {pid}', proc=proc) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "[...]/qutebrowser/utils/jinja.py", line 123, in render return environment.get_template(template).render(**kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [...] File "html/process.html", line 11, in block 'content' File "[...]/lib/python3.11/site-packages/markupsafe/__init__.py", line 42, in escape if hasattr(s, "__html__"): ^^^^^^^^^^^^^^^^^^^^^^ RuntimeError: wrapped C/C++ object of type GUIProcess has been deleted This can be reproduced with: qutebrowser --temp-basedir ':cmd-later 0 spawn -u -o /bin/echo test' We pass the `GUIProcess` to the Jinja template as `proc`, which then formats it as `{{ proc }}`` (to stringify it). For some reason, with the newest MarkupSafe/Jinja versions, this now triggers the `if hasattr(s, "__html__")` check in MarkupSafe (which has been around for a while). That then presumably causes PyQt to try and access the underlying C++ object for `GUIProcess``, but that has already been deleted. But why is it deleted in the first place, if we keep track of even completed processes data ever since we added `:process` in a3adba81c? It looks like the Qt parent-child relationship is the culprit here: When we pass a parent to the `GUIProcess`` from the userscript runner, it will get deleted as soon as said runner is cleaned up (which happens after the userscript has finished). We probably never noticed this before because we only accessed data from the Python wrapper and not from the C++ side, but it still seems like a good idea to avoid passing a parent for a long-lived object (with time-based cleanup) in the first place. --- doc/changelog.asciidoc | 6 ++++++ qutebrowser/browser/commands.py | 3 +-- qutebrowser/commands/userscripts.py | 2 +- qutebrowser/misc/editor.py | 2 +- qutebrowser/misc/guiprocess.py | 5 +++-- 5 files changed, 12 insertions(+), 6 deletions(-) diff --git a/doc/changelog.asciidoc b/doc/changelog.asciidoc index 4919fccc5..c1b8f1a2f 100644 --- a/doc/changelog.asciidoc +++ b/doc/changelog.asciidoc @@ -44,6 +44,12 @@ Changed - Updated mimetype information for getting a suitable extension when downloading a `data:` URL. +Fixed +~~~~~ + +- Crash with recent Jinja/Markupsafe versions when viewing a finished userscript + (or potentially editor) process via `:process`. + [[v3.3.1]] v3.3.1 (2024-10-12) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index a62a00797..f686f522c 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1137,8 +1137,7 @@ class CommandDispatcher: else: cmd = os.path.expanduser(cmd) proc = guiprocess.GUIProcess(what='command', verbose=verbose, - output_messages=output_messages, - parent=self._tabbed_browser) + output_messages=output_messages) if detach: ok = proc.start_detached(cmd, args) if not ok: diff --git a/qutebrowser/commands/userscripts.py b/qutebrowser/commands/userscripts.py index e2f24d28d..e929de3d2 100644 --- a/qutebrowser/commands/userscripts.py +++ b/qutebrowser/commands/userscripts.py @@ -156,7 +156,7 @@ class _BaseUserscriptRunner(QObject): self.proc = guiprocess.GUIProcess( 'userscript', additional_env=self._env, - output_messages=output_messages, verbose=verbose, parent=self) + output_messages=output_messages, verbose=verbose) self.proc.finished.connect(self.on_proc_finished) self.proc.error.connect(self.on_proc_error) self.proc.start(cmd, args) diff --git a/qutebrowser/misc/editor.py b/qutebrowser/misc/editor.py index 948b4ab9e..9f77fa75e 100644 --- a/qutebrowser/misc/editor.py +++ b/qutebrowser/misc/editor.py @@ -180,7 +180,7 @@ class ExternalEditor(QObject): line: the line number to pass to the editor column: the column number to pass to the editor """ - self._proc = guiprocess.GUIProcess(what='editor', parent=self) + self._proc = guiprocess.GUIProcess(what='editor') self._proc.finished.connect(self._on_proc_closed) self._proc.error.connect(self._on_proc_error) editor = config.val.editor.command diff --git a/qutebrowser/misc/guiprocess.py b/qutebrowser/misc/guiprocess.py index 6b1dcbf6b..2e4f33748 100644 --- a/qutebrowser/misc/guiprocess.py +++ b/qutebrowser/misc/guiprocess.py @@ -177,9 +177,10 @@ class GUIProcess(QObject): verbose: bool = False, additional_env: Mapping[str, str] = None, output_messages: bool = False, - parent: QObject = None, ): - super().__init__(parent) + # We do not accept a parent, as GUIProcesses keep track of themselves + # (see all_processes and _post_start() / _on_cleanup_timer()) + super().__init__() self.what = what self.verbose = verbose self._output_messages = output_messages