Subject: bug#77985: 31.0.50; infinite loop in `make-frame-names-alist`

From: martin rudalics <[email-masked]>
Date: Sun, 4 May 2025 12:02:59 +0200

> Thanks for the changes! I am incorporating them in my emacs config
 > since I keep running into the infinite loop :)

I now made a number of additional changes:

- I rewrote next_frame in the style of prev_frame so it has to scan the
   frame list once only.

- `make-frame-names-alist' now lists only frames on the same terminal as
   FRAME and skips frames whose 'no-other-frame' parameter is non-nil.

- I updated some doc-strings.

Please try with the attached diff.  If you don't find any problems, I'll
install it in a couple of days.

Thanks, martin

diff --git a/doc/lispref/frames.texi b/doc/lispref/frames.texi
index 197f4c17b46..f0d27455fed 100644
--- a/doc/lispref/frames.texi
+++ b/doc/lispref/frames.texi
@@ -2883,15 +2883,16 @@ Finding All Frames
 @defun frame-list
 This function returns a list of all the live frames, i.e., those that
 have not been deleted.  It is analogous to @code{buffer-list} for
-buffers, and includes frames on all terminals.  The list that you get
-is newly created, so modifying the list doesn't have any effect on the
-internals of Emacs.
+buffers, and includes frames on all terminals with the exception of
+tooltip frames (@pxref{Tooltips}).  The list that you get is newly
+created, so modifying the list doesn't have any effect on the internals
+of Emacs.
 @end defun
 
 @defun visible-frame-list
 This function returns a list of just the currently visible frames.
-@xref{Visibility of Frames}.  Frames on text terminals always count as
-visible, even though only the selected one is actually displayed.
+@xref{Visibility of Frames}.  Frames on text terminals will count as
+visible even though only the selected one is actually displayed.
 @end defun
 
 @defun frame-list-z-order &optional display
@@ -2914,7 +2915,7 @@ Finding All Frames
 specific terminal from an arbitrary starting point.  It returns the
 frame following @var{frame}, in the list of all live frames, on
 @var{frame}'s terminal.  The argument @var{frame} must specify a live
-frame and defaults to the selected frame.  It never returns a frame
+frame and defaults to the selected frame.  It does not return a frame
 whose @code{no-other-frame} parameter (@pxref{Frame Interaction
 Parameters}) is non-@code{nil}.
 
@@ -2934,6 +2935,10 @@ Finding All Frames
 @item anything else
 Consider all frames.
 @end table
+
+If this function does not find a suitable frame, it returns @var{frame}
+even if it would not qualify according to the @var{minibuf} argument or
+its @code{no-other-frame} parameter.
 @end defun
 
 @defun previous-frame &optional frame minibuf
diff --git a/lisp/frame.el b/lisp/frame.el
index cf8aff826c1..34d06bd4e58 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -1226,10 +1226,11 @@ select-frame-set-input-focus
     (set-mouse-position frame (1- (frame-width frame)) 0))))
 
 (defun other-frame (arg)
-  "Select the ARGth different visible frame on current display, and raise it.
-All frames are arranged in a cyclic order.
-This command selects the frame ARG steps away in that order.
-A negative ARG moves in the opposite order.
+  "Select the ARGth visible frame on current display, and raise it.
+All frames are arranged in a cyclic order.  This command selects the
+frame ARG steps away from the selected frame in that order.  A negative
+ARG moves in the opposite order.  It does not select a minibuffer-only
+frame.
 
 To make this command work properly, you must tell Emacs how the
 system (or the window manager) generally handles focus-switching
@@ -1291,18 +1292,38 @@ suspend-frame
       (suspend-tty)))
    (t (suspend-emacs))))
 
-(defun make-frame-names-alist ()
-  ;; Only consider the frames on the same display.
-  (let* ((current-frame (selected-frame))
-	 (falist
-	  (cons
-	   (cons (frame-parameter current-frame 'name) current-frame) nil))
-	 (frame (next-frame nil 0)))
-    (while (not (eq frame current-frame))
-      (progn
-	(push (cons (frame-parameter frame 'name) frame) falist)
-	(setq frame (next-frame frame 0))))
-    falist))
+(defun frame-list-1 (&optional frame)
+  "Return list of all live frames starting with FRAME.
+The optional argument FRAME must specify a live frame and defaults to
+the selected frame.  Tooltip frames are not included."
+  (let* ((frame (window-normalize-frame frame))
+	 (frames (frame-list)))
+    (unless (eq (car frames) frame)
+      (let ((tail frames))
+	(while tail
+	  (if (eq (cadr tail) frame)
+	      (let ((head (cdr tail)))
+		(setcdr tail nil)
+		(setq frames (nconc head frames))
+		(setq tail nil))
+	    (setq tail (cdr tail))))))
+    frames))
+
+(defun make-frame-names-alist (&optional frame)
+  "Return alist of frame names and frames starting with FRAME.
+Only visible or iconified frames on the same terminal as FRAME are
+listed.  Frames with a non-nil `no-other-frame' parmaeter are not
+listed.  The optional argument FRAME must specify a live frame and
+defaults to the selected frame."
+  (let ((frames (frame-list-1 frame))
+	(terminal (frame-parameter frame 'terminal))
+	alist)
+    (dolist (frame frames)
+      (when (and (frame-visible-p frame)
+		 (eq (frame-parameter frame 'terminal) terminal)
+		 (not (frame-parameter frame 'no-other-frame)))
+	(push (cons (frame-parameter frame 'name) frame) alist)))
+    (nreverse alist)))
 
 (defvar frame-name-history nil)
 (defun select-frame-by-name (name)
@@ -2816,32 +2837,29 @@ delete-other-frames
   (interactive "i\nP")
   (setq frame (window-normalize-frame frame))
   (let ((minibuffer-frame (window-frame (minibuffer-window frame)))
-        (this (next-frame frame t))
         (parent (frame-parent frame))
-        next)
+	(frames (frame-list)))
     ;; In a first round consider minibuffer-less frames only.
-    (while (not (eq this frame))
-      (setq next (next-frame this t))
-      (unless (or (eq (window-frame (minibuffer-window this)) this)
+    (dolist (this frames)
+      (unless (or (eq this frame)
+		  (eq this minibuffer-frame)
+		  (eq (window-frame (minibuffer-window this)) this)
                   ;; When FRAME is a child frame, delete its siblings
                   ;; only.
                   (and parent (not (eq (frame-parent this) parent)))
-                  ;; Do not delete a child frame of FRAME.
-                  (eq (frame-parent this) frame))
-        (if iconify (iconify-frame this) (delete-frame this)))
-      (setq this next))
+                  ;; Do not delete frame descending from FRAME.
+                  (frame-ancestor-p frame this))
+        (if iconify (iconify-frame this) (delete-frame this))))
     ;; In a second round consider all remaining frames.
-    (setq this (next-frame frame t))
-    (while (not (eq this frame))
-      (setq next (next-frame this t))
-      (unless (or (eq this minibuffer-frame)
+    (dolist (this frames)
+      (unless (or (eq this frame)
+		  (eq this minibuffer-frame)
                   ;; When FRAME is a child frame, delete its siblings
                   ;; only.
                   (and parent (not (eq (frame-parent this) parent)))
-                  ;; Do not delete a child frame of FRAME.
-                  (eq (frame-parent this) frame))
-        (if iconify (iconify-frame this) (delete-frame this)))
-      (setq this next))))
+                  ;; Do not delete frame descending from FRAME.
+                  (frame-ancestor-p frame this))
+        (if iconify (iconify-frame this) (delete-frame this))))))
 
 (defvar undelete-frame--deleted-frames nil
   "Internal variable used by `undelete-frame--save-deleted-frame'.")
diff --git a/src/frame.c b/src/frame.c
index 7dc9202d6f6..f2acb19c77d 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -2205,24 +2205,39 @@ candidate_frame (Lisp_Object candidate, Lisp_Object frame, Lisp_Object minibuf)
 static Lisp_Object
 next_frame (Lisp_Object frame, Lisp_Object minibuf)
 {
-  Lisp_Object f, tail;
-  int passed = 0;
+  Lisp_Object f, tail, next = Qnil;
+  bool passed = false;
 
   eassume (CONSP (Vframe_list));
 
-  while (passed < 2)
-    FOR_EACH_FRAME (tail, f)
-      {
-	if (passed)
-	  {
-	    f = candidate_frame (f, frame, minibuf);
-	    if (!NILP (f))
-	      return f;
-	  }
-	if (EQ (frame, f))
-	  passed++;
-      }
-  return frame;
+  FOR_EACH_FRAME (tail, f)
+    {
+      if (EQ (f, frame))
+	/* If we encounter FRAME, set PASSED to true.  */
+	passed = true;
+      else
+	{
+	  f = candidate_frame (f, frame, minibuf);
+
+	  if (!NILP (f))
+	    {
+	      if (passed)
+		/* If we passed FRAME already, return first suitable
+		   candidate following it.  */
+		return f;
+	      else if (NILP (next))
+		/* If we didn't pass FRAME and have no suitable
+		   candidate yet, set NEXT to the first suitable
+		   candidate preceding FRAME.  */
+		next = f;
+	    }
+	}
+    }
+
+  /* We have scanned all frames.  Return first candidate preceding FRAME
+     if we have found one.  Otherwise return FRAME regardless of whether
+     it is a suitable candidate or not.  */
+  return NILP (next) ? frame : next;
 }
 
 /* Return the previous frame in the frame list before FRAME.  */
@@ -2237,21 +2252,26 @@ prev_frame (Lisp_Object frame, Lisp_Object minibuf)
   FOR_EACH_FRAME (tail, f)
     {
       if (EQ (frame, f) && !NILP (prev))
+	/* If we encounter FRAME and already have found a suitable
+	   candidate preceding it, return that candidate.  */
 	return prev;
+
       f = candidate_frame (f, frame, minibuf);
+
       if (!NILP (f))
+	/* PREV is always the last suitable candidate we found.  */
 	prev = f;
     }
 
   /* We've scanned the entire list.  */
   if (NILP (prev))
     /* We went through the whole frame list without finding a single
-       acceptable frame.  Return the original frame.  */
+       acceptable frame.  Return FRAME.  */
     return frame;
   else
-    /* There were no acceptable frames in the list before FRAME; otherwise,
-       we would have returned directly from the loop.  Since PREV is the last
-       acceptable frame in the list, return it.  */
+    /* There were no acceptable frames in the list before FRAME;
+       otherwise, we would have returned directly from the loop.  Since
+       PREV is the last suitable frame in the list, return it.  */
     return prev;
 }
 
@@ -2259,7 +2279,7 @@ prev_frame (Lisp_Object frame, Lisp_Object minibuf)
 DEFUN ("next-frame", Fnext_frame, Snext_frame, 0, 2, 0,
        doc: /* Return the next frame in the frame list after FRAME.
 Only frames on the same terminal as FRAME are included in the list
-of candidate frames.  If omitted, FRAME defaults to the selected frame.
+of candidate frames.  FRAME defaults to the selected frame.
 
 If MINIFRAME is nil (the default), include all frames except
 minibuffer-only frames.
@@ -2271,7 +2291,9 @@ DEFUN ("next-frame", Fnext_frame, Snext_frame, 0, 2, 0,
 
 If MINIFRAME is 0, include only visible and iconified frames.
 
-If MINIFRAME is any other value, include all frames.  */)
+If MINIFRAME is any other value, include all frames.
+
+Return FRAME if no suitable next frame is found.  */)
   (Lisp_Object frame, Lisp_Object miniframe)
 {
   if (NILP (frame))
@@ -2282,15 +2304,22 @@ DEFUN ("next-frame", Fnext_frame, Snext_frame, 0, 2, 0,
 
 DEFUN ("previous-frame", Fprevious_frame, Sprevious_frame, 0, 2, 0,
        doc: /* Return the previous frame in the frame list before FRAME.
-It considers only frames on the same terminal as FRAME.
-By default, skip minibuffer-only frames.
-If omitted, FRAME defaults to the selected frame.
-If optional argument MINIFRAME is nil, exclude minibuffer-only frames.
-If MINIFRAME is a window, include only its own frame
-and any frame now using that window as the minibuffer.
-If MINIFRAME is `visible', include all visible frames.
-If MINIFRAME is 0, include all visible and iconified frames.
-Otherwise, include all frames.  */)
+Only frames on the same terminal as FRAME are included in the list
+of candidate frames.  FRAME defaults to the selected frame.
+
+If MINIFRAME is nil (the default), include all frames except
+minibuffer-only frames.
+
+If MINIFRAME is a window, include only its own frame and any frame now
+using that window as the minibuffer.
+
+If MINIFRAME is `visible', include only visible frames.
+
+If MINIFRAME is 0, include only visible and iconified frames.
+
+If MINIFRAME is any other value, include all frames.
+
+Return FRAME if no suitable previous frame is found.  */)
   (Lisp_Object frame, Lisp_Object miniframe)
 {
   if (NILP (frame))