patch 8.0.0087v8.0.0087

Problem:    When the channel callback gets job info the job may already have
            been deleted. (lifepillar)
Solution:   Do not delete the job when the channel is still useful. (ichizok,
            closes #1242, closes #1245)

7 files changed, 122 insertions, 59 deletions

diff --git a/src/channel.c b/src/channel.c
index 1ddb1ece35..778a30e17c 100644
--- a/src/channel.c
+++ b/src/channel.c
@@ -4433,19 +4433,66 @@ job_free(job_T *job)
     }
 }
 
+#if defined(EXITFREE) || defined(PROTO)
+    void
+job_free_all(void)
+{
+    while (first_job != NULL)
+	job_free(first_job);
+}
+#endif
+
+/*
+ * Return TRUE if we need to check if the process of "job" has ended.
+ */
+    static int
+job_need_end_check(job_T *job)
+{
+    return job->jv_status == JOB_STARTED
+		   && (job->jv_stoponexit != NULL || job->jv_exit_cb != NULL);
+}
+
+/*
+ * Return TRUE if the channel of "job" is still useful.
+ */
+    static int
+job_channel_still_useful(job_T *job)
+{
+    return job->jv_channel != NULL && channel_still_useful(job->jv_channel);
+}
+
+/*
+ * Return TRUE if the job should not be freed yet.  Do not free the job when
+ * it has not ended yet and there is a "stoponexit" flag, an exit callback
+ * or when the associated channel will do something with the job output.
+ */
+    static int
+job_still_useful(job_T *job)
+{
+    return job_need_end_check(job) || job_channel_still_useful(job);
+}
+
+/*
+ * 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).
+ */
     static void
 job_cleanup(job_T *job)
 {
     if (job->jv_status != JOB_ENDED)
 	return;
 
+    /* Ready to cleanup the job. */
+    job->jv_status = JOB_FINISHED;
+
     if (job->jv_exit_cb != NULL)
     {
 	typval_T	argv[3];
 	typval_T	rettv;
 	int		dummy;
 
-	/* invoke the exit callback; make sure the refcount is > 0 */
+	/* Invoke the exit callback. Make sure the refcount is > 0. */
 	++job->jv_refcount;
 	argv[0].v_type = VAR_JOB;
 	argv[0].vval.v_job = job;
@@ -4458,42 +4505,18 @@ job_cleanup(job_T *job)
 	--job->jv_refcount;
 	channel_need_redraw = TRUE;
     }
-    if (job->jv_refcount == 0)
+
+    /* 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, now that it ended it can be
-	 * freed. Careful: caller must not use "job" after this! */
+	/* 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);
     }
 }
 
-#if defined(EXITFREE) || defined(PROTO)
-    void
-job_free_all(void)
-{
-    while (first_job != NULL)
-	job_free(first_job);
-}
-#endif
-
-/*
- * Return TRUE if the job should not be freed yet.  Do not free the job when
- * it has not ended yet and there is a "stoponexit" flag, an exit callback
- * or when the associated channel will do something with the job output.
- */
-    static int
-job_still_useful(job_T *job)
-{
-    return (job->jv_stoponexit != NULL || job->jv_exit_cb != NULL
-	    || (job->jv_channel != NULL
-		&& channel_still_useful(job->jv_channel)));
-}
-
-    static int
-job_still_alive(job_T *job)
-{
-    return (job->jv_status == JOB_STARTED) && job_still_useful(job);
-}
-
 /*
  * Mark references in jobs that are still useful.
  */
@@ -4505,7 +4528,7 @@ set_ref_in_job(int copyID)
     typval_T	tv;
 
     for (job = first_job; job != NULL; job = job->jv_next)
-	if (job_still_alive(job))
+	if (job_still_useful(job))
 	{
 	    tv.v_type = VAR_JOB;
 	    tv.vval.v_job = job;
@@ -4514,26 +4537,33 @@ set_ref_in_job(int copyID)
     return abort;
 }
 
+/*
+ * Dereference "job".  Note that after this "job" may have been freed.
+ */
     void
 job_unref(job_T *job)
 {
     if (job != NULL && --job->jv_refcount <= 0)
     {
-	/* Do not free the job when it has not ended yet and there is a
-	 * "stoponexit" flag or an exit callback. */
-	if (!job_still_alive(job))
+	/* Do not free the job if there is a channel where the close callback
+	 * may get the job info. */
+	if (!job_channel_still_useful(job))
 	{
-	    job_free(job);
-	}
-	else if (job->jv_channel != NULL
-				    && !channel_still_useful(job->jv_channel))
-	{
-	    /* Do remove the link to the channel, otherwise it hangs
-	     * around until Vim exits. See job_free() for refcount. */
-	    ch_log(job->jv_channel, "detaching channel from job");
-	    job->jv_channel->ch_job = NULL;
-	    channel_unref(job->jv_channel);
-	    job->jv_channel = NULL;
+	    /* Do not free the job when it has not ended yet and there is a
+	     * "stoponexit" flag or an exit callback. */
+	    if (!job_need_end_check(job))
+	    {
+		job_free(job);
+	    }
+	    else if (job->jv_channel != NULL)
+	    {
+		/* Do remove the link to the channel, otherwise it hangs
+		 * around until Vim exits. See job_free() for refcount. */
+		ch_log(job->jv_channel, "detaching channel from job");
+		job->jv_channel->ch_job = NULL;
+		channel_unref(job->jv_channel);
+		job->jv_channel = NULL;
+	    }
 	}
     }
 }
@@ -4546,7 +4576,7 @@ free_unused_jobs_contents(int copyID, int mask)
 
     for (job = first_job; job != NULL; job = job->jv_next)
 	if ((job->jv_copyID & mask) != (copyID & mask)
-						     && !job_still_alive(job))
+						    && !job_still_useful(job))
 	{
 	    /* Free the channel and ordinary items it contains, but don't
 	     * recurse into Lists, Dictionaries etc. */
@@ -4566,7 +4596,7 @@ free_unused_jobs(int copyID, int mask)
     {
 	job_next = job->jv_next;
 	if ((job->jv_copyID & mask) != (copyID & mask)
-						     && !job_still_alive(job))
+						    && !job_still_useful(job))
 	{
 	    /* Free the job struct itself. */
 	    job_free_job(job);
@@ -4660,8 +4690,7 @@ has_pending_job(void)
 	/* Only should check if the channel has been closed, if the channel is
 	 * open the job won't exit. */
 	if (job->jv_status == JOB_STARTED && job->jv_exit_cb != NULL
-		&& (job->jv_channel == NULL
-		    || !channel_still_useful(job->jv_channel)))
+					    && !job_channel_still_useful(job))
 	    return TRUE;
     return FALSE;
 }
@@ -4676,14 +4705,18 @@ job_check_ended(void)
 {
     int		i;
 
+    if (first_job == NULL)
+	return;
+
     for (i = 0; i < MAX_CHECK_ENDED; ++i)
     {
+	/* NOTE: mch_detect_ended_job() must only return a job of which the
+	 * status was just set to JOB_ENDED. */
 	job_T	*job = mch_detect_ended_job(first_job);
 
 	if (job == NULL)
 	    break;
-	if (job_still_useful(job))
-	    job_cleanup(job); /* may free "job" */
+	job_cleanup(job); /* may free "job" */
     }
 
     if (channel_need_redraw)
@@ -4897,7 +4930,7 @@ job_status(job_T *job)
 {
     char	*result;
 
-    if (job->jv_status == JOB_ENDED)
+    if (job->jv_status >= JOB_ENDED)
 	/* No need to check, dead is dead. */
 	result = "dead";
     else if (job->jv_status == JOB_FAILED)
diff --git a/src/eval.c b/src/eval.c
index 7ca129b378..9d2c5ac72b 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -7290,7 +7290,7 @@ get_tv_string_buf_chk(typval_T *varp, char_u *buf)
 		if (job == NULL)
 		    return (char_u *)"no process";
 		status = job->jv_status == JOB_FAILED ? "fail"
-				: job->jv_status == JOB_ENDED ? "dead"
+				: job->jv_status >= JOB_ENDED ? "dead"
 				: "run";
 # ifdef UNIX
 		vim_snprintf((char *)buf, NUMBUFLEN,
diff --git a/src/os_unix.c b/src/os_unix.c
index 6197c3fbdd..a63eb6e891 100644
--- a/src/os_unix.c
+++ b/src/os_unix.c
@@ -5354,7 +5354,7 @@ mch_job_status(job_T *job)
     return "run";
 
 return_dead:
-    if (job->jv_status != JOB_ENDED)
+    if (job->jv_status < JOB_ENDED)
     {
 	ch_log(job->jv_channel, "Job ended");
 	job->jv_status = JOB_ENDED;
@@ -5398,7 +5398,7 @@ mch_detect_ended_job(job_T *job_list)
 		job->jv_exitval = WEXITSTATUS(status);
 	    else if (WIFSIGNALED(status))
 		job->jv_exitval = -1;
-	    if (job->jv_status != JOB_ENDED)
+	    if (job->jv_status < JOB_ENDED)
 	    {
 		ch_log(job->jv_channel, "Job ended");
 		job->jv_status = JOB_ENDED;
diff --git a/src/os_win32.c b/src/os_win32.c
index 9fcb054d0e..f8e0f117d7 100644
--- a/src/os_win32.c
+++ b/src/os_win32.c
@@ -4978,7 +4978,7 @@ mch_job_status(job_T *job)
 	    || dwExitCode != STILL_ACTIVE)
     {
 	job->jv_exitval = (int)dwExitCode;
-	if (job->jv_status != JOB_ENDED)
+	if (job->jv_status < JOB_ENDED)
 	{
 	    ch_log(job->jv_channel, "Job ended");
 	    job->jv_status = JOB_ENDED;
diff --git a/src/structs.h b/src/structs.h
index 31accea050..c749a3691d 100644
--- a/src/structs.h
+++ b/src/structs.h
@@ -1421,11 +1421,13 @@ struct partial_S
     dict_T	*pt_dict;	/* dict for "self" */
 };
 
+/* Status of a job.  Order matters! */
 typedef enum
 {
     JOB_FAILED,
     JOB_STARTED,
-    JOB_ENDED
+    JOB_ENDED,	    /* detected job done */
+    JOB_FINISHED    /* job done and cleanup done */
 } jobstatus_T;
 
 /*
diff --git a/src/testdir/test_channel.vim b/src/testdir/test_channel.vim
index c21e617b1e..42e0e04b81 100644
--- a/src/testdir/test_channel.vim
+++ b/src/testdir/test_channel.vim
@@ -1232,6 +1232,32 @@ func Test_out_cb_lambda()
   endtry
 endfunc
 
+func Test_close_and_exit_cb()
+  if !has('job')
+    return
+  endif
+  call ch_log('Test_close_and_exit_cb')
+
+  let dict = {'ret': {}}
+  func dict.close_cb(ch) dict
+    let self.ret['close_cb'] = job_status(ch_getjob(a:ch))
+  endfunc
+  func dict.exit_cb(job, status) dict
+    let self.ret['exit_cb'] = job_status(a:job)
+  endfunc
+
+  let g:job = job_start('echo', {
+        \ 'close_cb': dict.close_cb,
+        \ 'exit_cb': dict.exit_cb,
+        \ })
+  call assert_equal('run', job_status(g:job))
+  unlet g:job
+  call WaitFor('len(dict.ret) >= 2')
+  call assert_equal(2, len(dict.ret))
+  call assert_match('^\%(dead\|run\)', dict.ret['close_cb'])
+  call assert_equal('dead', dict.ret['exit_cb'])
+endfunc
+
 """"""""""
 
 let g:Ch_unletResponse = ''
diff --git a/src/version.c b/src/version.c
index b1bf796ded..074c7ebc76 100644
--- a/src/version.c
+++ b/src/version.c
@@ -765,6 +765,8 @@ static char *(features[]) =
 static int included_patches[] =
 {   /* Add new patch number below this line */
 /**/
+    87,
+/**/
     86,
 /**/
     85,