From 684e93054b82c6b5b215db7d3ecbad803eb81f0e Mon Sep 17 00:00:00 2001 From: zeertzjq Date: Wed, 25 Oct 2023 09:59:02 +0800 Subject: [PATCH] fix(terminal): assign channel to terminal earlier (#25771) --- src/nvim/api/vim.c | 9 ++- src/nvim/channel.c | 3 +- src/nvim/eval/funcs.c | 2 + src/nvim/terminal.c | 29 +++++--- test/functional/autocmd/termxx_spec.lua | 1 - test/functional/terminal/channel_spec.lua | 83 +++++++++++++++++++++++ 6 files changed, 111 insertions(+), 16 deletions(-) diff --git a/src/nvim/api/vim.c b/src/nvim/api/vim.c index 52022cba5d..c3c619e206 100644 --- a/src/nvim/api/vim.c +++ b/src/nvim/api/vim.c @@ -1034,9 +1034,12 @@ Integer nvim_open_term(Buffer buffer, DictionaryOf(LuaRef) opts, Error *err) topts.write_cb = term_write; topts.resize_cb = term_resize; topts.close_cb = term_close; - Terminal *term = terminal_open(buf, topts); - terminal_check_size(term); - chan->term = term; + channel_incref(chan); + terminal_open(&chan->term, buf, topts); + if (chan->term != NULL) { + terminal_check_size(chan->term); + } + channel_decref(chan); return (Integer)chan->id; } diff --git a/src/nvim/channel.c b/src/nvim/channel.c index 32a2f1021c..38c40d2328 100644 --- a/src/nvim/channel.c +++ b/src/nvim/channel.c @@ -786,9 +786,8 @@ void channel_terminal_open(buf_T *buf, Channel *chan) topts.resize_cb = term_resize; topts.close_cb = term_close; buf->b_p_channel = (OptInt)chan->id; // 'channel' option - Terminal *term = terminal_open(buf, topts); - chan->term = term; channel_incref(chan); + terminal_open(&chan->term, buf, topts); } static void term_write(char *buf, size_t size, void *data) diff --git a/src/nvim/eval/funcs.c b/src/nvim/eval/funcs.c index 4759b4ebe3..aae544f28d 100644 --- a/src/nvim/eval/funcs.c +++ b/src/nvim/eval/funcs.c @@ -8637,8 +8637,10 @@ static void f_termopen(typval_T *argvars, typval_T *rettv, EvalFuncData fptr) INTEGER_OBJ(pid), false, false, &err); api_clear_error(&err); + channel_incref(chan); channel_terminal_open(curbuf, chan); channel_create_event(chan, NULL); + channel_decref(chan); } /// "timer_info([timer])" function diff --git a/src/nvim/terminal.c b/src/nvim/terminal.c index b50ab4ddb0..a564738243 100644 --- a/src/nvim/terminal.c +++ b/src/nvim/terminal.c @@ -203,10 +203,11 @@ static void term_output_callback(const char *s, size_t len, void *user_data) /// /// @param buf Buffer used for presentation of the terminal. /// @param opts PTY process channel, various terminal properties and callbacks. -Terminal *terminal_open(buf_T *buf, TerminalOptions opts) +void terminal_open(Terminal **termpp, buf_T *buf, TerminalOptions opts) + FUNC_ATTR_NONNULL_ALL { // Create a new terminal instance and configure it - Terminal *rv = xcalloc(1, sizeof(Terminal)); + Terminal *rv = *termpp = xcalloc(1, sizeof(Terminal)); rv->opts = opts; rv->cursor.visible = true; // Associate the terminal instance with the new buffer @@ -251,18 +252,26 @@ Terminal *terminal_open(buf_T *buf, TerminalOptions opts) RESET_BINDING(curwin); // Reset cursor in current window. curwin->w_cursor = (pos_T){ .lnum = 1, .col = 0, .coladd = 0 }; - // Initialize to check if the scrollback buffer has been allocated inside a TermOpen autocmd + // Initialize to check if the scrollback buffer has been allocated in a TermOpen autocmd. rv->sb_buffer = NULL; // Apply TermOpen autocmds _before_ configuring the scrollback buffer. apply_autocmds(EVENT_TERMOPEN, NULL, NULL, false, buf); - // Local 'scrollback' _after_ autocmds. - buf->b_p_scbk = (buf->b_p_scbk < 1) ? SB_MAX : buf->b_p_scbk; aucmd_restbuf(&aco); - // Configure the scrollback buffer. - rv->sb_size = (size_t)buf->b_p_scbk; - rv->sb_buffer = xmalloc(sizeof(ScrollbackLine *) * rv->sb_size); + if (*termpp == NULL) { + return; // Terminal has already been destroyed. + } + + if (rv->sb_buffer == NULL) { + // Local 'scrollback' _after_ autocmds. + if (buf->b_p_scbk < 1) { + buf->b_p_scbk = SB_MAX; + } + // Configure the scrollback buffer. + rv->sb_size = (size_t)buf->b_p_scbk; + rv->sb_buffer = xmalloc(sizeof(ScrollbackLine *) * rv->sb_size); + } // Configure the color palette. Try to get the color from: // @@ -290,14 +299,13 @@ Terminal *terminal_open(buf_T *buf, TerminalOptions opts) } } } - - return rv; } /// Closes the Terminal buffer. /// /// May call terminal_destroy, which sets caller storage to NULL. void terminal_close(Terminal **termpp, int status) + FUNC_ATTR_NONNULL_ALL { Terminal *term = *termpp; if (term->destroy) { @@ -647,6 +655,7 @@ static int terminal_execute(VimState *state, int key) /// Frees the given Terminal structure and sets the caller storage to NULL (in the spirit of /// XFREE_CLEAR). void terminal_destroy(Terminal **termpp) + FUNC_ATTR_NONNULL_ALL { Terminal *term = *termpp; buf_T *buf = handle_get_buffer(term->buf_handle); diff --git a/test/functional/autocmd/termxx_spec.lua b/test/functional/autocmd/termxx_spec.lua index 2522a28a9d..332a936e3f 100644 --- a/test/functional/autocmd/termxx_spec.lua +++ b/test/functional/autocmd/termxx_spec.lua @@ -22,7 +22,6 @@ describe('autocmd TermClose', function() command('set shellcmdflag=EXE shellredir= shellpipe= shellquote= shellxquote=') end) - local function test_termclose_delete_own_buf() -- The terminal process needs to keep running so that TermClose isn't triggered immediately. nvim('set_option_value', 'shell', string.format('"%s" INTERACT', testprg('shell-test')), {}) diff --git a/test/functional/terminal/channel_spec.lua b/test/functional/terminal/channel_spec.lua index 51bf611860..8510df5347 100644 --- a/test/functional/terminal/channel_spec.lua +++ b/test/functional/terminal/channel_spec.lua @@ -8,6 +8,10 @@ local pcall_err = helpers.pcall_err local feed = helpers.feed local poke_eventloop = helpers.poke_eventloop local is_os = helpers.is_os +local meths = helpers.meths +local async_meths = helpers.async_meths +local testprg = helpers.testprg +local assert_alive = helpers.assert_alive describe('terminal channel is closed and later released if', function() local screen @@ -116,3 +120,82 @@ it('chansend sends lines to terminal channel in proper order', function() } end end) + +describe('no crash when TermOpen autocommand', function() + local screen + + before_each(function() + clear() + meths.set_option_value('shell', testprg('shell-test'), {}) + command('set shellcmdflag=EXE shellredir= shellpipe= shellquote= shellxquote=') + screen = Screen.new(60, 4) + screen:set_default_attr_ids({ + [0] = {bold = true, foreground = Screen.colors.Blue}; + }) + screen:attach() + end) + + it('processes job exit event when using termopen()', function() + command([[autocmd TermOpen * call input('')]]) + async_meths.command('terminal foobar') + screen:expect{grid=[[ + | + {0:~ }| + {0:~ }| + ^ | + ]]} + feed('') + screen:expect{grid=[[ + ^ready $ foobar | + | + [Process exited 0] | + | + ]]} + feed('i') + screen:expect{grid=[[ + ^ | + {0:~ }| + {0:~ }| + | + ]]} + assert_alive() + end) + + it('wipes buffer and processes events when using termopen()', function() + command([[autocmd TermOpen * bwipe! | call input('')]]) + async_meths.command('terminal foobar') + screen:expect{grid=[[ + | + {0:~ }| + {0:~ }| + ^ | + ]]} + feed('') + screen:expect{grid=[[ + ^ | + {0:~ }| + {0:~ }| + | + ]]} + assert_alive() + end) + + it('wipes buffer and processes events when using nvim_open_term()', function() + command([[autocmd TermOpen * bwipe! | call input('')]]) + async_meths.open_term(0, {}) + screen:expect{grid=[[ + | + {0:~ }| + {0:~ }| + ^ | + ]]} + feed('') + screen:expect{grid=[[ + ^ | + {0:~ }| + {0:~ }| + | + ]]} + assert_alive() + end) +end)