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

[Stefan Monnier]

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.


        Stefan


diff --git a/src/frame.c b/src/frame.c
index 91b9bec82c3..58b6ee50d23 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -1503,18 +1503,8 @@ do_switch_frame (Lisp_Object frame, int for_deletion, 
Lisp_Object norecord)
 
   sf->select_mini_window_flag = MINI_WINDOW_P (XWINDOW (sf->selected_window));
 
-  selected_frame = frame;
-
   move_minibuffers_onto_frame (sf, for_deletion);
 
-  if (f->select_mini_window_flag
-      && !NILP (Fminibufferp (XWINDOW (f->minibuffer_window)->contents, Qt)))
-    f->selected_window = f->minibuffer_window;
-  f->select_mini_window_flag = false;
-
-  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.  */
@@ -1528,6 +1518,16 @@ do_switch_frame (Lisp_Object frame, int for_deletion, 
Lisp_Object norecord)
         Fset_frame_selected_window (frame, w, Qnil);
     }
 
+  selected_frame = frame;
+
+  if (f->select_mini_window_flag
+      && !NILP (Fminibufferp (XWINDOW (f->minibuffer_window)->contents, Qt)))
+    f->selected_window = f->minibuffer_window;
+  f->select_mini_window_flag = false;
+
+  if (! FRAME_MINIBUF_ONLY_P (XFRAME (selected_frame)))
+    last_nonminibuf_frame = XFRAME (selected_frame);
+
   Fselect_window (f->selected_window, norecord);
 
   /* We want to make sure that the next event generates a frame-switch