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

[martin rudalics]

>> 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
#8  0x00000000004883f9 in redisplay () at ../../src/xdisp.c:14933
#9  0x000000000064edbd in read_char (commandflag=1, map=XIL(0x17e4f93), 
prev_event=XIL(0), used_mouse_menu=0x7fffffffe0ff, end_time=0x0) at 
../../src/keyboard.c:2493
#10 0x0000000000661ce5 in read_key_sequence (keybuf=0x7fffffffe290, 
prompt=XIL(0), dont_downcase_last=false, can_return_switch_frame=true, 
fix_current_buffer=true, prevent_redisplay=false) at ../../src/keyboard.c:9553
#11 0x000000000064b283 in command_loop_1 () at ../../src/keyboard.c:1350
#12 0x00000000007af676 in internal_condition_case (bfun=0x64ae07 <command_loop_1>, 
handlers=XIL(0x90), hfun=0x64a416 <cmd_error>) at ../../src/eval.c:1355
#13 0x000000000064a9ec in command_loop_2 (ignore=XIL(0)) at 
../../src/keyboard.c:1091
#14 0x00000000007aeb2a in internal_catch (tag=XIL(0xd110), func=0x64a9bf 
<command_loop_2>, arg=XIL(0)) at ../../src/eval.c:1116
#15 0x000000000064a98a in command_loop () at ../../src/keyboard.c:1070
#16 0x0000000000649efd in recursive_edit_1 () at ../../src/keyboard.c:714
#17 0x000000000064a0f5 in Frecursive_edit () at ../../src/keyboard.c:786
#18 0x00000000006404fe in main (argc=4, argv=0x7fffffffe788) at 
../../src/emacs.c:2054

Lisp Backtrace:
"redisplay_internal (C function)" (0x0)
(gdb) c

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
#4  0x00000000004a9aeb in redisplay () at ../../src/xdisp.c:14977
#5  0x0000000000768eb6 in read_char (commandflag=1, map=XIL(0x1c5c3d3), 
prev_event=XIL(0), used_mouse_menu=0x7fffffffe10f, end_time=0x0) at 
../../src/keyboard.c:2493
#6  0x00000000007800e3 in read_key_sequence (keybuf=0x7fffffffe2a0, 
prompt=XIL(0), dont_downcase_last=false, can_return_switch_frame=true, 
fix_current_buffer=true, prevent_redisplay=false) at ../../src/keyboard.c:9547
#7  0x000000000076549a in command_loop_1 () at ../../src/keyboard.c:1350
#8  0x000000000084133d in internal_condition_case (bfun=0x76501e <command_loop_1>, 
handlers=XIL(0x90), hfun=0x76462d <cmd_error>) at ../../src/eval.c:1355
#9  0x0000000000764c03 in command_loop_2 (ignore=XIL(0)) at 
../../src/keyboard.c:1091
#10 0x00000000008407f1 in internal_catch (tag=XIL(0xd200), func=0x764bd6 
<command_loop_2>, arg=XIL(0)) at ../../src/eval.c:1116
#11 0x0000000000764ba1 in command_loop () at ../../src/keyboard.c:1070
#12 0x0000000000764114 in recursive_edit_1 () at ../../src/keyboard.c:714
#13 0x000000000076430c in Frecursive_edit () at ../../src/keyboard.c:786
#14 0x000000000076048f in main (argc=3, argv=0x7fffffffe798) at 
../../src/emacs.c:2035

which also picks up the change of the internal border's background.

With an unaltered release I can't get init_iterator to notice the face
change without, in some clumsy way, switching to that child frame with
the mouse or something the like.

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.

>>          if (face_change)
>>            {
>>              face_change = false;
>>              XFRAME (w->frame)->face_change = 0;
>>              free_all_realized_faces (Qnil);
>>            }
>>          else
>>            {
>>              XFRAME (w->frame)->face_change = 0;
>>              free_all_realized_faces (w->frame);
>>            }
>
> You don't really need to free the faces everywhere here, only for the
> window W's frame, right?

Right.

> I think we shouldn't throw away face caches
> of other frames in this situation, it's too radical.

Agreed.

> I think the only situation where it can hurt is when this code is
> called while redisplay_internal runs.  IOW, we need to test the value
> of redisplaying_p.

OK.

>> At the very least I probably should set windows_or_buffers_changed.
>> There is this comment in redisplay_internal
>>
>>     /* If face_change, init_iterator will free all realized faces, which
>>        includes the faces referenced from current matrices.  So, we
>>        can't reuse current matrices in this case.  */
>>     if (face_change)
>>       windows_or_buffers_changed = 47;
>>
>> which hints at that (without considering that f->face_change is not
>> handled there).
>
> The call to free_all_realized_faces invalidates the current matrices
> of all the windows on the frame, so the stale face IDs will not be
> used after that on that frame, and therefore nothing else needs to be
> done in that case.  If _all_ the face caches are discarded on all
> frames, we need windows_or_buffers_changed to force redisplay_internal
> consider more than just the selected frame.  So if you modify your
> code to free faces only on the frame of the window for which you
> compute the mode-line height, the problem with
> windows_or_buffers_changed will disappear.

OK.

>> 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.

>> 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.

> The current matrix is used for the following purposes:
>
>    . outside of the redisplay cycle, for redisplaying portions of
>      windows when we get expose events -- in this case we simply write
>      to the glass the relevant portions of the matrix, without
>      reproducing it
>
>    . during a redisplay cycle, for decisions about redisplay
>      optimizations
>
>    . at the end of a redisplay cycle, for comparison with the desired
>      matrix, which is needed for figuring out what to write to the
>      glass
>
> For the first purpose, we have the following defenses:
>
>   . 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 ...

> free_all_realized_faces,
> and the next expose event will be handled correctly.

> 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.  Sheer luck, I presume.  Couldn't PRODUCE_GLYPHS set
inhibit_free_realized_faces iff redisplaying_p is true?

> 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.

>> 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?

>> 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.

martin