bug#73022: 31.0.50; Crash in build_frame_matrix_from_leaf_window after C-x 2 and reducing terminal size

[martin rudalics via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

 > It applies well; thanks.
 > By the way, (window-min-size) still answers 4 (with and without your patch).

That's expected.  But (window-min-size (minibuffer-window)) should now
always evaluate to 1 and (frame-windows-min-size) should now give 5
instead of 8.  As for 'window-min-size' you can

- either (setq window-min-height 0) and then (window-min-size)

- or directly (window-min-size nil nil t)

which should both yield 2.  BTW, neither of the min size changes should
have any effect on the bug per se.  But it might cause it to occur
earlier or later and also in the one window case you mention below.

 > Emacs feels a bit "stronger" in the C-x 2 scenario, meaning that it
 > survives shrinking+enlarging the terminal, but it still crashes
 > sometimes.

Without the patch I could make it crash reliably.  I can't with it.

 > But it also crashes in a new situation: just by shrinking the
 > normal/initial/unsplit frame, i.e. without needing to do C-x 2. It
 > didn't crash before, so the patch doesn't make the whole Emacs more
 > stable yet.
 >
 >
 > Without doing C-x 2:
 >
 > #4  0x00005555556c3016 in emacs_abort () at sysdep.c:2391
 > #5  0x000055555566d36f in cmcheckmagic (tty=0x55555608fa80) at cm.c:122
 > #6  0x0000555555671b1b in tty_write_glyphs (f=0x555556012ff0,
 > string=0x7ffff0f4ec10, len=80) at term.c:831
 > #7  0x000055555567c11d in write_glyphs (f=0x555556012ff0,
 > string=0x7ffff0f4dd10, len=80) at terminal.c:164
 > #8  0x0000555555591b60 in update_frame_line (f=0x555556012ff0, vpos=3,
 > updating_menu_p=false) at dispnew.c:5326
 >
 > 126          if (tty->termscript)
 > (gdb) p curY (tty)
 > $1 = 3
 > (gdb) p FrameRows (tty) - 1
 > $2 = 3
 > (gdb)

Maybe because the frame can now get smaller after resize_frame_windows.
IIUC it all depends on where the cursor within the frame is.  In the
split window case you probably get crashes sooner if the cursor is in
the lower window.  Maybe still sooner if you resize the frame while the
minibuffer is active.  I didn't try such scenarios.

 > This one still happens (this is after C-x 2):
 > #5  0x000055555558b697 in build_frame_matrix_from_leaf_window
 > (frame_matrix=0x555556050420,
 >      w=0x555556013210) at dispnew.c:2647
 >
 >>
 >> And it moves the assignment of FrameRows to handle_window_change_signal
 >> in dispnew.c.  Doing it in adjust_frame_size was silly (as Gerd Möllmann
 >> noticed earlier).  FrameRows should be the height of the tty which can
 >> be smaller than the height of our frame.  Emacs is supposed to store it
 >> but not to modify it according to our capabilities.
 >>
 >
 > I don't have the expert opinion of how it should be. But I agree that
 > a source of the problem is that the tty can be smaller than the frame.
 > Frame and terminal may have different sizes and this creates
 > inconsistencies.

Indeed.  The GUI code doesn't have the cursor problem.  There an
arbitrarily large Emacs frame may continue to live within a tiny WM
window with an invisible cursor.

 > For instance, Eli recently added this code (dispnew.c):
 >
 >    /* This should never happen, but evidently sometimes does if one
 >       resizes the frame quickly enough.  Prevent aborts in cmcheckmagic.  */
 >    if (vpos >= FRAME_TOTAL_LINES (f))
 >      return;
 >
 > But this is checking the *frame*.  Later, the assertion in
 > cmcheckmagic will be made about the *terminal*.

Right.  This should probably be

   if (FRAME_TERMCAP_P (f) && vpos >= FrameRows (FRAME_TTY (f)))
     return;

And it's not about resizing frames "quickly".  Here I can crash it in a
very slow fashion too.

 > I think the frame never gets smaller than 2 lines (there's code in
 > handle_window_change_signal to prevent it), but the tty can. So if
 > we're in a 1-line terminal, and updating the 2nd line, the above code
 > sees nothing wrong (the frame still has 2 lines) but cmcheckmagick
 > won't like it (the terminal doesn't have a 2nd line).
 > (Usual disclaimer: I don't know very well how this works. I'm often
 > brainstorming).

Right.  But I can get cmcheckmagic crash here with a 20 lines frame too.

 >> And it moves the assignment of FrameRows to handle_window_change_signal
 >> in dispnew.c.
 >
 > In practice, that code (in the new place you put it) is still inside an if: the
 >    if (width > 5 && height > 2)
 > in handle_window_change_signal. This means that in e.g. a 1-line
 > terminal, you won't be setting FrameRows to the actual number of lines
 > of terminal (1), as you wanted. It will still be the old value, e.g.
 > 2, and it may make cmcheckmagic crash by trying to do things in line 2
 > (which exists in the frame, which has 2 lines) but not in the terminal
 > (which doesn't have a 2nd line).

The 5/2 checks cannot handle split windows.  These are artifacts from
the past.

 > One option seems to have FrameRows always match the amount of terminal
 > lines. Can we remove the "if (… height >2)" limitation, and allow
 > 1-line, 2-line frames etc.? (To really match the terminal size). Other
 > changes may be needed in other places, to avoid working with very
 > small frames..

We should do that.  In principle we should allow height and width to
become zero.  Who knows what today's terminals permit?

 > Otherwise, if we let FrameRows be larger than the amount of terminal
 > lines, another option could be:
 > At the beginning of tty_write_glyphs, check whether we've been asked
 > to write glyphs in a line which is higher than the amount of lines in
 > the terminal. If so, return without writing anything. (Because the
 > line would be invisible).
 > Or maybe in some of the callers of tty_write_glyphs. Don't call
 > tty_write_glyphs to write glyphs in a line which is not visible (it
 > exists in the frame, but doesn't exist in the terminal).

I think FrameRows should always reflect the size of the terminal as it
was reported to us.  If it doesn't, we'll never be able to trace changes
of the terminal size correctly.

 > By the way, I saw that the cmcheckmagic code checks curY (tty) >=
 > FrameRows (tty) - 1, note the -1, whereas the dispnew.c code I quote
 > above doesn't use the -1. I hope this is ok. As mentioned they're
 > checking different things so it may be ok, though I don't understand
 > why the -1.

The cmcheckmagic code is about a very peculiar situation that is hardly
relevant for today's terminals I think.  Here I can never crash a build
that doesn't check assertions.  Can you?

martin