bug#58343: 29.0.50; ELisp code run in "inconsitent" selected-window state

[Alan Mackenzie]

Hello, Stefan.

On Thu, Oct 06, 2022 at 20:06:25 -0400, Stefan Monnier wrote:
> Package: Emacs
> Version: 29.0.50


> Hi Alan and friends,

> In commit dfa3e6f424b20fe27d9041b2ce7d69811df5d8cd, Alan added the
> following code to do_switch_frame:

>     diff --git a/src/frame.c b/src/frame.c
>     index ccac18d23c2..dc8045f41e6 100644
>     --- a/src/frame.c
>     +++ b/src/frame.c
>     @@ -1564,6 +1564,13 @@ do_switch_frame (Lisp_Object frame, int track, int 
> for_deletion, Lisp_Object nor
>        if (! FRAME_MINIBUF_ONLY_P (XFRAME (selected_frame)))
>          last_nonminibuf_frame = XFRAME (selected_frame);
     
>     +  /* If the selected window in the target frame is its mini-window, we 
> move
>     +     to a different window, the most recently used one, unless there is a
>     +     valid active minibuffer in the mini-window.  */
>     +  if (EQ (f->selected_window, f->minibuffer_window)
>     +      && NILP (Fminibufferp (XWINDOW (f->minibuffer_window)->contents, 
> Qt)))
>     +    Fset_frame_selected_window (frame, call1 (Qget_mru_window, frame), 
> Qnil);
>     +
>        Fselect_window (f->selected_window, norecord);
     
>        /* We want to make sure that the next event generates a frame-switch

> the problem with this is that it calls `Qget_mru_window` which is ELisp
> code, and that we're in the very middle of changing the selected frame,
> so we have already changed the `selected-frame` variable a few lines
> earlier, but the select-window has not yet been adjusted accordingly.

> Running ELisp code in a state where (selected-window) and
> (frame-selected-window) aren't equal is a recipe for problems.  I have
> already wasted many hours in the past tracking down bugs linked to this
> kind of situation (back when the mode-line was processed in such an
> inconsistent state, for example), and I really don't want to go there.

> So we should arrange to run this `get-mru-window` function at some other
> time, for example a few lines earlier before we set `selected-frame`.

> I don't understand this code nearly enough to know how to move the code,
> because it probably interacts with the other statements in non-trivial
> ways, so the patch below is just a naive suggestion (it seems to work
> here without triggering my many sprinkled assertions checking that
> `EQ (XFRAME (selected_frame)->selected_window, selected_window)`,
> but it's a far cry from a confirmation that it's right).
> Hopefully someone here is aware of some of the potential pitfalls.

One minor problem I see at the moment is that the call to
move_minibuffers_onto_frame wasn't moved together with the surrounding
code.  With its current calling convention, it needs to be called
_after_ selected_frame has been set, because it uses selected_frame.

Is there any reason this function call was left where it was?  Or could
it just be moved, too?  Otherwise, its calling convention will need to
be adapted.

Other than that, I haven't seen any problems as yet, though I admit I
haven't tried it out.

>         Stefan

[ .... ]

-- 
Alan Mackenzie (Nuremberg, Germany).