bug#24633: highlight-region func using (window-hscroll) in :align-to spec can cause inf loop

[npostavs@users.sourceforge.net]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: npostavs@users.sourceforge.net
>> Cc: 24633@debbugs.gnu.org
>> Date: Sun, 09 Oct 2016 08:29:51 -0400
>> 
>> Anyway, it doesn't seem worth going through this complexity.  I just
>> wonder if there is some way to stop bad lisp code from triggering a hard
>> lockup.  Can the display engine notice if it's looping and throw some
>> kind of error?  Maybe unset pre-redisplay-functions?
>
> I don't see how we could detect loops in general.  But for the
> particular case of infinite hscrolling, we could perhaps count the
> number of times hscroll_windows was called and returned a non-zero
> value, and forcibly stop the loop after some reasonable number of
> iterations.

I tried adding a counter in redisplay_internal, it prevents having a
tight loop in C.  Actually, the behaviour becomes like the split window
case: there is additional hscrolling each time the cursor blinks, but it
can be interrupted with C-g.  That seems adequate behaviour to me, what
do you think?

diff --git c/src/xdisp.c i/src/xdisp.c
index ba6518b..3bf3fd8 100644
--- c/src/xdisp.c
+++ i/src/xdisp.c
@@ -13528,6 +13528,9 @@ redisplay_internal (void)
   bool polling_stopped_here = false;
   Lisp_Object tail, frame;
 
+  enum { MAX_HSCROLL_RETRIES = 16 };
+  int hscroll_retries = 0;
+
   /* True means redisplay has to consider all windows on all
      frames.  False, only selected_window is considered.  */
   bool consider_all_windows_p;
@@ -14044,8 +14047,12 @@ redisplay_internal (void)
 		  if (!f->already_hscrolled_p)
 		    {
 		      f->already_hscrolled_p = true;
-		      if (hscroll_windows (f->root_window))
-			goto retry_frame;
+                      if (hscroll_retries <= MAX_HSCROLL_RETRIES
+                          && hscroll_windows (f->root_window))
+                        {
+                          hscroll_retries++;
+                          goto retry_frame;
+                        }
 		    }
 
 		  /* If the frame's redisplay flag was not set before
@@ -14143,8 +14150,12 @@ redisplay_internal (void)
 
       if (FRAME_VISIBLE_P (sf) && !FRAME_OBSCURED_P (sf))
 	{
-	  if (hscroll_windows (selected_window))
-	    goto retry;
+          if (hscroll_retries <= MAX_HSCROLL_RETRIES
+              && hscroll_windows (selected_window))
+            {
+              hscroll_retries++;
+              goto retry;
+            }
 
 	  XWINDOW (selected_window)->must_be_updated_p = true;
 	  pending = update_frame (sf, false, false);
@@ -14164,8 +14175,12 @@ redisplay_internal (void)
 	  XWINDOW (mini_window)->must_be_updated_p = true;
 	  pending |= update_frame (mini_frame, false, false);
 	  mini_frame->cursor_type_changed = false;
-	  if (!pending && hscroll_windows (mini_window))
-	    goto retry;
+          if (!pending && hscroll_retries <= MAX_HSCROLL_RETRIES
+              && hscroll_windows (mini_window))
+            {
+              hscroll_retries++;
+              goto retry;
+            }
 	}
     }



>> >> >> According to `(elisp) Pixel Specification',
>> >> >> 
>> >> >>        The form NUM specifies a fraction of the default frame font height
>> >> >>     or width.  The form `(NUM)' specifies an absolute number of pixels.
>> >> >
>> >> > I admire your courage in reading that documentation and then writing
>> >> > stuff like the above, which the documentation doesn't mention even
>> >> > remotely.
>> >> 
>> >> Uh, not sure how to read this, is it irony?
>> >
>> > Only a little.  I find this area severely under-documented.
>> 
>> The grammar in the doc seems complete to me.
>
> Do you really think that a formal grammar, whether accurate/complete
> or not, is a good way of describing a feature?

The formal grammar plus the informal description of what the parts mean
seems a perfectly fine description for _this_ feature.