Subject: bug#72721: 31.0.50; Visual-wrap-prefix-mode breaks Magit log buffers

From: Jim Porter <[email-masked]>
Date: Tue, 20 Aug 2024 10:33:06 -0700

On 8/20/2024 4:53 AM, Eli Zaretskii wrote:
>> Date: Mon, 19 Aug 2024 17:46:18 -0700
>> Cc: [email-masked]
>> From: Jim Porter <[email-masked]>
>>
>> On 8/19/2024 2:39 PM, Gautier Ponsinet wrote:
>>> Hello everyone,
>>>
>>> The new visual-wrap-prefix-mode breaks the rendering of the Magit Log
>>> buffers.
>>>
>>> In emacs -Q:
>>> * Install Magit and its dependencies and load Magit.
>>> * Go to a local repository (via M-x dired or M-x cd).
>>> * M-x global-visual-wrap-prefix-mode
>>> * M-x magit-log-current
>>>
>>> Could someone please confirm/reproduce?
>>
>> I can confirm this. I'm not quite sure of all the details, but it seem
>> that this is due to a bad interaction between overlays and the
>> 'min-width' display spec. The end result was that we were calling
>> 'get-text-property' with a (large-ish) buffer position when the OBJECT
>> arg was a string of length 1. That can happen in magit-log on the
>> mostly-blank line where it's making the ASCII art just below a merge
>> commit. (The leading whitespace makes 'visual-wrap-prefix-mode' do its
>> thing.)
>>
>> I'm not super familiar with how the display engine works, but I think we
>> don't want to call 'display_min_width' when we're working with an
>> overlay. See the attached patch.
> 
> I'd appreciate a reproducer without Magit, as I don't have it
> installed and would prefer not to have to.

Me too... I haven't been able to get a reduced test case yet since Magit 
is pretty complex and I haven't figured out what it's doing exactly. It 
*seems* to be due to overlays, but I only know that from examining 
things in GDB. I haven't deciphered the relevant Magit code yet.

>> Eli, I'm sure you understand this code much better than me. Does the
>> above make sense? I can also try to improve the commentary in the code,
>> but I'm just making some educated guesses as to what's happening here.
> 
> It looks like you are breaking min-width support for display strings?
> They are used on the mode line and also in other places, and in
> general, min-width should treat buffers and strings alike.  Can you
> explain the motivation for the proposed changes, and describe what you
> saw with the current code in this case?  Where's the call to
> get-text-property and why did it use a buffer position instead of a
> string position?

You're probably right. I think my patch was a little over-aggressive 
(see attached for a more-surgical one). This patch may still be wrong, 
but hopefully it gets a bit closer to what we want.

I think this is what's happening, in a bit more detail: 
magit-log-current uses overlays (I think to set up the right margin 
text?). When visual-wrap-prefix-mode ("vwpm") is enabled, the display 
engine goes through the buffer, finds the 'min-width' display property 
from vwpm and holds onto it. Next, it starts processing an overlay.

Eventually, that calls 'handle_display_prop' for the overlay which calls 
'display_min_width'. At this point, we have an object stored in
'it->min_width_property' (thanks to vwpm), the local variable 'object' 
is the overlay string, and 'bufpos' is the actual buffer position.

Finally we call 'get_display_property' with the bufpos and object (which 
calls 'Fget_text_property'), and kaboom: 'object' is a string of length 
1, but bufpos is much larger (~400 in my test).

I've also attached a backtrace, though I'm not sure how informative it 
is on its own.

From 13fe68ebc2eb2fe7ca0ee4ac4733b3abc3ed0cab Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Mon, 19 Aug 2024 17:38:47 -0700
Subject: [PATCH] Fix bad interaction between 'min-width' display spec and
 overlays

Previously, when iterating over overlays, we would pass the overlay
string and the buffer position to 'display_min_width', which would use
those values to try to get the display property.  However, the buffer
position is very likely out of bounds for the overlay string!

* src/xdisp.c (get_display_property): Rename BUFPOS to CHARPOS.
(display_min_width): Take CHARPOS instead of BUFPOS, and get BUFPOS on
our own.  This way, we can be sure that when calling
'get_display_property', we provide it with the correct kind of position.
(handle_display_prop): Pass the character pos of OBJECT's position to
'display_min_width' (bug#72721).
---
 src/xdisp.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 30771a1c83d..af93a824bee 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -5633,17 +5633,19 @@ find_display_property (Lisp_Object disp, Lisp_Object prop)
 }
 
 static Lisp_Object
-get_display_property (ptrdiff_t bufpos, Lisp_Object prop, Lisp_Object object)
+get_display_property (ptrdiff_t charpos, Lisp_Object prop, Lisp_Object object)
 {
-  return find_display_property (Fget_text_property (make_fixnum (bufpos),
+  return find_display_property (Fget_text_property (make_fixnum (charpos),
 						    Qdisplay, object),
 				prop);
 }
 
 static void
-display_min_width (struct it *it, ptrdiff_t bufpos,
+display_min_width (struct it *it, ptrdiff_t charpos,
 		   Lisp_Object object, Lisp_Object width_spec)
 {
+  ptrdiff_t bufpos = CHARPOS (it->current.pos);
+
   /* We're being called at the end of the `min-width' sequence,
      probably. */
   if (!NILP (it->min_width_property)
@@ -5658,9 +5660,9 @@ display_min_width (struct it *it, ptrdiff_t bufpos,
 		   get_display_property (0, Qmin_width, object)))
 	  /* In a buffer -- check that we're really right after the
 	     sequence of characters covered by this `min-width'.  */
-	  || (bufpos > BEGV
+	  || (bufpos > BEGV && charpos > 0
 	      && EQ (it->min_width_property,
-		     get_display_property (bufpos - 1, Qmin_width, object))))
+		     get_display_property (charpos - 1, Qmin_width, object))))
 	{
 	  Lisp_Object w = Qnil;
 	  double width;
@@ -5713,9 +5715,9 @@ display_min_width (struct it *it, ptrdiff_t bufpos,
 	      && !EQ (it->min_width_property,
 		      get_display_property (0, Qmin_width, object)))
 	  /* Buffer.  */
-	  || (bufpos > BEGV
+	  || (bufpos > BEGV && charpos > 0
 	      && !EQ (width_spec,
-		      get_display_property (bufpos - 1, Qmin_width, object))))
+		      get_display_property (charpos - 1, Qmin_width, object))))
 	{
 	  it->min_width_property = width_spec;
 	  it->min_width_start = it->current_x;
@@ -5795,10 +5797,10 @@ handle_display_prop (struct it *it)
   if (!STRINGP (it->string))
     object = it->w->contents;
 
-  /* Handle min-width ends. */
+  /* Handle min-width ends, except when processing an overlay.  */
   if (!NILP (it->min_width_property)
       && NILP (find_display_property (propval, Qmin_width)))
-    display_min_width (it, bufpos, object, Qnil);
+    display_min_width (it, CHARPOS (*position), object, Qnil);
 
   if (NILP (propval))
     return HANDLED_NORMALLY;
-- 
2.25.1