bug#51577: 27.2; Regression: reproducible hang with face functions

bug#51577: 27.2; Regression: reproducible hang with face functions

[Drew Adams]

emacs -Q

You might want to open buffer *Messages* in another frame, so you can
see its messages (not needed for the bug recipe).

Eval this code:

;;--------------

(defface alt-region '((t :background "gray70" :inherit region))
  "..." :group 'faces)

;; `selected-frame' here could be anything, so this is dicey.
;; But that's not important for the bug.
 (defvar orig-region-atts (face-all-attributes 'region (selected-frame)))

(defun foo (&optional arg)
  (interactive "P")
  (let* ((frame  (selected-frame))
         (alist  (if arg
                     (face-all-attributes 'alt-region frame)
                   orig-region-atts))
         (alist  (cons (cons :font 'unspecified) alist))
         (plist  ()))
    (message "BEFORE LOOP, ALIST: %S" alist)
    (while alist
      (push (caar alist) plist)
      (push (cdar alist) plist)
      (setq alist  (cdr alist)))
    (setq plist  (nreverse plist))
    (message "> LOOP. PLIST: %S" plist)
    (apply #'set-face-attribute 'region frame plist)))

;;--------------

You can do `M-x foo' and `C-u M-x foo' with no problem.  You can even
move the cursor with motion keys in between such `foo' invocations.
No problem.

If you do `C-x SPC', to activate the region before trying `foo' with
and without prefix arg, you can even move the cursor horizontally some.

But if you move the cursor vertically then Emacs hangs (e.g. with
`C-u M-x foo'), apparently in an infloop of some kind.  I have to use
the MS Windows Task Manager to kill Emacs.

Similarly, if you use the mouse to select a region, Emacs hangs.

There's no such bug in Emacs 26.3.

--------------------

In GNU Emacs 27.2 (build 1, x86_64-w64-mingw32)
 of 2021-03-26 built on CIRROCUMULUS
Repository revision: deef5efafb70f4b171265b896505b92b6eef24e6
Repository branch: HEAD
Windowing system distributor 'Microsoft Corp.', version 10.0.19042
System Description: Microsoft Windows 10 Pro (v10.0.2009.19042.1288)

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

Configured using:
 'configure --without-dbus --host=x86_64-w64-mingw32
 --without-compress-install 'CFLAGS=-O2 -static''

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND NOTIFY W32NOTIFY ACL GNUTLS LIBXML2
HARFBUZZ ZLIB TOOLKIT_SCROLL_BARS MODULES THREADS JSON PDUMPER LCMS2 GMP

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

Major mode: Emacs-Lisp

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-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

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny format-spec rfc822 mml
easymenu mml-sec password-cache epa derived epg epg-config gnus-util
rmail rmail-loaddefs text-property-search seq byte-opt gv bytecomp
byte-compile cconv mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util mail-prsvr mail-utils time-date subr-x cl-loaddefs cl-lib dired
dired-loaddefs tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel dos-w32 ls-lisp disp-table term/w32-win w32-win
w32-vars term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded 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 threads w32notify w32 lcms2 multi-tty make-network-process
emacs)

Memory information:
((conses 16 102857 17499)
 (symbols 48 6313 1)
 (strings 32 18104 1550)
 (string-bytes 1 539747)
 (vectors 16 9747)
 (vector-slots 8 140879 14626)
 (floats 8 33 282)
 (intervals 56 20766 4787)
 (buffers 1000 13))

bug#51577: 27.2; Regression: reproducible hang with face functions

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> Date: Wed, 3 Nov 2021 02:44:22 +0000
> 
> (defface alt-region '((t :background "gray70" :inherit region))
>   "..." :group 'faces)
> 
> ;; `selected-frame' here could be anything, so this is dicey.
> ;; But that's not important for the bug.
>  (defvar orig-region-atts (face-all-attributes 'region (selected-frame)))
> 
> (defun foo (&optional arg)
>   (interactive "P")
>   (let* ((frame  (selected-frame))
>          (alist  (if arg
>                      (face-all-attributes 'alt-region frame)
>                    orig-region-atts))
>          (alist  (cons (cons :font 'unspecified) alist))
>          (plist  ()))
>     (message "BEFORE LOOP, ALIST: %S" alist)
>     (while alist
>       (push (caar alist) plist)
>       (push (cdar alist) plist)
>       (setq alist  (cdr alist)))
>     (setq plist  (nreverse plist))
>     (message "> LOOP. PLIST: %S" plist)
>     (apply #'set-face-attribute 'region frame plist)))

You set the region face to inherit from itself, and you expect that to
work without causing an infloop when Emacs tries to resolve some face
attribute?  If FACE1 has some attribute 'unspecified', but inherits
from FACE2, Emacs will try to go up the inheritance chain to see if
some of the parent faces specifies that attribute.  If FACE1 inherits
from itself, going up the inheritance chain will never end.

IOW, it's a cockpit error.

bug#51577: [External] : Re: bug#51577: 27.2; Regression: reproducible hang with face functions

[Drew Adams]

> > (defface alt-region '((t :background "gray70" :inherit region))
> >   "..." :group 'faces)
> >
> > ;; `selected-frame' here could be anything, so this is dicey.
> > ;; But that's not important for the bug.
> >  (defvar orig-region-atts (face-all-attributes 'region (selected-
> frame)))
> >
> > (defun foo (&optional arg)
> >   (interactive "P")
> >   (let* ((frame  (selected-frame))
> >          (alist  (if arg
> >                      (face-all-attributes 'alt-region frame)
> >                    orig-region-atts))
> >          (alist  (cons (cons :font 'unspecified) alist))
> >          (plist  ()))
> >     (message "BEFORE LOOP, ALIST: %S" alist)
> >     (while alist
> >       (push (caar alist) plist)
> >       (push (cdar alist) plist)
> >       (setq alist  (cdr alist)))
> >     (setq plist  (nreverse plist))
> >     (message "> LOOP. PLIST: %S" plist)
> >     (apply #'set-face-attribute 'region frame plist)))
> 
> You set the region face to inherit from itself, and you expect that to
> work without causing an infloop when Emacs tries to resolve some face
> attribute?

Yes.  I expected Emacs to act as it has in the past,
and ignore such an :inherit as a no-op.

> If FACE1 has some attribute 'unspecified', but inherits
> from FACE2, Emacs will try to go up the inheritance chain to see if
> some of the parent faces specifies that attribute.  If FACE1 inherits
> from itself, going up the inheritance chain will never end.

Only if the implementation doesn't recognize the
inheritance loop.

> IOW, it's a cockpit error.

You can look at it that way.  But I'd expect that
inheritance of a face from itself would be a no-op.
That makes :inherit more flexible/usable.

And that's exactly what the case was in Emacs 26.
The `region' face spec with `:inherit region' is
innocuous in Emacs 26.

Why should the code now chase its tail down an
infinite rabbit hole?  Is this really by design?
Was something gained by this code change?  Or is
this just accidental collateral damage?

Not rhetorical questions.  It seems like a design
change has been made, but with no announcement.
I see nothing in NEWS, when searching for "face"
or "inherit".

bug#51577: [External] : Re: bug#51577: 27.2; Regression: reproducible hang with face functions

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> CC: "51577@debbugs.gnu.org" <51577@debbugs.gnu.org>
> Date: Wed, 3 Nov 2021 18:58:23 +0000
> 
> > You set the region face to inherit from itself, and you expect that to
> > work without causing an infloop when Emacs tries to resolve some face
> > attribute?
> 
> Yes.  I expected Emacs to act as it has in the past,
> and ignore such an :inherit as a no-op.

What happened in the past was the result of the implementation, not
the design.  The implementation changed (for valid reasons), so the
undefined behavior your code invokes now gets you in trouble.

> > If FACE1 has some attribute 'unspecified', but inherits
> > from FACE2, Emacs will try to go up the inheritance chain to see if
> > some of the parent faces specifies that attribute.  If FACE1 inherits
> > from itself, going up the inheritance chain will never end.
> 
> Only if the implementation doesn't recognize the
> inheritance loop.

What would you expect it to do if it did?

And how does your code differ from this:

  (while t nil)

Or would you consider it a bug that Emacs doesn't detect the infinite
while-loop, either?

> > IOW, it's a cockpit error.
> 
> You can look at it that way.  But I'd expect that
> inheritance of a face from itself would be a no-op.

Why?

> And that's exactly what the case was in Emacs 26.

Yes, undefined behavior can do different things in different versions.

> Why should the code now chase its tail down an
> infinite rabbit hole?  Is this really by design?

The design is to chase the inheritance chain, yes.  Why is it needed
is a long story, and isn't really relevant for the issue at hand, as
long as it isn't a bug or faulty design.  Faces aren't supposed to
inherit from themselves, period.

> Not rhetorical questions.  It seems like a design
> change has been made, but with no announcement.

The announcement _was_ made, but we announce features, not their
designs.

bug#51577: 27.2; Regression: reproducible hang with face functions

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> And how does your code differ from this:
>
>   (while t nil)
>
> Or would you consider it a bug that Emacs doesn't detect the infinite
> while-loop, either?

We do try to detect loops in other situations (to avoid hangs).  This is
in redisplay code, though (I think?), so perhaps it would be
prohibitively expensive to try to detect loops here?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51577: 27.2; Regression: reproducible hang with face functions

[martin rudalics]

 > We do try to detect loops in other situations (to avoid hangs).  This is
 > in redisplay code, though (I think?), so perhaps it would be
 > prohibitively expensive to try to detect loops here?

FWIW this hangs in some assq_no_quit in Emacs 27 only.  I have not been
able to reproduce it in Emacs 28.  Can you?

martin

bug#51577: 27.2; Regression: reproducible hang with face functions

[Lars Ingebrigtsen]

martin rudalics <rudalics@gmx.at> writes:

>> We do try to detect loops in other situations (to avoid hangs).  This is
>> in redisplay code, though (I think?), so perhaps it would be
>> prohibitively expensive to try to detect loops here?
>
> FWIW this hangs in some assq_no_quit in Emacs 27 only.  I have not been
> able to reproduce it in Emacs 28.  Can you?

No, I didn't try to reproduce the problem.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51577: 27.2; Regression: reproducible hang with face functions

[Eli Zaretskii]

> Cc: 51577@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Thu, 4 Nov 2021 19:48:56 +0100
> 
>  > We do try to detect loops in other situations (to avoid hangs).  This is
>  > in redisplay code, though (I think?), so perhaps it would be
>  > prohibitively expensive to try to detect loops here?
> 
> FWIW this hangs in some assq_no_quit in Emacs 27 only.  I have not been
> able to reproduce it in Emacs 28.  Can you?

No, it infloops in face_inherited_attr, and I see it in Emacs 28 as
well.

bug#51577: 27.2; Regression: reproducible hang with face functions

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Drew Adams <drew.adams@oracle.com>,  51577@debbugs.gnu.org
> Date: Thu, 04 Nov 2021 19:15:23 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > And how does your code differ from this:
> >
> >   (while t nil)
> >
> > Or would you consider it a bug that Emacs doesn't detect the infinite
> > while-loop, either?
> 
> We do try to detect loops in other situations (to avoid hangs).  This is
> in redisplay code, though (I think?), so perhaps it would be
> prohibitively expensive to try to detect loops here?

First, since this is triggered by redisplay, any errors must be
"silent": we log an error message in *Messages* and otherwise silently
do nothing.  Which in this case would mean the problematic face will
not be applied.  Not sure this is better than inflooping.

As for detecting loops: it could be tricky.  It is easy enough to
detect simple loops such as the one in this case, where a face
inherits directly from itself, and the value of the :inherit attribute
is the symbol of a face.  But inheritance loops could be less simple:
a face could inherit from itself indirectly, and the value of the
attribute could be a list, not a named face.  Detecting loops in those
cases would require recording face specs/names we already saw in some
list, and each time we get an :inherit attribute, check if its value
is already in the list.  Is that worth our while, if the result will
be a silent error message in *Messages*?

bug#51577: [External] : Re: bug#51577: 27.2; Regression: reproducible hang with face functions

[Drew Adams]

> As for detecting loops: it could be tricky.  It is easy enough to
> detect simple loops such as the one in this case, where a face
> inherits directly from itself, and the value of the :inherit attribute
> is the symbol of a face.  But inheritance loops could be less simple:
> a face could inherit from itself indirectly, and the value of the
> attribute could be a list, not a named face.  Detecting loops in those
> cases would require recording face specs/names we already saw in some
> list, and each time we get an :inherit attribute, check if its value
> is already in the list.  Is that worth our while, if the result will
> be a silent error message in *Messages*?

Yes, nested inherits are more complicated.

Whether some attempt is made in that regard
could maybe be a wishlist request.

Let's not let ideal become the enemy of good,
here.

I'd be happy just removing the regression, i.e.,
returning to the behavior prior to Emacs 27
(in this regard - I don't mean just reverting
code; how to remove the regression is up to you).

IOW, allow a face `foo' to have (and ignore) an
`:inherit foo' in its face spec.

Clearly, somehow Emacs 26 tolerated such simple
self-inheritance (as a no-op).  Whether it did
so in the right way or not, in terms of design
or implementation, I don't know.

bug#51577: 27.2; Regression: reproducible hang with face functions

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Detecting loops in those cases would require recording face
> specs/names we already saw in some list, and each time we get an
> :inherit attribute, check if its value is already in the list.  Is
> that worth our while, if the result will be a silent error message in
> *Messages*?

Well, the result is that Emacs doesn't hang.  :-)  I'm not so worried
about issuing any messages here.

But I wonder -- could we do this (expensive) checking in defface (or
rather custom-declare-face)?  That could be helpful -- evalling a
circular face definition should signal an error.

Do we have all the information needed to see whether we have a circular
definition at that point?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51577: [External] : Re: bug#51577: 27.2; Regression: reproducible hang with face functions

[Drew Adams]

> evalling a circular face definition should signal an error.

No.  The inheritance that causes the infloop
should just not be done (ignored.)  That sane
behavior is exactly what was broken, in the
case cited.

Whether you want to try to ambitiously deal
with nested inheritance etc. causing loops
is something else.  That's not what this bug
(regression) report is about.

Inheritance of a face by itself should be
ignored, silently.  Emacs should at least be
smart enough to do that.  (It used to be.)

bug#51577: [External] : Re: bug#51577: 27.2; Regression: reproducible hang with face functions

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> CC: "51577@debbugs.gnu.org" <51577@debbugs.gnu.org>
> Date: Fri, 5 Nov 2021 02:41:09 +0000
> Accept-Language: en-US
> 
> > evalling a circular face definition should signal an error.
> 
> No.  The inheritance that causes the infloop
> should just not be done (ignored.)

I disagree.  Inheritance loop is an error in defining a face, and
should be flagged as such when detected.

> That sane behavior is exactly what was broken, in the case cited.

That "sane" behavior was actually a subtle bug.  I invite you to file
a bug against Emacs 26 that it didn't detect that.

> Inheritance of a face by itself should be
> ignored, silently.

DISAGREE!!

bug#51577: 27.2; Regression: reproducible hang with face functions

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: drew.adams@oracle.com,  51577@debbugs.gnu.org
> Date: Thu, 04 Nov 2021 23:56:16 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Detecting loops in those cases would require recording face
> > specs/names we already saw in some list, and each time we get an
> > :inherit attribute, check if its value is already in the list.  Is
> > that worth our while, if the result will be a silent error message in
> > *Messages*?
> 
> Well, the result is that Emacs doesn't hang.  :-)  I'm not so worried
> about issuing any messages here.

I actually worried about the CPU cycles spent checking that end in
silence than about the silence itself.

> But I wonder -- could we do this (expensive) checking in defface (or
> rather custom-declare-face)?  That could be helpful -- evalling a
> circular face definition should signal an error.

Checking this at defface time could only solve some small subset of
the cases.  It wouldn't solve this one, for example, because the
original defface:

  (defface alt-region '((t :background "gray70" :inherit region))
    "..." :group 'faces)

is completely innocent.  It's only later, when the code does this:

    (apply #'set-face-attribute 'region frame plist)))

that it actually makes the 'region' face inherit from itself.

Similarly, inheritance loops can (and usually do) appear at run time,
because faces are added or their attributes modified or merged with
other faces.

So, bottom line, I don't think that detection at defface time will
help here.

bug#51577: 27.2; Regression: reproducible hang with face functions

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> It's only later, when the code does this:
>
>     (apply #'set-face-attribute 'region frame plist)))
>
> that it actually makes the 'region' face inherit from itself.

Well...   then we could do a check in `set-face-attribute' (too)?

> Similarly, inheritance loops can (and usually do) appear at run time,
> because faces are added or their attributes modified or merged with
> other faces.

Hm...  will merging lead to loops?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51577: 27.2; Regression: reproducible hang with face functions

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: drew.adams@oracle.com,  51577@debbugs.gnu.org
> Date: Fri, 05 Nov 2021 14:24:17 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It's only later, when the code does this:
> >
> >     (apply #'set-face-attribute 'region frame plist)))
> >
> > that it actually makes the 'region' face inherit from itself.
> 
> Well...   then we could do a check in `set-face-attribute' (too)?

That sets a single attribute, so you are proposing checking this for
each call?  The loop will only be evident near the end of the 'apply'
call.

> > Similarly, inheritance loops can (and usually do) appear at run time,
> > because faces are added or their attributes modified or merged with
> > other faces.
> 
> Hm...  will merging lead to loops?

That's exactly what happened in the case in point: we merged the
'default' font with the 'region' font.

bug#51577: 27.2; Regression: reproducible hang with face functions

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> > Similarly, inheritance loops can (and usually do) appear at run time,
>> > because faces are added or their attributes modified or merged with
>> > other faces.
>> 
>> Hm...  will merging lead to loops?
>
> That's exactly what happened in the case in point: we merged the
> 'default' font with the 'region' font.

I see.  OK, then I think there isn't much we can do on the usability
front here, and checking for loops at display time would be prohibitive,
apparently, so we're not going to fix this, and I'm closing this bug
report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no