Repository: qutebrowser/qutebrowser
Pull request: Resolve crash when editor tab is closed.
If an editor is open on a form in a tab and that tab is closed, rewire
the callback to print a warning. Previously, the callback would access a
deleted C++ object and cause a crash.
Resolves #2758.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/qutebrowser/qutebrowser/3333)
<!-- Reviewable:end -->


Problem statement:
Deleted C++ object: WebEngineView when closing editor after closing tab
When opening an editor for a form, then closing the tab, and then the editor, we get:

```
$ python3 -m qutebrowser --temp-basedir --backend webengine --debug tests/end2end/data/hello.txt tests/end2end/data/insert_mode_settings/html/textarea.html
[...]
19:40:01 DEBUG    commands   command:run:524 Calling qutebrowser.browser.commands.CommandDispatcher.open_editor(<qutebrowser.browser.commands.CommandDispatcher>)
19:40:01 DEBUG    webview    webenginetab:_js_cb_single:494 Got element from JS: {'attributes': {'id': 'qute-textarea'}, 'class_name': '', 'id': 1, 'outer_xml': '<textarea id="qute-textarea"></textarea>', 'rects': [{'bottom': 44, 'height': 36, 'left': 8, 'right': 189, 'top': 8, 'width': 181}], 'tag_name': 'TEXTAREA', 'text': '', 'value': ''}
19:40:01 DEBUG    webelem    webelem:is_editable:238 Checking if element is editable: <qutebrowser.browser.webengine.webengineelem.WebEngineElement html='<textarea id="qute-textarea"></textarea>'>
19:40:01 DEBUG    procs      editor:edit:126 Calling "gvim" with args ['-f', '/tmp/qutebrowser-editor-rp75_n9j']
19:40:01 DEBUG    procs      guiprocess:start:139 Starting process.
19:40:01 DEBUG    procs      guiprocess:_pre_start:133 Executing: gvim -f /tmp/qutebrowser-editor-rp75_n9j
19:40:01 DEBUG    procs      guiprocess:on_started:122 Process started.
[...]
19:40:04 DEBUG    commands   command:run:524 Calling qutebrowser.browser.commands.CommandDispatcher.tab_close(<qutebrowser.browser.commands.CommandDispatcher>, False, False, False, False, None)
[...]
19:40:06 DEBUG    procs      guiprocess:on_finished:98 Process finished with code 0, status 0.
19:40:06 DEBUG    procs      editor:on_proc_closed:70 Editor closed
19:40:06 DEBUG    procs      editor:on_proc_closed:87 Read back: 
19:40:06 ERROR    misc       crashsignal:exception_hook:205 Uncaught exception
Traceback (most recent call last):
  File "/home/florian/proj/qutebrowser/git/qutebrowser/browser/commands.py", line 1617, in on_editing_finished
    elem.set_value(text)
  File "/home/florian/proj/qutebrowser/git/qutebrowser/browser/webengine/webengineelem.py", line 133, in set_value
    self._js_call('set_value', value)
  File "/home/florian/proj/qutebrowser/git/qutebrowser/browser/webengine/webengineelem.py", line 103, in _js_call
    self._tab.run_js_async(js_code, callback=callback)
  File "/home/florian/proj/qutebrowser/git/qutebrowser/browser/webengine/webenginetab.py", line 611, in run_js_async
    self._widget.page().runJavaScript(code, world_id)
RuntimeError: wrapped C/C++ object of type WebEngineView has been deleted
```

Hints from issue discussion:
Some logs:

- https://crashes.qutebrowser.org/view/d988c01e
- https://crashes.qutebrowser.org/view/4357da17
- https://crashes.qutebrowser.org/view/64d4ce24
@rcorre This is the crash you reported recently :wink:

Commit message: Resolve crash when editor tab is closed.

If an editor is open on a form in a tab and that tab is closed, rewire
the callback to print a warning. Previously, the callback would access a
deleted C++ object and cause a crash.
Resolves #2758.

Patch to review:
diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py
index dc91a321e0e..3dcece04570 100644
--- a/qutebrowser/browser/commands.py
+++ b/qutebrowser/browser/commands.py
@@ -1621,6 +1621,9 @@ def _open_editor_cb(self, elem):
         ed = editor.ExternalEditor(self._tabbed_browser)
         ed.editing_finished.connect(functools.partial(
             self.on_editing_finished, elem))
+        tab = self._current_widget()
+        tab.shutting_down.connect(functools.partial(
+            self.on_editor_orphaned, ed))
         ed.edit(text, caret_position)
 
     @cmdutils.register(instance='command-dispatcher', scope='window')
@@ -1647,6 +1650,11 @@ def on_editing_finished(self, elem, text):
         except webelem.Error as e:
             raise cmdexc.CommandError(str(e))
 
+    def on_editor_orphaned(self, ed):
+        ed.editing_finished.disconnect()
+        ed.editing_finished.connect(
+            lambda: message.warning('Edited element was closed'))
+
     @cmdutils.register(instance='command-dispatcher', maxsplit=0,
                        scope='window')
     def insert_text(self, text):
diff --git a/tests/end2end/features/editor.feature b/tests/end2end/features/editor.feature
index 93b61df30dc..92de6ccdd24 100644
--- a/tests/end2end/features/editor.feature
+++ b/tests/end2end/features/editor.feature
@@ -115,6 +115,16 @@ Feature: Opening external editors
         And I run :click-element id qute-button
         Then the javascript message "text: foobar" should be logged
 
+    Scenario: Spawning an editor and closing the tab
+        When I set up a fake editor returning "foo" after 1s
+        And I open data/editor.html
+        And I run :click-element id qute-textarea
+        And I wait for "Entering mode KeyMode.insert (reason: clicking input)" in the log
+        And I run :open-editor
+        And I set tabs.last_close to blank
+        And I run :tab-close
+        Then the warning "Edited element was closed" should be shown
+
     @qtwebengine_todo: Caret mode is not implemented yet
     Scenario: Spawning an editor in caret mode
         When I set up a fake editor returning "foobar"
diff --git a/tests/end2end/features/test_editor_bdd.py b/tests/end2end/features/test_editor_bdd.py
index f66b94f7731..0cd85a351f3 100644
--- a/tests/end2end/features/test_editor_bdd.py
+++ b/tests/end2end/features/test_editor_bdd.py
@@ -64,3 +64,21 @@ def set_up_editor(quteproc, server, tmpdir, text):
 def set_up_editor_empty(quteproc, server, tmpdir):
     """Set up editor.command to a small python script inserting empty text."""
     set_up_editor(quteproc, server, tmpdir, "")
+
+
+@bdd.when(bdd.parsers.parse(
+    'I set up a fake editor returning "{text}" after {t}s'))
+def set_up_editor_delay(quteproc, server, tmpdir, text, t):
+    """Set up editor.command to a small python script inserting a text."""
+    script = tmpdir / 'script.py'
+    script.write(textwrap.dedent("""
+        import sys
+        import time
+
+        time.sleep({t})
+
+        with open(sys.argv[1], 'w', encoding='utf-8') as f:
+            f.write({text!r})
+    """.format(text=text, t=t)))
+    editor = json.dumps([sys.executable, str(script), '{}'])
+    quteproc.set_setting('editor.command', editor)


Code review comments:
diff_hunk: @@ -1647,6 +1650,11 @@ def on_editing_finished(self, elem, text):
         except webelem.Error as e:
             raise cmdexc.CommandError(str(e))
 
+    def on_editor_orphaned(self, ed):
path: qutebrowser/browser/commands.py
text: @user1:
Hmm, while the reconnecting works, I wonder whether there isn't a better solution. Can't we detect whether the view is gone in `on_editing_finished` instead?

@author:
The C++ object wrapped by the `WebEngineView` is deleted. A few ideas I had are:

- Can a `WebEngineView` check if the C++ object is deleted?
- Does `AbstractTab` expose any way to check if it is closed?
- Can I search the `TabbedBrowser` to see if it still contains a tab I hold a reference to?

I didn't see an obvious way to do any of these, so catching the shutdown signal from the tab seemed like the best solution. Any ideas?

@user1:
You can use `sip.isdeleted` on a PyQt object. So I guess adding a `is_deleted` method to `AbstractTab` which does `return sip.isdeleted(self._widget)` or so would be the nicest solution here - what do you think?

diff_hunk: @@ -1647,6 +1650,11 @@ def on_editing_finished(self, elem, text):
         except webelem.Error as e:
             raise cmdexc.CommandError(str(e))
 
+    def on_editor_orphaned(self, ed):
+        ed.editing_finished.disconnect()
+        ed.editing_finished.connect(
+            lambda: message.warning('Edited element was closed'))
path: qutebrowser/browser/commands.py
text: @user1:
I'd prefer "Edited element vanished" or so (we already use that terminology elsewhere).

diff_hunk: @@ -64,3 +64,21 @@ def set_up_editor(quteproc, server, tmpdir, text):
 def set_up_editor_empty(quteproc, server, tmpdir):
     """Set up editor.command to a small python script inserting empty text."""
     set_up_editor(quteproc, server, tmpdir, "")
+
+
+@bdd.when(bdd.parsers.parse(
+    'I set up a fake editor returning "{text}" after {t}s'))
+def set_up_editor_delay(quteproc, server, tmpdir, text, t):
+    """Set up editor.command to a small python script inserting a text."""
+    script = tmpdir / 'script.py'
+    script.write(textwrap.dedent("""
+        import sys
+        import time
+
+        time.sleep({t})
path: tests/end2end/features/test_editor_bdd.py
text: @user2:
This sounds like a recipe for flaky tests - can't we save the editor somewhere, and have another `@user1.when` function quitting the editor somehow (e.g. by making the Python script exit cleanly on `SIGUSR1` or so)?