bug#65843: 28.2; Too many iconified frames in .emacs.desktop -> crash (macOS)

[Alan Third <alan@idiocy.org>]

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

On Sat, Sep 09, 2023 at 04:35:56PM -0000, Christian Tanzer via Bug reports for GNU Emacs, the Swiss army knife of text editors wrote:
> 
> =============================================================================
> In Emacs 28 and Emacs 29, desktop-load crashes when there are too many
> iconified frames in the .emacs.desktop file.
> 
> The two attached .emacs.desktop files show the smallest test case that
> I could come up with:
> 
> - .emacs.desktop.works with 3 non-iconified and 4 iconified frames
>   loads without problem in Emacs 28
> 
>   + desktop-load of this file crashes in Emacs 29.1
> 
>   + a .emacs.desktop with 3 non-iconified and 3 iconified frames works
>     in Emacs 29.1
> 
> - .emacs.desktop.crashes with 3 non-iconified and 5 iconified frames
>   leads to a crash in Emacs 28 (see attached problem report generated
>   by macOS)
> 
> - I tried .emacs.desktop with up to 17 non-iconified frames in Emacs
>   28 without problems
> 
> - Up to Emacs 27, I've never seen a problem with desktop-load crashing
> =============================================================================

Can you please try the master branch with the two attached patches
applied? The v2 patch should be applied first.

The apple crash report is enough to see that the crash is happening
somewhere deep inside the AppKit code on frame creation. No idea why,
but maybe we'll be lucky and the fix will be the same as the last
frame-creation crash fix...
-- 
Alan Third

[-- Attachment #2: v2-0001-Simplify-the-EmacsLayer-double-buffering-code-bug.patch --]
[-- Type: text/x-diff, Size: 8169 bytes --]

From 72b13fa67981c40f0e3c63092aa796525fe61344 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Sun, 23 Jul 2023 12:00:30 +0100
Subject: [PATCH v2] Simplify the EmacsLayer double buffering code (bug#63187)

---
 src/nsfns.m  | 30 +++++++++++++++++++++++++++++-
 src/nsterm.h | 13 ++++++++++++-
 src/nsterm.m | 50 ++++++++++++++++++++++++++------------------------
 3 files changed, 67 insertions(+), 26 deletions(-)

diff --git a/src/nsfns.m b/src/nsfns.m
index a79892f73b6..082e06698b2 100644
--- a/src/nsfns.m
+++ b/src/nsfns.m
@@ -799,6 +799,26 @@ Turn the input menu (an NSMenu) into a lisp list for tracking on lisp side.
     }
 }
 
+static void
+ns_set_inhibit_double_buffering (struct frame *f,
+                                 Lisp_Object new_value,
+                                 Lisp_Object old_value)
+{
+#if defined (NS_IMPL_COCOA) && MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
+  if (!EQ (new_value, old_value))
+    {
+      FRAME_DOUBLE_BUFFERED (f) = NILP (new_value);
+
+      /* If the view or layer haven't been created yet this will be a
+         noop.  */
+      [(EmacsLayer *)[FRAME_NS_VIEW (f) layer]
+          setDoubleBuffered:FRAME_DOUBLE_BUFFERED (f)];
+
+      SET_FRAME_GARBAGED (f);
+    }
+#endif
+}
+
 static void
 ns_set_internal_border_width (struct frame *f, Lisp_Object arg, Lisp_Object oldval)
 {
@@ -1073,7 +1093,7 @@ Turn the input menu (an NSMenu) into a lisp list for tracking on lisp side.
   gui_set_alpha,
   0, /* x_set_sticky */
   ns_set_tool_bar_position,
-  0, /* x_set_inhibit_double_buffering */
+  ns_set_inhibit_double_buffering,
   ns_set_undecorated,
   ns_set_parent_frame,
   0, /* x_set_skip_taskbar */
@@ -1461,6 +1481,14 @@ Turn the input menu (an NSMenu) into a lisp list for tracking on lisp side.
   gui_default_parameter (f, parms, Qtitle, Qnil, "title", "Title",
                          RES_TYPE_STRING);
 
+#if defined (NS_IMPL_COCOA) && MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
+  tem = gui_display_get_arg (dpyinfo, parms, Qinhibit_double_buffering, NULL, NULL,
+                             RES_TYPE_BOOLEAN);
+  FRAME_DOUBLE_BUFFERED (f) = NILP (tem) || EQ (tem, Qunbound);
+  store_frame_param (f, Qinhibit_double_buffering,
+                     FRAME_DOUBLE_BUFFERED (f) ? Qnil : Qt);
+#endif
+
   parms = get_geometry_from_preferences (dpyinfo, parms);
   window_prompting = gui_figure_window_size (f, parms, false, true);
 
diff --git a/src/nsterm.h b/src/nsterm.h
index b6e5a813a6d..8d6c58290cc 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -746,9 +746,11 @@ #define NSTRACE_UNSILENCE()
   CGColorSpaceRef colorSpace;
   IOSurfaceRef currentSurface;
   CGContextRef context;
+  bool doubleBuffered;
 }
-- (id) initWithColorSpace: (CGColorSpaceRef)cs;
+- (id) initWithColorSpace: (CGColorSpaceRef)cs doubleBuffered: (bool)db;
 - (void) setColorSpace: (CGColorSpaceRef)cs;
+- (void) setDoubleBuffered: (bool)db;
 - (CGContextRef) getContext;
 @end
 #endif
@@ -996,6 +998,11 @@ #define KEY_NS_SHOW_PREFS              ((1<<28)|(0<<16)|14)
   /* Non-zero if we are doing an animation, e.g. toggling the tool bar.  */
   int in_animation;
 
+#if defined (NS_IMPL_COCOA) && MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
+  /* Is the frame double buffered?  */
+  bool double_buffered;
+#endif
+
 #ifdef NS_IMPL_GNUSTEP
   /* Zero if this is the first time a toolbar has been updated on this
      frame. */
@@ -1030,6 +1037,10 @@ #define FRAME_POINTER_TYPE(f) ((f)->output_data.ns->current_pointer)
 
 #define FRAME_FONT(f) ((f)->output_data.ns->font)
 
+#if defined (NS_IMPL_COCOA) && MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
+#define FRAME_DOUBLE_BUFFERED(f) ((f)->output_data.ns->double_buffered)
+#endif
+
 #ifdef __OBJC__
 #define XNS_SCROLL_BAR(vec) ((id) xmint_pointer (vec))
 #else
diff --git a/src/nsterm.m b/src/nsterm.m
index 78089906752..28502ad1a2a 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2704,11 +2704,10 @@ Hide the window (X11 semantics)
   {
     NSRect srcRect = NSMakeRect (x, from_y, width, height);
     NSPoint dest = NSMakePoint (x, to_y);
-    NSRect destRect = NSMakeRect (x, from_y, width, height);
     EmacsView *view = FRAME_NS_VIEW (f);
 
     [view copyRect:srcRect to:dest];
-#ifdef NS_IMPL_COCOA
+#if defined (NS_IMPL_COCOA) && MAC_OS_X_VERSION_MAX_ALLOWED < 101400
     [view setNeedsDisplayInRect:destRect];
 #endif
   }
@@ -8607,7 +8606,8 @@ - (instancetype)toggleToolbar: (id)sender
 - (CALayer *)makeBackingLayer
 {
   EmacsLayer *l = [[EmacsLayer alloc]
-                    initWithColorSpace:[[[self window] colorSpace] CGColorSpace]];
+                    initWithColorSpace:[[[self window] colorSpace] CGColorSpace]
+                        doubleBuffered:FRAME_DOUBLE_BUFFERED (emacsframe)];
   [l setDelegate:(id)self];
   [l setContentsScale:[[self window] backingScaleFactor]];
 
@@ -8664,8 +8664,10 @@ - (void)copyRect:(NSRect)srcRect to:(NSPoint)dest
                                NSHeight (srcRect));
 
 #if defined (NS_IMPL_COCOA) && MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
-  double scale = [[self window] backingScaleFactor];
   CGContextRef context = [(EmacsLayer *)[self layer] getContext];
+  CGContextFlush (context);
+
+  double scale = [[self window] backingScaleFactor];
   int bpp = CGBitmapContextGetBitsPerPixel (context) / 8;
   void *pixels = CGBitmapContextGetData (context);
   int rowSize = CGBitmapContextGetBytesPerRow (context);
@@ -10435,22 +10437,20 @@ @implementation EmacsLayer
    cache.  If no free surfaces are found in the cache then a new one
    is created.  */
 
-#define CACHE_MAX_SIZE 2
-
 - (id) initWithColorSpace: (CGColorSpaceRef)cs
+           doubleBuffered: (bool)db
 {
-  NSTRACE ("[EmacsLayer initWithColorSpace:]");
+  NSTRACE ("[EmacsLayer initWithColorSpace:doubleBuffered:]");
 
   self = [super init];
   if (self)
     {
-      cache = [[NSMutableArray arrayWithCapacity:CACHE_MAX_SIZE] retain];
       [self setColorSpace:cs];
+      [self setDoubleBuffered:db];
+      cache = [[NSMutableArray arrayWithCapacity:(doubleBuffered ? 2 : 1)] retain];
     }
   else
-    {
-      return nil;
-    }
+    return nil;
 
   return self;
 }
@@ -10467,6 +10467,15 @@ - (void) setColorSpace: (CGColorSpaceRef)cs
 }
 
 
+- (void) setDoubleBuffered: (bool)db
+{
+  if (doubleBuffered != db)
+    [self releaseSurfaces];
+
+  doubleBuffered = db;
+}
+
+
 - (void) dealloc
 {
   [self releaseSurfaces];
@@ -10538,7 +10547,7 @@ - (CGContextRef) getContext
             }
         }
 
-      if (!surface && [cache count] >= CACHE_MAX_SIZE)
+      if (!surface && [cache count] >= (doubleBuffered ? 2 : 1))
         {
           /* Just grab the first one off the cache.  This may result
              in tearing effects.  The alternative is to wait for one
@@ -10591,7 +10600,7 @@ - (CGContextRef) getContext
           return nil;
         }
 
-      CGContextTranslateCTM(context, 0, IOSurfaceGetHeight (currentSurface));
+      CGContextTranslateCTM(context, 0, IOSurfaceGetHeight (surface));
       CGContextScaleCTM(context, scale, -scale);
     }
 
@@ -10608,6 +10617,7 @@ - (void) releaseContext
   if (!context)
     return;
 
+  CGContextFlush (context);
   CGContextRelease (context);
   context = NULL;
 
@@ -10621,26 +10631,18 @@ - (void) display
 {
   NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "[EmacsLayer display]");
 
-  if (context)
+  if (context && context != [[NSGraphicsContext currentContext] CGContext])
     {
       [self releaseContext];
 
-#if CACHE_MAX_SIZE == 1
-      /* This forces the layer to see the surface as updated.  */
+      /* This forces the layer to see the surface as updated even if
+         we replace it with itself.  */
       [self setContents:nil];
-#endif
-
       [self setContents:(id)currentSurface];
 
       /* Put currentSurface back on the end of the cache.  */
       [cache addObject:(id)currentSurface];
       currentSurface = NULL;
-
-      /* Schedule a run of getContext so that if Emacs is idle it will
-         perform the buffer copy, etc.  */
-      [self performSelectorOnMainThread:@selector (getContext)
-                             withObject:nil
-                          waitUntilDone:NO];
     }
 }
 
-- 
2.40.1


[-- Attachment #3: 0001-Fix-crash-on-child-frame-creation-bug-65817.patch --]
[-- Type: text/x-diff, Size: 3606 bytes --]

From 893f079f51c3bc81d8719c48fe539f72b1025fb8 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Fri, 8 Sep 2023 19:33:06 +0100
Subject: [PATCH] Fix crash on child frame creation (bug#65817)

* src/nsterm.m ([EmacsView initFrameFromEmacs:]): Reorder the way the
frame and layers are created.
([EmacsView makeBackingLayer]): Change to the newly renamed method
below.
([EmacsLayer initWithColorSpace:doubleBuffered:]):
([EmacsLayer initWithDoubleBuffered:]): Rename the method and remove
the colorspace argument as it's no longer able to be set on initial
creation.
* src/nsterm.h: Use new method prototype.
---
 src/nsterm.h |  2 +-
 src/nsterm.m | 25 +++++++++++++------------
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/src/nsterm.h b/src/nsterm.h
index 8d6c58290cc..cb162039ad8 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -748,7 +748,7 @@ #define NSTRACE_UNSILENCE()
   CGContextRef context;
   bool doubleBuffered;
 }
-- (id) initWithColorSpace: (CGColorSpaceRef)cs doubleBuffered: (bool)db;
+- (id) initWithDoubleBuffered: (bool)db;
 - (void) setColorSpace: (CGColorSpaceRef)cs;
 - (void) setDoubleBuffered: (bool)db;
 - (CGContextRef) getContext;
diff --git a/src/nsterm.m b/src/nsterm.m
index 28502ad1a2a..a3603171bb0 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -7921,8 +7921,6 @@ - (instancetype) initFrameFromEmacs: (struct frame *)f
   maximizing_resize = NO;
 #endif
 
-  [[EmacsWindow alloc] initWithEmacsFrame:f];
-
 #if defined (NS_IMPL_COCOA) && MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
   /* These settings mean AppKit will retain the contents of the frame
      on resize.  Unfortunately it also means the frame will not be
@@ -7933,9 +7931,14 @@ - (instancetype) initFrameFromEmacs: (struct frame *)f
           NSViewLayerContentsRedrawOnSetNeedsDisplay];
   [self setLayerContentsPlacement:NSViewLayerContentsPlacementTopLeft];
 
-  /* initWithEmacsFrame can't create the toolbar before the layer is
-     set, so have another go at creating the toolbar here.  */
-  [(EmacsWindow*)[self window] createToolbar:f];
+  [[EmacsWindow alloc] initWithEmacsFrame:f];
+
+  /* Now the NSWindow has been created, we can finish up configuring
+     the layer.  */
+  [(EmacsLayer *)[self layer] setColorSpace:[[[self window] colorSpace] CGColorSpace]];
+  [(EmacsLayer *)[self layer] setContentsScale:[[self window] backingScaleFactor]];
+#else
+  [[EmacsWindow alloc] initWithEmacsFrame:f];
 #endif
 
   if (ns_drag_types)
@@ -8606,10 +8609,9 @@ - (instancetype)toggleToolbar: (id)sender
 - (CALayer *)makeBackingLayer
 {
   EmacsLayer *l = [[EmacsLayer alloc]
-                    initWithColorSpace:[[[self window] colorSpace] CGColorSpace]
-                        doubleBuffered:FRAME_DOUBLE_BUFFERED (emacsframe)];
+                    initWithDoubleBuffered:FRAME_DOUBLE_BUFFERED (emacsframe)];
+
   [l setDelegate:(id)self];
-  [l setContentsScale:[[self window] backingScaleFactor]];
 
   return l;
 }
@@ -10437,15 +10439,14 @@ @implementation EmacsLayer
    cache.  If no free surfaces are found in the cache then a new one
    is created.  */
 
-- (id) initWithColorSpace: (CGColorSpaceRef)cs
-           doubleBuffered: (bool)db
+- (id) initWithDoubleBuffered: (bool)db
 {
-  NSTRACE ("[EmacsLayer initWithColorSpace:doubleBuffered:]");
+  NSTRACE ("[EmacsLayer initWithDoubleBuffered:]");
 
   self = [super init];
   if (self)
     {
-      [self setColorSpace:cs];
+      [self setColorSpace:nil];
       [self setDoubleBuffered:db];
       cache = [[NSMutableArray arrayWithCapacity:(doubleBuffered ? 2 : 1)] retain];
     }
-- 
2.40.1