GNU bug report logs - #58343
29.0.50; ELisp code run in "inconsitent" selected-window state

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 58343 in the body.
You can then email your comments to 58343 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; ELisp code run in "inconsitent" selected-window state
Date: Thu, 06 Oct 2022 20:06:25 -0400

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

Message #8 received at 58343 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 58343 <at> debbugs.gnu.org
Subject: Re: bug#58343: 29.0.50; ELisp code run in "inconsitent"
 selected-window state
Date: Wed, 12 Oct 2022 18:08:47 +0000

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

Message #11 received at 58343 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 58343 <at> debbugs.gnu.org
Subject: Re: bug#58343: 29.0.50; ELisp code run in "inconsitent"
 selected-window state
Date: Wed, 12 Oct 2022 14:35:33 -0400

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;
     }
 }
 

Message #14 received at 58343 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: acm <at> muc.de, 58343 <at> debbugs.gnu.org
Subject: Re: bug#58343: 29.0.50; ELisp code run in "inconsitent"
 selected-window state
Date: Wed, 12 Oct 2022 19:13:51 +0000

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

Message #17 received at 58343 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 58343 <at> debbugs.gnu.org
Subject: Re: bug#58343: 29.0.50; ELisp code run in "inconsitent"
 selected-window state
Date: Wed, 12 Oct 2022 17:07:04 -0400

> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.