bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Michael Welsh Duggan]

I don't have a recipe for this.  Every now again I get the following in
my *Messages* buffer:

  redisplay--pre-redisplay-functions: (args-out-of-range 0)

I turned on debug-on-error, and got the following backtrace:

  Debugger entered--Lisp error: (args-out-of-range 0)
    get-char-property(0 cursor-sensor-functions)
    cursor-sensor--detect(#<window 224 on *Group*>)
    run-hook-with-args(cursor-sensor--detect #<window 224 on *Group*>)
    redisplay--pre-redisplay-functions(t)
    apply(redisplay--pre-redisplay-functions t)
    #[128 "\300\301\"\210\300\302\"\207" [apply redisplay--pre-redisplay-functions ignore nil] 4 nil nil](t)
    redisplay_internal\ \(C\ function\)()

This happens infrequently, about a couple of times a day.  I cannot make
this happen on demand, but I can add debugging code if led in the
correct direction.
  

In GNU Emacs 25.0.50.1 (x86_64-pc-linux-gnu, X toolkit)
 of 2015-10-09
Repository revision: f7acc9ce65bc6c1080aa6c85618cf854ed0047ee
Windowing system distributor 'The X.Org Foundation', version 11.0.11702000
System Description:	Debian GNU/Linux unstable (sid)

Configured using:
 'configure --without-toolkit-scroll-bars --with-x-toolkit=lucid
 'CFLAGS=-Og -ggdb3' --with-wide-int --with-gameuser=:staff'

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND GPM DBUS GCONF GSETTINGS NOTIFY ACL
GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB LUCID X11

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Dired by date

Minor modes in effect:
  display-time-mode: t
  magit-auto-revert-mode: t
  shell-dirtrack-mode: t
  diff-auto-refine-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t

Recent messages:
Saving /home/md5i/.newsrc.eld...
Saving file /home/md5i/.newsrc.eld...
Wrote /home/md5i/.newsrc.eld
Saving /home/md5i/.newsrc.eld...done
Checking new news...
Reading active file from ~/Mail via nnml...done
nnimap read 0k from localhost
Reading active file via nndraft...done
Checking new news...done
Scanning for dabbrevs...done

Load-path shadows:
/usr/share/emacs/site-lisp/rst hides /usr/local/share/emacs/25.0.50/lisp/textmodes/rst

Features:
(shadow emacsbug gnus-fun calc-alg calc-ext calc-menu calc calc-loaddefs
calc-macs doc-view css-mode smie image-mode pcmpl-unix debug js
nxml-uchnm rng-xsd xsd-regexp rng-cmpct rng-nxml rng-valid nxml-mode
nxml-outln nxml-rap nxml-glyph conf-mode cus-edit cus-start cus-load
tramp-cache rect python tramp-sh json dabbrev gud imenu man time-stamp
whitespace tabify two-column iso-transl make-mode dired-aux
vc-dispatcher vc-hg cc-mode cc-fonts cc-guess cc-menus cc-cmds misearch
multi-isearch gnus-dup shr-color color url-util url-parse url-vars shr
dom subr-x browse-url flow-fill jka-compr mm-archive qp sort gnus-cite
mail-extr gnus-async gnus-bcklg gnus-ml disp-table gnus-topic
cursor-sensor utf-7 nndraft nnmh epa-file network-stream nsm starttls
nnml gnus-agent gnus-srvr gnus-score score-mode nnvirtual gnus-msg
gnus-art mm-uu mml2015 mm-view mml-smime smime dig gnus-cache gnus-demon
nntp gnutls nnir gnus-sum gnus-group gnus-undo gnus-start gnus-cloud
nnimap nnmail mail-source tls utf7 netrc nnoo parse-time gnus-spec
gnus-int gnus-range gnus-win gnus gnus-ems nnheader flyspell ispell
yaml-mode uptimes pp descr-text time sieve-manage rng-loc rng-uri
rng-parse rng-match rng-dt rng-util rng-pttrn nxml-parse nxml-ns
nxml-enc xmltok nxml-util sgml-mode psvn wid-edit cl magit-key-mode
magit view tramp tramp-compat auth-source eieio byte-opt bytecomp
byte-compile cl-extra cconv eieio-core cl-macs gv gnus-util time-date
password-cache tramp-loaddefs trampver shell pcomplete advice grep
compile epa derived epg epg-config diff-mode autorevert filenotify
git-rebase-mode thingatpt git-commit-mode server log-edit easy-mmode
pcvs-util add-log mailcap message sendmail format-spec rfc822 mml
mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045
ietf-drums mm-util help-fns help-mode mail-prsvr mailabbrev mail-utils
gmm-utils mailheader lua-mode edmacro rx generated generic-x ediff-merg
ediff-wind ediff-diff ediff-mult ediff-help ediff-init ediff-util ediff
debian-el debian-el-loaddefs dired-x easymenu dired cc-styles cc-align
cc-engine cc-vars cc-defs ange-ftp comint ansi-color ring cl-seq kmacro
cl-loaddefs pcase cl-lib mule-util tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel x-win term/common-win x-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list newcomment
elisp-mode lisp-mode prog-mode register page menu-bar rfn-eshadow timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
frame cl-generic cham georgian utf-8-lang misc-lang vietnamese tibetan
thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek romanian
slovak czech european ethiopic indian cyrillic chinese charscript
case-table epa-hook jka-cmpr-hook help simple abbrev minibuffer
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote dbusbind inotify
dynamic-setting system-font-setting font-render-setting x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 885412 104614)
 (symbols 48 49376 228)
 (miscs 40 1199 2821)
 (strings 32 113818 15421)
 (string-bytes 1 3593164)
 (vectors 16 52064)
 (vector-slots 8 1440209 35827)
 (floats 8 761 638)
 (intervals 56 24560 199)
 (buffers 976 98)
 (heap 1024 103928 10949))

-- 
Michael Welsh Duggan
(md5i@md5i.com)

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Eli Zaretskii]

> From: Michael Welsh Duggan <mwd@md5i.com>
> Date: Thu, 22 Oct 2015 00:04:06 -0400
> 
> I don't have a recipe for this.  Every now again I get the following in
> my *Messages* buffer:
> 
>   redisplay--pre-redisplay-functions: (args-out-of-range 0)
> 
> I turned on debug-on-error, and got the following backtrace:
> 
>   Debugger entered--Lisp error: (args-out-of-range 0)
>     get-char-property(0 cursor-sensor-functions)
>     cursor-sensor--detect(#<window 224 on *Group*>)
>     run-hook-with-args(cursor-sensor--detect #<window 224 on *Group*>)
>     redisplay--pre-redisplay-functions(t)
>     apply(redisplay--pre-redisplay-functions t)
>     #[128 "\300\301\"\210\300\302\"\207" [apply 
> redisplay--pre-redisplay-functions ignore nil] 4 nil nil](t)
>     redisplay_internal\ \(C\ function\)()
> 
> This happens infrequently, about a couple of times a day.  I cannot make
> this happen on demand, but I can add debugging code if led in the
> correct direction.

Thanks for reporting this.  I'll try to help, although Stefan might
have better ideas.

From the backtrace it is obvious that the immediate reason for the
problem is the call to 'get-char-property' with an invalid 1st argument
of zero (it's a buffer position, so should be at least 1).

Looking at 'cursor-sensor--detect', I see this:

  (defun cursor-sensor--detect (window)
    (unless cursor-sensor-inhibit
      (let* ((point (window-point window))
	     ;; It's often desirable to make the cursor-sensor-functions property
	     ;; non-sticky on both ends, but that means get-pos-property might
	     ;; never see it.
	     (new (or (get-char-property point 'cursor-sensor-functions)
		      (unless (bobp)
			(get-char-property (1- point) 'cursor-sensor-functions))))

So my guess is that the value of 'point' (the variable, not the
function) is 1, i.e. beginning of buffer, and subtracting 1 from it
produces that zero.  Then the second call to 'get-char-property' barfs
as expected.

The code tries to protect itself against such a calamity by calling
'bobp', but that function looks at the buffer's value of point, which
might be different from what 'window-point' returns.

So my advice would be to replace the call to 'bobp' with an explicit
test of the value of the local variable 'point', and see if that fixes
the problem.

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Michael Welsh Duggan]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Michael Welsh Duggan <mwd@md5i.com>
>> Date: Thu, 22 Oct 2015 00:04:06 -0400
>> 
>> I don't have a recipe for this.  Every now again I get the following in
>> my *Messages* buffer:
>> 
>>   redisplay--pre-redisplay-functions: (args-out-of-range 0)
>> 
>> I turned on debug-on-error, and got the following backtrace:
>> 
>>   Debugger entered--Lisp error: (args-out-of-range 0)
>>     get-char-property(0 cursor-sensor-functions)
>>     cursor-sensor--detect(#<window 224 on *Group*>)
>>     run-hook-with-args(cursor-sensor--detect #<window 224 on *Group*>)
>>     redisplay--pre-redisplay-functions(t)
>>     apply(redisplay--pre-redisplay-functions t)
>>     #[128 "\300\301\"\210\300\302\"\207" [apply 
>> redisplay--pre-redisplay-functions ignore nil] 4 nil nil](t)
>>     redisplay_internal\ \(C\ function\)()
>> 
>> This happens infrequently, about a couple of times a day.  I cannot make
>> this happen on demand, but I can add debugging code if led in the
>> correct direction.
>
> Thanks for reporting this.  I'll try to help, although Stefan might
> have better ideas.
>
> From the backtrace it is obvious that the immediate reason for the
> problem is the call to 'get-char-property' with an invalid 1st argument
> of zero (it's a buffer position, so should be at least 1).
>
> Looking at 'cursor-sensor--detect', I see this:
>
>   (defun cursor-sensor--detect (window)
>     (unless cursor-sensor-inhibit
>       (let* ((point (window-point window))
> 	     ;; It's often desirable to make the cursor-sensor-functions property
> 	     ;; non-sticky on both ends, but that means get-pos-property might
> 	     ;; never see it.
> 	     (new (or (get-char-property point 'cursor-sensor-functions)
> 		      (unless (bobp)
> 			(get-char-property (1- point) 'cursor-sensor-functions))))
>
> So my guess is that the value of 'point' (the variable, not the
> function) is 1, i.e. beginning of buffer, and subtracting 1 from it
> produces that zero.  Then the second call to 'get-char-property' barfs
> as expected.
>
> The code tries to protect itself against such a calamity by calling
> 'bobp', but that function looks at the buffer's value of point, which
> might be different from what 'window-point' returns.
>
> So my advice would be to replace the call to 'bobp' with an explicit
> test of the value of the local variable 'point', and see if that fixes
> the problem.

I've changed the (bobp) to (= point 1).  I'll run it for a couple of
weeks and report back.

-- 
Michael Welsh Duggan
(md5i@md5i.com)

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Michael Welsh Duggan]

Michael Welsh Duggan <mwd@md5i.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> From the backtrace it is obvious that the immediate reason for the
>> problem is the call to 'get-char-property' with an invalid 1st argument
>> of zero (it's a buffer position, so should be at least 1).
>>
>> Looking at 'cursor-sensor--detect', I see this:
>>
>>   (defun cursor-sensor--detect (window)
>>     (unless cursor-sensor-inhibit
>>       (let* ((point (window-point window))
>> 	     ;; It's often desirable to make the cursor-sensor-functions property
>> 	     ;; non-sticky on both ends, but that means get-pos-property might
>> 	     ;; never see it.
>> 	     (new (or (get-char-property point 'cursor-sensor-functions)
>> 		      (unless (bobp)
>> 			(get-char-property (1- point) 'cursor-sensor-functions))))
>>
>> So my guess is that the value of 'point' (the variable, not the
>> function) is 1, i.e. beginning of buffer, and subtracting 1 from it
>> produces that zero.  Then the second call to 'get-char-property' barfs
>> as expected.
>>
>> The code tries to protect itself against such a calamity by calling
>> 'bobp', but that function looks at the buffer's value of point, which
>> might be different from what 'window-point' returns.
>>
>> So my advice would be to replace the call to 'bobp' with an explicit
>> test of the value of the local variable 'point', and see if that fixes
>> the problem.
>
> I've changed the (bobp) to (= point 1).  I'll run it for a couple of
> weeks and report back.

This change seems to have done the trick.  I have not encountered the
error in several days.

-- 
Michael Welsh Duggan
(md5i@md5i.com)

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Eli Zaretskii]

> From: Michael Welsh Duggan <mwd@md5i.com>
> Date: Sat, 31 Oct 2015 09:07:05 -0400
> 
> > I've changed the (bobp) to (= point 1).  I'll run it for a couple of
> > weeks and report back.
> 
> This change seems to have done the trick.  I have not encountered the
> error in several days.

Thanks for testing.  I pushed the change, and I'm arking this bug as
done.

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Sa., 31. Okt. 2015 um 14:34 Uhr:

> > From: Michael Welsh Duggan <mwd@md5i.com>
> > Date: Sat, 31 Oct 2015 09:07:05 -0400
> >
> > > I've changed the (bobp) to (= point 1).  I'll run it for a couple of
> > > weeks and report back.
> >
> > This change seems to have done the trick.  I have not encountered the
> > error in several days.
>
> Thanks for testing.  I pushed the change, and I'm arking this bug as
> done.
>
>
>
This change was  reverted in 76ef52267cf887e3e1aa6d25b3b16dd0601dd459.
It also doesn't seem correct. cursor-sensor--detect is only used in
pre-redisplay-functions, and the documentation of that variable says:
"Hook run just before redisplay.
It is called in each window that is to be redisplayed.  It takes one
argument,
which is the window that will be redisplayed.  When run, the
‘current-buffer’
is set to the buffer displayed in that window."
That means that (bobp) is correct and (= point 1) cannot give a different
result, unless narrowing is in effect (then only bobp is correct).
Given that replacing (bobp) with (= point 1) does solve this bug, the
documentation of pre-redisplay-functions must be incorrect, i.e. the
current buffer is not the buffer of the window passed as argument. I think
the only way how this can happen is that a previous entry in
pre-redisplay-functions has changed the current buffer. Probably the
implementation of redisplay--pre-redisplay-functions should be changed from
(with-current-buffer (window-buffer win)
          (run-hook-with-args 'pre-redisplay-functions win))
to
(run-hook-wrapped 'pre-redisplay-functions
  (lambda (func) (with-current-buffer (window-buffer win)
     (funcall func win)
     nil))
or so.

So we might try the following: Replace redisplay--pre-redisplay-functions
(and indeed, all hooks that document anything about the current buffer) as
above. Then add (cl-assert (eq (current-buffer) (window-buffer window)) to
cursor-sensor--detect and remove  code such as (window-point window),
because that must be equal to (point). WDYT? Or am I misunderstanding
something?

[-- Attachment #2: Type: text/html, Size: 2969 bytes --]

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 14 Sep 2016 16:54:23 +0000
> 
>  Thanks for testing. I pushed the change, and I'm arking this bug as
>  done.
> 
> This change was reverted in 76ef52267cf887e3e1aa6d25b3b16dd0601dd459.

Too bad people revert changes that fixed a bug without making sure
they don't reintroduce the same bug again.  Now Emacs 25.1 will be
released with this bug.

> Given that replacing (bobp) with (= point 1) does solve this bug, the documentation of pre-redisplay-functions
> must be incorrect, i.e. the current buffer is not the buffer of the window passed as argument. I think the only
> way how this can happen is that a previous entry in pre-redisplay-functions has changed the current buffer.
> Probably the implementation of redisplay--pre-redisplay-functions should be changed from 
> (with-current-buffer (window-buffer win)
> (run-hook-with-args 'pre-redisplay-functions win))
> to
> (run-hook-wrapped 'pre-redisplay-functions
> (lambda (func) (with-current-buffer (window-buffer win)
> (funcall func win)
> nil))
> or so.
> 
> So we might try the following: Replace redisplay--pre-redisplay-functions (and indeed, all hooks that
> document anything about the current buffer) as above. Then add (cl-assert (eq (current-buffer)
> (window-buffer window)) to cursor-sensor--detect and remove code such as (window-point window), because
> that must be equal to (point). WDYT? Or am I misunderstanding something?

Since the problem that caused the patch to be reverted was with
narrowing, why not simply use

  (= point (with-current-buffer (window-buffer window) (point-min)))

instead of

  (= point 1)

?

Anyway, my role in this bug was just to facilitate the debugging and
push a commit, I didn't really look at the issue deeper than what you
see in the discussion.

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Mi., 14. Sep. 2016 um 19:11 Uhr:

>
> > Given that replacing (bobp) with (= point 1) does solve this bug, the
> documentation of pre-redisplay-functions
> > must be incorrect, i.e. the current buffer is not the buffer of the
> window passed as argument. I think the only
> > way how this can happen is that a previous entry in
> pre-redisplay-functions has changed the current buffer.
> > Probably the implementation of redisplay--pre-redisplay-functions should
> be changed from
> > (with-current-buffer (window-buffer win)
> > (run-hook-with-args 'pre-redisplay-functions win))
> > to
> > (run-hook-wrapped 'pre-redisplay-functions
> > (lambda (func) (with-current-buffer (window-buffer win)
> > (funcall func win)
> > nil))
> > or so.
> >
> > So we might try the following: Replace
> redisplay--pre-redisplay-functions (and indeed, all hooks that
> > document anything about the current buffer) as above. Then add
> (cl-assert (eq (current-buffer)
> > (window-buffer window)) to cursor-sensor--detect and remove code such as
> (window-point window), because
> > that must be equal to (point). WDYT? Or am I misunderstanding something?
>
> Since the problem that caused the patch to be reverted was with
> narrowing, why not simply use
>
>   (= point (with-current-buffer (window-buffer window) (point-min)))
>
> instead of
>
>   (= point 1)
>
> ?
>
>
This has the same issues as described above.  There's more code in
cursor-sensor--detect that assumes (eq (current-buffer) (window-buffer
window)), e.g. the form (get-char-property point 'cursor-sensor-functions).
Therefore (= point (with-current-buffer (window-buffer window)
(point-min))) must be the same as (bobp).
Another mystery is the stack trace entry
"run-hook-with-args(cursor-sensor--detect #<window 224 on *Group*>)". How
can cursor-sensor--detect be the first argument to run-hook-with-args?
Shouldn't it always be pre-redisplay-functions?

[-- Attachment #2: Type: text/html, Size: 2934 bytes --]

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Philipp Stephani]

[-- Attachment #1.1: Type: text/plain, Size: 1943 bytes --]

Philipp Stephani <p.stephani2@gmail.com> schrieb am Mi., 14. Sep. 2016 um
18:54 Uhr:

> Eli Zaretskii <eliz@gnu.org> schrieb am Sa., 31. Okt. 2015 um 14:34 Uhr:
>
> > From: Michael Welsh Duggan <mwd@md5i.com>
> > Date: Sat, 31 Oct 2015 09:07:05 -0400
> >
> > > I've changed the (bobp) to (= point 1).  I'll run it for a couple of
> > > weeks and report back.
> >
> > This change seems to have done the trick.  I have not encountered the
> > error in several days.
>
> Thanks for testing.  I pushed the change, and I'm arking this bug as
> done.
>
>
>
> This change was  reverted in 76ef52267cf887e3e1aa6d25b3b16dd0601dd459.
> It also doesn't seem correct. cursor-sensor--detect is only used in
> pre-redisplay-functions, and the documentation of that variable says:
> "Hook run just before redisplay.
> It is called in each window that is to be redisplayed.  It takes one
> argument,
> which is the window that will be redisplayed.  When run, the
> ‘current-buffer’
> is set to the buffer displayed in that window."
> That means that (bobp) is correct and (= point 1) cannot give a different
> result, unless narrowing is in effect (then only bobp is correct).
> Given that replacing (bobp) with (= point 1) does solve this bug, the
> documentation of pre-redisplay-functions must be incorrect, i.e. the
> current buffer is not the buffer of the window passed as argument. I think
> the only way how this can happen is that a previous entry in
> pre-redisplay-functions has changed the current buffer. Probably the
> implementation of redisplay--pre-redisplay-functions should be changed from
> (with-current-buffer (window-buffer win)
>           (run-hook-with-args 'pre-redisplay-functions win))
> to
> (run-hook-wrapped 'pre-redisplay-functions
>   (lambda (func) (with-current-buffer (window-buffer win)
>      (funcall func win)
>      nil))
> or so.
>

I've attached a patch for this.

[-- Attachment #1.2: Type: text/html, Size: 3500 bytes --]

[-- Attachment #2: 0001-Restore-buffer-in-pre-redisplay-functions.txt --]
[-- Type: text/plain, Size: 3499 bytes --]

From 361a0488a67ffc601e207626b25327bf4868d24a Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Wed, 14 Sep 2016 21:08:07 +0200
Subject: [PATCH] Restore buffer in `pre-redisplay-functions'

Previously, a function in `pre-redisplay-functions' could change the
current buffer, breaking the contract of `pre-redisplay-functions'
that the current buffer is always the buffer displayed in the argument
window.

* lisp/simple.el (redisplay--pre-redisplay-functions): Fix
behavior when a function in `pre-redisplay-functions' changes the
current buffer.

* test/lisp/simple-tests.el
(pre-redisplay-function--current-buffer): Test for fixed behavior.
---
 lisp/simple.el            | 16 ++++++++++------
 test/lisp/simple-tests.el | 28 ++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 04a525c..9a4690e 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -5425,13 +5425,17 @@ pre-redisplay-functions
 is set to the buffer displayed in that window.")
 
 (defun redisplay--pre-redisplay-functions (windows)
+  (cond
+   ((null windows) (setq windows (list (selected-window))))
+   ((nlistp windows) (setq windows (window-list-1 nil nil t))))
   (with-demoted-errors "redisplay--pre-redisplay-functions: %S"
-    (if (null windows)
-        (with-current-buffer (window-buffer (selected-window))
-          (run-hook-with-args 'pre-redisplay-functions (selected-window)))
-      (dolist (win (if (listp windows) windows (window-list-1 nil nil t)))
-        (with-current-buffer (window-buffer win)
-          (run-hook-with-args 'pre-redisplay-functions win))))))
+    (dolist (win windows)
+      (run-hook-wrapped
+       'pre-redisplay-functions
+       (lambda (func)
+         (with-current-buffer (window-buffer win)
+           (funcall func win))
+         nil)))))
 
 (add-function :before pre-redisplay-function
               #'redisplay--pre-redisplay-functions)
diff --git a/test/lisp/simple-tests.el b/test/lisp/simple-tests.el
index d022240..da7a03d 100644
--- a/test/lisp/simple-tests.el
+++ b/test/lisp/simple-tests.el
@@ -374,5 +374,33 @@ simple-test-undo-with-switched-buffer
        (undo)
        (point)))))
 
+\f
+;;; `pre-redisplay-function'
+(ert-deftest pre-redisplay-function--current-buffer ()
+  "Verify that the functions in `pre-redisplay-functions' are called with the correct `current-buffer'.
+See Bug#21730."
+  (with-temp-buffer
+    (let ((buffer-1 (current-buffer)))
+      (with-temp-buffer
+        (let ((buffer-2 (current-buffer))
+              (pre-redisplay-functions pre-redisplay-functions)
+              (hook-1-calls 0)
+              (hook-2-calls 0))
+          (add-hook 'pre-redisplay-functions
+                    (lambda (window)
+                      (should (equal (current-buffer) (window-buffer window)))
+                      (cl-incf hook-1-calls)
+                      (set-buffer buffer-1))
+                    :append)
+          (add-hook 'pre-redisplay-functions
+                    (lambda (window)
+                      (should (equal (current-buffer) (window-buffer window)))
+                      (cl-incf hook-2-calls))
+                    :append)
+          (funcall pre-redisplay-function nil)
+          (should (equal (current-buffer) buffer-2))
+          (should (equal hook-1-calls 1))
+          (should (equal hook-2-calls 1)))))))
+
 (provide 'simple-test)
 ;;; simple-test.el ends here
-- 
2.8.0.rc3.226.g39d4020

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 14 Sep 2016 18:43:13 +0000
> Cc: 21730@debbugs.gnu.org, mwd@md5i.com
> 
>  Since the problem that caused the patch to be reverted was with
>  narrowing, why not simply use
> 
>  (= point (with-current-buffer (window-buffer window) (point-min)))
> 
>  instead of
> 
>  (= point 1)
> 
>  ?
> 
> This has the same issues as described above. There's more code in cursor-sensor--detect that assumes (eq
> (current-buffer) (window-buffer window)), e.g. the form (get-char-property point 'cursor-sensor-functions).
> Therefore (= point (with-current-buffer (window-buffer window) (point-min))) must be the same as (bobp).
> Another mystery is the stack trace entry "run-hook-with-args(cursor-sensor--detect #<window 224 on
> *Group*>)". How can cursor-sensor--detect be the first argument to run-hook-with-args? Shouldn't it always
> be pre-redisplay-functions?

If we want to dig deeper, I think we should first understand why bop
doesn't work.  Is it indeed because the window's buffer is not the
current buffer when the hook is called?

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 14 Sep 2016 19:11:57 +0000
> 
>  (with-current-buffer (window-buffer win)
>  (run-hook-with-args 'pre-redisplay-functions win))
>  to
>  (run-hook-wrapped 'pre-redisplay-functions
>  (lambda (func) (with-current-buffer (window-buffer win)
>  (funcall func win)
>  nil))
>  or so.
> 
> I've attached a patch for this. 

Thanks.  But we are going to entertain such changes, I think we really
should be sure the issue with the current buffer is the culprit.  See
my other message a few moments ago.

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Mi., 14. Sep. 2016 um 21:24 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Wed, 14 Sep 2016 18:43:13 +0000
> > Cc: 21730@debbugs.gnu.org, mwd@md5i.com
> >
> >  Since the problem that caused the patch to be reverted was with
> >  narrowing, why not simply use
> >
> >  (= point (with-current-buffer (window-buffer window) (point-min)))
> >
> >  instead of
> >
> >  (= point 1)
> >
> >  ?
> >
> > This has the same issues as described above. There's more code in
> cursor-sensor--detect that assumes (eq
> > (current-buffer) (window-buffer window)), e.g. the form
> (get-char-property point 'cursor-sensor-functions).
> > Therefore (= point (with-current-buffer (window-buffer window)
> (point-min))) must be the same as (bobp).
> > Another mystery is the stack trace entry
> "run-hook-with-args(cursor-sensor--detect #<window 224 on
> > *Group*>)". How can cursor-sensor--detect be the first argument to
> run-hook-with-args? Shouldn't it always
> > be pre-redisplay-functions?
>
> If we want to dig deeper, I think we should first understand why bop
> doesn't work.  Is it indeed because the window's buffer is not the
> current buffer when the hook is called?
>

Unfortunately the error doesn't trigger consistently. Maybe we are lucky
and somebody manages to detect such an issue.

I'd also suggest to add
(cl-assert (eq (current-buffer) (window-buffer window)))
to cursor-sensor--detect, because the function relies on this equality.

[-- Attachment #2: Type: text/html, Size: 2664 bytes --]

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 14 Sep 2016 19:48:23 +0000
> Cc: 21730@debbugs.gnu.org, mwd@md5i.com
> 
>  If we want to dig deeper, I think we should first understand why bop
>  doesn't work. Is it indeed because the window's buffer is not the
>  current buffer when the hook is called?
> 
> Unfortunately the error doesn't trigger consistently. Maybe we are lucky and somebody manages to detect
> such an issue.
> 
> I'd also suggest to add
> (cl-assert (eq (current-buffer) (window-buffer window))) 
> to cursor-sensor--detect, because the function relies on this equality. 

Michael, could you please look some more into this?  I think we need
to understand the reason why bobp doesn't work, although the hook was
supposed to be called with the window's buffer the current buffer.

TIA

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Sergio Durigan Junior]

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

On Wednesday, September 14 2016, Eli Zaretskii wrote:

[ Resending because I have now really unarchived the bug.  Sorry about
  the dup. ]

>> From: Philipp Stephani <p.stephani2@gmail.com>
>> Date: Wed, 14 Sep 2016 19:11:57 +0000
>> 
>>  (with-current-buffer (window-buffer win)
>>  (run-hook-with-args 'pre-redisplay-functions win))
>>  to
>>  (run-hook-wrapped 'pre-redisplay-functions
>>  (lambda (func) (with-current-buffer (window-buffer win)
>>  (funcall func win)
>>  nil))
>>  or so.
>> 
>> I've attached a patch for this. 
>
> Thanks.  But we are going to entertain such changes, I think we really
> should be sure the issue with the current buffer is the culprit.  See
> my other message a few moments ago.

It is pretty hard to diagnose and investigate this issue, but I think
I've managed to progress a bit.

The problem doesn't seem to be on bobp, and I happen to share Philipp's
opinion that the real issue is the assumption that cursor-sensor--detect
will be at the right buffer when it's not.  Here, let me show you.

On my machine, this error always happens when I'm running Gnus in one
buffer (via emacsclient), and while I am visiting a file in another
buffer, and in another window (i.e., another emacsclient).  The
backtrace I get is:

  Debugger entered--Lisp error: (args-out-of-range 0)
    get-char-property(0 cursor-sensor-functions)
    cursor-sensor--detect(#<window 5650 on *Group*>)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^
    run-hook-with-args(cursor-sensor--detect #<window 5650 on *Group*>)
    redisplay--pre-redisplay-functions(t)
    apply(redisplay--pre-redisplay-functions t)
    [ lots of other functions not interesting for us ]

I took the liberty to mark the interesting part above, which is the
argument to cursor-sensor--detect.  As you can notice, it's working on
the *Group* window, from Gnus.  Now, the interesting part that I noticed
and was able to verify is that this only happens when (a) I am not
working in that buffer, (b) the point is at (point-min) at that buffer,
but (c) the point is not (point-min) at the buffer I'm currently in.  In
this case, Emacs does some wrong calculations with point and ends up
with the value of 0, which makes sense if you consider that
(window-point) of that window is 1, but (bobp) is not t (because, as I
explained, I'm working in another buffer, and the value of point there
is not 1).

With all that said, I came to the conclusion that Phillip's rationale
makes sense and therefore his patch seems to be the best shot we have at
fixing this annoying bug.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@sergiodj.net>
> Cc: Philipp Stephani <p.stephani2@gmail.com>,  21730@debbugs.gnu.org,  mwd@md5i.com
> Date: Mon, 24 Jul 2017 22:06:56 -0400
> 
>   Debugger entered--Lisp error: (args-out-of-range 0)
>     get-char-property(0 cursor-sensor-functions)
>     cursor-sensor--detect(#<window 5650 on *Group*>)
>                           ^^^^^^^^^^^^^^^^^^^^^^^^^
>     run-hook-with-args(cursor-sensor--detect #<window 5650 on *Group*>)
>     redisplay--pre-redisplay-functions(t)
>     apply(redisplay--pre-redisplay-functions t)
>     [ lots of other functions not interesting for us ]
> 
> I took the liberty to mark the interesting part above, which is the
> argument to cursor-sensor--detect.  As you can notice, it's working on
> the *Group* window, from Gnus.  Now, the interesting part that I noticed
> and was able to verify is that this only happens when (a) I am not
> working in that buffer, (b) the point is at (point-min) at that buffer,
> but (c) the point is not (point-min) at the buffer I'm currently in.  In
> this case, Emacs does some wrong calculations with point and ends up
> with the value of 0, which makes sense if you consider that
> (window-point) of that window is 1, but (bobp) is not t (because, as I
> explained, I'm working in another buffer, and the value of point there
> is not 1).
> 
> With all that said, I came to the conclusion that Phillip's rationale
> makes sense and therefore his patch seems to be the best shot we have at
> fixing this annoying bug.

Thanks for following up.

Phillip's hypothesis assumes that there's another function in
pre-redisplay-function, which is called before cursor-sensor--detect
and switches to another buffer, leaving us there.  Do you indeed have
other functions on that list?  If so, what are they?

I'm asking because I'd like to be sure we are on the right track with
the root cause.

bug#21730: 25.0.50; Random errors in redisplay--pre-redisplay-functions

[Sergio Durigan Junior]

On Tuesday, July 25 2017, Eli Zaretskii wrote:

>> From: Sergio Durigan Junior <sergiodj@sergiodj.net>
>> Cc: Philipp Stephani <p.stephani2@gmail.com>,  21730@debbugs.gnu.org,  mwd@md5i.com
>> Date: Mon, 24 Jul 2017 22:06:56 -0400
>> 
>>   Debugger entered--Lisp error: (args-out-of-range 0)
>>     get-char-property(0 cursor-sensor-functions)
>>     cursor-sensor--detect(#<window 5650 on *Group*>)
>>                           ^^^^^^^^^^^^^^^^^^^^^^^^^
>>     run-hook-with-args(cursor-sensor--detect #<window 5650 on *Group*>)
>>     redisplay--pre-redisplay-functions(t)
>>     apply(redisplay--pre-redisplay-functions t)
>>     [ lots of other functions not interesting for us ]
>> 
>> I took the liberty to mark the interesting part above, which is the
>> argument to cursor-sensor--detect.  As you can notice, it's working on
>> the *Group* window, from Gnus.  Now, the interesting part that I noticed
>> and was able to verify is that this only happens when (a) I am not
>> working in that buffer, (b) the point is at (point-min) at that buffer,
>> but (c) the point is not (point-min) at the buffer I'm currently in.  In
>> this case, Emacs does some wrong calculations with point and ends up
>> with the value of 0, which makes sense if you consider that
>> (window-point) of that window is 1, but (bobp) is not t (because, as I
>> explained, I'm working in another buffer, and the value of point there
>> is not 1).
>> 
>> With all that said, I came to the conclusion that Phillip's rationale
>> makes sense and therefore his patch seems to be the best shot we have at
>> fixing this annoying bug.
>
> Thanks for following up.
>
> Phillip's hypothesis assumes that there's another function in
> pre-redisplay-function, which is called before cursor-sensor--detect
> and switches to another buffer, leaving us there.  Do you indeed have
> other functions on that list?  If so, what are they?

You know, that's a good question.

After looking into what I have here, I don't see any other function
being called before pre-redisplay-functions.  Everything seems fine and
I see the call to with-current-buffer before calling run-hook-with-args,
which means that, unless there is a bug somewhere, I don't see how the
buffer could have changed.

> I'm asking because I'd like to be sure we are on the right track with
> the root cause.

Sure thing.  Well, if you ask me, I'm convinced that what I explained in
my last message is what happens, but I still don't know *why* it
happens.  I'll see if I manage to investigate this a bit more.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/