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)

close_buffer() now doesn't decrement b_nwindows when not unloading buf, or when
buf isn't w_buffer after autocmds (they would've already decremented it).
Callers are now expected to decrement b_nwindows if w_buffer is not NULL after
close_buffer(), and when still intending to switch buffers or close win, for two
reasons:
- close_buffer() autocmds may have switched buffers.  The new w_buffer's
  b_nwindows would also need decrementing.
- After close_buffer(), callers may opt to not switch w_buffer or close win.
  b_nwindows would need to be incremented again.  (unless w_buffer is NULL from
  being unloaded; callers are already forced to find a new buffer then)
These were the main causes of b_nwindows bugs, as these cases could not be
reliably detected, and went largely unhandled.

NOTE: if close_buffer() autocmds switch buffers, close_buffer() is not called
for that new buffer before decrementing b_nwindows.  This may skip side-effects
like from 'bufhidden', but I think it's mostly harmless, and was already
happening in other places.

Let's see how this goes...  Other details: (I have lots to say!)

It's OK to pass a win to close_buffer() that isn't showing buf (used by
set_curbuf()).  In that case, we skip some side-effects and don't decrement
b_nwindows, but may still unload buf if hidden.

buf_freeall() now returns whether it freed anything.  Removes some repeated
checks in close_buffer().

Preserve close_buffer()'s behaviour when called by win_free_popup() after its
popup was already removed from the window list.  This made win_valid_any_tab()
return FALSE, so we skip things that originally checked it in that case.

Add "set_context" to close_buffer() to preserve do_ecmd()'s behaviour of only
setting b_last_cursor and/or calling buflist_setfpos() when not splitting
(see 7.2.041: https://groups.google.com/g/vim_dev/c/ZGgNvaylNzI/m/WHxjhnuxqB0J)
Without this, Test_marks_cmd() fails from its ' mark differing.  Don't use
oldwin though; it's not always the window with the closed buf, especially
after BufLeave autocmds in do_ecmd().  Also, only set context if win is really
displaying buf.

Don't bail in do_ecmd() if buf was deleted but curwin->w_buffer is NULL; that
leaves curwin open to a NULL buffer!  Use lastbuf instead, like set_curbuf().
I don't think it's possible for buf to be deleted by close_buffer() anyway, as
b_locked was set (which I can't see a way to bypass, unlike b_locked_split).
Maybe such checks can be removed, but I'd rather not risk that here.

Don't set curwin to previouswin in set_curbuf(); shouldn't be needed, otherwise
may lead to curbuf != curwin->w_buffer if autocmds switch to a window showing
buf, as that skips enter_buffer()?  Was introduced back in 7.3.557 to avoid
cases where autocmds switch windows, possibly leaving previouswin with a NULL
buffer.  Since 7.4.2312 and 7.4.2328, close_buffer() and buf_freeall() already
handles this.  I've added an assert() as a sanity check anyway.

In free_all_mem(), set b_nwindows to 0 before close_buffer() so buffers can be
wiped if still in a window before win_free_all().  Needed as close_buffer() now
skips unloading buffers that aren't hidden if win is NULL.  If it's possible for
free_all_mem()'s :tabonly! and :only! to not close all windows before freeing,
then this issue was also previously possible if b_nwindows > 1.

related: #19728

Signed-off-by: Sean Dewar <6256228+seandewar@users.noreply.github.com>
Signed-off-by: Christian Brabandt <cb@256bit.org>

Author: seandewar

src/alloc.c

@@ -496,7 +496,9 @@ free_all_mem(void)
 
 	set_bufref(&bufref, buf);
 	nextbuf = buf->b_next;
-	close_buffer(NULL, buf, DOBUF_WIPE, FALSE, FALSE);
+	// All windows were freed.  Reset b_nwindows so buffers can be wiped.
+	buf->b_nwindows = 0;
+	close_buffer(NULL, buf, DOBUF_WIPE, FALSE, FALSE, FALSE);
 	if (bufref_valid(&bufref))
 	    buf = nextbuf;	// didn't work, try next one
 	else

src/buffer.c

@@ -227,7 +227,7 @@ open_buffer(
     {
 	// 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);
 	FOR_ALL_BUFFERS(curbuf)
 	    if (curbuf->b_ml.ml_mfp != NULL)
 		break;
@@ -545,36 +545,52 @@ can_unload_buffer(buf_T *buf)
  * DOBUF_DEL		buffer is unloaded and removed from buffer list
  * DOBUF_WIPE		buffer is unloaded and really deleted
  * DOBUF_WIPE_REUSE	idem, and add to buf_reuse list
- * 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.
  *
+ * When "win" is 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.
+ *
  * When "abort_if_last" is TRUE then do not close the buffer if autocommands
  * cause there to be only one window with this buffer.  e.g. when ":quit" is
  * supposed to close the window but autocommands close all other windows.
  *
  * When "ignore_abort" is TRUE don't abort even when aborting() returns TRUE.
  *
- * Return TRUE when we got to the end and b_nwindows was decremented.
+ * When "set_context" is 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.
+ * Not done for closing popups.
+ *
+ * Return TRUE when we got to the end and unloaded "buf".
  */
     int
 close_buffer(
-    win_T	*win,		// if not NULL, set b_last_cursor
+    win_T	*win,
     buf_T	*buf,
     int		action,
     int		abort_if_last,
-    int		ignore_abort)
+    int		ignore_abort,
+    int		set_context)
 {
-    int		is_curbuf;
-    int		nwindows;
+    int		hiding_buf;
     bufref_T	bufref;
-    int		is_curwin = (curwin != NULL && curwin->w_buffer == buf);
-    win_T	*the_curwin = curwin;
+    int		is_curwin = (curwin != NULL && curwin == win
+				&& curwin->w_buffer == buf);
     tabpage_T	*the_curtab = curtab;
     int		unload_buf = (action != 0);
     int		wipe_buf = (action == DOBUF_WIPE || action == DOBUF_WIPE_REUSE);
     int		del_buf = (action == DOBUF_DEL || wipe_buf);
+    int		win_valid = win_valid_any_tab(win);
+    // win_valid is FALSE for closing popups, as they're first removed from the
+    // window list.  If so, skip some side-effects, but treat win as valid.
+    // Shouldn't be possible for autocmds to prematurely free win then, as it's
+    // not in the window list.
+    int		closed_popup = win && !win_valid && WIN_IS_POPUP(win);
 
     CHECK_CURBUF;
 
@@ -649,8 +665,7 @@ close_buffer(
     if ((del_buf || wipe_buf) && !can_unload_buffer(buf))
 	return FALSE;
 
-    // check no autocommands closed the window
-    if (win != NULL && 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.
@@ -666,7 +681,8 @@ close_buffer(
     set_bufref(&bufref, buf);
 
     // When the buffer is no longer in a window, trigger BufWinLeave
-    if (buf->b_nwindows == 1)
+    if ((win_valid || closed_popup) && win->w_buffer == buf
+	    && buf->b_nwindows == 1)
     {
 	++buf->b_locked;
 	++buf->b_locked_split;
@@ -707,32 +723,30 @@ close_buffer(
 	if (!ignore_abort && aborting())
 	    return FALSE;
 #endif
+	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();
     }
 
-    nwindows = buf->b_nwindows;
-
-    // decrease the link count from windows (unless not in any window)
-    if (buf->b_nwindows > 0)
-	--buf->b_nwindows;
+    // Remember if the buffer may be hidden soon, or is already hidden.
+    hiding_buf = buf->b_nwindows <= 0 || ((win_valid || closed_popup)
+	    && win->w_buffer == buf && buf->b_nwindows == 1);
 
 #ifdef FEAT_DIFF
-    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.
 #endif
 
-    // Return when a window is displaying the buffer or when it's not
-    // unloaded.
-    if (buf->b_nwindows > 0 || !unload_buf)
+    // 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.
@@ -741,42 +755,20 @@ close_buffer(
 
     // 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.
-    is_curbuf = (buf == curbuf);
-    buf->b_nwindows = nwindows;
-
-    buf_freeall(buf, (del_buf ? BFA_DEL : 0)
+    // 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));
-
-    // Autocommands may have deleted the buffer.
-    if (!bufref_valid(&bufref))
+		   + (ignore_abort ? BFA_IGNORE_ABORT : 0)))
 	return FALSE;
-#ifdef FEAT_EVAL
-    // autocmds may abort script processing
-    if (!ignore_abort && aborting())
-	return FALSE;
-#endif
-
-    // 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)
-	return FALSE;
-
-    if (win_valid_any_tab(win) && win->w_buffer == buf)
-	win->w_buffer = NULL;  // make sure we don't use the buffer now
 
-    // 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)
+    win_valid = win_valid && win_valid_any_tab(win);
+    if ((win_valid || closed_popup) && 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;
+	win->w_buffer = NULL;  // make sure we don't use the buffer now
+    }
 
     /*
      * Remove the buffer from the list.
@@ -839,6 +831,11 @@ close_buffer(
 	    buf->b_p_bl = FALSE;
     }
     // 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;
 }
 
@@ -872,8 +869,10 @@ buf_clear_file(buf_T *buf)
  * 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
+ *
+ * Return TRUE when we got to the end.
  */
-    void
+    int
 buf_freeall(buf_T *buf, int flags)
 {
     int		is_curbuf = (buf == curbuf);
@@ -892,23 +891,23 @@ buf_freeall(buf_T *buf, int flags)
 								  FALSE, buf)
 		&& !bufref_valid(&bufref))
 	    // autocommands deleted the buffer
-	    return;
+	    return FALSE;
     }
     if ((flags & BFA_DEL) && buf->b_p_bl)
     {
 	if (apply_autocmds(EVENT_BUFDELETE, buf->b_fname, buf->b_fname,
 								   FALSE, buf)
 		&& !bufref_valid(&bufref))
 	    // autocommands deleted the buffer
-	    return;
+	    return FALSE;
     }
     if (flags & BFA_WIPE)
     {
 	if (apply_autocmds(EVENT_BUFWIPEOUT, buf->b_fname, buf->b_fname,
 								  FALSE, buf)
 		&& !bufref_valid(&bufref))
 	    // autocommands deleted the buffer
-	    return;
+	    return FALSE;
     }
     --buf->b_locked;
     --buf->b_locked_split;
@@ -926,15 +925,15 @@ buf_freeall(buf_T *buf, int flags)
 #ifdef FEAT_EVAL
     // autocmds may abort script processing
     if ((flags & BFA_IGNORE_ABORT) == 0 && aborting())
-	return;
+	return FALSE;
 #endif
 
     // It's possible that autocommands change curbuf to the one being deleted.
     // This might cause 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)
-	return;
+	return FALSE;
 
     // If curbuf, stop Visual mode just before freeing, but after autocmds that
     // may restart it.  May trigger TextYankPost, but with textlock set.
@@ -982,6 +981,7 @@ buf_freeall(buf_T *buf, int flags)
     clear_buf_prop_types(buf);
 #endif
     buf->b_flags &= ~BF_READERR;    // a read error is no longer relevant
+    return TRUE;
 }
 
 /*
@@ -1230,11 +1230,11 @@ 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, 1L, BLN_CURBUF | BLN_LISTED);
 	    unblock_autocmds();
@@ -1321,7 +1321,7 @@ empty_curbuf(
     // 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)
 	need_fileinfo = FALSE;
     else if (retval == OK && !shortmess(SHM_FILEINFO))
@@ -1549,7 +1549,7 @@ do_buffer_ext(
 	{
 	    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;
 	}
 
@@ -1882,7 +1882,6 @@ set_curbuf(buf_T *buf, int action)
     bufref_T	newbufref;
     bufref_T	prevbufref;
     int		valid;
-    int		last_winid = get_last_winid();
 
     setpcmark();
     if ((cmdmod.cmod_flags & CMOD_KEEPALT) == 0)
@@ -1896,7 +1895,6 @@ set_curbuf(buf_T *buf, int action)
     prevbuf = curbuf;
     set_bufref(&prevbufref, prevbuf);
     set_bufref(&newbufref, buf);
-    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.
@@ -1912,34 +1910,25 @@ set_curbuf(buf_T *buf, int action)
 	if (prevbuf == curwin->w_buffer)
 	    reset_synblock(curwin);
 #endif
-	// 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 defined(FEAT_EVAL)
 	if (bufref_valid(&prevbufref) && !aborting())
 #else
 	if (bufref_valid(&prevbufref))
 #endif
 	{
-	    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, prevbuf,
+	    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
@@ -1952,10 +1941,6 @@ set_curbuf(buf_T *buf, int action)
 #endif
 	) || 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.
 	if (!valid)
@@ -1987,6 +1972,9 @@ 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;
@@ -3665,7 +3653,7 @@ setfname(
 		return FAIL;
 	    }
 	    // delete from the list
-	    close_buffer(NULL, obuf, DOBUF_WIPE, FALSE, FALSE);
+	    close_buffer(NULL, obuf, DOBUF_WIPE, FALSE, FALSE, FALSE);
 	}
 	sfname = vim_strsave(sfname);
 	if (ffname == NULL || sfname == NULL)
@@ -6438,7 +6426,7 @@ wipe_buffer(
     if (!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();