fix(pdf-roll): survive a revert that lands during a page render

[alberti42]

What happens

Open a PDF with pdf-view-roll-minor-mode and let something revert it — AUCTeX's TeX-after-compilation-finished-functions with TeX-revert-document-buffer in it, auto-revert-mode, a file-notification watcher. Recompiling the document while the PDF is on screen then often ends in:

Debugger entered--Lisp error: (wrong-type-argument overlayp nil)
  pdf-roll-display-image((image :type png :data "..." :scale default :width 1127 :rotation 0 ...) 20 #<window 19 on exam_retake.pdf>)
  pdf-roll-display-page(20 #<window 19 on exam_retake.pdf> t)
  pdf-roll-display-pages(19 #<window 19 on exam_retake.pdf> t nil)
  pdf-roll-pre-redisplay(#<window 19 on exam_retake.pdf>)
  run-hook-with-args(pdf-roll-pre-redisplay #<window 19 on exam_retake.pdf>)
  redisplay--pre-redisplay-functions(...)
  redisplay_internal\ \(C\ function\)()

pdf-roll-pre-redisplay wraps its body in with-demoted-errors, which is condition-case-unless-debug. So with debug-on-error off this shows up as a page that fails to paint: the overlay keeps the (space :width 25 :height 1000) placeholder it was built with, so the window shows blank space where the page should be, and goes on showing it until the page, the window size or the vscroll changes. With debug-on-error on you get the backtrace above.

Why

pdf-roll-display-page renders the page and hands the image to pdf-roll-display-image, which puts it on the overlay holding that page. Rendering is a synchronous query, and pdf-info-query waits for the server like this:

(while (and (not done)
            (eq (process-status (pdf-info-process)) 'run))
  (accept-process-output (pdf-info-process) 0.01))

accept-process-output runs timers, process filters and sentinels while it waits, and a revert of the PDF buffer arrives through exactly those: AUCTeX reverts from the TeX process sentinel, auto-revert-mode from a timer, file notifications from the event loop. So the buffer can be reverted in the middle of pdf-roll-display-pages, while that is inside redisplay.

Two separate things then happen to the overlays, and it matters which is which.

revert-buffer replaces the buffer text. A page overlay carries evaporate, so the replacement collapses it. This happens inside revert-buffer itself, before pdf-roll-initialize — which is an :after advice on revert-buffer-function — is reached at all. Tracing the entry to pdf-roll-initialize on a 285 kB document:

(initialize :overlays nil :point-max 284930)     ; the file is 284929 bytes

pdf-roll-initialize then rebuilds the overlays, and it builds them for (selected-window) — inside a sentinel or a timer, whatever window happened to be selected, which with AUCTeX is the window of the LaTeX source rather than the window being drawn.

When the render returns, the overlay it was going to write to is gone, and overlay-put gets nil.

The fix

Two parts, and both are needed.

Postpone the rebuild. While a render is in progress, pdf-roll-initialize puts its work on a timer that runs once the render has finished, so it does not replace a window's overlays part-way through a redisplay pass. The approach and the first commit here are @aikrahguzar's, from the review below, transposed onto master where pdf-roll-initialize differs a little. Two corrections came with it: (defvar pdf-roll--delay-revert) with no value form leaves the variable void rather than nil, so pdf-roll-initialize signalled (void-variable pdf-roll--delay-revert) on an ordinary revert-buffer and when the mode was turned on; and run-at-time runs its function with whatever buffer is current when it fires, while pdf-roll-initialize calls remove-overlays and erase-buffer on the current buffer under inhibit-read-only, so the buffer has to be captured and checked for liveness. The flag is bound in pdf-roll-display-page as well as in pdf-roll-pre-redisplay, because pdf-roll-scroll-forward and pdf-roll-scroll-backward also render as they walk from page to page and run as commands rather than as part of redisplay.

Skip the overlays that are already gone. The postponement does not prevent the error on its own, because the collapse happens in revert-buffer before the postponement can apply. pdf-roll-display-page reads the overlay and pdf-roll-display-image writes to it; both now skip a page whose overlay is missing. The revert changed the buffer, so a redisplay follows that draws the page again.

For that redisplay to draw anything, the window state may only be remembered when the pages really were drawn. pdf-roll-pre-redisplay records page, height, width and vscroll after pdf-roll-display-pages returns; recorded after a revert, that state tells the next redisplay nothing has changed, and the window stays on the empty buffer. Guarding the record on the window still having overlays keeps the recovery redisplay working.

pdf-roll-undisplay-pages needs the same care for its own reason: it is handed the difference between the displayed-pages a window remembers and the pages just drawn, and that list outlives the overlays. pdf-roll-initialize clears it with image-mode-window-put and no window argument, so only the selected window is cleared, and a document that is shorter after the revert leaves the others naming a page with no overlay, inside the buffer or past its end.

Measurements

The race was forced deterministically: a run-at-time 0 revert scheduled from a :before advice on pdf-view-create-page, so it fires while that render waits on the server, with a window other than the PDF's selected. 23-page document, counting only Error in image roll pre-redisplay: (wrong-type-argument overlayp nil):

lisp/pdf-roll.el	reproduced
master	2/2
postponement only	5/5
guards only	0/2
this branch	0/3

Tests

Two in test/pdf-roll-test.el: that pdf-roll-initialize called while a render is in progress leaves the buffer and its overlays alone and queues the work instead, and that pdf-roll-undisplay-pages skips a page with no overlay. The paths that render need a live window and a running server, so they are not covered in batch, in keeping with the note at the top of that file.

emacs -Q --batch -L lisp -l ert -l test/pdf-roll-test.el -f ert-run-tests-batch-and-exit
Ran 10 tests, 10 results as expected, 0 unexpected

No new byte-compile warnings.

Not fixed here

The same forced revert also produces Error during redisplay: (eval (pdf-misc-size-indication) t) signaled (wrong-type-argument overlayp nil), 4 to 6 times per run. pdf-misc-size-indication is only the caller, through the mode line's :eval; the unguarded read is in pdf-view-image-size, which in roll mode ends in (overlay-get (pdf-roll-page-overlay page window) 'display). It has the same cause as the above and is present on master.

Guarding that read is not sufficient on its own: image-size and image-display-size then fail on nil, and pdf-misc-size-indication needs a number to format a percentage. What pdf-view-image-size should report when the page has no image yet is a decision of its own, so it is not in this branch.

Relation to #361, #364 and #338

Independent of #361 and #364, and branched off master like both of them. Only test/pdf-roll-test.el conflicts, because each appends a section at the end of the file.

#338 does drop the pdf-roll-new-window-function call from pdf-roll-initialize, so the overlays are no longer rebuilt for whatever window happened to be selected. The error survives that: pdf-roll-display-page there still reads the overlay unguarded and pdf-roll-display-image still ends in an unguarded overlay-put, and the collapse in revert-buffer is untouched by it. This branch should apply to #338 with little change.

[aikrahguzar]

I can see the problem but I think this is the wrong way of solve it: it is much simpler if pdf-roll-pre-redisplay can be thought of as a synchronous function. This is broken by the possibility of revert so it better to try ensuring that revert can't pull the rug underneath it.

Does the following diff fix this?

diff --git a/lisp/pdf-roll.el b/lisp/pdf-roll.el
index 44b7100..411d561 100644
--- a/lisp/pdf-roll.el
+++ b/lisp/pdf-roll.el
@@ -52,6 +52,8 @@
 (defvar pdf-roll--state nil
   "Local variable that tracks window, point and vscroll to handle changes.")
 
+(defvar pdf-roll--delay-revert)
+
 ;;; Utility Macros and functions
 (defsubst pdf-roll-page-to-pos (page)
   "Get the buffer position displaying PAGE."
@@ -233,7 +235,8 @@ It should be added to `pre-redisplay-functions' buffer locally."
   (with-demoted-errors "Error in image roll pre-redisplay: %S"
     (unless (pdf-roll-page-overlay 1 win)
       (pdf-roll-new-window-function win))
-    (let* ((state (alist-get win pdf-roll--state))
+    (let* ((pdf-roll--delay-revert t)
+           (state (alist-get win pdf-roll--state))
            (pscrolling (memq last-command
                              '( pixel-scroll-precision pixel-scroll-start-momentum
                                 pixel-scroll-interpolate-up pixel-scroll-interpolate-down
@@ -405,16 +408,18 @@ If PIXELS is non-nil N is number of pixels instead of lines."
 It is also added to `revert-buffer-function'.
 
 It erases the buffer and adds one line containing a space for each page."
-  (image-mode-window-put 'displayed-pages nil)
-  (setq pdf-roll--state nil)
-  (remove-overlays)
-  (let ((pages (pdf-cache-number-of-pages))
-        (inhibit-read-only t))
-    (erase-buffer)
-    (dotimes (_i (* 2 (+ pages 1)))
-      (insert " \n"))
-    (delete-char -1)
-    (set-buffer-modified-p nil)))
+  (if pdf-roll--delay-revert
+      (run-at-time 0 nil #'pdf-roll-initialize)
+    (image-mode-window-put 'displayed-pages nil)
+    (setq pdf-roll--state nil)
+    (remove-overlays)
+    (let ((pages (pdf-cache-number-of-pages))
+          (inhibit-read-only t))
+      (erase-buffer)
+      (dotimes (_i (* 2 (+ pages 1)))
+        (insert " \n"))
+      (delete-char -1)
+      (set-buffer-modified-p nil))))
 
 (defvar pdf-view-roll-minor-mode-map
   (let ((map (make-sparse-keymap)))

The new overlays get 'window set to (selected-window), and inside a sentinel or a timer that is whatever window happened to be selected — with AUCTeX, the window of the LaTeX source, not the window being drawn.

I think this is fixed in #338. pdf-roll-initialize doesn't call the pdf-roll-new-window-function. As far as I can tell, pdf-roll-new-window-function is now only called by pdf-roll-pre-redisplay so overlays will only be created when we need to display in a window.

[alberti42]

Thanks for reviewing the PR so quickly! I built the branch around your patch, but testing it turned up something that changes the conclusion, so it is now in the branch together with the guards rather than in place of them.

Your commit is first, with you as the author, adjusted to master where pdf-roll-initialize differs a little. Two corrections came with it:

• (defvar pdf-roll--delay-revert) with no value form declares the variable special but leaves it void, so reading it outside the binding signals (void-variable pdf-roll--delay-revert). pdf-roll-initialize is reached from the revert-buffer-function advice and from the body of pdf-view-roll-minor-mode, neither of them inside the binding, so an ordinary revert-buffer and turning the mode on both failed. It defaults to nil now.
• run-at-time runs its function with whatever buffer happens to be current when it fires, and pdf-roll-initialize calls remove-overlays and erase-buffer on the current buffer under inhibit-read-only. The buffer is captured in the closure and checked with buffer-live-p.

I also bound the flag in pdf-roll-display-page as well as in pdf-roll-pre-redisplay. pdf-roll-scroll-forward and pdf-roll-scroll-backward render as they walk from page to page and run as commands, so a revert arriving during one of their renders was not covered. Your binding stays: it covers the whole redisplay pass rather than the renders alone, and pdf-roll-display-pages also waits on the server through pdf-cache-number-of-pages.

On its own the postponement does not stop the error. I forced the race deterministically, with a run-at-time 0 revert scheduled from a :before advice on pdf-view-create-page, so that it fires while that render is waiting on the server:

;; -*- lexical-binding: t; -*-
;; One-shot: make a revert land inside the next page render.
(defvar my-pdf-race-armed t)

(defun my-pdf-race (&rest _)
  (when my-pdf-race-armed
    (setq my-pdf-race-armed nil)
    (let* ((buf (current-buffer))
           (pdfwin (get-buffer-window buf t))
           ;; Any other window on the PDF's frame.  With AUCTeX the revert
           ;; comes from the TeX process sentinel, so the selected window is
           ;; the one showing the LaTeX source, not the one being drawn.
           (win (car (seq-remove (lambda (w) (eq w pdfwin))
                                 (window-list (window-frame pdfwin))))))
      (unless win (user-error "Split the PDF's frame first"))
      (run-at-time 0 nil
                   (lambda ()
                     (when (buffer-live-p buf)
                       (with-selected-window win
                         (with-current-buffer buf (revert-buffer t t)))))))))

(advice-add 'pdf-view-create-page :before #'my-pdf-race)

Open a PDF with pdf-view-roll-minor-mode in a frame that has a second window, then M-x pdf-roll-goto-page to a page that has not been rendered yet, and read *Messages*. (advice-remove 'pdf-view-create-page #'my-pdf-race) afterwards, and (setq my-pdf-race-armed t) to arm it again.

Four details matter. The lexical-binding cookie is required: without it the timer's lambda does not capture buf and win, and nothing reproduces. The one-shot guard is needed, or every render schedules another revert and every revert triggers more renders. debug-on-error has to be nil, so that with-demoted-errors logs to *Messages* instead of trying to enter the debugger from inside redisplay. And the window that is selected when the revert fires decides the outcome, which is part of why the bug looks intermittent: with the PDF's own window selected, pdf-roll-initialize rebuilds the overlays for that same window and master survives.

23-page document, counting only Error in image roll pre-redisplay: (wrong-type-argument overlayp nil):

master                     2/2 reproduced
postponement only          5/5 reproduced
guards only                0/2
postponement and guards    0/3

The reason is that pdf-roll-initialize is not what takes the overlays away. It is an :after advice on revert-buffer-function, and revert-buffer has already replaced the buffer text by the time it runs; a page overlay carries evaporate, so the replacement collapses it. Tracing the entry to pdf-roll-initialize:

(initialize :delay t :overlays nil :point-max 284930)    ; the file is 284929 bytes

The buffer already holds the raw PDF and has no page overlays left. So the postponement delays the rebuild, not the collapse, and a render that was waiting still returns to an overlay that is gone.

I kept it all the same, for the reason you gave: it stops pdf-roll-initialize rebuilding a window's overlays part-way through a redisplay pass, which is worth avoiding on its own. It just is not sufficient, so pdf-roll-display-page and pdf-roll-display-image also skip a page whose overlay is missing.

On #338: you are right that pdf-roll-initialize no longer calls pdf-roll-new-window-function there, so the part of my description about overlays being rebuilt for whatever window was selected does not apply to that branch. The error survives it, though, for the reason above — the collapse in revert-buffer is untouched by it, and both the read in pdf-roll-display-page and the write in pdf-roll-display-image are unguarded there.

One more thing the same test showed, which I have left alone: Error during redisplay: (eval (pdf-misc-size-indication) t) signaled (wrong-type-argument overlayp nil) from the mode line, 4 to 6 times per run. pdf-misc-size-indication is only the caller; the unguarded read is in pdf-view-image-size, which in roll mode ends in (overlay-get (pdf-roll-page-overlay page window) 'display). Same cause as above, and it is present on master. Guarding that read is not enough on its own, though: image-size and image-display-size then fail on nil, and pdf-misc-size-indication needs a number to format. What pdf-view-image-size should report when the page has no image yet is a decision of its own, so I left it out of this branch and am happy to open a separate PR for it.