fix(api): on_detach consistently before buf_freeall autocmds (#35369)

Problem: on_detach may be called after buf_freeall and other important things,
plus its textlock restrictions are insufficient. This can cause issues such as
leaks, internal errors and crashes.

Solution: disable buffer updates in buf_freeall, before autocommands (like the
order after #35355 and when do_ecmd reloads a buffer). Don't do so in
free_buffer_stuff; it's not safe to run user code there, and buf_freeall already
runs before then; just free them to avoid leaks if buf_freeall autocommands
registered more for some reason.

(cherry picked from commit 2211953266)

Co-authored-by: Sean Dewar <6256228+seandewar@users.noreply.github.com>

src/nvim/buffer.c

@@ -657,10 +657,6 @@ bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool i
 
   buf->b_nwindows = nwindows;
 
-  // Disable buffer-updates for the current buffer.
-  // No need to check `unload_buf`: in that case the function returned above.
-  buf_updates_unload(buf, false);
-
   buf_freeall(buf, ((del_buf ? BFA_DEL : 0)
                     + (wipe_buf ? BFA_WIPE : 0)
                     + (ignore_abort ? BFA_IGNORE_ABORT : 0)));
@@ -789,6 +785,11 @@ void buf_freeall(buf_T *buf, int flags)
   bufref_T bufref;
   set_bufref(&bufref, buf);
 
+  buf_updates_unload(buf, false);
+  if (!bufref_valid(&bufref)) {
+    // on_detach callback deleted the buffer.
+    return;
+  }
   if ((buf->b_ml.ml_mfp != NULL)
       && apply_autocmds(EVENT_BUFUNLOAD, buf->b_fname, buf->b_fname, false, buf)
       && !bufref_valid(&bufref)) {
@@ -935,7 +936,7 @@ static void free_buffer_stuff(buf_T *buf, int free_flags)
   map_clear_mode(buf, MAP_ALL_MODES, true, true);   // clear local abbrevs
   XFREE_CLEAR(buf->b_start_fenc);
 
-  buf_updates_unload(buf, false);
+  buf_free_callbacks(buf);
 }
 
 /// Go to another buffer.  Handles the result of the ATTENTION dialog.

src/nvim/ex_cmds.c

@@ -2512,13 +2512,11 @@ int do_ecmd(int fnum, char *ffname, char *sfname, exarg_T *eap, linenr_T newlnum
         goto theend;
       }
       u_unchanged(curbuf);
-      buf_updates_unload(curbuf, false);
       buf_freeall(curbuf, BFA_KEEP_UNDO);
 
       // Tell readfile() not to clear or reload undo info.
       readfile_flags = READ_KEEP_UNDO;
     } else {
-      buf_updates_unload(curbuf, false);
       buf_freeall(curbuf, 0);  // Free all things for buffer.
     }
     // If autocommands deleted the buffer we were going to re-edit, give

test/functional/lua/buffer_updates_spec.lua

@@ -14,6 +14,8 @@ local feed = n.feed
 local expect_events = t.expect_events
 local write_file = t.write_file
 local dedent = t.dedent
+local matches = t.matches
+local pcall_err = t.pcall_err
 
 local origlines = {
   'original line 1',
@@ -1386,42 +1388,138 @@ describe('lua: nvim_buf_attach on_bytes', function()
 end)
 
 describe('nvim_buf_attach on_detach', function()
-  it('is invoked before unloading buffer', function()
+  it('called before buf_freeall autocommands', function()
     exec_lua(function()
-      _G.logs = {} ---@type table<integer, string[]>
+      vim.api.nvim_create_autocmd({ 'BufUnload', 'BufDelete', 'BufWipeout' }, {
+        callback = function(args)
+          table.insert(
+            _G.events,
+            ('%s: %d %s'):format(
+              args.event,
+              args.buf,
+              tostring(vim.api.nvim_buf_is_loaded(args.buf))
+            )
+          )
+        end,
+      })
+      function _G.on_detach(_, b)
+        table.insert(
+          _G.events,
+          ('on_detach: %d %s'):format(b, tostring(vim.api.nvim_buf_is_loaded(b)))
+        )
+      end
+      _G.events = {}
+      vim.cmd 'new'
+      vim.bo.bufhidden = 'wipe'
+      vim.api.nvim_buf_attach(0, false, { on_detach = _G.on_detach })
+      vim.cmd 'quit!'
     end)
-    local function setup(bufnr)
+
-      exec_lua(function()
+    eq(
-        _G.logs[bufnr] = {}
+      { 'on_detach: 2 true', 'BufUnload: 2 true', 'BufDelete: 2 true', 'BufWipeout: 2 true' },
-        vim.api.nvim_create_autocmd({ 'BufUnload', 'BufWipeout' }, {
+      exec_lua('return _G.events')
-          buffer = bufnr,
+    )
-          callback = function(ev)
+    eq(false, api.nvim_buf_is_valid(2))
-            table.insert(_G.logs[bufnr], ev.event)
+
-          end,
+    exec_lua(function()
-        })
+      _G.events = {}
-        vim.api.nvim_buf_attach(bufnr, false, {
+      local buf = vim.api.nvim_create_buf(false, true)
-          on_detach = function()
+      vim.api.nvim_buf_attach(buf, false, { on_detach = _G.on_detach })
-            table.insert(_G.logs[bufnr], 'on_detach')
+      vim.api.nvim_buf_delete(buf, { force = true })
-          end,
+    end)
-        })
+
-      end)
+    -- Was unlisted, so no BufDelete.
-    end
+    eq(
-    -- Test with two buffers because the :bw works differently for the last buffer.
+      { 'on_detach: 3 true', 'BufUnload: 3 true', 'BufWipeout: 3 true' },
-    -- Before #35355, the order was as follows:
+      exec_lua('return _G.events')
-    -- * non-last buffers: BufUnload → BufWipeout → on_detach
+    )
-    -- * the last buffer (with text): BufUnload → on_detach → BufWipeout
+    eq(false, api.nvim_buf_is_valid(3))
-    local buf1 = api.nvim_get_current_buf()
+
-    local buf2 = api.nvim_create_buf(true, false)
+    exec_lua(function()
-    api.nvim_open_win(buf2, false, { split = 'below' })
+      _G.events = {}
-    api.nvim_buf_set_lines(buf1, 0, -1, true, { 'abc' })
+      vim.api.nvim_buf_attach(1, false, { on_detach = _G.on_detach })
-    api.nvim_buf_set_lines(buf2, 0, -1, true, { 'abc' })
+      vim.api.nvim_create_autocmd('BufUnload', {
-    setup(buf1)
+        buffer = 1,
-    setup(buf2)
+        once = true,
-    api.nvim_buf_delete(buf1, { force = true })
+        callback = function()
-    api.nvim_buf_delete(buf2, { force = true })
+          vim.api.nvim_buf_attach(1, false, {
-    local logs = exec_lua('return _G.logs')
+            on_detach = function(...)
-    local order = { 'on_detach', 'BufUnload', 'BufWipeout' }
+              vim.fn.bufload(1) -- Leaks the memfile it were to run inside free_buffer_stuff.
-    eq(order, logs[buf1])
+              return _G.on_detach(...)
-    eq(order, logs[buf2])
+            end,
+          })
+          table.insert(_G.events, 'local BufUnload')
+        end,
+      })
+      vim.cmd 'edit asdf' -- Reuses buffer 1.
+    end)
+
+    -- on_detach shouldn't run after autocommands when reusing a buffer (in free_buffer_stuff), even
+    -- if those autocommands registered it, as curbuf may be in a semi-unloaded state at that point.
+    eq({
+      'on_detach: 1 true',
+      'BufUnload: 1 true',
+      'local BufUnload',
+      'BufDelete: 1 true',
+      'BufWipeout: 1 true',
+    }, exec_lua('return _G.events'))
+
+    exec_lua(function()
+      _G.events = {}
+      vim.api.nvim_buf_attach(0, false, { on_detach = _G.on_detach })
+      vim.cmd 'edit'
+    end)
+
+    -- Re-edit buffer; on_detach is called.
+    eq({ 'on_detach: 1 true', 'BufUnload: 1 true' }, exec_lua('return _G.events'))
+    eq(true, api.nvim_buf_is_valid(1))
+
+    exec_lua(function()
+      vim.cmd '%bwipeout!'
+      vim.bo.modified = true
+      _G.events = {}
+      vim.api.nvim_buf_attach(0, false, { on_detach = _G.on_detach })
+      vim.api.nvim_buf_delete(0, { force = true })
+    end)
+
+    -- on_detach must still be first when wiping the last buffer if it's listed and non-reusable.
+    -- Previously: BufUnload → BufDelete → on_detach → BufWipeout.
+    eq(
+      { 'on_detach: 4 true', 'BufUnload: 4 true', 'BufDelete: 4 true', 'BufWipeout: 4 false' },
+      exec_lua('return _G.events')
+    )
+  end)
+
+  it('disallows splitting', function()
+    command('new | setlocal bufhidden=wipe')
+    local buf = api.nvim_get_current_buf()
+    exec_lua(function()
+      vim.api.nvim_buf_attach(0, false, {
+        on_detach = function()
+          -- Used to allow opening more views into a closing buffer, resulting in open windows to an
+          -- unloaded buffer.
+          vim.cmd [=[execute "normal! \<C-W>s"]=]
+        end,
+      })
+    end)
+    matches('E1159: Cannot split a window when closing the buffer$', pcall_err(command, 'quit!'))
+    eq({}, fn.win_findbuf(buf))
+    eq(false, api.nvim_buf_is_valid(buf))
   end)
 end)
+
+it('nvim_buf_attach from buf_freeall autocommands does not leak', function()
+  exec_lua(function()
+    local b = vim.api.nvim_create_buf(true, true)
+    vim.api.nvim_create_autocmd('BufWipeout', {
+      buffer = b,
+      once = true,
+      callback = function()
+        vim.api.nvim_buf_attach(b, false, {})
+        _G.autocmd_fired = true
+      end,
+    })
+    vim.api.nvim_buf_delete(b, { force = true })
+  end)
+  eq(true, exec_lua('return _G.autocmd_fired'))
+end)