bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Pip Cet <pipcet@gmail.com>]

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

On Thu, Jun 13, 2019 at 4:36 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Thu, 13 Jun 2019 13:48:40 +0000
> >
> > I've looked at the code, and it appears no particular provisions are
> > being made to make sure we switch to the modified buffer before
> > calling signal_after_change().
> >
> > As far as I can tell, this makes `put-text-property' with a buffer
> > argument pretty useless.
>
> Only if you have a buffer-local value of after-change-functions.

I'm not sure what you're saying. I'm seeing weird behavior in these cases:
 - buffer-local value of after-change-functions
 - global value of after-change functions (current-buffer is wrong!)
 - overlay property modification-hooks

(let ((buffer1 (generate-new-buffer "A"))
      (buffer2 (generate-new-buffer "B")))
  (with-current-buffer buffer2
    (insert "BBB"))
  (with-current-buffer buffer1
    (insert "AAA")
    (overlay-put (make-overlay 1 4 buffer1) 'modification-hooks
         (list (lambda (&rest args)
           (message "%S"
                args))))
    (goto-char 2)
    (insert "AAA")
    (put-text-property 1 4 'read-only t buffer2)))

produces three calls to the lambda, but should produce only two. (The
last_overlay_modification_hooks logic is a bit weird, thus the second
insertion).

That seems pretty wrong to me. In which cases do you think we're
seeing the right behavior?

Here's a first patch, which adds a "buffer" argument to
signal_after_change, to be explicit about where the change happens. It
should be pretty cheap in the case where we don't switch buffers.

[-- Attachment #2: 0001-Switch-to-modified-buffer-in-signal_after_change.patch --]
[-- Type: text/x-patch, Size: 17265 bytes --]

From ff40a90cc1e4ade9848b6cb01ea9e415d4e82d8f Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Thu, 13 Jun 2019 14:06:46 +0000
Subject: [PATCH] Switch to modified buffer in signal_after_change

* src/insdel.c (signal_after_change): Add buffer argument.
* src/casefiddle.c (casify_region),
  src/decompress.c (unwind_decompress, Fzlib_decompress_region),
  src/editfns.c (Freplace_buffer_contents, Ftranslate_region_internal,
  Ftranspose_regions), src/fileio.c (Finsert_file_contents),
  src/fns.c (Fbase64_decode_region), src/insdel.c (insert,
  insert_and_inherit, insert_before_markers,
  insert_before_markers_and_inherit, insert_from_string,
  insert_from_string_before_markers, insert_from_buffer,
  replace_range, del_range_1, del_range_byte, del_range_both,
  Fcombine_after_change_execute), src/json.c (Fjson_insert),
  src/print.c (PRINTFINISH): use current_buffer.
* src/textprop.c (add_text_properties_1, set_text_properties,
  Fremove_text_properties, Fremove_list_of_text_properties): use
  correct buffer.
---
 src/casefiddle.c |  2 +-
 src/decompress.c |  4 ++--
 src/editfns.c    |  8 ++++----
 src/fileio.c     |  2 +-
 src/fns.c        |  2 +-
 src/insdel.c     | 30 +++++++++++++++++-------------
 src/json.c       |  2 +-
 src/lisp.h       |  2 +-
 src/print.c      |  3 ++-
 src/textprop.c   | 22 +++++++++++-----------
 10 files changed, 41 insertions(+), 36 deletions(-)

diff --git a/src/casefiddle.c b/src/casefiddle.c
index 3f407eaded..d7e29d10ae 100644
--- a/src/casefiddle.c
+++ b/src/casefiddle.c
@@ -509,7 +509,7 @@ casify_region (enum case_action flag, Lisp_Object b, Lisp_Object e)
 
   if (start >= 0)
     {
-      signal_after_change (start, end - start - added, end - start);
+      signal_after_change (current_buffer, start, end - start - added, end - start);
       update_compositions (start, end, CHECK_ALL);
     }
 
diff --git a/src/decompress.c b/src/decompress.c
index 4ca6a50b2a..831cc78fe5 100644
--- a/src/decompress.c
+++ b/src/decompress.c
@@ -88,7 +88,7 @@ unwind_decompress (void *ddata)
       update_compositions (data->start, data->start, CHECK_HEAD);
       /* "Balance" the before-change-functions call, which would
          otherwise be left "hanging". */
-      signal_after_change (data->orig, data->start - data->orig,
+      signal_after_change (current_buffer, data->orig, data->start - data->orig,
                            data->start - data->orig);
     }
   /* Put point where it was, or if the buffer has shrunk because the
@@ -227,7 +227,7 @@ DEFUN ("zlib-decompress-region", Fzlib_decompress_region,
   del_range_2 (istart, istart, /* byte and char offsets are the same. */
                iend, iend, 0);
 
-  signal_after_change (istart, iend - istart, unwind_data.nbytes);
+  signal_after_change (current_buffer, istart, iend - istart, unwind_data.nbytes);
   update_compositions (istart, istart, CHECK_HEAD);
 
   return unbind_to (count, ret);
diff --git a/src/editfns.c b/src/editfns.c
index ee538e50e2..9ada23ff61 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -2148,7 +2148,7 @@ DEFUN ("replace-buffer-contents", Freplace_buffer_contents,
 
   if (modification_hooks_inhibited)
     {
-      signal_after_change (BEGV, size_a, ZV - BEGV);
+      signal_after_change (current_buffer, BEGV, size_a, ZV - BEGV);
       update_compositions (BEGV, ZV, CHECK_INSIDE);
     }
 
@@ -2412,7 +2412,7 @@ #define COMBINING_BOTH (COMBINING_BEFORE | COMBINING_AFTER)
 
   if (changed > 0)
     {
-      signal_after_change (changed,
+      signal_after_change (current_buffer, changed,
 			   last_changed - changed, last_changed - changed);
       update_compositions (changed, last_changed, CHECK_ALL);
     }
@@ -2600,7 +2600,7 @@ DEFUN ("translate-region-internal", Ftranslate_region_internal,
 		  record_change (pos, 1);
 		  while (str_len-- > 0)
 		    *p++ = *str++;
-		  signal_after_change (pos, 1, 1);
+		  signal_after_change (current_buffer, pos, 1, 1);
 		  update_compositions (pos, pos + 1, CHECK_BORDER);
 		}
 	      characters_changed++;
@@ -4445,7 +4445,7 @@ DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5,
       adjust_markers_bytepos (start1, start1_byte, end2, end2_byte, 0);
     }
 
-  signal_after_change (start1, end2 - start1, end2 - start1);
+  signal_after_change (current_buffer, start1, end2 - start1, end2 - start1);
   return Qnil;
 }
 
diff --git a/src/fileio.c b/src/fileio.c
index b141f568d7..056da2cb9d 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -4555,7 +4555,7 @@ because (1) it preserves some marker positions and (2) it puts less data
   if (inserted > 0 && total > 0
       && (NILP (visit) || !NILP (replace)))
     {
-      signal_after_change (PT, 0, inserted);
+      signal_after_change (current_buffer, PT, 0, inserted);
       update_compositions (PT, PT, CHECK_BORDER);
     }
 
diff --git a/src/fns.c b/src/fns.c
index eaa2c07fbe..4b0c271b46 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -3577,7 +3577,7 @@ DEFUN ("base64-decode-region", Fbase64_decode_region, Sbase64_decode_region,
      and delete the old.  (Insert first in order to preserve markers.)  */
   TEMP_SET_PT_BOTH (XFIXNAT (beg), ibeg);
   insert_1_both (decoded, inserted_chars, decoded_length, 0, 1, 0);
-  signal_after_change (XFIXNAT (beg), 0, inserted_chars);
+  signal_after_change (current_buffer, XFIXNAT (beg), 0, inserted_chars);
   SAFE_FREE ();
 
   /* Delete the original text.  */
diff --git a/src/insdel.c b/src/insdel.c
index 85fffd8fd1..f9499a6ab3 100644
--- a/src/insdel.c
+++ b/src/insdel.c
@@ -669,7 +669,7 @@ insert (const char *string, ptrdiff_t nbytes)
       ptrdiff_t len = chars_in_text ((unsigned char *) string, nbytes), opoint;
       insert_1_both (string, len, nbytes, 0, 1, 0);
       opoint = PT - len;
-      signal_after_change (opoint, 0, len);
+      signal_after_change (current_buffer, opoint, 0, len);
       update_compositions (opoint, PT, CHECK_BORDER);
     }
 }
@@ -684,7 +684,7 @@ insert_and_inherit (const char *string, ptrdiff_t nbytes)
       ptrdiff_t len = chars_in_text ((unsigned char *) string, nbytes), opoint;
       insert_1_both (string, len, nbytes, 1, 1, 0);
       opoint = PT - len;
-      signal_after_change (opoint, 0, len);
+      signal_after_change (current_buffer, opoint, 0, len);
       update_compositions (opoint, PT, CHECK_BORDER);
     }
 }
@@ -729,7 +729,7 @@ insert_before_markers (const char *string, ptrdiff_t nbytes)
       ptrdiff_t len = chars_in_text ((unsigned char *) string, nbytes), opoint;
       insert_1_both (string, len, nbytes, 0, 1, 1);
       opoint = PT - len;
-      signal_after_change (opoint, 0, len);
+      signal_after_change (current_buffer, opoint, 0, len);
       update_compositions (opoint, PT, CHECK_BORDER);
     }
 }
@@ -745,7 +745,7 @@ insert_before_markers_and_inherit (const char *string,
       ptrdiff_t len = chars_in_text ((unsigned char *) string, nbytes), opoint;
       insert_1_both (string, len, nbytes, 1, 1, 1);
       opoint = PT - len;
-      signal_after_change (opoint, 0, len);
+      signal_after_change (current_buffer, opoint, 0, len);
       update_compositions (opoint, PT, CHECK_BORDER);
     }
 }
@@ -959,7 +959,7 @@ insert_from_string (Lisp_Object string, ptrdiff_t pos, ptrdiff_t pos_byte,
 
   insert_from_string_1 (string, pos, pos_byte, length, length_byte,
 			inherit, 0);
-  signal_after_change (opoint, 0, PT - opoint);
+  signal_after_change (current_buffer, opoint, 0, PT - opoint);
   update_compositions (opoint, PT, CHECK_BORDER);
 }
 
@@ -979,7 +979,7 @@ insert_from_string_before_markers (Lisp_Object string,
 
   insert_from_string_1 (string, pos, pos_byte, length, length_byte,
 			inherit, 1);
-  signal_after_change (opoint, 0, PT - opoint);
+  signal_after_change (current_buffer, opoint, 0, PT - opoint);
   update_compositions (opoint, PT, CHECK_BORDER);
 }
 
@@ -1135,7 +1135,7 @@ insert_from_buffer (struct buffer *buf,
   ptrdiff_t opoint = PT;
 
   insert_from_buffer_1 (buf, charpos, nchars, inherit);
-  signal_after_change (opoint, 0, PT - opoint);
+  signal_after_change (current_buffer, opoint, 0, PT - opoint);
   update_compositions (opoint, PT, CHECK_BORDER);
 }
 
@@ -1530,7 +1530,7 @@ replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object new,
   if (adjust_match_data)
     update_search_regs (from, to, from + SCHARS (new));
 
-  signal_after_change (from, nchars_del, GPT - from);
+  signal_after_change (current_buffer, from, nchars_del, GPT - from);
   update_compositions (from, GPT, CHECK_BORDER);
 }
 \f
@@ -1698,7 +1698,7 @@ del_range_1 (ptrdiff_t from, ptrdiff_t to, bool prepare, bool ret_string)
   to_byte = CHAR_TO_BYTE (to);
 
   deletion = del_range_2 (from, from_byte, to, to_byte, ret_string);
-  signal_after_change (from, to - from, 0);
+  signal_after_change (current_buffer, from, to - from, 0);
   update_compositions (from, from, CHECK_HEAD);
   return deletion;
 }
@@ -1740,7 +1740,7 @@ del_range_byte (ptrdiff_t from_byte, ptrdiff_t to_byte)
   }
 
   del_range_2 (from, from_byte, to, to_byte, 0);
-  signal_after_change (from, to - from, 0);
+  signal_after_change (current_buffer, from, to - from, 0);
   update_compositions (from, from, CHECK_HEAD);
 }
 
@@ -1784,7 +1784,7 @@ del_range_both (ptrdiff_t from, ptrdiff_t from_byte,
     }
 
   del_range_2 (from, from_byte, to, to_byte, 0);
-  signal_after_change (from, to - from, 0);
+  signal_after_change (current_buffer, from, to - from, 0);
   update_compositions (from, from, CHECK_HEAD);
 }
 
@@ -2174,7 +2174,8 @@ signal_before_change (ptrdiff_t start_int, ptrdiff_t end_int,
    after the change.  */
 
 void
-signal_after_change (ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
+signal_after_change (struct buffer *buffer,
+                     ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
 {
   ptrdiff_t count = SPECPDL_INDEX ();
   struct rvoe_arg rvoe_arg;
@@ -2183,6 +2184,9 @@ signal_after_change (ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
   if (inhibit_modification_hooks)
     return;
 
+  record_unwind_current_buffer ();
+  set_buffer_internal (buffer);
+
   /* If we are deferring calls to the after-change functions
      and there are no before-change functions,
      just record the args that we were going to use.  */
@@ -2338,7 +2342,7 @@ DEFUN ("combine-after-change-execute", Fcombine_after_change_execute,
      Turn off the flag that defers them.  */
   record_unwind_protect (Fcombine_after_change_execute_1,
 			 Vcombine_after_change_calls);
-  signal_after_change (begpos, endpos - begpos - change, endpos - begpos);
+  signal_after_change (current_buffer, begpos, endpos - begpos - change, endpos - begpos);
   update_compositions (begpos, endpos, CHECK_ALL);
 
   return unbind_to (count, Qnil);
diff --git a/src/json.c b/src/json.c
index e2a4424463..ecb47f3ff4 100644
--- a/src/json.c
+++ b/src/json.c
@@ -789,7 +789,7 @@ DEFUN ("json-insert", Fjson_insert, Sjson_insert, 1, MANY,
     }
 
   /* Call after-change hooks.  */
-  signal_after_change (PT, 0, inserted);
+  signal_after_change (current_buffer, PT, 0, inserted);
   if (inserted > 0)
     {
       update_compositions (PT, PT, CHECK_BORDER);
diff --git a/src/lisp.h b/src/lisp.h
index 77fc22d118..a1ed4a3ce9 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3685,7 +3685,7 @@ #define CONS_TO_INTEGER(cons, type, var)				\
 extern void prepare_to_modify_buffer (ptrdiff_t, ptrdiff_t, ptrdiff_t *);
 extern void prepare_to_modify_buffer_1 (ptrdiff_t, ptrdiff_t, ptrdiff_t *);
 extern void invalidate_buffer_caches (struct buffer *, ptrdiff_t, ptrdiff_t);
-extern void signal_after_change (ptrdiff_t, ptrdiff_t, ptrdiff_t);
+extern void signal_after_change (struct buffer *, ptrdiff_t, ptrdiff_t, ptrdiff_t);
 extern void adjust_after_insert (ptrdiff_t, ptrdiff_t, ptrdiff_t,
 				 ptrdiff_t, ptrdiff_t);
 extern void adjust_markers_for_delete (ptrdiff_t, ptrdiff_t,
diff --git a/src/print.c b/src/print.c
index 406abbf4a3..e94eaf36f9 100644
--- a/src/print.c
+++ b/src/print.c
@@ -178,7 +178,8 @@ #define PRINTFINISH							\
        else								\
 	 insert_1_both (print_buffer, print_buffer_pos,			\
 			print_buffer_pos_byte, 0, 1, 0);		\
-       signal_after_change (PT - print_buffer_pos, 0, print_buffer_pos);\
+       signal_after_change (current_buffer, PT - print_buffer_pos,      \
+                            0, print_buffer_pos);                       \
      }									\
    if (free_print_buffer)						\
      {									\
diff --git a/src/textprop.c b/src/textprop.c
index ae42c44185..293ac02d9c 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -1215,7 +1215,7 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 	  if (interval_has_all_properties (properties, i))
 	    {
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+		signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				     XFIXNUM (end) - XFIXNUM (start));
 
 	      eassert (modified);
@@ -1226,7 +1226,7 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 	    {
 	      add_properties (properties, i, object, set_type);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+		signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				     XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
@@ -1237,7 +1237,7 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 	  copy_properties (unchanged, i);
 	  add_properties (properties, i, object, set_type);
 	  if (BUFFERP (object))
-	    signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+	    signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				 XFIXNUM (end) - XFIXNUM (start));
 	  return Qt;
 	}
@@ -1398,7 +1398,7 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties,
   set_text_properties_1 (start, end, properties, object, i);
 
   if (BUFFERP (object) && !NILP (coherent_change_p))
-    signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+    signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 			 XFIXNUM (end) - XFIXNUM (start));
   return Qt;
 }
@@ -1565,7 +1565,7 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 	    {
 	      eassert (modified);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+		signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				     XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
@@ -1574,7 +1574,7 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 	    {
 	      remove_properties (properties, Qnil, i, object);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+		signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				     XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
@@ -1585,7 +1585,7 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 	  copy_properties (unchanged, i);
 	  remove_properties (properties, Qnil, i, object);
 	  if (BUFFERP (object))
-	    signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+	    signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				 XFIXNUM (end) - XFIXNUM (start));
 	  return Qt;
 	}
@@ -1663,7 +1663,7 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 	      if (modified)
 		{
 		  if (BUFFERP (object))
-		    signal_after_change (XFIXNUM (start),
+		    signal_after_change (XBUFFER (object), XFIXNUM (start),
 					 XFIXNUM (end) - XFIXNUM (start),
 					 XFIXNUM (end) - XFIXNUM (start));
 		  return Qt;
@@ -1677,7 +1677,7 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 		modify_text_properties (object, start, end);
 	      remove_properties (Qnil, properties, i, object);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+		signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				     XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
@@ -1690,7 +1690,7 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 		modify_text_properties (object, start, end);
 	      remove_properties (Qnil, properties, i, object);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+		signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				     XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
@@ -1709,7 +1709,7 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
           if (modified)
             {
               if (BUFFERP (object))
-                signal_after_change (XFIXNUM (start),
+                signal_after_change (XBUFFER (object), XFIXNUM (start),
                                      XFIXNUM (end) - XFIXNUM (start),
                                      XFIXNUM (end) - XFIXNUM (start));
               return Qt;
-- 
2.20.1