bug#39977: 28.0.50; Unhelpful stack trace

[martin rudalics <rudalics@gmx.at>]

[-- Attachment #1: Type: text/plain, Size: 4199 bytes --]

 >>   >> (and (frame-live-p (selected-frame))
 >>   >>        (window-live-p (selected-window))
 >>   >>        (eq (frame-selected-window (selected-frame))
 >>   >> 	 (selected-window)))
 >>   >
 >>   > I agree.  But note that selected-frame could switch frames internally,
 >>   > if the last selected frame is dead; as long as selected-frame also
 >>   > adjusts the selected window, the above will still hold.
 >>
 >> Do you mean 'select-frame' instead of 'selected-frame'?
 >
 > No, I meant selected-frame.

So you meant a 'selected-frame' based on the SELECTED_FRAME I proposed.
The problem is that that macro won't protect the invariant if it was
broken before, for example, by setting the selected window to a dead
window as in Madhu's scenario.

 > Perhaps we should make unwind_format_mode_line less fragile, then.

The problematic step happens already at the time we set it up.  In a
nutshell, Madhu's scenario goes as follows: In display_mode_lines we do

   Lisp_Object old_selected_window = selected_window;
   Lisp_Object old_selected_frame = selected_frame;
...
       display_mode_line (w, CURRENT_MODE_LINE_FACE_ID_3 (sel_w, sel_w, w),
			 NILP (window_mode_line_format)
			 ? BVAR (current_buffer, mode_line_format)
			 : window_mode_line_format);
...
   XFRAME (new_frame)->selected_window = old_frame_selected_window;
   selected_frame = old_selected_frame;
   selected_window = old_selected_window;

where display_mode_line deletes both, old_selected_frame and
old_selected_window.  So we end up with selected_frame and
selected_window both referencing dead objects.  The subsequent call of
gui_consider_frame_title now does

       record_unwind_protect (unwind_format_mode_line,
			     format_mode_line_unwind_data
			       (f, current_buffer, selected_window, false));

where selected_window is already a dead window.  Since in
unwind_format_mode_line old_window is non-nil, it will call

       Fselect_window (old_window, Qt);

which first chokes on

    CHECK_LIVE_WINDOW (window);

which is the error reported when emacs does not crash and finally on

   sf = SELECTED_FRAME ();

which crashes emacs due to the fact that selected_frame is dead.  In
either case, making the unwind_format_mode_line less fragile won't avoid
any crash, it might just postpone it.

 >> And I have no idea yet why we need an extra unwind for restoring
 >> selected_frame and selected_window.  Shouldn't these go hand in hand
 >> with what unwind_format_mode_line does?  Does the one even know
 >> about the other?
 >
 > I don't think I understand what extra unwind are you talking about
 > here.  Can you provide a more specific pointer to the relevant code?

I meant the fact that we already do unwind_format_mode_line when
formatting the mode line and that function could restore the selected
window in a safe way.  I'm far from proposing to use that approach when
drawing the mode lines, though.

Attached find a patch which should solve the more grave problems caused
by a function deleting the previously selected frame or window.  It
intentionally does not change SELECTED_FRAME.  Any abort there should be
reserved to obscure bugs we have not been able to trace yet.  Please
read it with your usual care, it took me some time to convince myself
that it selects its frame in a reasonable way.

On master, I would then like to use restore_selected_window also for
gui_consider_frame_title.  The overhead caused by that is a great
annoyance (especially when debugging frame switching code) and we could
then hopefully get rid of the old_window stuff and the Bug#32777 fix as
well.

What any patch I provide here cannot do is to fix problems when the mode
line code deletes the selected window right away.  Code like


(defvar window (split-window))

(defvar foo
   '(:eval
     (if (or (not (window-live-p window))
	    (eq window (frame-first-window)))
	(setq window (split-window))
       (delete-window window))))

(setq-default mode-line-format foo)


will continue to segfault unless you can cure that.  I tried to fix it
in the spirit of

		if (!FRAME_LIVE_P (it->f))
		  signal_error (":eval deleted the frame being displayed", elt);

but that just caused emacs to hang.

martin

[-- Attachment #2: restore_selected_window.diff --]
[-- Type: text/x-patch, Size: 5609 bytes --]

diff --git a/src/xdisp.c b/src/xdisp.c
index a4de2698ca..e0273aba3f 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -12149,12 +12149,12 @@ unwind_format_mode_line (Lisp_Object vector)
   mode_line_string_face_prop = AREF (vector, 5);

   /* Select window before buffer, since it may change the buffer.  */
-  if (!NILP (old_window))
+  if (WINDOW_LIVE_P (old_window))
     {
       /* If the operation that we are unwinding had selected a window
 	 on a different frame, reset its frame-selected-window.  For a
 	 text terminal, reset its top-frame if necessary.  */
-      if (!NILP (target_frame_window))
+      if (WINDOW_LIVE_P (target_frame_window))
 	{
 	  Lisp_Object frame
 	    = WINDOW_FRAME (XWINDOW (target_frame_window));
@@ -12171,7 +12171,7 @@ unwind_format_mode_line (Lisp_Object vector)
       /* Restore point of target_frame_window's buffer (Bug#32777).
 	 But do this only after old_window has been reselected to
 	 avoid that the window point of target_frame_window moves.  */
-      if (!NILP (target_frame_window))
+      if (WINDOW_LIVE_P (target_frame_window))
 	{
 	  Lisp_Object buffer = AREF (vector, 10);

@@ -12594,18 +12598,58 @@ update_menu_bar (struct frame *f, bool save_match_data, bool hooks_run)

 #ifdef HAVE_WINDOW_SYSTEM

-/* Select `frame' temporarily without running all the code in
-   do_switch_frame.
-   FIXME: Maybe do_switch_frame should be trimmed down similarly
-   when `norecord' is set.  */
+/* Restore WINDOW as the selected window and its frame as the selected
+   frame.  If WINDOW is dead but the selected frame is live, make the
+   latter's selected window the selected window.  If both, WINDOW and
+   the selected frame, are dead, assign selected frame and window from
+   some arbitrary live frame.  Abort if no such frame can be found.  */
 static void
-fast_set_selected_frame (Lisp_Object frame)
+restore_selected_window (Lisp_Object window)
 {
-  if (!EQ (selected_frame, frame))
+  if (WINDOW_LIVE_P (window))
+    /* If WINDOW is live, make it the selected window and set the
+       selected frame to its frame.  */
     {
-      selected_frame = frame;
-      selected_window = XFRAME (frame)->selected_window;
+      selected_window = window;
+      selected_frame = XWINDOW (window)->frame;
     }
+  else if (FRAMEP (selected_frame) && FRAME_LIVE_P (XFRAME (selected_frame)))
+    /* If WINDOW is dead but the selected frame is still live, make the
+       latter's selected window the selected one.  */
+    selected_window = FRAME_SELECTED_WINDOW (XFRAME (selected_frame));
+  else
+    /* If WINDOW and the selected frame are dead, choose some live,
+       non-child and non-tooltip frame as the new selected frame and
+       make its selected window the selected window.  */
+    {
+      Lisp_Object tail;
+      Lisp_Object frame UNINIT;
+
+      FOR_EACH_FRAME (tail, frame)
+	{
+	  struct frame *f = XFRAME (frame);
+
+	  if (!FRAME_PARENT_FRAME (f) && !FRAME_TOOLTIP_P (f))
+	    {
+	      selected_frame = frame;
+	      selected_window = FRAME_SELECTED_WINDOW (f);
+
+	      return;
+	    }
+	}
+
+      /* Abort if we cannot find a live frame.  */
+      emacs_abort ();
+    }
+}
+
+/* Restore WINDOW, if live, as its frame's selected window.  */
+static void
+restore_frame_selected_window (Lisp_Object window)
+{
+  if (WINDOW_LIVE_P (window))
+    /* If WINDOW is live, make it its frame's selected window.  */
+    FRAME_SELECTED_WINDOW (XFRAME (XWINDOW (window)->frame)) = window;
 }

 #endif /* HAVE_WINDOW_SYSTEM */
@@ -12681,9 +12725,10 @@ update_tab_bar (struct frame *f, bool save_match_data)
 		       XFRAME (selected_frame)->selected_window));
 #ifdef HAVE_WINDOW_SYSTEM
 	  Lisp_Object frame;
-	  record_unwind_protect (fast_set_selected_frame, selected_frame);
+	  record_unwind_protect (restore_selected_window, selected_window);
 	  XSETFRAME (frame, f);
-	  fast_set_selected_frame (frame);
+	  selected_frame = frame;
+	  selected_window = FRAME_SELECTED_WINDOW (f);
 #endif

 	  /* Build desired tab-bar items from keymaps.  */
@@ -13625,9 +13670,10 @@ update_tool_bar (struct frame *f, bool save_match_data)
 		       /* Since we only explicitly preserve selected_frame,
 			  check that selected_window would be redundant.  */
 		       XFRAME (selected_frame)->selected_window));
-	  record_unwind_protect (fast_set_selected_frame, selected_frame);
+	  record_unwind_protect (restore_selected_window, selected_window);
 	  XSETFRAME (frame, f);
-	  fast_set_selected_frame (frame);
+	  selected_frame = frame;
+	  selected_window = FRAME_SELECTED_WINDOW (f);

 	  /* Build desired tool-bar items from keymaps.  */
           new_tool_bar
@@ -24939,11 +24985,14 @@ redisplay_mode_lines (Lisp_Object window, bool force)
 display_mode_lines (struct window *w)
 {
   Lisp_Object old_selected_window = selected_window;
-  Lisp_Object old_selected_frame = selected_frame;
   Lisp_Object new_frame = w->frame;
-  Lisp_Object old_frame_selected_window = XFRAME (new_frame)->selected_window;
+  ptrdiff_t count = SPECPDL_INDEX ();
   int n = 0;

+  record_unwind_protect (restore_selected_window, selected_window);
+  record_unwind_protect
+    (restore_frame_selected_window, XFRAME (new_frame)->selected_window);
+
   if (window_wants_mode_line (w))
     {
       Lisp_Object window;
@@ -25009,9 +25058,8 @@ display_mode_lines (struct window *w)
       ++n;
     }

-  XFRAME (new_frame)->selected_window = old_frame_selected_window;
-  selected_frame = old_selected_frame;
-  selected_window = old_selected_window;
+  unbind_to (count, Qnil);
+
   if (n > 0)
     w->must_be_updated_p = true;
   return n;