bug#7587: 23.2; `format-mode-line' makes Emacs crash

bug#7587: 23.2; `format-mode-line' makes Emacs crash

[Michael Heerdegen]

Hello,

if I evaluate the following sexp in a fresh emacs -Q (e.g. with
`eval-expression'):

(progn
  (face-remap-add-relative 'mode-line :height 1.0)
  (format-mode-line mode-line-format t))

Emacs instantly crashes.



In GNU Emacs 23.2.1 (i486-pc-linux-gnu, X toolkit, Xaw3d scroll bars)
 of 2010-08-14 on raven, modified by Debian
Windowing system distributor `The X.Org Foundation', version 11.0.10707000
configured using `configure  '--build' 'i486-linux-gnu' '--build' 'i486-linux-gnu' '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib' '--localstatedir=/var/lib' '--infodir=/usr/share/info' '--mandir=/usr/share/man' '--with-pop=yes' '--enable-locallisppath=/etc/emacs23:/etc/emacs:/usr/local/share/emacs/23.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/23.2/site-lisp:/usr/share/emacs/site-lisp:/usr/share/emacs/23.2/leim' '--with-x=yes' '--with-x-toolkit=lucid' '--with-toolkit-scroll-bars' '--without-gconf' 'build_alias=i486-linux-gnu' 'CFLAGS=-DDEBIAN -g -O2' 'LDFLAGS=-g' 'CPPFLAGS=''

Important settings:
  value of $LC_ALL: de_DE.utf8
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: C
  value of $LANG: de_DE.utf8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

bug#7587: 23.2; `format-mode-line' makes Emacs crash

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Tue, 07 Dec 2010 22:01:25 +0100
> Cc: 
> 
> if I evaluate the following sexp in a fresh emacs -Q (e.g. with
> `eval-expression'):
> 
> (progn
>   (face-remap-add-relative 'mode-line :height 1.0)
>   (format-mode-line mode-line-format t))
> 
> Emacs instantly crashes.

This happens because, for some reason I don't understand, when
face-remapping-alist is non-nil, lookup_basic_face insists on getting
only one of the basic faces, and otherwise aborts:

  if (NILP (Vface_remapping_alist))
    return face_id;		/* Nothing to do.  */

  switch (face_id)
    {
    case DEFAULT_FACE_ID:		name = Qdefault;		break;
    case MODE_LINE_FACE_ID:		name = Qmode_line;		break;
    case MODE_LINE_INACTIVE_FACE_ID:	name = Qmode_line_inactive;	break;
    case HEADER_LINE_FACE_ID:		name = Qheader_line;		break;
    case TOOL_BAR_FACE_ID:		name = Qtool_bar;		break;
    case FRINGE_FACE_ID:		name = Qfringe;			break;
    case SCROLL_BAR_FACE_ID:		name = Qscroll_bar;		break;
    case BORDER_FACE_ID:		name = Qborder;			break;
    case CURSOR_FACE_ID:		name = Qcursor;			break;
    case MOUSE_FACE_ID:			name = Qmouse;			break;
    case MENU_FACE_ID:			name = Qmenu;			break;

    default:
      abort ();	    /* the caller is supposed to pass us a basic face id */
    }

This is inconsistent, because if face-remapping-alist _is_ nil, the
function returns face_id unaltered, no matter if it is or isn't a
basic face.

Any objections to the following patch?  On the release branch as well?

=== modified file 'src/xfaces.c'
--- src/xfaces.c	2010-08-06 10:12:41 +0000
+++ src/xfaces.c	2010-12-10 15:58:25 +0000
@@ -4673,9 +4673,6 @@ lookup_basic_face (struct frame *f, int 
     case CURSOR_FACE_ID:		name = Qcursor;			break;
     case MOUSE_FACE_ID:			name = Qmouse;			break;
     case MENU_FACE_ID:			name = Qmenu;			break;
-
-    default:
-      abort ();	    /* the caller is supposed to pass us a basic face id */
     }
 
   /* Do a quick scan through Vface_remapping_alist, and return immediately

bug#7587: 23.2; `format-mode-line' makes Emacs crash

[Eli Zaretskii]

> Date: Fri, 10 Dec 2010 18:02:21 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 7587@debbugs.gnu.org
> 
> === modified file 'src/xfaces.c'
> --- src/xfaces.c	2010-08-06 10:12:41 +0000
> +++ src/xfaces.c	2010-12-10 15:58:25 +0000
> @@ -4673,9 +4673,6 @@ lookup_basic_face (struct frame *f, int 
>      case CURSOR_FACE_ID:		name = Qcursor;			break;
>      case MOUSE_FACE_ID:			name = Qmouse;			break;
>      case MENU_FACE_ID:			name = Qmenu;			break;
> -
> -    default:
> -      abort ();	    /* the caller is supposed to pass us a basic face id */
>      }
>  
>    /* Do a quick scan through Vface_remapping_alist, and return immediately

Sorry, wrong patch.  I meant this one:

=== modified file 'src/xfaces.c'
--- src/xfaces.c	2010-08-06 10:12:41 +0000
+++ src/xfaces.c	2010-12-10 16:36:37 +0000
@@ -4675,7 +4675,7 @@ lookup_basic_face (struct frame *f, int 
     case MENU_FACE_ID:			name = Qmenu;			break;
 
     default:
-      abort ();	    /* the caller is supposed to pass us a basic face id */
+      return face_id;		/* Nothing to do.  */
     }
 
   /* Do a quick scan through Vface_remapping_alist, and return immediately

bug#7587: 23.2; `format-mode-line' makes Emacs crash

[Stefan Monnier]

> This happens because, for some reason I don't understand, when
> face-remapping-alist is non-nil, lookup_basic_face insists on getting
> only one of the basic faces, and otherwise aborts:

I don't know anything about this code, but the name `lookup_basic_face'
suggests it only works for basic faces, so maybe the problem is that the
function gets incorrectly called for a non-basic face.


        Stefan

bug#7587: 23.2; `format-mode-line' makes Emacs crash

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: michael_heerdegen@web.de,  7587@debbugs.gnu.org
> Date: Fri, 10 Dec 2010 15:50:31 -0500
> 
> > This happens because, for some reason I don't understand, when
> > face-remapping-alist is non-nil, lookup_basic_face insists on getting
> > only one of the basic faces, and otherwise aborts:
> 
> I don't know anything about this code

Who does?  Miles, do you?

> but the name `lookup_basic_face' suggests it only works for basic
> faces

Actually, it works for non-basic faces as well--but only if
face-remapping-alist is nil.  See this part of it, which I cited:

  if (NILP (Vface_remapping_alist))
    return face_id;		/* Nothing to do.  */

The patch I suggest simply make lookup_basic_face behave consistently
regardless of the value of face-remapping-alist.

> so maybe the problem is that the function gets incorrectly called
> for a non-basic face.

It _does_ get called with a basic face (`mode-line'), but that face
has been remapped.  Remapping actually creates a new face ID in the
face cache, but lookup_basic_face gets an ID (an integer), not a
symbol Qmode_line etc.  So if the face ID that lookup_basic_face gets
is not one of the IDs reserved for the 12 basic faces, _and_
face-remapping-alist is non-nil, lookup_basic_face always pukes.

Here's more context about the chain of calls that leads to the crash:

The call to lookup_basic_face comes from init_iterator:

  /* Perhaps remap BASE_FACE_ID to a user-specified alternative.  */
  if (! NILP (Vface_remapping_alist))
    remapped_base_face_id = lookup_basic_face (XFRAME (w->frame), base_face_id);

init_iterator does this because it expects a truly basic face in
base_face_id, and it wants to use its face-remapped variety, as the
user would expect.  But in this case, we pass to it an already
remapped basic face, see this part of format-mode-line:

  if (!NILP (face))
    {
      if (EQ (face, Qt))
	face = (EQ (window, selected_window) ? Qmode_line : Qmode_line_inactive);
      face_id = lookup_named_face (XFRAME (WINDOW_FRAME (w)), face, 0);
    }
  ...
  init_iterator (&it, w, -1, -1, NULL, face_id);

lookup_named_face returns the face ID of the remapped mode-line face.
So in this case, face_id is already an ID of the remapped mode-line
face, and returning it unaltered from lookup_basic_face is TRT.

As an alternative, we could refrain from calling lookup_basic_face in
init_iterator if the argument base_face_id is not one of the 12 basic
face IDs.

bug#7587: 23.2; `format-mode-line' makes Emacs crash

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

> The call to lookup_basic_face comes from init_iterator:
> init_iterator does this because it expects a truly basic face in
> base_face_id, and it wants to use its face-remapped variety, as the
> user would expect.  But in this case, we pass to it an already
> remapped basic face, see this part of format-mode-line:

> lookup_named_face returns the face ID of the remapped mode-line face.
> So in this case, face_id is already an ID of the remapped mode-line
> face, and returning it unaltered from lookup_basic_face is TRT.

format-mode-line should not pass non-basic faces to init_iterator.  I'm
not even sure what the FACE argument to format-mode-line is useful for;
it doesn't seem to be used anywhere in the sources.

I've changed format-mode-line so that if you pass a face argument to
FACES, it should be one of `default', `mode-line', etc.  Any other face
gets treated as `default'.

bug#7587: 23.2; `format-mode-line' makes Emacs crash

[Drew Adams]

> I've changed format-mode-line so that if you pass a face argument to
> FACES, it should be one of `default', `mode-line', etc.  Any 
> other face gets treated as `default'.

I guess that restriction will be documented in the doc string, then? Thx.

bug#7587: 23.2; `format-mode-line' makes Emacs crash

[Eli Zaretskii]

> From: Chong Yidong <cyd@stupidchicken.com>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Miles Bader <miles@gnu.org>,
>         michael_heerdegen@web.de, 7587@debbugs.gnu.org
> Date: Thu, 16 Dec 2010 10:17:51 +0800
> 
> format-mode-line should not pass non-basic faces to init_iterator.

Where do you see this condition enforced or assumed in the code?  I
see only the commentary before init_iterator, but I'm not sure which
code (except lookup_basic_face) would really puke or DTWT with a
non-basic face.  And support for face remapping already means that you
can sneak a non-basic face in, anyway.

> I'm not even sure what the FACE argument to format-mode-line is
> useful for

The doc string and the ELisp manual clearly answer that question:
format-mode-line can be used to compute the formatted string without
displaying it.  When you use this API that way, you can specify a face
for the parts of mode line that don't have their faces specified in
the mode-line format.  IOW, it sounds like a deliberate feature, given
the description in the ELisp manual.

> it doesn't seem to be used anywhere in the sources.

?? init_iterator uses it (well, its index in the face cache) to
initialize the appropriate member of the iterator object.

> I've changed format-mode-line so that if you pass a face argument to
> FACES, it should be one of `default', `mode-line', etc.  Any other face
> gets treated as `default'.

Given some history of discussions here, and the fact that no one seems
to be sure how this code should really work and why, I wonder why you
went ahead with the change without discussing it first.  I'm afraid
that this change breaks backward compatibility for no good reason.

Anyway, this being an incompatible change, it should be in NEWS.  In
addition, the ELisp manual should be modified accordingly.

Also, there seems to be a bug in the change itself, in this line:

  +    : EQ (face, Qmode_line_inactive) ? MODE_LINE_FACE_ID

bug#7587: 23.2; `format-mode-line' makes Emacs crash

[Eli Zaretskii]

> From: "Drew Adams" <drew.adams@oracle.com>
> Cc: <michael_heerdegen@web.de>, <7587@debbugs.gnu.org>,
>         "'Miles Bader'" <miles@gnu.org>
> Date: Wed, 15 Dec 2010 20:39:01 -0800
> 
> > I've changed format-mode-line so that if you pass a face argument to
> > FACES, it should be one of `default', `mode-line', etc.  Any 
> > other face gets treated as `default'.
> 
> I guess that restriction will be documented in the doc string, then? Thx.

That's part of the change committed by Chong, yes.

bug#7587: 23.2; `format-mode-line' makes Emacs crash

[Eli Zaretskii]

> From: Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 16 Dec 2010 00:35:51 -0500
> Cc: michael_heerdegen@web.de, 7587@debbugs.gnu.org
> 
> Anyway, this being an incompatible change, it should be in NEWS.  In
> addition, the ELisp manual should be modified accordingly.
> 
> Also, there seems to be a bug in the change itself, in this line:
> 
>   +    : EQ (face, Qmode_line_inactive) ? MODE_LINE_FACE_ID

I fixed these now.

bug#7587: 23.2; `format-mode-line' makes Emacs crash

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

>>   +    : EQ (face, Qmode_line_inactive) ? MODE_LINE_FACE_ID
>
> I fixed these now.

Thanks.

> > I'm not even sure what the FACE argument to format-mode-line is
> > useful for
>
> The doc string and the ELisp manual clearly answer that question:
> format-mode-line can be used to compute the formatted string without
> displaying it.  When you use this API that way, you can specify a face
> for the parts of mode line that don't have their faces specified in
> the mode-line format.  IOW, it sounds like a deliberate feature, given
> the description in the ELisp manual.
>
> > it doesn't seem to be used anywhere in the sources.
>
> ?? init_iterator uses it (well, its index in the face cache) to
> initialize the appropriate member of the iterator object.

I mean, format-mode-line with a non-nil FACE argument doesn't seem to be
used anywhere.

> Given some history of discussions here, and the fact that no one seems
> to be sure how this code should really work and why, I wonder why you
> went ahead with the change without discussing it first.  I'm afraid
> that this change breaks backward compatibility for no good reason.

Given that the backward compatible behavior is to segfault, this
breakage might be acceptable.

Less flippantly, the FACE argument, if non-nil, is still applied as a
text property to the returned string.  So I don't think there's going to
be any noticeable difference (again, we don't have any concrete use
cases, so it's difficult to say).  The docstring change might not even
be necessary.

bug#7587: 23.2; `format-mode-line' makes Emacs crash

[Eli Zaretskii]

> From: Chong Yidong <cyd@stupidchicken.com>
> Cc: michael_heerdegen@web.de, 7587@debbugs.gnu.org
> Date: Mon, 20 Dec 2010 23:29:48 +0800
> 
> I mean, format-mode-line with a non-nil FACE argument doesn't seem to be
> used anywhere.

Not in Emacs sources, no.  But it's a feature that some application
could plausibly want.

> > Given some history of discussions here, and the fact that no one seems
> > to be sure how this code should really work and why, I wonder why you
> > went ahead with the change without discussing it first.  I'm afraid
> > that this change breaks backward compatibility for no good reason.
> 
> Given that the backward compatible behavior is to segfault, this
> breakage might be acceptable.

It only crashed if some faces were remapped, otherwise it would "just
work".  (And it isn't a segfault, it's a deliberate call to `abort'.)

> Less flippantly, the FACE argument, if non-nil, is still applied as a
> text property to the returned string.

Which makes me wonder why we need to pass to init_iterator anything
but DEFAULT_FACE_ID...

> So I don't think there's going to be any noticeable difference
> (again, we don't have any concrete use cases, so it's difficult to
> say).  The docstring change might not even be necessary.

If FACE is still applied, then the doc string change (and part of what
I wrote in NEWS and the ELisp manual) should indeed be reverted.

bug#7587: 23.2; `format-mode-line' makes Emacs crash

[Eli Zaretskii]

> Date: Mon, 20 Dec 2010 20:33:22 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: michael_heerdegen@web.de, 7587@debbugs.gnu.org
> 
> > Less flippantly, the FACE argument, if non-nil, is still applied as a
> > text property to the returned string.
> 
> Which makes me wonder why we need to pass to init_iterator anything
> but DEFAULT_FACE_ID...
> 
> > So I don't think there's going to be any noticeable difference
> > (again, we don't have any concrete use cases, so it's difficult to
> > say).  The docstring change might not even be necessary.
> 
> If FACE is still applied, then the doc string change (and part of what
> I wrote in NEWS and the ELisp manual) should indeed be reverted.

Done.