bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Thu, 7 May 2020 10:34:53 +0200
> 
>  >> With Bug#40639 and Bug#41071 the situation is simple: The internal
>  >> border changed face but when we enter init_iterator
>  >> inhibit_free_realized_faces is true and we don't handle it.
>  >
>  > Can you show the backtrace from this call to init_iterator?
> 
> I run emacs -Q and evaluate there the following form
> 
> (progn
>    (set-face-background 'internal-border "red")
>    (select-window
>     (display-buffer-in-child-frame
>      (get-buffer-create "*scratch*")
>      '((child-frame-parameters
>         .
>         ((left . 100)
>       (top  . 100)
>       (height . 10)
>       (width . 20)
>       (minibuffer . nil)
>       (internal-border-width . 50)))))))
> 
> Now _if I remove the following two lines_
> 
>      if ((IT)->glyph_row != NULL)                        \
>        inhibit_free_realized_faces = true;            \
> 
> from PRODUCE_GLYPHS, evaluating the form above does pick up the face
> triggered by
> 
> XFRAME (w->frame)->face_change
> 
> true as
> 
> #0  0x0000000000463cbf in init_iterator (it=0x7fffffff8c60, w=0x193e060, 
> charpos=1, bytepos=1, row=0x1947f90, base_face_id=DEFAULT_FACE_ID) at 
> ../../src/xdisp.c:2975
> #1  0x00000000004652e8 in start_display (it=0x7fffffff8c60, w=0x193e060, 
> pos=...) at ../../src/xdisp.c:3298
> #2  0x0000000000497468 in try_window (window=XIL(0x193e065), pos=..., 
> flags=1) at ../../src/xdisp.c:19120
> #3  0x00000000004941e9 in redisplay_window (window=XIL(0x193e065), 
> just_this_one_p=false) at ../../src/xdisp.c:18544
> #4  0x000000000048be75 in redisplay_window_0 (window=XIL(0x193e065)) at 
> ../../src/xdisp.c:16258
> #5  0x00000000007af751 in internal_condition_case_1 (bfun=0x48be33 
> <redisplay_window_0>, arg=XIL(0x193e065), handlers=XIL(0x7ffff3e98ee3), 
> hfun=0x48bdfb <redisplay_window_error>) at ../../src/eval.c:1379
> #6  0x000000000048bdcd in redisplay_windows (window=XIL(0x193e065)) at 
> ../../src/xdisp.c:16238
> #7  0x000000000048a7fb in redisplay_internal () at ../../src/xdisp.c:15706
> [...]
> With PRODUCE_GLYPHS unaltered, that breakpoint does _not_ trigger.  Only
> if I set frame titles for child frames (as I do on master now) I can get
> a backtrace like the below
> 
> #0  0x00000000004821dc in init_iterator (it=0x7fffffffb4a0, w=0x1a4b800, 
> charpos=-1, bytepos=-1, row=0x0, base_face_id=DEFAULT_FACE_ID) at 
> ../../src/xdisp.c:2988
> #1  0x00000000004a575b in gui_consider_frame_title (frame=XIL(0x1a5e8f5)) at 
> ../../src/xdisp.c:12447
> #2  0x00000000004a5c07 in prepare_menu_bars () at ../../src/xdisp.c:12544
> #3  0x00000000004aaeaf in redisplay_internal () at ../../src/xdisp.c:15392

The second call happens before the first one, so it looks more correct
to me.  Why did you think you didn't need to set the frame title for
child frames?

> Disclaimer: In all those runs I do not know whether the
> 
>    (set-face-background 'internal-border "red")
> 
> set the face_change flag or something else did.

It doesn't.  It sets the face_change flag of each frame instead.  See
internal-set-lisp-face-attribute.

>  >> Nevertheless, the fact that inhibit_free_realized_faces is true when
>  >> entering the iterator after a face change is IMO a bug.  We apparently
>  >> always run the iterator with the old faces first.  Only after we have
>  >> (incidentally?) detected some change that forces us to "retry", we have
>  >> redisplay set inhibit_free_realized_faces to false itself and the face
>  >> change will get applied in the next iteration.  If so, the outcome of
>  >> the previous iterations gets lost if a face changed there.  Does such a
>  >> strategy give us any gains?
>  >
>  > I don't think I follow.  redisplay_internal resets the
>  > inhibit_free_realized_faces flag to zero near its beginning.  It is
>  > true that we also reset it when we retry, but the first try doesn't
>  > bypass this resetting.  Am I missing something?
> 
> It might have been a wrong conclusion on my side.  Maybe the problem is
> caused by the fact that face_change is not set by 'set-face-background'
> and the face change that triggered the backtraces above came from
> somewhere else.

See above.  It could be that we somehow fail to redisplay the child
frame thoroughly enough, though.

>  >> Again, the question I tried to ask earlier was: Does a current matrix in
>  >> between two redisplays rely on the old realized faces?
>  >
>  > Of course it does.  The glyph matrices don't hold any face
>  > information, they only hold the ID of each face, and the ID is just
>  > the index of the face's cache slot.  If the face cache is thrown away,
>  > we will not be able to use the current matrix.
> 
> So if I set `inhibit-free-realized-faces' to nil at some arbitrary time
> things may go wrong.

Only if you do that in code that can run while redisplay_internal is
doing its job.

>  >   . expose_frame:
>  >
>  >    if (FRAME_FACE_CACHE (f) == NULL
>  >        || FRAME_FACE_CACHE (f)->used < BASIC_FACE_ID_SENTINEL)
>  >      {
>  >        redisplay_trace ("expose_frame no faces\n");
>  >        return;
>  >      }
>  >
>  >   . expose_window (called by expose_frame):
>  >
>  >    if (w->current_matrix == NULL)
>  >      return false;
>  >
>  > So this case is covered, i.e. you can call
> 
> ... from Elisp, at any time, I presume ...

Yes, but not limited to that.

>  > The other two are the reason why we set the
>  > inhibit_free_realized_faces in PRODUCE_GLYPHS: the flag must be set
>  > during redisplay, until update_frame finished its job.  Otherwise we
>  > will sooner or later crash.
> 
> I'm running without this for a couple of days now and it did not crash
> so far.

Try setting things up such that faces are modified by some Lisp that
runs during redisplay, like some :eval form of the mode line or one of
the hooks called by the display code.  That's how crashes with face =
NULL invariably start.

> Sheer luck, I presume.  Couldn't PRODUCE_GLYPHS set
> inhibit_free_realized_faces iff redisplaying_p is true?

No, because some functions we call from Lisp actually write into the
current matrix.  For example, tool-bar-height and tab-bar-height.

>  > So I think each function that might need to notice face changes as
>  > soon as they happened, should be able to reset
>  > inhibit_free_realized_faces, provided that (a) it makes sure
>  > redisplaying_p is zero, and (b) it only does that if necessary and for
>  > a single frame.  The latter part is because clearing the face cache
>  > will cause all the move_it_* functions to work harder, because they
>  > will have to recompute all the faces.
> 
> So it's not so entirely trivial to do that in Elisp.

Why do we need to do this in Lisp?

>  >> If so, the answer is that inhibit_free_realized_faces should be
>  >> always true but for the small window within retrying in
>  >> redisplay_internal.
>  >
>  > I don't think I agree, but maybe I'm missing something.
> 
> OK.  But IIUC so far we do not allow inhibit_free_realized_faces nil
> outside of redisplay_internal.  Unless someone sets
> 'inhibit-free-realized-faces' which is, in general, to recommended.
> Right?

Yes, but I see no reason not to reset it in other places, provided
that we do it carefully and only when absolutely needed.

>  >> In either case, it strikes me as a strange idea that redisplay_internal
>  >> saves and restores the value of a variable which is apparently always
>  >> true when it is entered (I suppose redisplay_internal is not re-entrant
>  >> given
>  >>
>  >>     /* I don't think this happens but let's be paranoid.  */
>  >>     if (redisplaying_p)
>  >>       return;
>  >
>  > redisplay_internal is non-reentrant, but I see no harm in restoring
>  > the value on exit.
> 
> If someone sets it to nil now, observing the precautions you gave above,
> things change radically.  We may restore the value to nil, which was
> hardly the case before.

That'd be shooting ourselves in the foot during the next redisplay,
but it isn't a catastrophe, AFAIU.