bug#34847: 27.0.50; auto-revert-buffers occasionally selects a killed buffer

[Michael Albinus <michael.albinus@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 1164 bytes --]

Dale Sedivec <dale@codefu.org> writes:

Hi Dale,

Thanks for the report.

> Lately, while in the process of using Magit, I've frequently gotten
> "Selecting deleting buffer" errors.
>
> I have global-auto-revert-mode on.  I *suspect* this happens because
> *something* kills a buffer between when auto-revert-buffers collects
> the list of buffers with (buffer-list) and when auto-revert-buffers
> starts its final traversal of buffers in bufs (which starts out as
> (buffer-list) in global-auto-revert-mode).
>
> Around line 795 inside auto-revert-buffers, the code is:
>
>           (with-current-buffer buf
>             (if (buffer-live-p buf)
>                 ...
>               ;; Remove dead buffer from `auto-revert-buffer-list'.
>               (auto-revert-remove-current-buffer)))
>
> It seems like reversing this so that with-current-buffer is only
> called after buffer-live-p is checked might be a good solution to
> avoid trying to select a deleted buffer?

Something like this. But the final `auto-revert-remove-current-buffer'
needs the buffer to be removed as the current one. So it is a bit more
complex.

I came up with the following patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff, Size: 4464 bytes --]

*** /tmp/ediffWnT0dx	2019-03-24 15:30:02.022068542 +0100
--- /home/albinus/src/emacs/lisp/autorevert.el	2019-03-24 15:26:41.756960378 +0100
***************
*** 343,352 ****

  ;; Functions:

! (defun auto-revert-remove-current-buffer ()
    "Remove dead buffer from `auto-revert-buffer-list'."
    (setq auto-revert-buffer-list
!         (delq (current-buffer) auto-revert-buffer-list)))

  ;;;###autoload
  (define-minor-mode auto-revert-mode
--- 343,352 ----

  ;; Functions:

! (defun auto-revert-remove-current-buffer (&optional buffer)
    "Remove dead buffer from `auto-revert-buffer-list'."
    (setq auto-revert-buffer-list
!         (delq (or buffer (current-buffer)) auto-revert-buffer-list)))

  ;;;###autoload
  (define-minor-mode auto-revert-mode
***************
*** 772,781 ****
        (setq bufs (delq nil
                         (mapcar
                          (lambda (buf)
!                           (with-current-buffer buf
!                             (and (or (not (file-remote-p default-directory))
!                                      (file-remote-p default-directory nil t))
!                                  buf)))
                          bufs)))
        ;; Partition `bufs' into two halves depending on whether or not
        ;; the buffers are in `auto-revert-remaining-buffers'.  The two
--- 772,783 ----
        (setq bufs (delq nil
                         (mapcar
                          (lambda (buf)
!                           (and (buffer-live-p buf)
!                                (with-current-buffer buf
!                                  (and
!                                   (or (not (file-remote-p default-directory))
!                                       (file-remote-p default-directory nil t))
!                                       buf))))
                          bufs)))
        ;; Partition `bufs' into two halves depending on whether or not
        ;; the buffers are in `auto-revert-remaining-buffers'.  The two
***************
*** 792,815 ****
  		  (not (and auto-revert-stop-on-user-input
  			    (input-pending-p))))
  	(let ((buf (car bufs)))
!           (with-current-buffer buf
!             (if (buffer-live-p buf)
!                 (progn
!                   ;; Test if someone has turned off Auto-Revert Mode
!                   ;; in a non-standard way, for example by changing
!                   ;; major mode.
!                   (if (and (not auto-revert-mode)
!                            (not auto-revert-tail-mode)
!                            (memq buf auto-revert-buffer-list))
!                       (auto-revert-remove-current-buffer))
!                   (when (auto-revert-active-p)
!                     ;; Enable file notification.
!                     (when (and auto-revert-use-notify
!                                (not auto-revert-notify-watch-descriptor))
!                       (auto-revert-notify-add-watch))
!                     (auto-revert-handler)))
                ;; Remove dead buffer from `auto-revert-buffer-list'.
!               (auto-revert-remove-current-buffer))))
  	(setq bufs (cdr bufs)))
        (setq auto-revert-remaining-buffers bufs)
        ;; Check if we should cancel the timer.
--- 794,816 ----
  		  (not (and auto-revert-stop-on-user-input
  			    (input-pending-p))))
  	(let ((buf (car bufs)))
!           (if (not (buffer-live-p buf))
                ;; Remove dead buffer from `auto-revert-buffer-list'.
!               (auto-revert-remove-current-buffer buf)
!             (with-current-buffer buf
!               ;; Test if someone has turned off Auto-Revert Mode
!               ;; in a non-standard way, for example by changing
!               ;; major mode.
!               (if (and (not auto-revert-mode)
!                        (not auto-revert-tail-mode)
!                        (memq buf auto-revert-buffer-list))
!                   (auto-revert-remove-current-buffer))
!               (when (auto-revert-active-p)
!                 ;; Enable file notification.
!                 (when (and auto-revert-use-notify
!                            (not auto-revert-notify-watch-descriptor))
!                   (auto-revert-notify-add-watch))
!                 (auto-revert-handler)))))
  	(setq bufs (cdr bufs)))
        (setq auto-revert-remaining-buffers bufs)
        ;; Check if we should cancel the timer.

[-- Attachment #3: Type: text/plain, Size: 725 bytes --]


> Reproducing this is random for me but many times a day recently.
> Alternatively, here is a contrived recipe to reproduce this error, but
> *not* using global-auto-revert-mode, instead purposely putting a dead
> buffer in auto-revert-buffer-list.  I think it still hits the same
> code path inside auto-revert-buffers.
>
> ~~~~~~
> (require 'autorevert)
> (let ((buf (generate-new-buffer "foo")))
>   (push buf auto-revert-buffer-list)
>   (kill-buffer buf)
>   (auto-revert-buffers))

This recipe fails earlier, in the lambda form checking remote buffers.
I've fixed this case as well.

Could you pls check whether the patch works for you with magit? (I
don't use magit myself)

> Regards,
> Dale

Best regards, Michael.