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

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

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

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Alan,

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

Aha!

> Is there any reason this function call was left where it was?

I was afraid that swapping `move_minibuffers_onto_frame` and

  if (EQ (f->selected_window, f->minibuffer_window)
      /* The following test might fail if the mini-window contains a
	 non-active minibuffer.  */
      && NILP (Fminibufferp (XWINDOW (f->minibuffer_window)->contents, Qt)))
    {
      Lisp_Object w = call1 (Qget_mru_window, frame);
      if (WINDOW_LIVE_P (w)) /* W can be nil in minibuffer-only frames.  */
        Fset_frame_selected_window (frame, w, Qnil);
    }

would not result in the same final result, e.g. because the name
"move_minibuffers_onto_frame" suggests it might change the result of
`XWINDOW (f->minibuffer_window)->contents`.

> Or could it just be moved, too?  Otherwise, its calling convention
> will need to be adapted.

The patch below refrains from moving it and passes it the to-be-selected
frame as argument instead.


        Stefan


diff --git a/src/frame.c b/src/frame.c
index 91b9bec82c3..24de0701d13 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -1503,17 +1503,7 @@ 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);
+  move_minibuffers_onto_frame (sf, frame, for_deletion);
 
   /* 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
@@ -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
@@ -2110,7 +2110,7 @@ delete_frame (Lisp_Object frame, Lisp_Object force)
   else
     /* Ensure any minibuffers on FRAME are moved onto the selected
        frame.  */
-    move_minibuffers_onto_frame (f, true);
+    move_minibuffers_onto_frame (f, selected_frame, true);
 
   /* Don't let echo_area_window to remain on a deleted frame.  */
   if (EQ (f->minibuffer_window, echo_area_window))
diff --git a/src/lisp.h b/src/lisp.h
index 56f24d82810..5f6721595c0 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4792,7 +4792,7 @@ fast_string_match_ignore_case (Lisp_Object regexp, Lisp_Object string)
 
 extern Lisp_Object Vminibuffer_list;
 extern Lisp_Object last_minibuf_string;
-extern void move_minibuffers_onto_frame (struct frame *, bool);
+extern void move_minibuffers_onto_frame (struct frame *, Lisp_Object, bool);
 extern bool is_minibuffer (EMACS_INT, Lisp_Object);
 extern EMACS_INT this_minibuffer_depth (Lisp_Object);
 extern EMACS_INT minibuf_level;
diff --git a/src/minibuf.c b/src/minibuf.c
index bedc5644807..aebd17c0b76 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -187,13 +187,15 @@ zip_minibuffer_stacks (Lisp_Object dest_window, Lisp_Object source_window)
 
 /* If `minibuffer_follows_selected_frame' is t, or we're about to
    delete a frame which potentially "contains" minibuffers, move them
-   from the old frame to the selected frame.  This function is
+   from the old frame to the to-be-selected frame.  This function is
    intended to be called from `do_switch_frame' in frame.c.  OF is the
-   old frame, FOR_DELETION is true if OF is about to be deleted.  */
+   old frame, SF is the to-be-selected frame, and FOR_DELETION is true
+   if OF is about to be deleted.  */
 void
-move_minibuffers_onto_frame (struct frame *of, bool for_deletion)
+move_minibuffers_onto_frame (struct frame *of, Lisp_Object sf,
+                             bool for_deletion)
 {
-  struct frame *f = XFRAME (selected_frame);
+  struct frame *f = XFRAME (sf);
 
   minibuf_window = f->minibuffer_window;
   if (!(minibuf_level
@@ -206,7 +208,7 @@ move_minibuffers_onto_frame (struct frame *of, bool for_deletion)
     {
       zip_minibuffer_stacks (f->minibuffer_window, of->minibuffer_window);
       if (for_deletion && XFRAME (MB_frame) != of)
-	MB_frame = selected_frame;
+	MB_frame = sf;
     }
 }
 

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

[Alan Mackenzie]

Hello, Stefan.

On Wed, Oct 12, 2022 at 14:35:33 -0400, Stefan Monnier wrote:
> Hi Alan,

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

> Aha!

> > Is there any reason this function call was left where it was?

> I was afraid that swapping `move_minibuffers_onto_frame` and

>   if (EQ (f->selected_window, f->minibuffer_window)
>       /* The following test might fail if the mini-window contains a
> 	 non-active minibuffer.  */
>       && NILP (Fminibufferp (XWINDOW (f->minibuffer_window)->contents, Qt)))
>     {
>       Lisp_Object w = call1 (Qget_mru_window, frame);
>       if (WINDOW_LIVE_P (w)) /* W can be nil in minibuffer-only frames.  */
>         Fset_frame_selected_window (frame, w, Qnil);
>     }

> would not result in the same final result, e.g. because the name
> "move_minibuffers_onto_frame" suggests it might change the result of
> `XWINDOW (f->minibuffer_window)->contents`.

Yes, I suppose you're right.

> > Or could it just be moved, too?  Otherwise, its calling convention
> > will need to be adapted.

> The patch below refrains from moving it and passes it the to-be-selected
> frame as argument instead.

Yes.  That's a better solution than moving the function call around,
particularly given that there're are only two calls to it, both in
frame.c.

Just one small thing: the name sf is a bad name for a Lisp_Object -
there's a de facto convention in minibuf.c and frame.c that f, sf, and
of, etc are struct *frame's, and selected_frame, frame, etc., are
Lisp_Object's which are frames.  In fact sf is used as a struct *frame
elsewhere in minibuf.c.

Let me guess - originally this sf was a struct *frame, but got changed
to a Lisp_Object when it became clear that it was needed to set MB_frame
with.  ;-)

>         Stefan

[ .... ]

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Just one small thing: the name sf is a bad name for a Lisp_Object -

Very good point, thanks.

> Let me guess - originally this sf was a struct *frame, but got changed
> to a Lisp_Object when it became clear that it was needed to set MB_frame
> with.  ;-)

Did you install a camera in my office?

I pushed the updated code, thank you,


        Stefan