bug#11417: 24.0.96; infinite looping in xdisp.c

[Eli Zaretskii <eliz@gnu.org>]

> Date: Thu, 10 May 2012 20:43:26 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: cyd@gnu.org, 11417@debbugs.gnu.org
> 
>   emacs -Q -nw
>   C-x C-f xdisp.c RET
>   M-: (let ((ov (make-overlay 4928 4933 nil t t)) (fringe (propertize "!" 'display (list 'left-fringe 'question-mark)))) (overlay-put ov 'before-string fringe)) RET
> 
> (The last one is a single long line to type into the minibuffer.)
> 
> Redisplay only shows part of the screen after that, but Emacs doesn't
> yet infloop.  Move a cursor a bit, and it will.
> 
> Of course, the choice of the file (xdisp.c) and the position where to
> put the overlay are arbitrary.
> 
> This only happens in 'emacs -nw", a GUI session (which can actually
> display the fringe bitmap) doesn't have any problems with this recipe.
> 
> I will work on fixing this.

The immediate cause of this bug is that the display iterator would
return zero at the end of the overlay string, and the display engine
would take that as a sign that it is done with redisplay, instead of
popping the stack and continuing to display the rest.

Unfortunately, fixing this and a couple of other blunders exposed by
the fix requires changes in places that are heavily used by the
display engine, and are not limited to left-fringe/right-fringe
display specs on a TTY alone.  See the diffs below.  They seem quite
simple, almost no-brainers, to me, but I've been wrong before when the
innermost bowels of the display engine are concerned.

So I'm not sure whether to apply this to the emacs-24 branch or the
trunk.  Here are the pros and cons for applying to the branch:

 . Pros:
   . The changes are by themselves quite simple.
   . They fix a number of problems that could bite us in other use cases:
     . the fact that left-fringe/right-fringe display property was
       incorrectly reported to the rest of the code as non-replacing,
       whereas in fact it was (the underlying text is not displayed);
     . incorrect condition in iterate_out_of_display_property for
       determining whether we are iterating a string or a buffer;
     . improper termination of display when there's more stuff on the
       iterator stack;
     . call to get_overlay_strings_1 without a check that we already
       have overlay strings loaded.

 . Cons:
   . This problem existed since v22.1, where left-fringe/right-fringe
     was first introduced.
   . Features that display bitmaps on the fringe generally need to
     treat the TTY case specially anyway.  This bug was exposed
     because Leo's code didn't.
   . Some of the changes touch very sensitive parts of the display
     engine on its lowest level: the iteration itself.
   . Prudence would suggest another pretest, should these changes be
     applied to the branch.

So I will await decision by Stefan and Chong before committing this.
In the meantime, Leo, please see that this fixes your problem, and
doesn't introduce any new ones.  TIA.

Here are the changes:

=== modified file 'src/xdisp.c'
--- src/xdisp.c	2012-05-11 14:05:06 +0000
+++ src/xdisp.c	2012-05-12 10:03:08 +0000
@@ -839,6 +839,7 @@ static int try_cursor_movement (Lisp_Obj
 static int trailing_whitespace_p (EMACS_INT);
 static intmax_t message_log_check_duplicate (EMACS_INT, EMACS_INT);
 static void push_it (struct it *, struct text_pos *);
+static void iterate_out_of_display_property (struct it *);
 static void pop_it (struct it *);
 static void sync_frame_with_window_matrix_rows (struct window *);
 static void select_frame_for_redisplay (Lisp_Object);
@@ -3125,7 +3126,15 @@ handle_stop (struct it *it)
 		 overlays even if the actual buffer text is replaced.  */
 	      if (!handle_overlay_change_p
 		  || it->sp > 1
-		  || !get_overlay_strings_1 (it, 0, 0))
+		  /* Don't call get_overlay_strings_1 if we already
+		     have overlay strings loaded, because doing so
+		     will load them again and push the iterator state
+		     onto the stack one more time, which is not
+		     expected by the rest of the code that processes
+		     overlay strings.  */
+		  || (it->n_overlay_strings <= 0
+		      ? !get_overlay_strings_1 (it, 0, 0)
+		      : 0))
 		{
 		  if (it->ellipsis_p)
 		    setup_for_ellipsis (it, 0);
@@ -4681,7 +4690,19 @@ handle_single_display_spec (struct it *i
 	  if (!FRAME_WINDOW_P (it->f))
 	    /* If we return here, POSITION has been advanced
 	       across the text with this property.  */
-	    return 1;
+	    {
+	      /* Synchronize the bidi iterator with POSITION.  This is
+		 needed because we are not going to push the iterator
+		 on behalf of this display property, so there will be
+		 no pop_it call to do this synchronization for us.  */
+	      if (it->bidi_p)
+		{
+		  it->position = *position;
+		  iterate_out_of_display_property (it);
+		  *position = it->position;
+		}
+	      return 1;
+	    }
 	}
       else if (!frame_window_p)
 	return 1;
@@ -4692,7 +4713,15 @@ handle_single_display_spec (struct it *i
 	  || !(fringe_bitmap = lookup_fringe_bitmap (value)))
 	/* If we return here, POSITION has been advanced
 	   across the text with this property.  */
-	return 1;
+	{
+	  if (it && it->bidi_p)
+	    {
+	      it->position = *position;
+	      iterate_out_of_display_property (it);
+	      *position = it->position;
+	    }
+	  return 1;
+	}
 
       if (it)
 	{
@@ -5611,7 +5640,7 @@ push_it (struct it *it, struct text_pos 
 static void
 iterate_out_of_display_property (struct it *it)
 {
-  int buffer_p = BUFFERP (it->object);
+  int buffer_p = !STRINGP (it->string);
   EMACS_INT eob = (buffer_p ? ZV : it->end_charpos);
   EMACS_INT bob = (buffer_p ? BEGV : 0);
 
@@ -6780,6 +6809,16 @@ get_next_display_element (struct it *it)
 	       && FACE_FROM_ID (it->f, face_id)->box == FACE_NO_BOX);
 	}
     }
+  /* If we reached the end of the object we've been iterating (e.g., a
+     display string or an overlay string), and there's something on
+     IT->stack, proceed with what's on the stack.  It doesn't make
+     sense to return zero if there's unprocessed stuff on the stack,
+     because otherwise that stuff will never be displayed.  */
+  if (!success_p && it->sp > 0)
+    {
+      set_iterator_to_next (it, 0);
+      success_p = get_next_display_element (it);
+    }
 
   /* Value is 0 if end of buffer or string reached.  */
   return success_p;
@@ -6961,7 +7000,7 @@ set_iterator_to_next (struct it *it, int
          display vector entry (these entries may contain faces).  */
       it->face_id = it->saved_face_id;
 
-      if (it->dpvec + it->current.dpvec_index == it->dpend)
+      if (it->dpvec + it->current.dpvec_index >= it->dpend)
 	{
 	  int recheck_faces = it->ellipsis_p;
 
@@ -6999,6 +7038,26 @@ set_iterator_to_next (struct it *it, int
     case GET_FROM_STRING:
       /* Current display element is a character from a Lisp string.  */
       xassert (it->s == NULL && STRINGP (it->string));
+      /* Don't advance past string end.  These conditions are true
+	 when set_iterator_to_next is called at the end of
+	 get_next_display_element, in which case the Lisp string is
+	 already exhausted, and all we want is pop the iterator
+	 stack.  */
+      if (it->current.overlay_string_index >= 0)
+	{
+	  /* This is an overlay string, so there's no padding with
+	     spaces, and the number of characters in the string is
+	     where the string ends.  */
+	  if (IT_STRING_CHARPOS (*it) >= SCHARS (it->string))
+	    goto consider_string_end;
+	}
+      else
+	{
+	  /* Not an overlay string.  There could be padding, so test
+	     against it->end_charpos . */
+	  if (IT_STRING_CHARPOS (*it) >= it->end_charpos)
+	    goto consider_string_end;
+	}
       if (it->cmp_it.id >= 0)
 	{
 	  int i;