bug#30823: 25.3; modification-hooks of overlays are not run in some cases

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Ren Victor]

Two factors：

   1.  A commit of Emacs， modification-hooks might not be run in some cases:
    http://git.savannah.gnu.org/cgit/emacs.git/commit/src?id=564d811725596f15ecf543777e11504b47d2af86

   2.  In ggtags, an overlay is deleted in the overlay's modification-hooks:
    https://github.com/leoliu/ggtags/blob/eec392d2d639030c5a51bce8431f2815ad8e7bc5/ggtags.el#L2306

Deleted overlay ceases to be attached to the buffer. If the buffer of
the first overlay in the saved array doesn't match the current buffer,
then all the modification hooks will not be run in this buffer.

Thus modes that depends on modification-hooks won't work together with
ggtags-highlight-tag mode.

I think Emacs should support `delete-overlay' in modificaiton-hooks of
overlays, like ggtags.  So I report this bug.



In GNU Emacs 25.3.2 (x86_64-pc-linux-gnu, GTK+ Version 3.18.9)
 of 2017-09-13 built on lcy01-32
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000
System Description:    Ubuntu 16.04.4 LTS

Configured using:
 'configure --build=x86_64-linux-gnu --prefix=/usr
 '--includedir=${prefix}/include' '--mandir=${prefix}/share/man'
 '--infodir=${prefix}/share/info' --sysconfdir=/etc --localstatedir=/var
 --disable-silent-rules '--libdir=${prefix}/lib/x86_64-linux-gnu'
 '--libexecdir=${prefix}/lib/x86_64-linux-gnu' --disable-maintainer-mode
 --disable-dependency-tracking --prefix=/usr --sharedstatedir=/var/lib
 --program-suffix=25 --with-modules --with-x=yes --with-x-toolkit=gtk3
 'CFLAGS=-g -O2 -fstack-protector-strong -Wformat
 -Werror=format-security' 'CPPFLAGS=-Wdate-time -D_FORTIFY_SOURCE=2'
 'LDFLAGS=-Wl,-Bsymbolic-functions -Wl,-z,relro''

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 MODULES

Important settings:
  value of $LC_MONETARY: en_US.UTF-8
  value of $LC_NUMERIC: en_US.UTF-8
  value of $LC_TIME: en_US.UTF-8
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=fcitx
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  savehist-mode: t
  desktop-save-mode: t
  yas-global-mode: t
  yas-minor-mode: t
  pyvenv-mode: t
  diff-auto-refine-mode: t
  global-git-commit-mode: t
  async-bytecomp-package-mode: t
  shell-dirtrack-mode: t
  global-ede-mode: t
  ede-minor-mode: t
  global-semanticdb-minor-mode: t
  global-semantic-idle-scheduler-mode: t
  global-semantic-stickyfunc-mode: t
  semantic-mode: t
  outline-minor-mode: t
  winner-mode: t
  midnight-mode: t
  ido-ubiquitous-mode: t
  ido-everywhere: t
  show-paren-mode: t
  override-global-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent messages:
Quit
Mark set
Starting "look" process...
Spell-checking suspended; use C-u M-$ to resume
Quit
semantic-analyze-possible-completions: Nothing to complete
<C-return> is undefined
Mark set [3 times]
Mark saved where search started
Mark set
Quit [2 times]

Load-path shadows:
/home/victor/.emacs.d/site-lisp/other/emacs-goodies-el/htmlize hides
/home/victor/.emacs.d/elpa/htmlize-20161211.1019/htmlize
/home/victor/.emacs.d/site-lisp/other/xml-rpc hides
/home/victor/.emacs.d/elpa/xml-rpc-20160430.1458/xml-rpc
/usr/share/emacs/site-lisp/dictionaries-common/flyspell hides
/usr/share/emacs/25.3/lisp/textmodes/flyspell
/usr/share/emacs/site-lisp/dictionaries-common/ispell hides
/usr/share/emacs/25.3/lisp/textmodes/ispell

Features:
(shadow sort mail-extr emacsbug semantic/analyze/complete
semantic/db-typecache semantic/ia semantic/senator ispell misearch
multi-isearch semantic/tag-write time-stamp semantic/edit thingatpt
sh-script smie bug-reference inversion ede/locate ede/emacs ede/dired
ggtags ewoc vc-git semantic/tag-file semantic/db-file data-debug
cedet-files semantic/bovine/c semantic/decorate/include
semantic/decorate/mode semantic/decorate pulse hideif
semantic/bovine/c-by semantic/lex-spp semantic/bovine/gcc
semantic/bovine semantic/analyze/refs semantic/db-find semantic/db-ref
semantic/analyze semantic/sort semantic/scope semantic/analyze/fcn
eassist derived xcscope tempo-snippets tempo cc-mode cc-fonts cc-guess
cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs savehist
desktop frameset yasnippet highlight-indentation flymake company elpy
pyvenv elpy-profile elpy-django elpy-refactor python tramp-sh tramp
tramp-compat auth-source tramp-loaddefs trampver ucs-normalize json map
grep compile files-x etags xref project magit-bookmark magit-obsolete
magit-blame magit-stash magit-bisect magit-remote magit-commit
magit-sequence magit-notes magit-worktree magit-branch magit-files
magit-refs magit-status magit magit-repos magit-apply magit-wip
magit-log magit-diff smerge-mode diff-mode magit-core magit-autorevert
autorevert filenotify magit-process magit-margin magit-mode magit-git
magit-section magit-popup git-commit magit-utils crm log-edit message
rfc822 mml mml-sec password-cache epg mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader pcvs-util add-log
with-editor async-bytecomp async shell dash linum ascii ind-util
ede/speedbar ede/files ede ede/detect ede/base ede/auto ede/source
eieio-speedbar speedbar sb-image dframe eieio-custom semantic/dep
cedet-cscope semantic/db-mode semantic/db eieio-base semantic/idle
semantic/format ezimage semantic/tag-ls semantic/find semantic/ctxt
semantic/util-modes semantic/util semantic semantic/tag semantic/lex
semantic/fw eieio eieio-core mode-local cedet bookmark pp ox-latex
ox-icalendar ox-html ox-ascii ox-publish ox org-element org-w3m
org-rmail org-mhe org-irc org-info org-gnus gnus-util org-docview
doc-view subr-x jka-compr image-mode org-bibtex bibtex org-bbdb
org-timer org-agenda org-drill org-learn org-id hi-lock org org-macro
org-footnote org-pcomplete org-list org-faces org-entities foldout
noutline outline org-version ob-emacs-lisp ob ob-tangle ob-ref ob-lob
ob-table ob-exp org-src ob-keys ob-comint ob-core ob-eval org-compat
org-macs org-loaddefs format-spec review ediff-merg ediff-wind
ediff-diff ediff-mult ediff-help ediff-init ediff-util ediff clearcase
tq executable find-dired em-smart pcomplete comint ansi-color esh-var
esh-io esh-cmd esh-opt esh-ext esh-proc esh-arg esh-groups eshell
esh-module esh-mode esh-util windmove winner ring iedit-rect iedit
help-macro iedit-lib multiple-cursors-core advice rect sgml-mode server
find-func midnight timid dired-x dired ido-completing-read+ cl-seq
memoize s cus-edit ido avoid appt diary-lib diary-loaddefs cal-menu
calendar cal-loaddefs china-util color-theme edmacro kmacro wid-edit
sendmail rfc2047 rfc2045 ietf-drums mm-util help-fns mail-prsvr
mail-utils reporter tango-dark-theme which-func imenu paren cus-start
cus-load use-package diminish cl bind-key cl-macs easy-mmode finder-inf
info package epg-config seq byte-opt gv bytecomp byte-compile cl-extra
help-mode easymenu cconv cl-loaddefs pcase cl-lib time-date 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 move-toolbar gtk x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 827761 49911)
 (symbols 48 60403 0)
 (miscs 40 6321 1476)
 (strings 32 187894 21294)
 (string-bytes 1 5238739)
 (vectors 16 83599)
 (vector-slots 8 1620433 24408)
 (floats 8 1710 398)
 (intervals 56 11676 53)
 (buffers 976 45)
 (heap 1024 103137 3680))

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Eli Zaretskii]

> From: Ren Victor <victorhge@gmail.com>
> Date: Thu, 15 Mar 2018 12:15:57 +0800
> 
> Two factors：
> 
>    1.  A commit of Emacs， modification-hooks might not be run in some cases:
>     http://git.savannah.gnu.org/cgit/emacs.git/commit/src?id=564d811725596f15ecf543777e11504b47d2af86
> 
>    2.  In ggtags, an overlay is deleted in the overlay's modification-hooks:
>     https://github.com/leoliu/ggtags/blob/eec392d2d639030c5a51bce8431f2815ad8e7bc5/ggtags.el#L2306
> 
> Deleted overlay ceases to be attached to the buffer. If the buffer of
> the first overlay in the saved array doesn't match the current buffer,
> then all the modification hooks will not be run in this buffer.
> 
> Thus modes that depends on modification-hooks won't work together with
> ggtags-highlight-tag mode.
> 
> I think Emacs should support `delete-overlay' in modificaiton-hooks of
> overlays, like ggtags.  So I report this bug.

Thanks.  Can you provide a recipe starting from "emacs -Q" to
reproduce the problem?

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Ren Victor]

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

On Thu, Mar 15, 2018 at 2:00 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Ren Victor <victorhge@gmail.com>
>> Date: Thu, 15 Mar 2018 12:15:57 +0800
>>
>> Two factors：
>>
>>    1.  A commit of Emacs， modification-hooks might not be run in some cases:
>>     http://git.savannah.gnu.org/cgit/emacs.git/commit/src?id=564d811725596f15ecf543777e11504b47d2af86
>>
>>    2.  In ggtags, an overlay is deleted in the overlay's modification-hooks:
>>     https://github.com/leoliu/ggtags/blob/eec392d2d639030c5a51bce8431f2815ad8e7bc5/ggtags.el#L2306
>>
>> Deleted overlay ceases to be attached to the buffer. If the buffer of
>> the first overlay in the saved array doesn't match the current buffer,
>> then all the modification hooks will not be run in this buffer.
>>
>> Thus modes that depends on modification-hooks won't work together with
>> ggtags-highlight-tag mode.
>>
>> I think Emacs should support `delete-overlay' in modificaiton-hooks of
>> overlays, like ggtags.  So I report this bug.
>
> Thanks.  Can you provide a recipe starting from "emacs -Q" to
> reproduce the problem?

I wrote a ert case which is encolsed.

emacs -Q -batch -l ert -l bug30823.el -f ert-run-tests-batch-and-exit

[-- Attachment #2: bug30823.el --]
[-- Type: text/x-emacs-lisp, Size: 604 bytes --]

(require 'ert)

(ert-deftest test-modification-hooks ()
  "Test for bug#30823."
  (let ((check-point nil)
	(ov-delete nil)
	(ov-set nil))
    (with-temp-buffer
      (insert "abc")
      (setq ov-set (make-overlay 1 3))
      (overlay-put ov-set 'modification-hooks
		   (list (lambda (o after &rest _args)
			   (and after (setq check-point t)))))
      (setq ov-delete (make-overlay 1 3))
      (overlay-put ov-delete 'modification-hooks
		   (list (lambda (o after &rest _args)
			   (and (not after) (delete-overlay o)))))
      (goto-char 2)
      (insert "1")
      (should (eq check-point t)))))

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Noam Postavsky]

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

tags 30823 + patch
quit

Ren Victor <victorhge@gmail.com> writes:

> I wrote a ert case which is encolsed.
>
> emacs -Q -batch -l ert -l bug30823.el -f ert-run-tests-batch-and-exit

Thanks, this patch seems to fix it.


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

From 41cb2b33bc62a23a0561b94f3d25e1282935a08c Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 31 Mar 2018 09:33:41 -0400
Subject: [PATCH v1] Don't skip modification hooks if 1st overlay is deleted
 (Bug#30823)

The fix for Bug#21824 "Don't invoke overlay modification hooks in
wrong buffer" from 2015-11-06 prevented running of overlay hooks if
the first overlay registered when running the hooks with after=nil was
deleted (since a deleted overlay has no buffer, it was considered as
not from the current buffer).  Therefore, revert that change and
instead just inhibit modification hooks when performing message
coalescing (because in that case, we aren't doing the necessary
preparation for running modification hooks, or even running them with
after=nil at all).
* src/buffer.c (report_overlay_modification): Remove checking of
buffer overlay.
* src/xdisp.c (message_dolog): Let-bind inhibit-modification-hooks
to t around del_range_both calls
* test/src/buffer-tests.el (test-modification-hooks): New test.
---
 src/buffer.c             | 17 -----------------
 src/xdisp.c              |  9 +++++++++
 test/src/buffer-tests.el | 19 +++++++++++++++++++
 3 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 14837372d3..a5d65da2e8 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -4543,23 +4543,6 @@ report_overlay_modification (Lisp_Object start, Lisp_Object end, bool after,
     Lisp_Object *copy;
     ptrdiff_t i;
 
-    if (size)
-      {
-	Lisp_Object ovl
-	  = XVECTOR (last_overlay_modification_hooks)->contents[1];
-
-	/* If the buffer of the first overlay in the array doesn't
-	   match the current buffer, then these modification hooks
-	   should not be run in this buffer.  This could happen when
-	   some code calls some insdel functions, such as del_range_1,
-	   with the PREPARE argument false -- in that case this
-	   function is never called to record the overlay modification
-	   hook functions in the last_overlay_modification_hooks
-	   array, so anything we find there is not ours.  */
-	if (XMARKER (OVERLAY_START (ovl))->buffer != current_buffer)
-	  return;
-      }
-
     USE_SAFE_ALLOCA;
     SAFE_ALLOCA_LISP (copy, size);
     memcpy (copy, XVECTOR (last_overlay_modification_hooks)->contents,
diff --git a/src/xdisp.c b/src/xdisp.c
index df5335e4ac..082b40b742 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -10403,6 +10403,13 @@ message_dolog (const char *m, ptrdiff_t nbytes, bool nlflag, bool multibyte)
 	  ptrdiff_t this_bol, this_bol_byte, prev_bol, prev_bol_byte;
 	  printmax_t dups;
 
+          /* Since we call del_range_both passing false for PREPARE,
+             we aren't prepared to run modification hooks (we could
+             end up calling modification hooks from another buffer and
+             only with AFTER=t, Bug#21824).  */
+          ptrdiff_t count = SPECPDL_INDEX ();
+          specbind (Qinhibit_modification_hooks, Qt);
+
 	  insert_1_both ("\n", 1, 1, true, false, false);
 
 	  scan_newline (Z, Z_BYTE, BEG, BEG_BYTE, -2, false);
@@ -10448,6 +10455,8 @@ message_dolog (const char *m, ptrdiff_t nbytes, bool nlflag, bool multibyte)
 			    -XFASTINT (Vmessage_log_max) - 1, false);
 	      del_range_both (BEG, BEG_BYTE, PT, PT_BYTE, false);
 	    }
+
+          unbind_to (count, Qnil);
 	}
       BEGV = marker_position (oldbegv);
       BEGV_BYTE = marker_byte_position (oldbegv);
diff --git a/test/src/buffer-tests.el b/test/src/buffer-tests.el
index f9c477fbfd..5d091875b5 100644
--- a/test/src/buffer-tests.el
+++ b/test/src/buffer-tests.el
@@ -45,6 +45,25 @@
             (should (eq buf (current-buffer))))
         (when msg-ov (delete-overlay msg-ov))))))
 
+(ert-deftest test-modification-hooks ()
+  "Test for bug#30823."
+  (let ((check-point nil)
+	(ov-delete nil)
+	(ov-set nil))
+    (with-temp-buffer
+      (insert "abc")
+      (setq ov-set (make-overlay 1 3))
+      (overlay-put ov-set 'modification-hooks
+		   (list (lambda (_o after &rest _args)
+			   (and after (setq check-point t)))))
+      (setq ov-delete (make-overlay 1 3))
+      (overlay-put ov-delete 'modification-hooks
+		   (list (lambda (o after &rest _args)
+			   (and (not after) (delete-overlay o)))))
+      (goto-char 2)
+      (insert "1")
+      (should (eq check-point t)))))
+
 (ert-deftest test-generate-new-buffer-name-bug27966 ()
   (should-not (string-equal "nil"
                             (progn (get-buffer-create "nil")
-- 
2.11.0

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Noam Postavsky]

Noam Postavsky <npostavs@gmail.com> writes:

> @@ -10403,6 +10403,13 @@ message_dolog (const char *m, ptrdiff_t nbytes, bool nlflag, bool multibyte)
>  	  ptrdiff_t this_bol, this_bol_byte, prev_bol, prev_bol_byte;
>  	  printmax_t dups;
>  
> +          /* Since we call del_range_both passing false for PREPARE,
> +             we aren't prepared to run modification hooks (we could
> +             end up calling modification hooks from another buffer and
> +             only with AFTER=t, Bug#21824).  */
> +          ptrdiff_t count = SPECPDL_INDEX ();
> +          specbind (Qinhibit_modification_hooks, Qt);
> +
>  	  insert_1_both ("\n", 1, 1, true, false, false);
>  
>  	  scan_newline (Z, Z_BYTE, BEG, BEG_BYTE, -2, false);

Coming back to this, there is also the possibility of passing true for
PREPARE, though I'm not sure if that would be better or worse.  Any
comments?

(adding Stefan to Cc since I think this somewhat relates to/collides
with the patch for *Messages* buffer text properties in
https://lists.gnu.org/archive/html/emacs-devel/2018-05/msg00600.html).

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  30823@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 17 Aug 2018 16:52:54 -0400
> 
> Noam Postavsky <npostavs@gmail.com> writes:
> 
> > @@ -10403,6 +10403,13 @@ message_dolog (const char *m, ptrdiff_t nbytes, bool nlflag, bool multibyte)
> >  	  ptrdiff_t this_bol, this_bol_byte, prev_bol, prev_bol_byte;
> >  	  printmax_t dups;
> >  
> > +          /* Since we call del_range_both passing false for PREPARE,
> > +             we aren't prepared to run modification hooks (we could
> > +             end up calling modification hooks from another buffer and
> > +             only with AFTER=t, Bug#21824).  */
> > +          ptrdiff_t count = SPECPDL_INDEX ();
> > +          specbind (Qinhibit_modification_hooks, Qt);
> > +
> >  	  insert_1_both ("\n", 1, 1, true, false, false);
> >  
> >  	  scan_newline (Z, Z_BYTE, BEG, BEG_BYTE, -2, false);
> 
> Coming back to this, there is also the possibility of passing true for
> PREPARE, though I'm not sure if that would be better or worse.  Any
> comments?

AFAIR, we never want to use PREPARE = true when dealing with the
*Messages* buffer, you can see that elsewhere in message_dolog.  The
reason I believe is that we might trigger infinite recursion if the
modification hooks log a message for some reason.

Btw, I'm somewhat worried by the solution being proposed: it removes a
general safety device and replaces it by a solution that targets only
bug#21824, a much narrower class of problems.  Is that wise?

Can we turn the table and ask whether it makes sense to delete an
overlay from the modification hooks of that same overlay?  Maybe
ggtags needs to find a better/safer solution for whatever feature it
wants to implement?

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Stefan Monnier]

> Can we turn the table and ask whether it makes sense to delete an
> overlay from the modification hooks of that same overlay?

Yes, it very much does make sense: e.g. you want to keep track of
a "region unmodified" status, so you place an overlay over that region
with a modification hook that sets a variable to nil to indicate that
the region was modified, and once that is done there's no point in
keeping the overlay any more so you can delete it immediately from that
modification-hook.


        Stefan

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: Noam Postavsky <npostavs@gmail.com>, victorhge@gmail.com,
>         30823@debbugs.gnu.org
> Date: Sat, 18 Aug 2018 23:48:02 -0400
> 
> > Can we turn the table and ask whether it makes sense to delete an
> > overlay from the modification hooks of that same overlay?
> 
> Yes, it very much does make sense: e.g. you want to keep track of
> a "region unmodified" status, so you place an overlay over that region
> with a modification hook that sets a variable to nil to indicate that
> the region was modified, and once that is done there's no point in
> keeping the overlay any more so you can delete it immediately from that
> modification-hook.

I see that I tried too hard to be gentle, and that must have made my
question unclear, because that's not what I was asking.  I was asking
whether we want to support code which does this, because maybe it is
unreasonable to delete an overlay from within its modification hook.

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Stefan Monnier]

> I see that I tried too hard to be gentle, and that must have made my
> question unclear, because that's not what I was asking.  I was asking
> whether we want to support code which does this, because maybe it is
> unreasonable to delete an overlay from within its modification hook.

My answer was saying that yes we want to support that.  I don't see
a good reason why this should be technically difficult to support.


        Stefan

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: npostavs@gmail.com, victorhge@gmail.com, 30823@debbugs.gnu.org
> Date: Sun, 19 Aug 2018 11:43:57 -0400
> 
> My answer was saying that yes we want to support that.  I don't see
> a good reason why this should be technically difficult to support.

Well, the bugs in question are one reason.

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Stefan Monnier]

>> Coming back to this, there is also the possibility of passing true for
>> PREPARE, though I'm not sure if that would be better or worse.  Any
>> comments?
> AFAIR, we never want to use PREPARE = true when dealing with the
> *Messages* buffer, you can see that elsewhere in message_dolog.  The
> reason I believe is that we might trigger infinite recursion if the
> modification hooks log a message for some reason.

The current code already allows running `message` in this way (and that
leads to suboptimal behavior, tho nothing really serious).  I think we
should use `true` here and then actively try and detect nested uses of
`message` and deal with those in an ad-hoc way (e.g. bind
inhibit-modification-hooks during the nested call so the recursion is at
most 2 deep).

The benefit is that it makes this part of the code more "normal" and
will probably fix/avoid other bugs like this one.

The patch I sent in
https://lists.gnu.org/archive/html/emacs-devel/2018-05/msg00600.html
went in this direction and my experimentation with it did not encounter
any serious problem.  IOW I think the comment near message_dolog is
largely out of date.


        Stefan

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  >   I was asking
  > > whether we want to support code which does this, because maybe it is
  > > unreasonable to delete an overlay from within its modification hook.

I think I see natural occasions to want to do just that, so I think we had
better support it.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: Noam Postavsky <npostavs@gmail.com>, victorhge@gmail.com,
>         30823@debbugs.gnu.org
> Date: Sun, 19 Aug 2018 16:46:09 -0400
> 
> >> Coming back to this, there is also the possibility of passing true for
> >> PREPARE, though I'm not sure if that would be better or worse.  Any
> >> comments?
> > AFAIR, we never want to use PREPARE = true when dealing with the
> > *Messages* buffer, you can see that elsewhere in message_dolog.  The
> > reason I believe is that we might trigger infinite recursion if the
> > modification hooks log a message for some reason.
> 
> The current code already allows running `message` in this way (and that
> leads to suboptimal behavior, tho nothing really serious).  I think we
> should use `true` here and then actively try and detect nested uses of
> `message` and deal with those in an ad-hoc way (e.g. bind
> inhibit-modification-hooks during the nested call so the recursion is at
> most 2 deep).

That doesn't cater to some of the uses of 'message', as I explained in
the discussion to which you pointed.

> The patch I sent in
> https://lists.gnu.org/archive/html/emacs-devel/2018-05/msg00600.html
> went in this direction and my experimentation with it did not encounter
> any serious problem.  IOW I think the comment near message_dolog is
> largely out of date.

Once again, interested readers may wish to read the whole discussion,
because some of the issues raised there are not taken care of by this
function, and some of the comment is justified, as I tried to explain.

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Cc: eliz@gnu.org, victorhge@gmail.com, npostavs@gmail.com,
> 	30823@debbugs.gnu.org
> Date: Sun, 19 Aug 2018 23:02:54 -0400
> 
>   >   I was asking
>   > > whether we want to support code which does this, because maybe it is
>   > > unreasonable to delete an overlay from within its modification hook.
> 
> I think I see natural occasions to want to do just that, so I think we had
> better support it.

I have no doubt that it's be nice to have.  However, there are
practical difficulties with allowing that, and in particular a simple
enough device I added to try to support it doesn't work in a slightly
more complicated case.  When this stuff fails, it is usually goes up
in smoke, and debugging that is not easy.  So if someone has better
ideas, patches are welcome.

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

> Btw, I'm somewhat worried by the solution being proposed: it removes a
> general safety device and replaces it by a solution that targets only
> bug#21824, a much narrower class of problems.  Is that wise?

IMO, a safety device which causes new bugs is disqualified from its job.
So yes, replacing this device with a more targeted fix seems like the
Right Thing to me.  Furthermore, we're currently calling the after
change hooks without the before change hooks which is just asking for
trouble (as exemplified by Bug#21824 and this one).

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: victorhge@gmail.com,  30823@debbugs.gnu.org,  monnier@iro.umontreal.ca
> Date: Thu, 23 Aug 2018 08:13:59 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Btw, I'm somewhat worried by the solution being proposed: it removes a
> > general safety device and replaces it by a solution that targets only
> > bug#21824, a much narrower class of problems.  Is that wise?
> 
> IMO, a safety device which causes new bugs is disqualified from its job.

I'm not sure it caused a new bug.

I'm hard pressed for free time lately, so I'd be grateful if you could
see whether it would be possible to make the original change smarter,
so that it avoids causing the current issue.  If not, I will try to
look into it in a couple of weeks or so.

> Furthermore, we're currently calling the after change hooks without
> the before change hooks which is just asking for trouble (as
> exemplified by Bug#21824 and this one).

That's a separate issue, isn't it?  We could refrain from calling the
after-change hooks as well.

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noam Postavsky <npostavs@gmail.com>
>> Cc: victorhge@gmail.com,  30823@debbugs.gnu.org,  monnier@iro.umontreal.ca
>> Date: Thu, 23 Aug 2018 08:13:59 -0400
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Btw, I'm somewhat worried by the solution being proposed: it removes a
>> > general safety device and replaces it by a solution that targets only
>> > bug#21824, a much narrower class of problems.  Is that wise?
>> 
>> IMO, a safety device which causes new bugs is disqualified from its job.
>
> I'm not sure it caused a new bug.

It causes it in the sense that reverting the fix for #21284 stops this
new bug from happening.

> I'm hard pressed for free time lately, so I'd be grateful if you could
> see whether it would be possible to make the original change smarter,
> so that it avoids causing the current issue.  If not, I will try to
> look into it in a couple of weeks or so.
>
>> Furthermore, we're currently calling the after change hooks without
>> the before change hooks which is just asking for trouble (as
>> exemplified by Bug#21824 and this one).
>
> That's a separate issue, isn't it?  We could refrain from calling the
> after-change hooks as well.

It's not a separate issue.  The original reason for #21824 is that we
called the after-change hooks without doing the setup (i.e., passing
PREPARE=false to del_range_both).  With the addition of the "safety
device", #21824 is avoided, but this bug is caused instead.  Refraining
from calling after-change hooks is exactly what my patch does, this
fixes both cases.

This makes the "safety device" redundant, but with the after-change
suppression added it doesn't do any harm; so if you insist, we can leave
it in.  I don't think it's a good idea to have such things cluttering up
the source though.

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: victorhge@gmail.com,  30823@debbugs.gnu.org,  monnier@iro.umontreal.ca
> Date: Thu, 30 Aug 2018 23:14:53 -0400
> 
> This makes the "safety device" redundant, but with the after-change
> suppression added it doesn't do any harm; so if you insist, we can leave
> it in.  I don't think it's a good idea to have such things cluttering up
> the source though.

Not sure I follow this part: are you saying that we shouldn't protect
ourselves from overlay modification hooks that record a wrong buffer?

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noam Postavsky <npostavs@gmail.com>
>> Cc: victorhge@gmail.com,  30823@debbugs.gnu.org,  monnier@iro.umontreal.ca
>> Date: Thu, 30 Aug 2018 23:14:53 -0400
>> 
>> This makes the "safety device" redundant, but with the after-change
>> suppression added it doesn't do any harm; so if you insist, we can leave
>> it in.  I don't think it's a good idea to have such things cluttering up
>> the source though.
>
> Not sure I follow this part: are you saying that we shouldn't protect
> ourselves from overlay modification hooks that record a wrong buffer?

Hmm, I'm not sure I follow you on this.  As far as I can tell, it rather
protects against a particular bug in the C code: calling modification
hooks without calling prepare_to_modify_buffer.  Once this is fixed,
there is no need for it.  Furthermore, the "protection" is somewhat
dubious, since it also prevents running hooks in the correct buffer
(i.e., this bug).

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: victorhge@gmail.com,  30823@debbugs.gnu.org,  monnier@iro.umontreal.ca
> Date: Sat, 01 Sep 2018 12:38:19 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Noam Postavsky <npostavs@gmail.com>
> >> Cc: victorhge@gmail.com,  30823@debbugs.gnu.org,  monnier@iro.umontreal.ca
> >> Date: Thu, 30 Aug 2018 23:14:53 -0400
> >> 
> >> This makes the "safety device" redundant, but with the after-change
> >> suppression added it doesn't do any harm; so if you insist, we can leave
> >> it in.  I don't think it's a good idea to have such things cluttering up
> >> the source though.
> >
> > Not sure I follow this part: are you saying that we shouldn't protect
> > ourselves from overlay modification hooks that record a wrong buffer?
> 
> Hmm, I'm not sure I follow you on this.  As far as I can tell, it rather
> protects against a particular bug in the C code: calling modification
> hooks without calling prepare_to_modify_buffer.

No, the protection was meant to be more general: to avoid calling
overlay modification hooks when the overlay in question is from the
wrong buffer.  The particular bug in C code which unearthed the
problem was just one such case, but we have no reason to believe that
it's the only such case.

I'm not opposed to making the change you suggested for xdisp.c
(although maybe it should go to master, not to emacs-26), but I would
like to keep the protection in buffer.c.  It just needs to be more
fine-grained, to avoid causing adverse side effects, such as the
problem reported here.

With that in mind, WDYT about the patch below, which replaces the
buffer.c portion of your patch?  I've ran the tests for both bug#21824
and for this bug, and they both pass with the patch installed and with
unmodified xdisp.c.

Thanks.

diff --git a/src/buffer.c b/src/buffer.c
index b0cee71..179360c 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -4543,23 +4543,6 @@ report_overlay_modification (Lisp_Object start, Lisp_Object end, bool after,
     Lisp_Object *copy;
     ptrdiff_t i;
 
-    if (size)
-      {
-	Lisp_Object ovl
-	  = XVECTOR (last_overlay_modification_hooks)->contents[1];
-
-	/* If the buffer of the first overlay in the array doesn't
-	   match the current buffer, then these modification hooks
-	   should not be run in this buffer.  This could happen when
-	   some code calls some insdel functions, such as del_range_1,
-	   with the PREPARE argument false -- in that case this
-	   function is never called to record the overlay modification
-	   hook functions in the last_overlay_modification_hooks
-	   array, so anything we find there is not ours.  */
-	if (XMARKER (OVERLAY_START (ovl))->buffer != current_buffer)
-	  return;
-      }
-
     USE_SAFE_ALLOCA;
     SAFE_ALLOCA_LISP (copy, size);
     memcpy (copy, XVECTOR (last_overlay_modification_hooks)->contents,
@@ -4570,7 +4553,12 @@ report_overlay_modification (Lisp_Object start, Lisp_Object end, bool after,
 	Lisp_Object prop_i, overlay_i;
 	prop_i = copy[i++];
 	overlay_i = copy[i++];
-	call_overlay_mod_hooks (prop_i, overlay_i, after, arg1, arg2, arg3);
+	/* It is possible that the recorded overlay has been deleted
+	   (which makes its markers' buffers be nil), or that (due to
+	   some bug) it belongs to a different buffer.  Only run this
+	   hook if the overlay belongs to the current buffer.  */
+	if (XMARKER (OVERLAY_START (overlay_i))->buffer == current_buffer)
+	  call_overlay_mod_hooks (prop_i, overlay_i, after, arg1, arg2, arg3);
       }
 
     SAFE_FREE ();

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

> No, the protection was meant to be more general: to avoid calling
> overlay modification hooks when the overlay in question is from the
> wrong buffer.

Ah, well I see your new patch fulfills this mission better (the old one
only looked the first overlay, so it seemed rather specific to
bug#21824).

> I'm not opposed to making the change you suggested for xdisp.c
> (although maybe it should go to master, not to emacs-26), but I would
> like to keep the protection in buffer.c.

Funny, I feel the same but in reverse.  Your patch should only affect
the case where overlays are deleted/moved by modification hooks which is
already a grey area, so the change is *probably* okay; but I would put
it in master in case of unforseen side effects.

> With that in mind, WDYT about the patch below, which replaces the
> buffer.c portion of your patch?  I've ran the tests for both bug#21824
> and for this bug, and they both pass with the patch installed and with
> unmodified xdisp.c.

I can confirm it works, and the change seems generally sensible.  I
think it does make sense to have the xdisp.c change as well.  The choice
of branch is up to you, of course.

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: victorhge@gmail.com,  30823@debbugs.gnu.org,  monnier@iro.umontreal.ca
> Date: Wed, 12 Sep 2018 21:34:37 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > No, the protection was meant to be more general: to avoid calling
> > overlay modification hooks when the overlay in question is from the
> > wrong buffer.
> 
> Ah, well I see your new patch fulfills this mission better (the old one
> only looked the first overlay, so it seemed rather specific to
> bug#21824).

Yes, because the original change only considered the case of a wrong
buffer, it didn't consider the case of a deleted overlay, where the
buffer is nil.

> > I'm not opposed to making the change you suggested for xdisp.c
> > (although maybe it should go to master, not to emacs-26), but I would
> > like to keep the protection in buffer.c.
> 
> Funny, I feel the same but in reverse.  Your patch should only affect
> the case where overlays are deleted/moved by modification hooks which is
> already a grey area, so the change is *probably* okay; but I would put
> it in master in case of unforseen side effects.

My rationale was that the changes in buffer.c fix a regression,
whereas the changes in xdisp.c fix a potential problem for which we
don't yet have a bug report.

> I can confirm it works, and the change seems generally sensible.  I
> think it does make sense to have the xdisp.c change as well.  The choice
> of branch is up to you, of course.

Well, unless you feel strongly against, I'd prefer to have the xdisp.c
change on master, and the buffer.c change (with the added test) on
emacs-26.

Thanks.

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

> Well, unless you feel strongly against, I'd prefer to have the xdisp.c
> change on master, and the buffer.c change (with the added test) on
> emacs-26.

I'm okay with that.

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: victorhge@gmail.com,  30823@debbugs.gnu.org,  monnier@iro.umontreal.ca
> Date: Fri, 14 Sep 2018 08:03:21 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Well, unless you feel strongly against, I'd prefer to have the xdisp.c
> > change on master, and the buffer.c change (with the added test) on
> > emacs-26.
> 
> I'm okay with that.

OK, I've now pushed the buffer.c changes to the emacs-26 branch.
Please push the xdisp.c changes to master, and then we can close this
bug report.

Thanks.

bug#30823: 25.3; modification-hooks of overlays are not run in some cases

[Noam Postavsky]

tags 30823 fixed
close 30823 26.2
quit

Eli Zaretskii <eliz@gnu.org> writes:

> OK, I've now pushed the buffer.c changes to the emacs-26 branch.
> Please push the xdisp.c changes to master, and then we can close this
> bug report.

Done.

[1: ffbe561ee5]: 2018-09-15 09:44:30 -0400
  Don't call modification hooks unprepared
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=ffbe561ee5acb0b9edc5f4c995c287fb2485c315