patch 8.1.0845: having job_status() free the job causes problems

Problem:    Having job_status() free the job causes problems.
Solution:   Do not actually free the job or terminal yet, put it in a list and
            free it a bit later. Do not use a terminal after checking the job
            status.  (closes #3873)

diff --git a/src/channel.c b/src/channel.c
index 419939897dce58ba94d6e986e41c43d1b593d419..cf68271925574bce64856feb2f90a9d007fedd0a 100644 (file)
--- a/src/channel.c
+++ b/src/channel.c
@@ -5161,8 +5161,11 @@ job_free_contents(job_T *job)
     }
 }
 
+/*
+ * Remove "job" from the list of jobs.
+ */
     static void
-job_free_job(job_T *job)
+job_unlink(job_T *job)
 {
     if (job->jv_next != NULL)
        job->jv_next->jv_prev = job->jv_prev;
@@ -5170,6 +5173,12 @@ job_free_job(job_T *job)
        first_job = job->jv_next;
     else
        job->jv_prev->jv_next = job->jv_next;
+}
+
+    static void
+job_free_job(job_T *job)
+{
+    job_unlink(job);
     vim_free(job);
 }
 
@@ -5183,12 +5192,44 @@ job_free(job_T *job)
     }
 }
 
+job_T *jobs_to_free = NULL;
+
+/*
+ * Put "job" in a list to be freed later, when it's no longer referenced.
+ */
+    static void
+job_free_later(job_T *job)
+{
+    job_unlink(job);
+    job->jv_next = jobs_to_free;
+    jobs_to_free = job;
+}
+
+    static void
+free_jobs_to_free_later(void)
+{
+    job_T *job;
+
+    while (jobs_to_free != NULL)
+    {
+       job = jobs_to_free;
+       jobs_to_free = job->jv_next;
+       job_free_contents(job);
+       vim_free(job);
+    }
+}
+
 #if defined(EXITFREE) || defined(PROTO)
     void
 job_free_all(void)
 {
     while (first_job != NULL)
        job_free(first_job);
+    free_jobs_to_free_later();
+
+# ifdef FEAT_TERMINAL
+    free_unused_terminals();
+# endif
 }
 #endif
 
@@ -5359,6 +5400,8 @@ win32_build_cmd(list_T *l, garray_T *gap)
  * NOTE: Must call job_cleanup() only once right after the status of "job"
  * changed to JOB_ENDED (i.e. after job_status() returned "dead" first or
  * mch_detect_ended_job() returned non-NULL).
+ * If the job is no longer used it will be removed from the list of jobs, and
+ * deleted a bit later.
  */
     void
 job_cleanup(job_T *job)
@@ -5394,15 +5437,13 @@ job_cleanup(job_T *job)
        channel_need_redraw = TRUE;
     }
 
-    /* Do not free the job in case the close callback of the associated channel
-     * isn't invoked yet and may get information by job_info(). */
+    // Do not free the job in case the close callback of the associated channel
+    // isn't invoked yet and may get information by job_info().
     if (job->jv_refcount == 0 && !job_channel_still_useful(job))
-    {
-       /* The job was already unreferenced and the associated channel was
-        * detached, now that it ended it can be freed. Careful: caller must
-        * not use "job" after this! */
-       job_free(job);
-    }
+       // The job was already unreferenced and the associated channel was
+       // detached, now that it ended it can be freed. However, a caller might
+       // still use it, thus free it a bit later.
+       job_free_later(job);
 }
 
 /*
@@ -5609,9 +5650,12 @@ job_check_ended(void)
        if (job == NULL)
            break;
        did_end = TRUE;
-       job_cleanup(job); // may free "job"
+       job_cleanup(job); // may add "job" to jobs_to_free
     }
 
+    // Actually free jobs that were cleaned up.
+    free_jobs_to_free_later();
+
     if (channel_need_redraw)
     {
        channel_need_redraw = FALSE;

diff --git a/src/misc2.c b/src/misc2.c
index df3f8e169be3c942b5a5e9ac52cfb0aba7b8a8d5..657e164913139f59c7b194e34eae95ea1633035b 100644 (file)
--- a/src/misc2.c
+++ b/src/misc2.c
@@ -6386,6 +6386,9 @@ parse_queued_messages(void)
        // changes, e.g. stdin may have been closed.
        if (job_check_ended())
            continue;
+# endif
+# ifdef FEAT_TERMINAL
+       free_unused_terminals();
 # endif
        break;
     }

diff --git a/src/proto/terminal.pro b/src/proto/terminal.pro
index a318fc87facb1f00ec79e6ebd1b301077be3897f..e6914c0ba0cdfcec3ce5601f06ff127fd077e5ec 100644 (file)
--- a/src/proto/terminal.pro
+++ b/src/proto/terminal.pro
@@ -5,6 +5,7 @@ void ex_terminal(exarg_T *eap);
 int term_write_session(FILE *fd, win_T *wp);
 int term_should_restore(buf_T *buf);
 void free_terminal(buf_T *buf);
+void free_unused_terminals(void);
 void write_to_term(buf_T *buffer, char_u *msg, channel_T *channel);
 int term_job_running(term_T *term);
 int term_none_open(term_T *term);

diff --git a/src/terminal.c b/src/terminal.c
index f33521a9f39fb3e498278735560516f4549ed2c1..87530af2eff46f143b7e3eb847a148208b9d16c9 100644 (file)
--- a/src/terminal.c
+++ b/src/terminal.c
@@ -803,10 +803,17 @@ free_scrollback(term_T *term)
     ga_clear(&term->tl_scrollback);
 }
 
+
+// Terminals that need to be freed soon.
+term_T *terminals_to_free = NULL;
+
 /*
  * Free a terminal and everything it refers to.
  * Kills the job if there is one.
  * Called when wiping out a buffer.
+ * The actual terminal structure is freed later in free_unused_terminals(),
+ * because callbacks may wipe out a buffer while the terminal is still
+ * referenced.
  */
     void
 free_terminal(buf_T *buf)
@@ -816,6 +823,8 @@ free_terminal(buf_T *buf)
 
     if (term == NULL)
        return;
+
+    // Unlink the terminal form the list of terminals.
     if (first_term == term)
        first_term = term->tl_next;
     else
@@ -834,27 +843,41 @@ free_terminal(buf_T *buf)
            job_stop(term->tl_job, NULL, "kill");
        job_unref(term->tl_job);
     }
+    term->tl_next = terminals_to_free;
+    terminals_to_free = term;
+
+    buf->b_term = NULL;
+    if (in_terminal_loop == term)
+       in_terminal_loop = NULL;
+}
 
-    free_scrollback(term);
+    void
+free_unused_terminals()
+{
+    while (terminals_to_free != NULL)
+    {
+       term_T      *term = terminals_to_free;
 
-    term_free_vterm(term);
-    vim_free(term->tl_title);
+       terminals_to_free = term->tl_next;
+
+       free_scrollback(term);
+
+       term_free_vterm(term);
+       vim_free(term->tl_title);
 #ifdef FEAT_SESSION
-    vim_free(term->tl_command);
+       vim_free(term->tl_command);
 #endif
-    vim_free(term->tl_kill);
-    vim_free(term->tl_status_text);
-    vim_free(term->tl_opencmd);
-    vim_free(term->tl_eof_chars);
+       vim_free(term->tl_kill);
+       vim_free(term->tl_status_text);
+       vim_free(term->tl_opencmd);
+       vim_free(term->tl_eof_chars);
 #ifdef WIN3264
-    if (term->tl_out_fd != NULL)
-       fclose(term->tl_out_fd);
+       if (term->tl_out_fd != NULL)
+           fclose(term->tl_out_fd);
 #endif
-    vim_free(term->tl_cursor_color);
-    vim_free(term);
-    buf->b_term = NULL;
-    if (in_terminal_loop == term)
-       in_terminal_loop = NULL;
+       vim_free(term->tl_cursor_color);
+       vim_free(term);
+    }
 }
 
 /*
@@ -1275,6 +1298,7 @@ term_convert_key(term_T *term, int c, char *buf)
 /*
  * Return TRUE if the job for "term" is still running.
  * If "check_job_status" is TRUE update the job status.
+ * NOTE: "term" may be freed by callbacks.
  */
     static int
 term_job_running_check(term_T *term, int check_job_status)
@@ -1285,10 +1309,15 @@ term_job_running_check(term_T *term, int check_job_status)
        && term->tl_job != NULL
        && channel_is_open(term->tl_job->jv_channel))
     {
+       job_T *job = term->tl_job;
+
+       // Careful: Checking the job status may invoked callbacks, which close
+       // the buffer and terminate "term".  However, "job" will not be freed
+       // yet.
        if (check_job_status)
-           job_status(term->tl_job);
-       return (term->tl_job->jv_status == JOB_STARTED
-               || term->tl_job->jv_channel->ch_keep_open);
+           job_status(job);
+       return (job->jv_status == JOB_STARTED
+               || (job->jv_channel != NULL && job->jv_channel->ch_keep_open));
     }
     return FALSE;
 }
@@ -2151,9 +2180,8 @@ terminal_loop(int blocking)
 #ifdef FEAT_GUI
        if (!curbuf->b_term->tl_system)
 #endif
-           /* TODO: skip screen update when handling a sequence of keys. */
-           /* Repeat redrawing in case a message is received while redrawing.
-            */
+           // TODO: skip screen update when handling a sequence of keys.
+           // Repeat redrawing in case a message is received while redrawing.
            while (must_redraw != 0)
                if (update_screen(0) == FAIL)
                    break;

diff --git a/src/version.c b/src/version.c
index cf7c47eb4222f472526cf55677d37de6006978b6..3609fb0b822fb556b5748c684811fe8259889139 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -783,6 +783,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    845,
 /**/
     844,
 /**/