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

[Eli Zaretskii <eliz@gnu.org>]

> 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 ();