bug#18422: 24.3.93; Assertion violation when resizing mini-window on a TTY

bug#18422: 24.3.93; Assertion violation when resizing mini-window on a TTY

[Eli Zaretskii]

The following recipe causes an assertion violation on MS-Windows (I
don't have access to a Unix TTY with a mouse to try there):

  emacs -Q -nw
  M-: (setq resize-mini-windows nil) RET

  Drag the mode line with the mouse -- you get assertion violation:

    dispnew.c:684: Emacs fatal error: assertion failed: start >= 0 && start < matrix->nrows

The immediate reason for this is that clear_glyph_matrix_rows is
called with both start = 0 and end = 0.  But the root cause is that
Emacs thinks the mini-buffer window has a zero height.  This comes
from here:

  static int
  required_matrix_height (struct window *w)
  {
  #ifdef HAVE_WINDOW_SYSTEM
    struct frame *f = XFRAME (w->frame);

    if (FRAME_WINDOW_P (f))
      {
	int ch_height = FRAME_SMALLEST_FONT_HEIGHT (f);
	int window_pixel_height = window_box_height (w) + eabs (w->vscroll);

	return (((window_pixel_height + ch_height - 1)
		 / ch_height) * w->nrows_scale_factor
		/* One partially visible line at the top and
		   bottom of the window.  */
		+ 2
		/* 2 for header and mode line.  */
		+ 2);
      }
  #endif /* HAVE_WINDOW_SYSTEM */

    return WINDOW_TOTAL_LINES (w); <<<<<<<<<<<<<<<<<<<<<
  }

The total_lines value is zero.  It turns out that total_lines is
assigned the zero value in resize-mini-window-internal:

      w->total_lines = XFASTINT (w->new_total);  <<<<<<<<<<<<<<
      w->pixel_height = XFASTINT (w->new_pixel);

and w->new_total is zero because no one set it to any other value.  In
window--resize-mini-window we do this:

	(window--resize-this-window root (- delta) nil nil t)
	(set-window-new-pixel window (+ height delta))
	;; The following routine catches the case where we want to resize
	;; a minibuffer-only frame.
	(when (resize-mini-window-internal window)
	  (window--pixel-to-total frame)

So by the time resize-mini-window-internal is called, w->new_pixel is
already set as appropriate, but w->new_total is set only after the
call to resize-mini-window-internal returns.

Now, on GUI frames this problem doesn't happen, because, as seen from
required_matrix_height above, total_lines is never used there to
determine the matrix dimensions.  Instead, GUI frames calculate their
height in pixels, which is already set by this time.

I can fix this problem with the following semi-kludgey change (which
will need a comment to explain the above, if and when it's committed):

=== modified file 'src/window.c'
--- src/window.c	2014-08-09 11:12:45 +0000
+++ src/window.c	2014-09-07 19:19:19 +0000
@@ -4807,6 +4807,8 @@ DEFUN ("resize-mini-window-internal", Fr
       w->total_lines = XFASTINT (w->new_total);
       w->top_line = r->top_line + r->total_lines;
       w->pixel_height = XFASTINT (w->new_pixel);
+      if (!FRAME_WINDOW_P (f))
+	w->total_lines = w->pixel_height;
       w->pixel_top = r->pixel_top + r->pixel_height;
 
       fset_redisplay (f);


Is this the right fix?


In GNU Emacs 24.3.93.28 (i686-pc-mingw32)
 of 2014-09-07 on HOME-C4E4A596F7
Repository revision: 117483 monnier@iro.umontreal.ca-20140905173712-p7fyv6iaijldb29c
Windowing system distributor `Microsoft Corp.', version 5.1.2600
Configured using:
 `configure --prefix=/d/usr --enable-checking=yes,glyphs 'CFLAGS=-O0
 -g3''

Important settings:
  value of $LANG: ENU
  locale-coding-system: cp1255

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
M-x r e p o r t - e m a c s - b u g <return>

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 mml
easymenu mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util help-fns mail-prsvr mail-utils time-date tooltip electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel dos-w32 ls-lisp
w32-common-fns disp-table w32-win w32-vars tool-bar dnd fontset image
regexp-opt fringe tabulated-list newcomment lisp-mode prog-mode register
page menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock
font-lock syntax facemenu font-core frame cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese hebrew
greek romanian slovak czech european ethiopic indian cyrillic chinese
case-table epa-hook jka-cmpr-hook help simple abbrev minibuffer nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process w32notify w32
multi-tty emacs)

Memory information:
((conses 8 74225 7254)
 (symbols 32 17536 0)
 (miscs 32 33 97)
 (strings 16 10776 4344)
 (string-bytes 1 269292)
 (vectors 8 9555)
 (vector-slots 4 384789 5962)
 (floats 8 57 68)
 (intervals 28 238 95)
 (buffers 508 11))

bug#18422: 24.3.93; Assertion violation when resizing mini-window on a TTY

[martin rudalics]

One possible fix would be:

=== modified file 'lisp/window.el'
--- lisp/window.el	2014-08-29 10:39:17 +0000
+++ lisp/window.el	2014-09-08 08:45:23 +0000
@@ -2388,6 +2388,8 @@
  	;; why we do not do that.
  	(window--resize-this-window root (- delta) nil nil t)
  	(set-window-new-pixel window (+ height delta))
+	(set-window-new-total window (/ (window-new-pixel window)
+					(frame-char-height frame)))
  	;; The following routine catches the case where we want to resize
  	;; a minibuffer-only frame.
  	(when (resize-mini-window-internal window)



But I'm afraid that other windows might be affected as well so I'd
prefer to do this instead:

=== modified file 'src/dispnew.c'
--- src/dispnew.c	2014-09-08 06:00:58 +0000
+++ src/dispnew.c	2014-09-08 09:04:18 +0000
@@ -1708,7 +1708,7 @@
      }
  #endif /* HAVE_WINDOW_SYSTEM */

-  return WINDOW_TOTAL_LINES (w);
+  return WINDOW_PIXEL_HEIGHT (w);
  }


@@ -1734,7 +1734,7 @@
      }
  #endif /* HAVE_WINDOW_SYSTEM */

-  return w->total_cols;
+  return WINDOW_PIXEL_WIDTH (w);
  }




martin

bug#18422: 24.3.93; Assertion violation when resizing mini-window on a TTY

[Eli Zaretskii]

> Date: Mon, 08 Sep 2014 11:31:34 +0200
> From: martin rudalics <rudalics@gmx.at>
> 
> One possible fix would be:
> 
> === modified file 'lisp/window.el'
> --- lisp/window.el	2014-08-29 10:39:17 +0000
> +++ lisp/window.el	2014-09-08 08:45:23 +0000
> @@ -2388,6 +2388,8 @@
>   	;; why we do not do that.
>   	(window--resize-this-window root (- delta) nil nil t)
>   	(set-window-new-pixel window (+ height delta))
> +	(set-window-new-total window (/ (window-new-pixel window)
> +					(frame-char-height frame)))
>   	;; The following routine catches the case where we want to resize
>   	;; a minibuffer-only frame.
>   	(when (resize-mini-window-internal window)
> 
> 
> 
> But I'm afraid that other windows might be affected as well so I'd
> prefer to do this instead:
> 
> === modified file 'src/dispnew.c'
> --- src/dispnew.c	2014-09-08 06:00:58 +0000
> +++ src/dispnew.c	2014-09-08 09:04:18 +0000
> @@ -1708,7 +1708,7 @@
>       }
>   #endif /* HAVE_WINDOW_SYSTEM */
> 
> -  return WINDOW_TOTAL_LINES (w);
> +  return WINDOW_PIXEL_HEIGHT (w);
>   }
> 
> 
> @@ -1734,7 +1734,7 @@
>       }
>   #endif /* HAVE_WINDOW_SYSTEM */
> 
> -  return w->total_cols;
> +  return WINDOW_PIXEL_WIDTH (w);
>   }

I already tried this before reporting the bug.  It doesn't work,
because we then hit these assertions in fake_current_matrices:

	  eassert (m->matrix_h == WINDOW_TOTAL_LINES (w));
	  eassert (m->matrix_w == WINDOW_TOTAL_COLS (w));

In general, I think it's a bad idea to have bogus values in
WINDOW_TOTAL_LINES and WINDOW_TOTAL_COLS, they are not documented to
be invalid under any circumstances, so the code relies on them.

What exactly frightens you in your first proposal?  Perhaps we should
install my patch on the emacs-24 branch and your window.el patch on
the trunk?

bug#18422: 24.3.93; Assertion violation when resizing mini-window on a TTY

[martin rudalics]

 > I already tried this before reporting the bug.  It doesn't work,
 > because we then hit these assertions in fake_current_matrices:
 >
 > 	  eassert (m->matrix_h == WINDOW_TOTAL_LINES (w));
 > 	  eassert (m->matrix_w == WINDOW_TOTAL_COLS (w));

We would have to use pixel sizes there too and maybe elsewhere.

 > In general, I think it's a bad idea to have bogus values in
 > WINDOW_TOTAL_LINES and WINDOW_TOTAL_COLS, they are not documented to
 > be invalid under any circumstances, so the code relies on them.

Agreed.  The "total" values are bogus from the moment a function like
`resize-mini-window-internal' is executed until `window--pixel-to-total'
is executed.  Unfortunately, I currently adjust the frame glyphs right
in between.  I should either move the adjust_frame_glyphs call to
Fwindow_resize_apply_total or set the new total sizes before calling
window_resize_apply and have the latter call window_resize_apply_total.

 > What exactly frightens you in your first proposal?  Perhaps we should
 > install my patch on the emacs-24 branch and your window.el patch on
 > the trunk?

I'm afraid that the current bug is only the tip of an iceberg.  We
should close the window of vulnerability sketched above.  On the
emacs-24 branch.  Neither of the two schemes from the previous paragraph
is entirely trivial so using pixel values instead of character values
would put us more on the safe side.

Eventually, I intend to remove the total_lines/total_cols fields from
the frame structure and have FRAME_TOTAL_COLS and FRAME_TOTAL_LINES
calculate the values directly from the pixel sizes.  I refrained from
doing this already because I'm never sure how these values should get
rounded.

martin

bug#18422: 24.3.93; Assertion violation when resizing mini-window on a TTY

[Eli Zaretskii]

> Date: Wed, 10 Sep 2014 10:03:27 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: 18422@debbugs.gnu.org
> 
>  > I already tried this before reporting the bug.  It doesn't work,
>  > because we then hit these assertions in fake_current_matrices:
>  >
>  > 	  eassert (m->matrix_h == WINDOW_TOTAL_LINES (w));
>  > 	  eassert (m->matrix_w == WINDOW_TOTAL_COLS (w));
> 
> We would have to use pixel sizes there too and maybe elsewhere.

Like I said: I don't think it's a good idea to give up on keeping the
"total" members valid at all times.

>  > In general, I think it's a bad idea to have bogus values in
>  > WINDOW_TOTAL_LINES and WINDOW_TOTAL_COLS, they are not documented to
>  > be invalid under any circumstances, so the code relies on them.
> 
> Agreed.  The "total" values are bogus from the moment a function like
> `resize-mini-window-internal' is executed until `window--pixel-to-total'
> is executed.  Unfortunately, I currently adjust the frame glyphs right
> in between.  I should either move the adjust_frame_glyphs call to
> Fwindow_resize_apply_total or set the new total sizes before calling
> window_resize_apply and have the latter call window_resize_apply_total.

Please do one or the other, I don't think we can leave the release
branch in its current state.  It's all too easy to trigger this
problem.

>  > What exactly frightens you in your first proposal?  Perhaps we should
>  > install my patch on the emacs-24 branch and your window.el patch on
>  > the trunk?
> 
> I'm afraid that the current bug is only the tip of an iceberg.  We
> should close the window of vulnerability sketched above.  On the
> emacs-24 branch.  Neither of the two schemes from the previous paragraph
> is entirely trivial so using pixel values instead of character values
> would put us more on the safe side.

I think the iceberg whose tip you envision includes the invalidity of
the "total" values.  IOW, this is imaginary safety, more bugs are
lurking out there.

> Eventually, I intend to remove the total_lines/total_cols fields from
> the frame structure and have FRAME_TOTAL_COLS and FRAME_TOTAL_LINES
> calculate the values directly from the pixel sizes.  I refrained from
> doing this already because I'm never sure how these values should get
> rounded.

That might be a good plan, but it' not for the release branch.

bug#18422: 24.3.93; Assertion violation when resizing mini-window on a TTY

[martin rudalics]

 >> The "total" values are bogus from the moment a function like
 >> `resize-mini-window-internal' is executed until `window--pixel-to-total'
 >> is executed.  Unfortunately, I currently adjust the frame glyphs right
 >> in between.  I should either move the adjust_frame_glyphs call to
 >> Fwindow_resize_apply_total or set the new total sizes before calling
 >> window_resize_apply and have the latter call window_resize_apply_total.

This analysis was downright silly.  If it were accurate, then
adjust_frame_glyphs would have choked immediately and everywhere.  When
I assign pixel sizes I also assign the total sizes as an estimate in the
sense that partially visible characters are counted as full.

Later I have to recalculate sizes to make sure that the individual total
sizes of subwindows sum up to the total size of their parent window.
This is needed to make some older functions (in particular those from
windmove) work and can result in some windows getting a smaller total
size than at the time adjust_frame_glyphs was called.  This is, however,
not an issue for -nw since there are no partial visible characters.

 > Please do one or the other, I don't think we can leave the release
 > branch in its current state.  It's all too easy to trigger this
 > problem.

I checked in a fix for the bug.  It was entirely restricted to manually
resizing the minibuffer window, something apparently nobody tried since
last year with -nw.

 >> I'm afraid that the current bug is only the tip of an iceberg.  We
 >> should close the window of vulnerability sketched above.  On the
 >> emacs-24 branch.  Neither of the two schemes from the previous paragraph
 >> is entirely trivial so using pixel values instead of character values
 >> would put us more on the safe side.
 >
 > I think the iceberg whose tip you envision includes the invalidity of
 > the "total" values.  IOW, this is imaginary safety, more bugs are
 > lurking out there.

Hopefully, the iceberg melt by now.

 >> Eventually, I intend to remove the total_lines/total_cols fields from
 >> the frame structure and have FRAME_TOTAL_COLS and FRAME_TOTAL_LINES
 >> calculate the values directly from the pixel sizes.  I refrained from
 >> doing this already because I'm never sure how these values should get
 >> rounded.
 >
 > That might be a good plan, but it' not for the release branch.

It would neither help nor work anyway.

martin

bug#18422: 24.3.93; Assertion violation when resizing mini-window on a TTY

[Eli Zaretskii]

> Date: Thu, 11 Sep 2014 11:25:53 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: 18422@debbugs.gnu.org
> 
> I checked in a fix for the bug.  It was entirely restricted to manually
> resizing the minibuffer window, something apparently nobody tried since
> last year with -nw.

Thanks, it works for me.  You can close the bug, unless you think
something else needs to be done.

> Hopefully, the iceberg melt by now.

Let's hope.

bug#18422: 24.3.93; Assertion violation when resizing mini-window on a TTY

[martin rudalics]

> Thanks, it works for me.  You can close the bug, unless you think
> something else needs to be done.

Closing.

Thanks, martin