From 6617f85b76fbd8300683bdf53c5cb5490662b6dd Mon Sep 17 00:00:00 2001 From: Sean Dewar <6256228+seandewar@users.noreply.github.com> Date: Tue, 24 Mar 2026 18:25:03 +0000 Subject: [PATCH] vim-patch:9.2.0253: various issues with wrong b_nwindows after closing buffers Problem: close_buffer() callers incorrectly handle b_nwindows, especially after nasty autocmds, allowing it to go out-of-sync. May lead to buffers that can't be unloaded, or buffers that are prematurely freed whilst displayed. Solution: Modify close_buffer() and review its callers; let them decrement b_nwindows if it didn't unload the buffer. Remove some now unneeded workarounds like 8.2.2354, 9.1.0143, 9.1.0764, which didn't always work (Sean Dewar) (endless yapping omitted) related: vim/vim#19728 https://github.com/vim/vim/commit/bf21df1c7bc772e3a29961c961d0821584d50ee0 b_nwindows = 0 change for free_all_mem() was already ported. Originally Nvim returned true when b_nwindows was decremented before the end was reached (to better indicate the decrement). That's not needed anymore, so just return true only at the end, like Vim. (retval isn't used anywhere now anyways) Set textlock for dict watchers at the end of close_buffer() to prevent them from switching windows, as that can leave a window with a NULL buffer. (possible before this PR, but the new assert catches it; added a test) Despite textlock, things still aren't ideal, as watchers may observe the buffer as unloaded and hidden (b_nwindows was decremented), yet still in a window... Likewise, for Nvim, wipe_qf_buffer()'s comment may not be entirely accurate; autocmds are blocked, but on_detach callbacks (textlocked) and dict watchers may still run. Might be problematic, but those aren't new issues. Co-authored-by: Sean Dewar <6256228+seandewar@users.noreply.github.com> --- src/nvim/buffer.c | 163 ++++++++---------- src/nvim/ex_cmds.c | 47 +++-- src/nvim/ex_getln.c | 4 +- src/nvim/memory.c | 2 +- src/nvim/quickfix.c | 14 +- src/nvim/window.c | 28 +-- .../ex_cmds/dict_notifications_spec.lua | 9 + test/old/testdir/test_autocmd.vim | 133 +++++++++++++- test/old/testdir/test_visual.vim | 20 +++ test/unit/buffer_spec.lua | 42 ++--- 10 files changed, 294 insertions(+), 168 deletions(-) diff --git a/src/nvim/buffer.c b/src/nvim/buffer.c index 641a2a7554..80ded39984 100644 --- a/src/nvim/buffer.c +++ b/src/nvim/buffer.c @@ -262,7 +262,7 @@ int open_buffer(bool read_stdin, exarg_T *eap, int flags_arg) if (ml_open(curbuf) == FAIL) { // There MUST be a memfile, otherwise we can't do anything // If we can't create one for the current buffer, take another buffer - close_buffer(NULL, curbuf, 0, false, false); + close_buffer(curwin, curbuf, 0, false, false, false); curbuf = NULL; FOR_ALL_BUFFERS(buf) { @@ -526,7 +526,12 @@ void buf_close_terminal(buf_T *buf) /// Close the link to a buffer. /// -/// @param win If not NULL, set b_last_cursor. +/// @param win If not NULL and we reached the end and unloaded "buf", b_nwindows is decremented +/// if w_buffer was "buf" after autocmds. w_buffer is set to NULL in that case. +/// Otherwise, w_buffer->b_nwindows is not decremented; callers should decrement it if +/// they still intend to switch "win"'s buffer or close "win"! If "win" was initially +/// curwin displaying "buf", it is re-entered if autocmds switched windows, if still +/// open. /// @param buf /// @param action Used when there is no longer a window for the buffer. /// Possible values: @@ -534,8 +539,7 @@ void buf_close_terminal(buf_T *buf) /// DOBUF_UNLOAD buffer is unloaded /// DOBUF_DEL buffer is unloaded and removed from buffer list /// DOBUF_WIPE buffer is unloaded and really deleted -/// When doing all but the first one on the current buffer, the -/// caller should get a new buffer very soon! +/// When doing all but the first, the caller should get a new buffer very soon! /// The 'bufhidden' option can force freeing and deleting. /// @param abort_if_last /// If true, do not close the buffer if autocommands cause @@ -544,15 +548,19 @@ void buf_close_terminal(buf_T *buf) /// close all other windows. /// @param ignore_abort /// If true, don't abort even when aborting() returns true. -/// @return true if b_nwindows was decremented directly by this call (e.g: not via autocmds). -bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool ignore_abort) +/// @param set_context +/// If true, also call buflist_setfpos for "win" if it's showing "buf", and set +/// b_last_cursor if "win" is the buffer's only window. +/// @return true when we got to the end and unloaded "buf". +bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool ignore_abort, + bool set_context) + FUNC_ATTR_NONNULL_ARG(2) { bool unload_buf = (action != 0); bool del_buf = (action == DOBUF_DEL || action == DOBUF_WIPE); bool wipe_buf = (action == DOBUF_WIPE); - bool is_curwin = (curwin != NULL && curwin->w_buffer == buf); - win_T *the_curwin = curwin; + bool is_curwin = (curwin != NULL && curwin == win && curwin->w_buffer == buf); tabpage_T *the_curtab = curtab; CHECK_CURBUF; @@ -587,9 +595,8 @@ bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool i return false; } - // check no autocommands closed the window - if (win != NULL // Avoid bogus clang warning. - && win_valid_any_tab(win)) { + bool win_valid = win_valid_any_tab(win); + if (set_context && win_valid && win->w_buffer == buf) { // Set b_last_cursor when closing the last window for the buffer. // Remember the last cursor position and window options of the buffer. // This used to be only for the current window, but then options like @@ -606,7 +613,7 @@ bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool i set_bufref(&bufref, buf); // When the buffer is no longer in a window, trigger BufWinLeave - if (buf->b_nwindows == 1) { + if (win_valid && win->w_buffer == buf && buf->b_nwindows == 1) { buf->b_locked++; buf->b_locked_split++; if (apply_autocmds(EVENT_BUFWINLEAVE, buf->b_fname, buf->b_fname, false, @@ -646,32 +653,29 @@ bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool i if (!ignore_abort && aborting()) { return false; } + win_valid = win_valid && win_valid_any_tab(win); } // If the buffer was in curwin and the window has changed, go back to that // window, if it still exists. This avoids that ":edit x" triggering a // "tabnext" BufUnload autocmd leaves a window behind without a buffer. - if (is_curwin && curwin != the_curwin && win_valid_any_tab(the_curwin)) { + if (is_curwin && curwin != win && win_valid) { block_autocmds(); - goto_tabpage_win(the_curtab, the_curwin); + goto_tabpage_win(the_curtab, win); unblock_autocmds(); } - int nwindows = buf->b_nwindows; + // Remember if the buffer may be hidden soon, or is already hidden. + bool hiding_buf = buf->b_nwindows <= 0 + || (win_valid && win->w_buffer == buf && buf->b_nwindows == 1); - // decrease the link count from windows (unless not in any window) - if (buf->b_nwindows > 0) { - buf->b_nwindows--; - } - - if (diffopt_hiddenoff() && !unload_buf && buf->b_nwindows == 0) { + if (diffopt_hiddenoff() && !unload_buf && hiding_buf) { diff_buf_delete(buf); // Clear 'diff' for hidden buffer. } - // Return when a window is displaying the buffer or when it's not - // unloaded. - if (buf->b_nwindows > 0 || !unload_buf) { - return true; + // Return when another window is displaying the buffer or when not unloaded. + if (!hiding_buf || !unload_buf) { + return false; } // Always remove the buffer when there is no file name. @@ -681,52 +685,25 @@ bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool i // Free all things allocated for this buffer. // Also calls the "BufDelete" autocommands when del_buf is true. - // Remember if we are closing the current buffer. Restore the number of - // windows, so that autocommands in buf_freeall() don't get confused. - bool is_curbuf = (buf == curbuf); - - buf->b_nwindows = nwindows; - - buf_freeall(buf, ((del_buf ? BFA_DEL : 0) - + (wipe_buf ? BFA_WIPE : 0) - + (ignore_abort ? BFA_IGNORE_ABORT : 0))); - - if (!bufref_valid(&bufref)) { - // Autocommands may have deleted the buffer. - return false; - } - // autocmds may abort script processing. - if (!ignore_abort && aborting()) { - return false; - } - - // It's possible that autocommands change curbuf to the one being deleted. - // This might cause the previous curbuf to be deleted unexpectedly. But - // in some cases it's OK to delete the curbuf, because a new one is - // obtained anyway. Therefore only return if curbuf changed to the - // deleted buffer. - if (buf == curbuf && !is_curbuf) { + // Abort if nothing was freed, or if autocommands delete the buffer. + if (!buf_freeall(buf, ((del_buf ? BFA_DEL : 0) + + (wipe_buf ? BFA_WIPE : 0) + + (ignore_abort ? BFA_IGNORE_ABORT : 0)))) { return false; } bool clear_w_buf = false; - if (win != NULL // Avoid bogus clang warning. - && win_valid_any_tab(win) - && win->w_buffer == buf) { + win_valid = win_valid && win_valid_any_tab(win); + if (win_valid && win->w_buffer == buf) { + // Autocommands may have opened (despite b_locked_split) or closed + // windows for this buffer. Decrement for the close we do here. + buf->b_nwindows--; // Defer clearing w_buffer until after operations that may invoke dict // watchers (e.g., buf_clear_file()), so callers like tabpagebuflist() - // never see a window in the winlist with a NULL buffer. + // never see a window in the window list with a NULL buffer. clear_w_buf = true; } - // Autocommands may have opened or closed windows for this buffer. - // Decrement the count for the close we do here. - // Don't decrement b_nwindows if the buffer wasn't displayed in any window - // before calling buf_freeall(). - if (nwindows > 0 && buf->b_nwindows > 0) { - buf->b_nwindows--; - } - // Remove the buffer from the list. // Do not wipe out the buffer if it is used in a window, or if autocommands // wiped out all other buffers (unless when inside free_all_mem() where all @@ -771,7 +748,10 @@ bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool i // Init the options when loaded again. buf->b_p_initialized = false; } + // Dict watchers set b_locked, but also don't want them messing with windows... + textlock++; buf_clear_file(buf); + textlock--; if (clear_w_buf) { win->w_buffer = NULL; } @@ -780,6 +760,11 @@ bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool i } } // NOTE: at this point "curbuf" may be invalid! + + // When closing curbuf for curwin, is_curwin checks should've ensured + // autocmds don't switch windows, unless they closed curwin. Otherwise + // callers may leave the window open to a NULL buffer! + assert(!win_valid || !is_curwin || win == curwin); return true; } @@ -816,7 +801,9 @@ void buf_clear(void) /// BFA_WIPE buffer is going to be wiped out /// BFA_KEEP_UNDO do not free undo information /// BFA_IGNORE_ABORT don't abort even when aborting() returns true -void buf_freeall(buf_T *buf, int flags) +/// @return true when we got to the end. +bool buf_freeall(buf_T *buf, int flags) + FUNC_ATTR_NONNULL_ALL { bool is_curbuf = (buf == curbuf); int is_curwin = (curwin != NULL && curwin->w_buffer == buf); @@ -840,21 +827,21 @@ void buf_freeall(buf_T *buf, int flags) && apply_autocmds(EVENT_BUFUNLOAD, buf->b_fname, buf->b_fname, false, buf) && !bufref_valid(&bufref)) { // Autocommands deleted the buffer. - return; + return false; } if ((flags & BFA_DEL) && buf->b_p_bl && apply_autocmds(EVENT_BUFDELETE, buf->b_fname, buf->b_fname, false, buf) && !bufref_valid(&bufref)) { // Autocommands may delete the buffer. - return; + return false; } if ((flags & BFA_WIPE) && apply_autocmds(EVENT_BUFWIPEOUT, buf->b_fname, buf->b_fname, false, buf) && !bufref_valid(&bufref)) { // Autocommands may delete the buffer. - return; + return false; } buf->b_locked--; buf->b_locked_split--; @@ -869,7 +856,7 @@ void buf_freeall(buf_T *buf, int flags) } // autocmds may abort script processing if ((flags & BFA_IGNORE_ABORT) == 0 && aborting()) { - return; + return false; } // It's possible that autocommands change curbuf to the one being deleted. @@ -877,7 +864,7 @@ void buf_freeall(buf_T *buf, int flags) // it's OK to delete the curbuf, because a new one is obtained anyway. // Therefore only return if curbuf changed to the deleted buffer. if (buf == curbuf && !is_curbuf) { - return; + return false; } // If curbuf, stop Visual mode just before freeing, but after autocmds that may restart it. @@ -918,6 +905,7 @@ void buf_freeall(buf_T *buf, int flags) } syntax_clear(&buf->b_s); // reset syntax info buf->b_flags &= ~BF_READERR; // a read error is no longer relevant + return true; } /// Free a buffer structure and the things it contains related to the buffer @@ -1077,11 +1065,11 @@ void handle_swap_exists(bufref_T *old_curbuf) // open a new, empty buffer. swap_exists_action = SEA_NONE; // don't want it again swap_exists_did_quit = true; - close_buffer(curwin, curbuf, DOBUF_UNLOAD, false, false); + close_buffer(curwin, curbuf, DOBUF_UNLOAD, false, false, true); if (old_curbuf == NULL || !bufref_valid(old_curbuf) || old_curbuf->br_buf == curbuf) { - // Block autocommands here because curwin->w_buffer is NULL. + // Block autocommands here because curwin->w_buffer may be NULL. block_autocmds(); buf = buflist_new(NULL, NULL, 1, BLN_CURBUF | BLN_LISTED); unblock_autocmds(); @@ -1261,7 +1249,7 @@ static int empty_curbuf(bool close_others, int forceit, int action) // the old one. But do_ecmd() may have done that already, check // if the buffer still exists. if (buf != curbuf && bufref_valid(&bufref) && buf->b_nwindows == 0) { - close_buffer(NULL, buf, action, false, false); + close_buffer(NULL, buf, action, false, false, false); } if (!close_others) { @@ -1466,7 +1454,7 @@ static int do_buffer_ext(int action, int start, int dir, int count, int flags) close_windows(buf, false); if (buf != curbuf && bufref_valid(&bufref) && buf->b_nwindows <= 0) { - close_buffer(NULL, buf, action, false, false); + close_buffer(NULL, buf, action, false, false, false); } return OK; } @@ -1667,7 +1655,6 @@ void set_curbuf(buf_T *buf, int action, bool update_jumplist) int unload = (action == DOBUF_UNLOAD || action == DOBUF_DEL || action == DOBUF_WIPE); OptInt old_tw = curbuf->b_p_tw; - const int last_winid = get_last_winid(); if (update_jumplist) { setpcmark(); @@ -1687,7 +1674,6 @@ void set_curbuf(buf_T *buf, int action, bool update_jumplist) bufref_T prevbufref; set_bufref(&prevbufref, prevbuf); set_bufref(&newbufref, buf); - const int prev_nwindows = prevbuf->b_nwindows; // Autocommands may delete the current buffer and/or the buffer we want to // go to. In those cases don't close the buffer. @@ -1697,33 +1683,23 @@ void set_curbuf(buf_T *buf, int action, bool update_jumplist) if (prevbuf == curwin->w_buffer) { reset_synblock(curwin); } - // autocommands may have opened a new window - // with prevbuf, grr - if (unload - || (prev_nwindows <= 1 && last_winid != get_last_winid() - && action == DOBUF_GOTO && !buf_hide(prevbuf))) { + if (unload) { close_windows(prevbuf, false); } if (bufref_valid(&prevbufref) && !aborting()) { - win_T *previouswin = curwin; - // Do not sync when in Insert mode and the buffer is open in // another window, might be a timer doing something in another // window. if (prevbuf == curbuf && ((State & MODE_INSERT) == 0 || curbuf->b_nwindows <= 1)) { u_sync(false); } - close_buffer(prevbuf == curwin->w_buffer ? curwin : NULL, + close_buffer(curwin, prevbuf, unload ? action : (action == DOBUF_GOTO && !buf_hide(prevbuf) && !bufIsChanged(prevbuf)) ? DOBUF_UNLOAD : 0, - false, false); - if (curwin != previouswin && win_valid(previouswin)) { - // autocommands changed curwin, Grr! - curwin = previouswin; - } + false, false, true); } } // An autocommand may have deleted "buf", already entered it (e.g., when @@ -1731,11 +1707,6 @@ void set_curbuf(buf_T *buf, int action, bool update_jumplist) // If curwin->w_buffer is null, enter_buffer() will make it valid again bool valid = buf_valid(buf); if ((valid && buf != curbuf && !aborting()) || curwin->w_buffer == NULL) { - // autocommands changed curbuf and we will move to another - // buffer soon, so decrement curbuf->b_nwindows - if (curbuf != NULL && prevbuf != curbuf) { - curbuf->b_nwindows--; - } // If the buffer is not valid but curwin->w_buffer is NULL we must // enter some buffer. Using the last one is hopefully OK. enter_buffer(valid ? buf : lastbuf); @@ -1764,6 +1735,10 @@ static void enter_buffer(buf_T *buf) end_visual_mode(); } + if (curwin->w_buffer != NULL) { + curwin->w_buffer->b_nwindows--; + } + // Get the buffer in the current window. curwin->w_buffer = buf; curbuf = buf; @@ -3091,7 +3066,7 @@ int setfname(buf_T *buf, char *ffname_arg, char *sfname_arg, bool message) return FAIL; } // delete from the list - close_buffer(NULL, obuf, DOBUF_WIPE, false, false); + close_buffer(NULL, obuf, DOBUF_WIPE, false, false, false); } sfname = xstrdup(sfname); #ifdef CASE_INSENSITIVE_FILENAME @@ -4189,7 +4164,7 @@ void wipe_buffer(buf_T *buf, bool aucmd) // Don't trigger BufDelete autocommands here. block_autocmds(); } - close_buffer(NULL, buf, DOBUF_WIPE, false, true); + close_buffer(NULL, buf, DOBUF_WIPE, false, true, false); if (!aucmd) { unblock_autocmds(); } diff --git a/src/nvim/ex_cmds.c b/src/nvim/ex_cmds.c index d983f05629..875650eafa 100644 --- a/src/nvim/ex_cmds.c +++ b/src/nvim/ex_cmds.c @@ -2498,12 +2498,6 @@ int do_ecmd(int fnum, char *ffname, char *sfname, exarg_T *eap, linenr_T newlnum // autocommands try to edit a closing buffer, which like splitting, can // result in more windows displaying it; abort if (buf->b_locked_split) { - // window was split, but not editing the new buffer, reset b_nwindows again - if (oldwin == NULL - && curwin->w_buffer != NULL - && curwin->w_buffer->b_nwindows > 1) { - curwin->w_buffer->b_nwindows--; - } emsg(_(e_cannot_switch_to_a_closing_buffer)); goto theend; } @@ -2591,7 +2585,6 @@ int do_ecmd(int fnum, char *ffname, char *sfname, exarg_T *eap, linenr_T newlnum auto_buf = true; } else { win_T *the_curwin = curwin; - buf_T *was_curbuf = curbuf; // Set w_locked to avoid that autocommands close the window. // Set b_locked for the same reason. @@ -2602,15 +2595,14 @@ int do_ecmd(int fnum, char *ffname, char *sfname, exarg_T *eap, linenr_T newlnum buf_copy_options(buf, BCO_ENTER); } - // Close the link to the current buffer. This will set - // oldwin->w_buffer to NULL. + // Close the link to the current buffer. This may set + // curwin->w_buffer to NULL. u_sync(false); - const bool did_decrement - = close_buffer(oldwin, curbuf, - (flags & ECMD_HIDE) - || (curbuf->terminal && terminal_running(curbuf->terminal)) - ? 0 : DOBUF_UNLOAD, - false, false); + close_buffer(curwin, curbuf, + (flags & ECMD_HIDE) + || (curbuf->terminal && terminal_running(curbuf->terminal)) + ? 0 : DOBUF_UNLOAD, + false, false, oldwin != NULL); // Autocommands may have closed the window. if (win_valid(the_curwin)) { @@ -2626,20 +2618,17 @@ int do_ecmd(int fnum, char *ffname, char *sfname, exarg_T *eap, linenr_T newlnum } // Be careful again, like above. if (!bufref_valid(&au_new_curbuf)) { - // New buffer has been deleted. - delbuf_msg(new_name); // Frees new_name. - au_new_curbuf = save_au_new_curbuf; - goto theend; + // New buffer was deleted. If curwin->w_buffer is NULL, we + // must enter some buffer. Hopefully the last one is OK. + if (curwin->w_buffer == NULL) { + buf = lastbuf; + } else { + delbuf_msg(new_name); // Frees new_name. + au_new_curbuf = save_au_new_curbuf; + goto theend; + } } if (buf == curbuf) { // already in new buffer - // close_buffer() has decremented the window count, - // increment it again here and restore w_buffer. - if (did_decrement && buf_valid(was_curbuf)) { - was_curbuf->b_nwindows++; - } - if (win_valid_any_tab(oldwin) && oldwin->w_buffer == NULL) { - oldwin->w_buffer = was_curbuf; - } auto_buf = true; } else { // We could instead free the synblock @@ -2649,6 +2638,10 @@ int do_ecmd(int fnum, char *ffname, char *sfname, exarg_T *eap, linenr_T newlnum curwin->w_s = &(buf->b_s); } + if (curwin->w_buffer != NULL) { + curwin->w_buffer->b_nwindows--; + } + curwin->w_buffer = buf; curbuf = buf; curbuf->b_nwindows++; diff --git a/src/nvim/ex_getln.c b/src/nvim/ex_getln.c index 05d424ceb2..ce61832dd9 100644 --- a/src/nvim/ex_getln.c +++ b/src/nvim/ex_getln.c @@ -4611,7 +4611,7 @@ static int open_cmdwin(void) } // win_close() autocommands may have already deleted the buffer. if (newbuf_status == OK && bufref_valid(&bufref) && bufref.br_buf != curbuf) { - close_buffer(NULL, bufref.br_buf, DOBUF_WIPE, false, false); + close_buffer(NULL, bufref.br_buf, DOBUF_WIPE, false, false, false); } cmdwin_type = 0; @@ -4796,7 +4796,7 @@ static int open_cmdwin(void) // win_close() may have already wiped the buffer when 'bh' is // set to 'wipe', autocommands may have closed other windows if (bufref_valid(&bufref) && bufref.br_buf != curbuf) { - close_buffer(NULL, bufref.br_buf, DOBUF_WIPE, false, false); + close_buffer(NULL, bufref.br_buf, DOBUF_WIPE, false, false, false); } // Restore window sizes. diff --git a/src/nvim/memory.c b/src/nvim/memory.c index 50062122e8..cc222f4259 100644 --- a/src/nvim/memory.c +++ b/src/nvim/memory.c @@ -957,7 +957,7 @@ void free_all_mem(void) // callbacks are called, so free them before closing the buffer. buf_free_callbacks(buf); - close_buffer(NULL, buf, DOBUF_WIPE, false, false); + close_buffer(NULL, buf, DOBUF_WIPE, false, false, false); // Didn't work, try next one. buf = bufref_valid(&bufref) ? nextbuf : firstbuf; } diff --git a/src/nvim/quickfix.c b/src/nvim/quickfix.c index f7eae6f825..fbbe439455 100644 --- a/src/nvim/quickfix.c +++ b/src/nvim/quickfix.c @@ -1777,11 +1777,11 @@ static void wipe_qf_buffer(qf_info_T *qi) buf_T *const qfbuf = buflist_findnr(qi->qf_bufnr); if (qfbuf != NULL && qfbuf->b_nwindows == 0) { bool buf_was_null = false; - // can happen when curwin is going to be closed e.g. curwin->w_buffer - // was already closed in win_close(), and we are now closing the - // window related location list buffer from win_free_mem() - // but close_buffer() calls CHECK_CURBUF() macro and requires - // curwin->w_buffer == curbuf + // Can happen when curwin is closing (e.g: w_buffer was unloaded in + // win_close()) and we are now closing the window-related location list + // buffer from win_free(). close_buffer() calls CHECK_CURBUF() and + // requires curwin->w_buffer == curbuf. Should be OK to not increment + // b_nwindows, especially as autocmds are blocked in win_free(). if (curwin->w_buffer == NULL) { curwin->w_buffer = curbuf; buf_was_null = true; @@ -1789,7 +1789,7 @@ static void wipe_qf_buffer(qf_info_T *qi) // If the quickfix buffer is not loaded in any window, then // wipe the buffer. - close_buffer(NULL, qfbuf, DOBUF_WIPE, false, false); + close_buffer(NULL, qfbuf, DOBUF_WIPE, false, false, false); qi->qf_bufnr = INVALID_QFBUFNR; if (buf_was_null) { curwin->w_buffer = NULL; @@ -6079,7 +6079,7 @@ static void unload_dummy_buffer(buf_T *buf, char *dirname_start) return; } - close_buffer(NULL, buf, DOBUF_UNLOAD, false, true); + close_buffer(NULL, buf, DOBUF_UNLOAD, false, true, false); // When autocommands/'autochdir' option changed directory: go back. restore_start_dir(dirname_start); diff --git a/src/nvim/window.c b/src/nvim/window.c index 4df00525e1..bc19c82d86 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -2757,7 +2757,7 @@ static bool close_last_window_tabpage(win_T *win, bool free_buf, tabpage_T *prev /// "abort_if_last" is passed to close_buffer(): abort closing if all other /// windows are closed. /// -/// @return whether close_buffer() decremented b_nwindows +/// @return @see close_buffer(). static bool win_close_buffer(win_T *win, int action, bool abort_if_last) FUNC_ATTR_NONNULL_ALL { @@ -2779,7 +2779,7 @@ static bool win_close_buffer(win_T *win, int action, bool abort_if_last) bufref_T bufref; set_bufref(&bufref, curbuf); win->w_locked = true; - retval = close_buffer(win, win->w_buffer, action, abort_if_last, true); + retval = close_buffer(win, win->w_buffer, action, abort_if_last, true, true); if (win_valid_any_tab(win)) { win->w_locked = false; } @@ -2798,8 +2798,7 @@ static bool win_close_buffer(win_T *win, int action, bool abort_if_last) /// call this to make the window have a buffer again. /// /// @param bufref reference to win->w_buffer before calling close_buffer() -/// @param did_decrement whether close_buffer() decremented b_nwindows -static void win_unclose_buffer(win_T *win, bufref_T *bufref, bool did_decrement) +static void win_unclose_buffer(win_T *win, bufref_T *bufref) { if (win->w_buffer == NULL) { // If the buffer was removed from the window we have to give it any buffer. @@ -2809,10 +2808,6 @@ static void win_unclose_buffer(win_T *win, bufref_T *bufref, bool did_decrement) curbuf = curwin->w_buffer; } win_init_empty(win); - } else if (did_decrement && win->w_buffer == bufref->br_buf && bufref_valid(bufref)) { - // close_buffer() decremented the window count, but we're keeping the window. - // As the window is still viewing the buffer, increment the count. - win->w_buffer->b_nwindows++; } } @@ -2950,7 +2945,7 @@ int win_close(win_T *win, bool free_buf, bool force) bufref_T bufref; set_bufref(&bufref, win->w_buffer); - bool did_decrement = win_close_buffer(win, free_buf ? DOBUF_UNLOAD : 0, true); + win_close_buffer(win, free_buf ? DOBUF_UNLOAD : 0, true); if (win_valid(win) && win->w_buffer == NULL && !win->w_floating && last_window(win)) { @@ -2978,7 +2973,7 @@ int win_close(win_T *win, bool free_buf, bool force) if (first_tabpage->tp_next != NULL) { emsg(e_floatonly); } - win_unclose_buffer(win, &bufref, did_decrement); + win_unclose_buffer(win, &bufref); return FAIL; } if (close_last_window_tabpage(win, free_buf, prev_curtab)) { @@ -3012,6 +3007,10 @@ int win_close(win_T *win, bool free_buf, bool force) // which may have changed since the last set_bufref. (e.g: close_buffer autocmds) set_bufref(&bufref, win->w_buffer); + if (win->w_buffer != NULL) { + win->w_buffer->b_nwindows--; + } + // Free the memory used for the window and get the window that received // the screen space. int dir; @@ -3189,7 +3188,6 @@ bool win_close_othertab(win_T *win, int free_buf, tabpage_T *tp, bool force) FUNC_ATTR_NONNULL_ALL { assert(tp != curtab); - bool did_decrement = false; // Commands that may call win_close_othertab() already check this, but // check here again just in case. @@ -3251,7 +3249,7 @@ bool win_close_othertab(win_T *win, int free_buf, tabpage_T *tp, bool force) if (win->w_buffer != NULL) { // Close the link to the buffer. - did_decrement = close_buffer(win, win->w_buffer, free_buf ? DOBUF_UNLOAD : 0, false, true); + close_buffer(win, win->w_buffer, free_buf ? DOBUF_UNLOAD : 0, false, true, true); } // Careful: Autocommands may have closed the tab page or made it the @@ -3302,6 +3300,10 @@ bool win_close_othertab(win_T *win, int free_buf, tabpage_T *tp, bool force) // which may have changed since the last set_bufref. (e.g: close_buffer autocmds) set_bufref(&bufref, win->w_buffer); + if (win->w_buffer != NULL) { + win->w_buffer->b_nwindows--; + } + // Free the memory used for the window. int dir; win_free_mem(win, &dir, tp); @@ -3323,7 +3325,7 @@ bool win_close_othertab(win_T *win, int free_buf, tabpage_T *tp, bool force) leave_open: if (win_valid_any_tab(win)) { - win_unclose_buffer(win, &bufref, did_decrement); + win_unclose_buffer(win, &bufref); } return false; } diff --git a/test/functional/ex_cmds/dict_notifications_spec.lua b/test/functional/ex_cmds/dict_notifications_spec.lua index 30d066a058..076b9a6542 100644 --- a/test/functional/ex_cmds/dict_notifications_spec.lua +++ b/test/functional/ex_cmds/dict_notifications_spec.lua @@ -6,10 +6,12 @@ local clear, source = n.clear, n.source local api = n.api local insert = n.insert local eq, next_msg = t.eq, n.next_msg +local matches = t.matches local exc_exec = n.exc_exec local exec_lua = n.exec_lua local command = n.command local eval = n.eval +local pcall_err = t.pcall_err describe('Vimscript dictionary notifications', function() local channel @@ -433,6 +435,13 @@ describe('Vimscript dictionary notifications', function() insert('t') eq('E937: Attempt to delete a buffer that is in use: [No Name]', api.nvim_get_vvar('errmsg')) assert_alive() + + command([[enew | set modified | call dictwatcheradd(b:, 'changedtick', {-> execute('split')})]]) + -- Used to instead leave a window open to a NULL buffer. + matches( + 'E565: Not allowed to change text or change window: split$', + pcall_err(command, 'bdelete!') + ) end) it('does not cause use-after-free when unletting from callback', function() diff --git a/test/old/testdir/test_autocmd.vim b/test/old/testdir/test_autocmd.vim index c6bc608204..2c4808434b 100644 --- a/test/old/testdir/test_autocmd.vim +++ b/test/old/testdir/test_autocmd.vim @@ -4318,10 +4318,11 @@ func Test_autocmd_creates_new_window_on_bufleave() setlocal bufhidden=wipe autocmd BufLeave diffsplit c.txt bn - call assert_equal(1, winnr('$')) + " curbuf set for the new split opened for c.txt, due to BufLeave + call assert_equal(2, winnr('$')) call assert_equal('a.txt', bufname('%')) - bw a.txt - bw c.txt + call assert_equal('b.txt', bufname('#')) + %bw! endfunc " Ensure `expected` was just recently written as a Vim session @@ -4620,6 +4621,32 @@ func Test_autocmd_BufWinLeave_with_vsp() exe "bw! " .. dummy endfunc +func Test_autocmd_BufWinLeave_with_vsp2() + edit Xfoo + split Xbar + split + let s:fired = 0 + augroup testing + autocmd! + autocmd BufWinLeave Xfoo ++once ++nested + \ execute 'autocmd WinEnter * ++once let s:fired = 1' + \ .. '| call assert_equal(3, win_findbuf(bufnr(''Xbar''))->len())' + \ .. '| quit' + \| call assert_fails('vsplit Xfoo', 'E1546:') + augroup END + bw Xfoo + call assert_equal(1, s:fired) + " After 9.1.0764, Xbar's b_nwindows would be 0 if autocmds closed the new + " split before E1546, causing it to be unloaded despite being in a window. + call assert_equal(0, bufexists('Xfoo')) + call assert_equal(1, win_findbuf(bufnr('Xbar'))->len()) + call assert_equal(1, bufloaded('Xbar')) + + call CleanUpTestAuGroup() + unlet! s:fired + %bw! +endfunc + func Test_OptionSet_cmdheight() set mouse=a laststatus=2 au OptionSet cmdheight :let &l:ch = v:option_new @@ -5203,4 +5230,104 @@ func Test_win_tabclose_autocmd() bw! endfunc +func Test_buffer_b_nwindows() + " In these cases, b_nwindows of the Xbars was 1 despite being in no windows. + " Would cause weird failures in other tests, as they would be un-deletable. + edit Xfoo1 + augroup testing + autocmd! + autocmd BufUnload * ++once edit Xbar1 + augroup END + bdelete + call assert_equal([], win_findbuf(bufnr('Xfoo1'))) + call assert_equal([], win_findbuf(bufnr('Xbar1'))) + call assert_equal(1, bufexists('Xfoo1')) + call assert_equal(1, bufexists('Xbar1')) + %bw! + call assert_equal(0, bufexists('Xfoo1')) + call assert_equal(0, bufexists('Xbar1')) + + split Xbar2 + enew + augroup testing + autocmd! + autocmd BufWinLeave * ++once buffer Xbar2 + augroup END + quit + call assert_equal([], win_findbuf(bufnr('Xbar2'))) + call assert_equal(1, bufexists('Xbar2')) + %bw! + call assert_equal(0, bufexists('Xbar2')) + + edit Xbar3 + enew + setlocal bufhidden=hide + let s:win = win_getid() + tabnew + augroup testing + autocmd! + autocmd BufHidden * ++once call win_execute(s:win, 'buffer Xbar3') + augroup END + tabonly + call assert_equal([], win_findbuf(bufnr('Xbar3'))) + call assert_equal(1, bufexists('Xbar3')) + %bw! + call assert_equal(0, bufexists('Xbar3')) + unlet! s:win + + edit Xbar4 + split Xfoo4 + augroup testing + autocmd! + autocmd BufWinLeave * ++once call assert_equal('Xfoo4', bufname()) + \| edit Xbar4 + augroup END + edit Xbar4 + call assert_equal(0, bufloaded('Xfoo4')) + call assert_equal(1, bufexists('Xfoo4')) + " After 8.2.2354, Xfoo4 wrongly had b_nwindows of 1, so couldn't be wiped. + call assert_equal([], win_findbuf('Xfoo4')) + %bw! + call assert_equal(0, bufexists('Xfoo4')) + + call CleanUpTestAuGroup() + %bw! +endfunc + +" Test that an autocmd triggered by v:swapchoice == 'q' that switches buffers +" doesn't cause b_nwindows to be wrong. +func Test_SwapExists_b_nwindows() + let lines =<< trim END + set nocompatible directory=. + + let g:buf = bufnr() + new + + func SwapExists() + let v:swapchoice = 'q' + autocmd BufWinLeave * ++nested ++once buffer Xfoo + endfunc + + func SafeState() + edit Xfoo + edit