bug#65817: 30.0.50; Abort with NSInvalidArgumentException on macOS Big Sur

[Alan Third <alan@idiocy.org>]

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

On Fri, Sep 08, 2023 at 09:01:06AM +0200, Gerd Möllmann wrote:
> System Description:  macOS 11.7.9
> 
> This is a 10 year old Mac.  I can't reproduce it at will.  Below is what
> was printed to the terminal, then Emacs aborted.

This is odd. The system is trying to draw the frame to the screen
*long* before we've finished creating it.

I don't know why this would be happening, it's possible that we've hit
an edge case where we accidentally mark the view for display early,
but I don't know how we would be doing that.

> 	5   emacs                               0x000000010033d481 -[EmacsView lockFocus] + 49
> 	6   emacs                               0x000000010032ee27 ns_focus + 87
> 	7   emacs                               0x000000010033df80 ns_clear_frame_area + 400
> 	8   emacs                               0x000000010033dda1 -[EmacsView drawRect:] + 321
<snip>
> 	21  AppKit                              0x00007fff2314c06f -[NSWindow addChildWindow:ordered:] + 640
> 	22  emacs                               0x000000010033fe08 -[EmacsWindow setParentChildRelationships] + 696
> 	23  emacs                               0x000000010033f2ad -[EmacsWindow initWithEmacsFrame:fullscreen:screen:] + 1485
> 	24  emacs                               0x000000010033ecd0 -[EmacsWindow initWithEmacsFrame:] + 48
> 	25  emacs                               0x000000010033ace5 -[EmacsView initFrameFromEmacs:] + 1045
> 	26  emacs                               0x00000001003579a1 Fx_create_frame + 7937

At the point lockFocus is called here, we haven't yet set up the layer
we want to draw into. I can't find any indication that addChildWindow
may trigger a display, but this is Apple's documentation I'm talking
about...

I've attached a patch that reorders some of the initialisation,
hopefully avoiding this problem.

I've also attached another patch that you may need to apply first to
make the second one apply cleanly.
-- 
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