bug#16051: 24.3.50; Emacs hang - resize frame manually

[Eli Zaretskii]

> Date: Sat, 21 Dec 2013 18:13:25 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: Jarek Czekalski <jarekczek@poczta.onet.pl>, 16051@debbugs.gnu.org
> 
>  > I don't think it's important, certainly not extremely important.  You
>  > can hang Emacs with as little as '(while t)'.  Users who do
>  > unreasonable things should expect trouble, because Emacs gives them a
>  > lot of rope to hang themselves.
> 
> If I do drag _very_ fast as described by the OP, Emacs reliably crashes
> here as:
> 
> 
> #0  terminate_due_to_signal (sig=22, backtrace_limit=2147483647) at 
> emacs.c:351
> #1  0x0116400a in die (msg=0x14763e0 "row >= 0 && row < matrix->nrows", 
> file=0x147618c "dispnew.c", line=1369) at alloc.c:6742
> #2  0x01004ed3 in matrix_row (matrix=0x36e2800, row=7) at dispnew.c:1369
> #3  0x01040eaa in hscroll_window_tree (window=...) at xdisp.c:12610
> #4  0x01041398 in hscroll_windows (window=...) at xdisp.c:12737
> #5  0x010437bd in redisplay_internal () at xdisp.c:13631
> #6  0x01044126 in redisplay_preserve_echo_area (from_where=11) at 
> xdisp.c:13856

It turns out this bug has 3 separate parts.  2 of them are very clear
and uncontroversial, so I simply fixed them; see revision 115718 on
the trunk.  After that, there are no more assertion violations like
above, and no hangs, and it is much harder to cause a redisplay loop.
Also, the only redisplay loops I can trigger are immediately
interrupted by C-g.  But since such loops can still be triggered,
there's another part to this riddle.  

Here, the problem and the solution seem clear to me, but since the
code is quite deliberately doing what it does, I'd like to ask Martin
about the underlying logic.  I'm talking about this part of
redisplay_tool_bar:

          if (change_height_p)
            {
              int new_lines = ((new_height + FRAME_LINE_HEIGHT (f) - 1)
                               / FRAME_LINE_HEIGHT (f));

              XSETFRAME (frame, f);
              Fmodify_frame_parameters (frame,
                                        list1 (Fcons (Qtool_bar_lines,
                                                      make_number 
(new_lines))));
              /* Always do that now.  */
              clear_glyph_matrix (w->desired_matrix);
              f->n_tool_bar_rows = nrows;
              f->fonts_changed = 1;
              return 1;
            }

Why does it make sense to "always do that"?  Suppose new_lines is
exactly equal to the current number of canonical lines used by the
tool-bar window (it can happen even if pixel sizes are different, due
to integer truncation).  Or suppose you are asking for N lines, but
don't get it because the frame is too small.  Why set the
fonts_changed flag in those cases?  That flag causes redisplay to give
up, abandon the current glyph matrices, and retry anew.  What happens
with those endless cycles is precisely that: redisplay_tool_bar sets
the fonts_changed flag, which causes redisplay_internal to retry,
which again calls redisplay_tool_bar, which again sets the flag,
etc., ad nauseam.

If there is some reasoning behind this "always do that" logic, please
describe it.  Otherwise, I propose the patch below, which cures the
problem altogether for me; if you agree, I will install it.

As an aside, I stared for a long time at this part of
w32fns.c:x_set_tool_bar_lines (which is part of the issue, because it
is called by modify-frame-parameters, when the tool-bar-lines
parameter is changed), and I still don't get why it does what it does:

      root_height = WINDOW_PIXEL_HEIGHT (XWINDOW (FRAME_ROOT_WINDOW (f)));
      if (root_height - delta < unit)
        {
          delta = root_height - unit;
          nlines = (root_height / unit) + min (1, (root_height % unit));
        }

First, delta is recomputed, but the result is never used.  More
importantly, you assign to nlines a value that is the size of the root
window _plus_ one line?  Did you mean minus instead?

Finally, the corresponding xfns.c implementation still works in lines,
not in pixels, as w32fns.c did before your pixelwise changes.  Is this
disparity on purpose or an oversight?

Here's the patch I propose to solve the last part of this bug.  It
abandons the "do it always" method, and only changes the
tool-bar-lines parameter and sets the fonts_changed flag if the
required height of the tool bar differs from the current height by at
least 1 canonical line, and if it is smaller than the maximum number
of lines any window can get on this frame.

--- src/xdisp.c~1       2013-12-23 18:20:09.678832900 +0200
+++ src/xdisp.c 2013-12-23 18:50:56.993834000 +0200
@@ -12289,18 +12289,24 @@ redisplay_tool_bar (struct frame *f)
 
          if (change_height_p)
            {
+             int old_lines = WINDOW_TOTAL_LINES (w);
              int new_lines = ((new_height + FRAME_LINE_HEIGHT (f) - 1)
                               / FRAME_LINE_HEIGHT (f));
+             int max_lines =
+               WINDOW_TOTAL_LINES (XWINDOW (FRAME_ROOT_WINDOW (f))) - 1;
 
-             XSETFRAME (frame, f);
-             Fmodify_frame_parameters (frame,
-                                       list1 (Fcons (Qtool_bar_lines,
-                                                     make_number 
(new_lines))));
-             /* Always do that now.  */
-             clear_glyph_matrix (w->desired_matrix);
-             f->n_tool_bar_rows = nrows;
-             f->fonts_changed = 1;
-             return 1;
+             if (new_lines <= max_lines
+                 && eabs (new_lines - old_lines) >= 1)
+               {
+                 XSETFRAME (frame, f);
+                 Fmodify_frame_parameters (frame,
+                                           list1 (Fcons (Qtool_bar_lines,
+                                                         make_number 
(new_lines))));
+                 clear_glyph_matrix (w->desired_matrix);
+                 f->n_tool_bar_rows = nrows;
+                 f->fonts_changed = 1;
+                 return 1;
+               }
            }
        }
     }