[vte] widget: Always emit the child-exited signal

[Christian Persch <chpe src gnome org>]

commit 0276859709692ec53f35a8cc158d7553a195c730
Author: Christian Persch <chpe src gnome org>
Date:   Mon Aug 20 22:47:46 2018 +0200

    widget: Always emit the child-exited signal
    
    When there is still a child process running when the
    VteTerminal is destroyed, it gets kill()ed, but since
    the child-exited signal was only emitted from the
    VteReaper callback, it was never actually emitted, since
    by that time VteTerminal has ceased to exist.
    
    Instead of waiting for the reaper, immediately emit
    the child-exited signal in VteTerminal::dispose(). The
    exit status is synthesised to be WIFSIGNALED() with
    WTERMSIG() == SIGKILL.
    
    https://gitlab.gnome.org/GNOME/gnome-terminal/issues/16

 src/app/app.cc     |  8 +++++-
 src/vte.cc         | 79 +++++++++++++++++++++++++++---------------------------
 src/vtegtk.cc      | 13 +++++++++
 src/vteinternal.hh |  7 ++---
 src/widget.cc      | 19 +++++++++++++
 src/widget.hh      |  4 +++
 6 files changed, 86 insertions(+), 44 deletions(-)
---
diff --git a/src/app/app.cc b/src/app/app.cc
index a2bde0b4..b155efa3 100644
--- a/src/app/app.cc
+++ b/src/app/app.cc
@@ -22,6 +22,7 @@
 #include <locale.h>
 #include <unistd.h>
 #include <sys/types.h>
+#include <sys/wait.h>
 
 #include <glib.h>
 #include <glib/gprintf.h>
@@ -1530,7 +1531,12 @@ window_child_exited_cb(VteTerminal* term,
                        int status,
                        VteappWindow* window)
 {
-        verbose_printerr("Child exited with status %x\n", status);
+        if (WIFEXITED(status))
+                verbose_printerr("Child exited with status %x\n", WEXITSTATUS(status));
+        else if (WIFSIGNALED(status))
+                verbose_printerr("Child terminated by signal %d\n", WTERMSIG(status));
+        else
+                verbose_printerr("Child terminated\n");
 
         if (options.output_filename != nullptr) {
                 auto file = g_file_new_for_commandline_arg(options.output_filename);
diff --git a/src/vte.cc b/src/vte.cc
index 7c38b522..32323f46 100644
--- a/src/vte.cc
+++ b/src/vte.cc
@@ -3012,7 +3012,7 @@ reaper_child_exited_cb(VteReaper *reaper,
                        int status,
                        vte::terminal::Terminal* that)
 {
-        GPid pid = GPid(ipid);
+        pid_t pid = pid_t{ipid};
 
         auto terminal = that->m_terminal;
         /* keep the vte::terminal::Terminal in a death grip */
@@ -3023,8 +3023,8 @@ reaper_child_exited_cb(VteReaper *reaper,
 }
 
 void
-Terminal::child_watch_done(GPid pid,
-                                     int status)
+Terminal::child_watch_done(pid_t pid,
+                           int status)
 {
        if (pid != m_pty_pid)
                 return;
@@ -3061,9 +3061,8 @@ Terminal::child_watch_done(GPid pid,
         set_pty(nullptr);
 
         /* Tell observers what's happened. */
-        _vte_debug_print(VTE_DEBUG_SIGNALS,
-                         "Emitting `child-exited'.\n");
-        g_signal_emit(object, signals[SIGNAL_CHILD_EXITED], 0, status);
+        if (m_real_widget)
+                m_real_widget->emit_child_exited(status);
 
         g_object_thaw_notify(object);
 }
@@ -3182,7 +3181,7 @@ Terminal::pty_scroll_lock_changed(bool locked)
 
 /*
  * Terminal::watch_child:
- * @child_pid: a #GPid
+ * @child_pid: a #pid_t
  *
  * Watches @child_pid. When the process exists, the #VteTerminal::child-exited
  * signal will be called with the child's exit status.
@@ -3199,7 +3198,7 @@ Terminal::pty_scroll_lock_changed(bool locked)
  * the %G_SPAWN_DO_NOT_REAP_CHILD flag MUST have been passed.
  */
 void
-Terminal::watch_child (GPid child_pid)
+Terminal::watch_child (pid_t child_pid)
 {
         // FIXMEchpe: support passing child_pid = -1 to remove the wathch
         g_assert(child_pid != -1);
@@ -8017,7 +8016,6 @@ Terminal::Terminal(vte::platform::Widget* w,
         set_size(VTE_COLUMNS, VTE_ROWS);
        m_pty_input_source = 0;
        m_pty_output_source = 0;
-       m_pty_pid = -1;
 
        /* Scrolling options. */
        m_scroll_on_keystroke = TRUE;
@@ -8310,6 +8308,13 @@ Terminal::~Terminal()
        int sel;
        guint i;
 
+        terminate_child();
+        set_pty(nullptr);
+        remove_update_timeout(this);
+
+        /* Stop processing input. */
+        stop_processing(this);
+
        /* Free the draw structure. */
        if (m_draw != NULL) {
                _vte_draw_free(m_draw);
@@ -8390,33 +8395,9 @@ Terminal::~Terminal()
                 g_object_unref(m_reaper);
         }
 
-       /* Stop processing input. */
-       stop_processing(this);
-
        /* Discard any pending data. */
        _vte_byte_array_free(m_outgoing);
-
-       /* Stop the child and stop watching for input from the child. */
-       if (m_pty_pid != -1) {
-#ifdef HAVE_GETPGID
-               pid_t pgrp;
-               pgrp = getpgid(m_pty_pid);
-               if (pgrp != -1) {
-                       kill(-pgrp, SIGHUP);
-               }
-#endif
-               kill(m_pty_pid, SIGHUP);
-       }
-       disconnect_pty_read();
-       disconnect_pty_write();
-       if (m_pty_channel != NULL) {
-               g_io_channel_unref (m_pty_channel);
-       }
-       if (m_pty != NULL) {
-                g_object_unref(m_pty);
-       }
-
-       remove_update_timeout(this);
+        m_outgoing = nullptr;
 
        /* Free public-facing data. */
        if (m_vadjustment != NULL) {
@@ -10356,13 +10337,13 @@ Terminal::set_pty(VtePty *new_pty)
         if (new_pty == m_pty)
                 return false;
 
-        if (m_pty != NULL) {
+        if (m_pty != nullptr) {
                 disconnect_pty_read();
                 disconnect_pty_write();
 
-                if (m_pty_channel != NULL) {
+                if (m_pty_channel != nullptr) {
                         g_io_channel_unref (m_pty_channel);
-                        m_pty_channel = NULL;
+                        m_pty_channel = nullptr;
                 }
 
                /* Take one last shot at processing whatever data is pending,
@@ -10383,11 +10364,11 @@ Terminal::set_pty(VtePty *new_pty)
                _vte_byte_array_clear(m_outgoing);
 
                 g_object_unref(m_pty);
-                m_pty = NULL;
+                m_pty = nullptr;
         }
 
-        if (new_pty == NULL) {
-                m_pty = NULL;
+        if (new_pty == nullptr) {
+                m_pty = nullptr;
                 return true;
         }
 
@@ -10415,6 +10396,24 @@ Terminal::set_pty(VtePty *new_pty)
         return true;
 }
 
+bool
+Terminal::terminate_child() noexcept
+{
+       if (m_pty_pid == -1)
+                return false;
+
+#ifdef HAVE_GETPGID
+        auto pgrp = getpgid(m_pty_pid);
+        if (pgrp != -1) {
+                kill(-pgrp, SIGHUP);
+        }
+#endif
+        kill(m_pty_pid, SIGHUP);
+        m_pty_pid = -1;
+
+        return true;
+}
+
 /* We need this bit of glue to ensure that accessible objects will always
  * get signals. */
 void
diff --git a/src/vtegtk.cc b/src/vtegtk.cc
index 16422454..bdff9fc5 100644
--- a/src/vtegtk.cc
+++ b/src/vtegtk.cc
@@ -403,6 +403,18 @@ vte_terminal_init(VteTerminal *terminal)
         gtk_widget_set_has_window(&terminal->widget, FALSE);
 }
 
+static void
+vte_terminal_dispose(GObject *object)
+{
+       _vte_debug_print(VTE_DEBUG_LIFECYCLE, "vte_terminal_dispose()\n");
+
+       VteTerminal *terminal = VTE_TERMINAL (object);
+        WIDGET(terminal)->dispose();
+
+       /* Call the inherited dispose() method. */
+       G_OBJECT_CLASS(vte_terminal_parent_class)->dispose(object);
+}
+
 static void
 vte_terminal_finalize(GObject *object)
 {
@@ -679,6 +691,7 @@ vte_terminal_class_init(VteTerminalClass *klass)
 
        /* Override some of the default handlers. */
         gobject_class->constructed = vte_terminal_constructed;
+        gobject_class->dispose = vte_terminal_dispose;
        gobject_class->finalize = vte_terminal_finalize;
         gobject_class->get_property = vte_terminal_get_property;
         gobject_class->set_property = vte_terminal_set_property;
diff --git a/src/vteinternal.hh b/src/vteinternal.hh
index 31cbe476..785901b8 100644
--- a/src/vteinternal.hh
+++ b/src/vteinternal.hh
@@ -360,7 +360,7 @@ public:
         guint m_pty_input_source;
         guint m_pty_output_source;
         gboolean m_pty_input_active;
-        GPid m_pty_pid;                        /* pid of child process */
+        pid_t m_pty_pid{-1};           /* pid of child process */
         VteReaper *m_reaper;
 
        /* Queue of chunks of data read from the PTY.
@@ -848,8 +848,9 @@ public:
         void feed_child_using_modes(char const* data,
                                     gssize length);
 
-        void watch_child (GPid child_pid);
-        void child_watch_done(GPid pid,
+        void watch_child (pid_t child_pid);
+        bool terminate_child () noexcept;
+        void child_watch_done(pid_t pid,
                               int status);
 
         void im_commit(char const* text);
diff --git a/src/widget.cc b/src/widget.cc
index 89149ded..4baf42db 100644
--- a/src/widget.cc
+++ b/src/widget.cc
@@ -20,9 +20,12 @@
 
 #include "widget.hh"
 
+#include <sys/wait.h> // for W_EXITCODE
+
 #include <new>
 #include <string>
 
+#include "vtegtk.hh"
 #include "debug.h"
 
 using namespace std::literals;
@@ -119,6 +122,22 @@ Widget::create_cursor(GdkCursorType cursor_type) const noexcept
        return gdk_cursor_new_for_display(gtk_widget_get_display(m_widget), cursor_type);
 }
 
+void
+Widget::dispose() noexcept
+{
+        if (m_terminal->terminate_child()) {
+                int status = W_EXITCODE(0, SIGKILL);
+                emit_child_exited(status);
+        }
+}
+
+void
+Widget::emit_child_exited(int status) noexcept
+{
+        _vte_debug_print(VTE_DEBUG_SIGNALS, "Emitting `child-exited'.\n");
+        g_signal_emit(object(), signals[SIGNAL_CHILD_EXITED], 0, status);
+}
+
 bool
 Widget::im_filter_keypress(GdkEventKey* event) noexcept
 {
diff --git a/src/widget.hh b/src/widget.hh
index cbcac913..78185efe 100644
--- a/src/widget.hh
+++ b/src/widget.hh
@@ -44,12 +44,14 @@ public:
         Widget& operator= (Widget const&) = delete;
         Widget& operator= (Widget&&) = delete;
 
+        GObject* object() const noexcept { return reinterpret_cast<GObject*>(m_widget); }
         GtkWidget* gtk() const noexcept { return m_widget; }
         VteTerminal* vte() const noexcept { return reinterpret_cast<VteTerminal*>(m_widget); }
 
         vte::terminal::Terminal* terminal() const noexcept { return m_terminal; }
 
         void constructed() noexcept { m_terminal->widget_constructed(); }
+        void dispose() noexcept;
         void realize() noexcept;
         void unrealize() noexcept;
         void map() noexcept;
@@ -95,6 +97,8 @@ public:
         int hscroll_policy() const noexcept { return m_terminal->m_hscroll_policy; }
         int vscroll_policy() const noexcept { return m_terminal->m_vscroll_policy; }
 
+        void emit_child_exited(int status) noexcept;
+
 protected:
 
         enum class Cursor {