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

[Jim Porter <jporterbugs@gmail.com>]

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

On 8/20/2024 4:53 AM, Eli Zaretskii wrote:
>> Date: Mon, 19 Aug 2024 17:46:18 -0700
>> Cc: eliz@gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> 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.

[-- Attachment #2: 0001-Fix-bad-interaction-between-min-width-display-spec-a.patch --]
[-- Type: text/plain, Size: 3421 bytes --]

From 13fe68ebc2eb2fe7ca0ee4ac4733b3abc3ed0cab Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
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


[-- Attachment #3: backtrace.txt --]
[-- Type: text/plain, Size: 2987 bytes --]

#0  raise (sig=sig@entry=5) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00005555555e78aa in display_min_width
    (it=it@entry=0x7fffffff7500, bufpos=bufpos@entry=440, object=object@entry=0x5555572a2f14, width_spec=width_spec@entry=0x0) at ../../src/xdisp.c:5653
#2  0x00005555555e9fe0 in handle_display_prop (it=0x7fffffff7500)
    at ../../src/lisp.h:1178
#3  0x00005555555e2f85 in handle_stop (it=0x7fffffff7500)
    at ../../src/xdisp.c:4162
#4  0x00005555555f0998 in next_element_from_buffer (it=0x7fffffff7500)
    at ../../src/xdisp.c:9701
#5  0x00005555555ee675 in get_next_display_element (it=it@entry=0x7fffffff7500)
    at ../../src/xdisp.c:8267
#6  0x00005555555f4ea8 in display_line
    (it=0x7fffffff7500, cursor_vpos=<optimized out>) at ../../src/xdisp.c:25395
#7  0x00005555555f98f1 in try_window
    (window=window@entry=0x5555572e1ecd, pos=..., flags=flags@entry=1)
    at ../../src/xdisp.c:21214
#8  0x0000555555618790 in redisplay_window
    (window=0x5555572e1ecd, just_this_one_p=<optimized out>)
    at ../../src/xdisp.c:20594
#9  0x000055555561ade3 in redisplay_window_0
    (window=window@entry=0x5555572e1ecd) at ../../src/xdisp.c:18077
#10 0x00005555557676fc in internal_condition_case_1
    (bfun=bfun@entry=0x55555561adb0 <redisplay_window_0>, arg=arg@entry=0x5555572e1ecd, handlers=<optimized out>, hfun=hfun@entry=0x5555555d2380 <redisplay_window_error>) at ../../src/eval.c:1622
#11 0x00005555555ceff9 in redisplay_windows (window=0x5555572e1ecd)
    at ../../src/xdisp.c:18046
#12 0x00005555555cf01d in redisplay_windows (window=0x5555572e1cad)
    at ../../src/xdisp.c:18040
#13 0x00005555556019c6 in redisplay_internal () at ../../src/xdisp.c:17445
#14 0x00005555556eb5b6 in read_char
    (commandflag=1, map=0x7fffe74a7ad3, prev_event=0x0, used_mouse_menu=0x7fffffffdd8b, end_time=0x0) at ../../src/keyboard.c:2673
#15 0x00005555556eded5 in read_key_sequence
    (keybuf=0x7fffffffdee0, prompt=0x0, dont_downcase_last=<optimized out>, can_return_switch_frame=true, fix_current_buffer=true, prevent_redisplay=<optimized out>, disable_text_conversion_p=false) at ../../src/keyboard.c:10747
#16 0x00005555556efccf in command_loop_1 () at ../../src/lisp.h:1178
#17 0x0000555555767667 in internal_condition_case
    (bfun=bfun@entry=0x5555556efb00 <command_loop_1>, handlers=handlers@entry=0x90, hfun=hfun@entry=0x5555556e2f90 <cmd_error>) at ../../src/eval.c:1598
#18 0x00005555556db6ea in command_loop_2 (handlers=handlers@entry=0x90)
    at ../../src/keyboard.c:1163
#19 0x0000555555767559 in internal_catch
    (tag=tag@entry=0x12450, func=func@entry=0x5555556db6c0 <command_loop_2>, arg=arg@entry=0x90) at ../../src/eval.c:1277
#20 0x00005555556db686 in command_loop () at ../../src/lisp.h:1178
#21 0x00005555556e2ae7 in recursive_edit_1 () at ../../src/keyboard.c:749
#22 0x00005555556e2ea4 in Frecursive_edit () at ../../src/keyboard.c:832
#23 0x00005555555b28c3 in main (argc=<optimized out>, argv=<optimized out>)
    at ../../src/emacs.c:2624