fix(terminal): assign channel to terminal earlier (#25771)

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('<CR>')
+    screen:expect{grid=[[
+      ^ready $ foobar                                              |
+                                                                  |
+      [Process exited 0]                                          |
+                                                                  |
+    ]]}
+    feed('i<CR>')
+    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('<CR>')
+    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('<CR>')
+    screen:expect{grid=[[
+      ^                                                            |
+      {0:~                                                           }|
+      {0:~                                                           }|
+                                                                  |
+    ]]}
+    assert_alive()
+  end)
+end)