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

From: martin rudalics <[email-masked]>
Date: Tue, 29 Apr 2025 10:37:13 +0200

> This patch passed all my, admittedly limited, test cases.

Thanks for checking.

 > In the patch, `make-frame-names-alist` returns only frames on the same
 > display. I don't know if this is a behaviour change or to make it
 > consistent with `next-frame` (the doc of `next-frame` doesn't mention
 > displays -- might worth clarifying there if this is the behaviour).

Thanks for spotting this.  'make-frame-names-alist' didn't have a
doc-string.  It only contained the comment

   ;; Only consider the frames on the same display.

so I thought that was the intended behavior.  In fact it wasn't so I
changed that now.

 > Another minor/unimportant point (feel free to ignore) is that the
 > patch also allows `make-frame-names-alist` to accept a FRAME
 > argument. I think it would be more natural to return frames on the
 > same display as the passed FRAME rather than the selected one.

I added the "(&optional frame)" because with a few exceptions most
frame-related functions have it so coders don't have to think twice when
calling any of these functions.

I attach a new version which also implements 'frame-list-1' in a
different way.  It's illustrative to look into how 'frame-list' and
'frame-list-1' relate.  With the following

(defun make-frames (number)
   (let ((i 0))
     (set-frame-parameter nil 'name (number-to-string (setq i (1+ i))))
     (while (< i number)
       (set-frame-parameter
        (make-frame) 'name (number-to-string (setq i (1+ i)))))))

make four frames via

(make-frames 4)

with titles "1" to "4".  Now when evaluating

(frame-list)
(frame-list-1)

the result for the latter will change when you switch to another frame
while the former remains unaltered.  I have no idea if it's that what we
want for 'make-frame-names-alist' but 'frame-list-1' should preserve the
old behavior.

Note here that we always list frames in the reverse order of their
creation.  This means that with respect to the selected frame,
'next-frame' conceptually returns the frame created previously and
'previous-frame' the one created next (unless these functions wrap
around, obviously) since both of these are based on the underlying
Vframe_list which is reproduced by 'frame-list'.

martin
diff --git a/lisp/frame.el b/lisp/frame.el
index cf8aff826c1..66fb427d78b 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -1291,18 +1291,33 @@ 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 are listed.  The optional argument
+FRAME must specify a live frame and defaults to the selected frame."
+  (let ((frames (frame-list-1 frame))
+	alist)
+    (dolist (frame frames)
+      (when (frame-visible-p frame)
+	(push (cons (frame-parameter frame 'name) frame) alist)))
+    (nreverse alist)))
 
 (defvar frame-name-history nil)
 (defun select-frame-by-name (name)
@@ -2816,32 +2831,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..8001e8e0197 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -2205,7 +2205,7 @@ 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;
+  Lisp_Object f, f1, tail;
   int passed = 0;
 
   eassume (CONSP (Vframe_list));
@@ -2215,9 +2215,9 @@ next_frame (Lisp_Object frame, Lisp_Object minibuf)
       {
 	if (passed)
 	  {
-	    f = candidate_frame (f, frame, minibuf);
-	    if (!NILP (f))
-	      return f;
+	    f1 = candidate_frame (f, frame, minibuf);
+	    if (!NILP (f1))
+	      return f1;
 	  }
 	if (EQ (frame, f))
 	  passed++;