To: vim_dev@googlegroups.com Subject: Patch 8.0.0849 Fcc: outbox From: Bram Moolenaar Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ------------ Patch 8.0.0849 Problem: Crash when job exit callback wipes the terminal. Solution: Check for b_term to be NULL. (Yasuhiro Matsumoto, closes #1922) Implement options for term_start() to be able to test. Make term_wait() more reliable. Files: src/terminal.c, src/testdir/test_terminal.vim, src/channel.c *** ../vim-8.0.0848/src/terminal.c 2017-08-03 14:49:22.618906222 +0200 --- src/terminal.c 2017-08-03 17:01:23.459854636 +0200 *************** *** 40,45 **** --- 40,46 ---- * - MS-Windows: no redraw for 'updatetime' #1915 * - in bash mouse clicks are inserting characters. * - mouse scroll: when over other window, scroll that window. + * - add argument to term_wait() for waiting time. * - For the scrollback buffer store lines in the buffer, only attributes in * tl_scrollback. * - When the job ends: *************** *** 146,152 **** /* * Functions with separate implementation for MS-Windows and Unix-like systems. */ ! static int term_and_job_init(term_T *term, int rows, int cols, char_u *cmd); static void term_report_winsize(term_T *term, int rows, int cols); static void term_free_vterm(term_T *term); --- 147,153 ---- /* * Functions with separate implementation for MS-Windows and Unix-like systems. */ ! static int term_and_job_init(term_T *term, int rows, int cols, char_u *cmd, jobopt_T *opt); static void term_report_winsize(term_T *term, int rows, int cols); static void term_free_vterm(term_T *term); *************** *** 185,199 **** } /* ! * ":terminal": open a terminal window and execute a job in it. */ ! void ! ex_terminal(exarg_T *eap) { exarg_T split_ea; win_T *old_curwin = curwin; term_T *term; - char_u *cmd = eap->arg; if (check_restricted() || check_secure()) return; --- 186,234 ---- } /* ! * Initialize job options for a terminal job. ! * Caller may overrule some of them. */ ! static void ! init_job_options(jobopt_T *opt) ! { ! clear_job_options(opt); ! ! opt->jo_mode = MODE_RAW; ! opt->jo_out_mode = MODE_RAW; ! opt->jo_err_mode = MODE_RAW; ! opt->jo_set = JO_MODE | JO_OUT_MODE | JO_ERR_MODE; ! ! opt->jo_io[PART_OUT] = JIO_BUFFER; ! opt->jo_io[PART_ERR] = JIO_BUFFER; ! opt->jo_set |= JO_OUT_IO + JO_ERR_IO; ! ! opt->jo_modifiable[PART_OUT] = 0; ! opt->jo_modifiable[PART_ERR] = 0; ! opt->jo_set |= JO_OUT_MODIFIABLE + JO_ERR_MODIFIABLE; ! ! opt->jo_set |= JO_OUT_BUF + JO_ERR_BUF; ! } ! ! /* ! * Set job options mandatory for a terminal job. ! */ ! static void ! setup_job_options(jobopt_T *opt, int rows, int cols) ! { ! opt->jo_io_buf[PART_OUT] = curbuf->b_fnum; ! opt->jo_io_buf[PART_ERR] = curbuf->b_fnum; ! opt->jo_pty = TRUE; ! opt->jo_term_rows = rows; ! opt->jo_term_cols = cols; ! } ! ! static void ! term_start(char_u *cmd, jobopt_T *opt) { exarg_T split_ea; win_T *old_curwin = curwin; term_T *term; if (check_restricted() || check_secure()) return; *************** *** 256,264 **** (char_u *)"terminal", OPT_FREE|OPT_LOCAL, 0); set_term_and_win_size(term); /* System dependent: setup the vterm and start the job in it. */ ! if (term_and_job_init(term, term->tl_rows, term->tl_cols, cmd) == OK) { /* store the size we ended up with */ vterm_get_size(term->tl_vterm, &term->tl_rows, &term->tl_cols); --- 291,300 ---- (char_u *)"terminal", OPT_FREE|OPT_LOCAL, 0); set_term_and_win_size(term); + setup_job_options(opt, term->tl_rows, term->tl_cols); /* System dependent: setup the vterm and start the job in it. */ ! if (term_and_job_init(term, term->tl_rows, term->tl_cols, cmd, opt) == OK) { /* store the size we ended up with */ vterm_get_size(term->tl_vterm, &term->tl_rows, &term->tl_cols); *************** *** 274,279 **** --- 310,329 ---- } /* + * ":terminal": open a terminal window and execute a job in it. + */ + void + ex_terminal(exarg_T *eap) + { + jobopt_T opt; + + init_job_options(&opt); + /* TODO: get options from before the command */ + + term_start(eap->arg, &opt); + } + + /* * Free the scrollback buffer for "term". */ static void *************** *** 965,972 **** update_cursor(curbuf->b_term, FALSE); c = term_vgetc(); ! if (curbuf->b_term->tl_vterm == NULL ! || !term_job_running(curbuf->b_term)) /* job finished while waiting for a character */ break; --- 1015,1021 ---- update_cursor(curbuf->b_term, FALSE); c = term_vgetc(); ! if (!term_use_loop()) /* job finished while waiting for a character */ break; *************** *** 993,1000 **** #ifdef FEAT_CMDL_INFO clear_showcmd(); #endif ! if (curbuf->b_term->tl_vterm == NULL ! || !term_job_running(curbuf->b_term)) /* job finished while waiting for a character */ break; --- 1042,1048 ---- #ifdef FEAT_CMDL_INFO clear_showcmd(); #endif ! if (!term_use_loop()) /* job finished while waiting for a character */ break; *************** *** 1614,1648 **** } /* - * Set job options common for Unix and MS-Windows. - */ - static void - setup_job_options(jobopt_T *opt, int rows, int cols) - { - clear_job_options(opt); - opt->jo_mode = MODE_RAW; - opt->jo_out_mode = MODE_RAW; - opt->jo_err_mode = MODE_RAW; - opt->jo_set = JO_MODE | JO_OUT_MODE | JO_ERR_MODE; - - opt->jo_io[PART_OUT] = JIO_BUFFER; - opt->jo_io[PART_ERR] = JIO_BUFFER; - opt->jo_set |= JO_OUT_IO + JO_ERR_IO; - - opt->jo_modifiable[PART_OUT] = 0; - opt->jo_modifiable[PART_ERR] = 0; - opt->jo_set |= JO_OUT_MODIFIABLE + JO_ERR_MODIFIABLE; - - opt->jo_io_buf[PART_OUT] = curbuf->b_fnum; - opt->jo_io_buf[PART_ERR] = curbuf->b_fnum; - opt->jo_pty = TRUE; - opt->jo_set |= JO_OUT_BUF + JO_ERR_BUF; - - opt->jo_term_rows = rows; - opt->jo_term_cols = cols; - } - - /* * Create a new vterm and initialize it. */ static void --- 1662,1667 ---- *************** *** 2089,2100 **** f_term_start(typval_T *argvars, typval_T *rettv) { char_u *cmd = get_tv_string_chk(&argvars[0]); ! exarg_T ea; if (cmd == NULL) return; ! ea.arg = cmd; ! ex_terminal(&ea); if (curbuf->b_term != NULL) rettv->vval.v_number = curbuf->b_fnum; --- 2108,2126 ---- f_term_start(typval_T *argvars, typval_T *rettv) { char_u *cmd = get_tv_string_chk(&argvars[0]); ! jobopt_T opt; if (cmd == NULL) return; ! init_job_options(&opt); ! /* TODO: allow more job options */ ! if (argvars[1].v_type != VAR_UNKNOWN ! && get_job_options(&argvars[1], &opt, ! JO_TIMEOUT_ALL + JO_STOPONEXIT ! + JO_EXIT_CB + JO_CLOSE_CALLBACK) == FAIL) ! return; ! ! term_start(cmd, &opt); if (curbuf->b_term != NULL) rettv->vval.v_number = curbuf->b_fnum; *************** *** 2109,2127 **** buf_T *buf = term_get_buf(argvars); if (buf == NULL) return; /* Get the job status, this will detect a job that finished. */ ! if (buf->b_term->tl_job != NULL) ! (void)job_status(buf->b_term->tl_job); ! /* Check for any pending channel I/O. */ ! vpeekc_any(); ! ui_delay(10L, FALSE); ! ! /* Flushing messages on channels is hopefully sufficient. ! * TODO: is there a better way? */ ! parse_queued_messages(); } # ifdef WIN3264 --- 2135,2174 ---- buf_T *buf = term_get_buf(argvars); if (buf == NULL) + { + ch_log(NULL, "term_wait(): invalid argument"); return; + } /* Get the job status, this will detect a job that finished. */ ! if (buf->b_term->tl_job == NULL ! || STRCMP(job_status(buf->b_term->tl_job), "dead") == 0) ! { ! /* The job is dead, keep reading channel I/O until the channel is ! * closed. */ ! while (buf->b_term != NULL && !buf->b_term->tl_channel_closed) ! { ! mch_char_avail(); ! parse_queued_messages(); ! ui_delay(10L, FALSE); ! } ! mch_char_avail(); ! parse_queued_messages(); ! } ! else ! { ! mch_char_avail(); ! parse_queued_messages(); ! /* Wait for 10 msec for any channel I/O. */ ! /* TODO: use delay from optional argument */ ! ui_delay(10L, TRUE); ! mch_char_avail(); ! ! /* Flushing messages on channels is hopefully sufficient. ! * TODO: is there a better way? */ ! parse_queued_messages(); ! } } # ifdef WIN3264 *************** *** 2209,2220 **** * Return OK or FAIL. */ static int ! term_and_job_init(term_T *term, int rows, int cols, char_u *cmd) { WCHAR *p; channel_T *channel = NULL; job_T *job = NULL; - jobopt_T opt; DWORD error; HANDLE jo = NULL, child_process_handle, child_thread_handle; void *winpty_err; --- 2256,2266 ---- * Return OK or FAIL. */ static int ! term_and_job_init(term_T *term, int rows, int cols, char_u *cmd, jobopt_T *opt) { WCHAR *p; channel_T *channel = NULL; job_T *job = NULL; DWORD error; HANDLE jo = NULL, child_process_handle, child_thread_handle; void *winpty_err; *************** *** 2298,2305 **** create_vterm(term, rows, cols); ! setup_job_options(&opt, rows, cols); ! channel_set_job(channel, job, &opt); job->jv_channel = channel; job->jv_proc_info.hProcess = child_process_handle; --- 2344,2350 ---- create_vterm(term, rows, cols); ! channel_set_job(channel, job, opt); job->jv_channel = channel; job->jv_proc_info.hProcess = child_process_handle; *************** *** 2381,2398 **** * Return OK or FAIL. */ static int ! term_and_job_init(term_T *term, int rows, int cols, char_u *cmd) { typval_T argvars[2]; - jobopt_T opt; create_vterm(term, rows, cols); /* TODO: if the command is "NONE" only create a pty. */ argvars[0].v_type = VAR_STRING; argvars[0].vval.v_string = cmd; ! setup_job_options(&opt, rows, cols); ! term->tl_job = job_start(argvars, &opt); if (term->tl_job != NULL) ++term->tl_job->jv_refcount; --- 2426,2442 ---- * Return OK or FAIL. */ static int ! term_and_job_init(term_T *term, int rows, int cols, char_u *cmd, jobopt_T *opt) { typval_T argvars[2]; create_vterm(term, rows, cols); /* TODO: if the command is "NONE" only create a pty. */ argvars[0].v_type = VAR_STRING; argvars[0].vval.v_string = cmd; ! ! term->tl_job = job_start(argvars, opt); if (term->tl_job != NULL) ++term->tl_job->jv_refcount; *** ../vim-8.0.0848/src/testdir/test_terminal.vim 2017-08-03 13:51:02.388784785 +0200 --- src/testdir/test_terminal.vim 2017-08-03 16:36:16.435060718 +0200 *************** *** 86,91 **** --- 86,108 ---- unlet g:job endfunc + func! s:Nasty_exit_cb(job, st) + exe g:buf . 'bwipe!' + let g:buf = 0 + endfunc + + func Test_terminal_nasty_cb() + let cmd = Get_cat_cmd() + let g:buf = term_start(cmd, {'exit_cb': function('s:Nasty_exit_cb')}) + let g:job = term_getjob(g:buf) + + call WaitFor('job_status(g:job) == "dead"') + call WaitFor('g:buf == 0') + unlet g:buf + unlet g:job + call delete('Xtext') + endfunc + func Check_123(buf) let l = term_scrape(a:buf, 0) call assert_true(len(l) == 0) *************** *** 113,125 **** call assert_equal('123', l) endfunc ! func Test_terminal_scrape() if has('win32') ! let cmd = 'cmd /c "cls && color 2 && echo 123"' else call writefile(["\[32m123"], 'Xtext') ! let cmd = "cat Xtext" endif let buf = term_start(cmd) let termlist = term_list() --- 130,146 ---- call assert_equal('123', l) endfunc ! func Get_cat_cmd() if has('win32') ! return 'cmd /c "cls && color 2 && echo 123"' else call writefile(["\[32m123"], 'Xtext') ! return "cat Xtext" endif + endfunc + + func Test_terminal_scrape() + let cmd = Get_cat_cmd() let buf = term_start(cmd) let termlist = term_list() *** ../vim-8.0.0848/src/channel.c 2017-08-03 14:49:22.614906252 +0200 --- src/channel.c 2017-08-03 15:49:26.096113512 +0200 *************** *** 4160,4166 **** hashitem_T *hi; ch_part_T part; - opt->jo_set = 0; if (tv->v_type == VAR_UNKNOWN) return OK; if (tv->v_type != VAR_DICT) --- 4160,4165 ---- *************** *** 4616,4621 **** --- 4615,4621 ---- int dummy; /* Invoke the exit callback. Make sure the refcount is > 0. */ + ch_log(job->jv_channel, "Invoking exit callback %s", job->jv_exit_cb); ++job->jv_refcount; argv[0].v_type = VAR_JOB; argv[0].vval.v_job = job; *************** *** 4888,4894 **** if (get_job_options(&argvars[1], &opt, JO_MODE_ALL + JO_CB_ALL + JO_TIMEOUT_ALL + JO_STOPONEXIT + JO_EXIT_CB + JO_OUT_IO + JO_BLOCK_WRITE) == FAIL) ! goto theend; } /* Check that when io is "file" that there is a file name. */ --- 4888,4894 ---- if (get_job_options(&argvars[1], &opt, JO_MODE_ALL + JO_CB_ALL + JO_TIMEOUT_ALL + JO_STOPONEXIT + JO_EXIT_CB + JO_OUT_IO + JO_BLOCK_WRITE) == FAIL) ! goto theend; } /* Check that when io is "file" that there is a file name. */ *** ../vim-8.0.0848/src/version.c 2017-08-03 14:49:22.618906222 +0200 --- src/version.c 2017-08-03 14:57:41.315216159 +0200 *************** *** 771,772 **** --- 771,774 ---- { /* Add new patch number below this line */ + /**/ + 849, /**/ -- Courtroom Quote #19: Q: Doctor, how many autopsies have you performed on dead people? A: All my autopsies have been performed on dead people. /// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ an exciting new programming language -- http://www.Zimbu.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///