bug#25265: make-thread crashes in OS X 10.6

[Alan Third <alan@idiocy.org>]

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

On Sat, Dec 24, 2016 at 12:06:45PM +0100, Charles A. Roelli wrote:
> Running Emacs -Q on the latest commit (e36a3882),
> 
> M-: (make-thread (lambda () "string"))
> 
> appears to hang Emacs immediately.

I’ve been working on this on and off for a while now, and I just can’t
fix it.

I’ve attached two patches that together are the best I’ve managed to
achieve, but unfortunately it randomly freezes up with 100% CPU usage.

I’ve not yet managed to work out what it’s doing when it goes into the
100% CPU loop. I can only assume I’ve missed some crucial case in
ns_select or something.

The first patch stops the NS port from using SIGIO, as it seems to be
the source of a number of problems. The second removes the NS event
loop in ns_select as it requires block_input/unblock_input wrapped
around it, but that’s what’s causing the crash in make-thread.

Instead it just looks for whether there is a new NS event as I don’t
think that requires blocking input.

-- 
Alan Third

[-- Attachment #2: Remove SIGIO stuff --]
[-- Type: text/plain, Size: 2831 bytes --]

From bff3dbfe43a24ee924edd777a7a876b627f94f11 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Sat, 29 Apr 2017 23:35:16 +0100
Subject: [PATCH] Don't use SIGIO in NS

* src/keyboard.c (handle_user_signal) [HAVE_NS]: Don't handle SIGIO.
* src/keyboard.h (handle_input_available_signal) [HAVE_NS]: Make
available to external code.
* src/nsterm.m (hold_event, ns_Select): Call handle_input_available_signal
directly.
* src/sysdep.c (emacs_sigaction_init) [HAVE_NS]: Ignore SIGIO.
---
 src/keyboard.c |  3 ++-
 src/keyboard.h | 11 +++++++++++
 src/nsterm.m   |  4 ++--
 src/sysdep.c   |  2 +-
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index c9fa2a9f5e..d22f7b08c6 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -7264,7 +7264,8 @@ handle_user_signal (int sig)
           }
 
 	p->npending++;
-#ifdef USABLE_SIGIO
+#if defined (USABLE_SIGIO) && !defined (HAVE_NS)
+        /* Dont use the interrupt handler in NS. */
 	if (interrupt_input)
 	  handle_input_available_signal (sig);
 	else
diff --git a/src/keyboard.h b/src/keyboard.h
index 2219c01135..29279bef86 100644
--- a/src/keyboard.h
+++ b/src/keyboard.h
@@ -495,6 +495,17 @@ extern void mark_kboards (void);
 extern const char *const lispy_function_keys[];
 #endif
 
+#ifdef HAVE_NS
+/* The NS port only uses SIGIO internally, and even then only in two
+   places in the code, but it causes crashes if certain code is not
+   properly wrapped in block_input/unblock_input.
+
+   Since it's only used internally, we should be able to disable it,
+   and call the SIGIO handler directly.
+ */
+extern void handle_input_available_signal (int sig);
+#endif
+
 extern char const DEV_TTY[];
 
 INLINE_HEADER_END
diff --git a/src/nsterm.m b/src/nsterm.m
index c22c5a70ba..00b7a89472 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -472,7 +472,7 @@ - (NSColor *)colorUsingDefaultColorSpace
 
   hold_event_q.q[hold_event_q.nr++] = *event;
   /* Make sure ns_read_socket is called, i.e. we have input.  */
-  raise (SIGIO);
+  handle_input_available_signal (SIGIO);
   send_appdefined = YES;
 }
 
@@ -4289,7 +4289,7 @@ in certain situations (rapid incoming events).
   if (hold_event_q.nr > 0)
     {
       /* We already have events pending. */
-      raise (SIGIO);
+      handle_input_available_signal (SIGIO);
       errno = EINTR;
       return -1;
     }
diff --git a/src/sysdep.c b/src/sysdep.c
index 91b2a5cb94..70fdf92964 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -1606,7 +1606,7 @@ emacs_sigaction_init (struct sigaction *action, signal_handler_t handler)
     {
       sigaddset (&action->sa_mask, SIGINT);
       sigaddset (&action->sa_mask, SIGQUIT);
-#ifdef USABLE_SIGIO
+#if defined (USABLE_SIGIO) && !defined (HAVE_NS)
       sigaddset (&action->sa_mask, SIGIO);
 #endif
     }
-- 
2.12.0


[-- Attachment #3: Remove NS event loop from ns_select --]
[-- Type: text/plain, Size: 5727 bytes --]

From f759adb30eaf12af474057ebbc98f5f76cc590bf Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Sun, 30 Apr 2017 10:04:16 +0100
Subject: [PATCH] Remove event loop from ns_select

* src/nsterm.m (ns_select): Remove event processing loop and replace
with simple test for a new event.
(ns_send_appdefined): Remove redundant timer code.
---
 src/nsterm.m | 92 ++++++++++++++++++++++++++++--------------------------------
 1 file changed, 43 insertions(+), 49 deletions(-)

diff --git a/src/nsterm.m b/src/nsterm.m
index 00b7a89472..462ab176c9 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -282,7 +282,6 @@ - (NSColor *)colorUsingDefaultColorSpace
 static BOOL send_appdefined = YES;
 #define NO_APPDEFINED_DATA (-8)
 static int last_appdefined_event_data = NO_APPDEFINED_DATA;
-static NSTimer *timed_entry = 0;
 static NSTimer *scroll_repeat_entry = nil;
 static fd_set select_readfds, select_writefds;
 enum { SELECT_HAVE_READ = 1, SELECT_HAVE_WRITE = 2, SELECT_HAVE_TMO = 4 };
@@ -4074,14 +4073,6 @@ in certain situations (rapid incoming events).
       /* We only need one NX_APPDEFINED event to stop NXApp from running.  */
       send_appdefined = NO;
 
-      /* Don't need wakeup timer any more */
-      if (timed_entry)
-        {
-          [timed_entry invalidate];
-          [timed_entry release];
-          timed_entry = nil;
-        }
-
       nxev = [NSEvent otherEventWithType: NSEventTypeApplicationDefined
                                 location: NSMakePoint (0, 0)
                            modifierFlags: 0
@@ -4277,9 +4268,11 @@ in certain situations (rapid incoming events).
 {
   int result;
   int t, k, nr = 0;
-  struct input_event event;
   char c;
 
+  NSDate *timeout_date = nil;
+  NSEvent *ns_event;
+
   NSTRACE_WHEN (NSTRACE_GROUP_EVENTS, "ns_select");
 
 #ifdef HAVE_NATIVE_FS
@@ -4337,70 +4330,71 @@ in certain situations (rapid incoming events).
       /* Inform fd_handler that select should be called */
       c = 'g';
       emacs_write_sig (selfds[1], &c, 1);
+      /* We rely on fd_handler timing out to cause
+         nextEventMatchingMask below to return, so set it's timeout to
+         an unreasonably long time. */
+      timeout_date = [NSDate distantFuture];
     }
   else if (nr == 0 && timeout)
     {
-      /* No file descriptor, just a timeout, no need to wake fd_handler  */
+      /* No file descriptor, just a timeout, no need to wake
+         fd_handler. Set nextEventMatchingMask timeout. */
       double time = timespectod (*timeout);
-      timed_entry = [[NSTimer scheduledTimerWithTimeInterval: time
-                                                      target: NSApp
-                                                    selector:
-                                  @selector (timeout_handler:)
-                                                    userInfo: 0
-                                                     repeats: NO]
-                      retain];
-    }
-  else /* No timeout and no file descriptors, can this happen?  */
-    {
-      /* Send appdefined so we exit from the loop */
-      ns_send_appdefined (-1);
+      timeout_date = [NSDate dateWithTimeIntervalSinceNow: time];
     }
 
-  block_input ();
-  ns_init_events (&event);
-
-  [NSApp run];
+  /* Listen for a new NSEvent. */
+  ns_event = [NSApp nextEventMatchingMask: NSEventMaskAny
+                                untilDate: timeout_date
+                                   inMode: NSDefaultRunLoopMode
+                                  dequeue: NO];
 
-  ns_finish_events ();
   if (nr > 0 && readfds)
     {
       c = 's';
       emacs_write_sig (selfds[1], &c, 1);
     }
-  unblock_input ();
-
-  t = last_appdefined_event_data;
 
-  if (t != NO_APPDEFINED_DATA)
+  if (ns_event != nil)
     {
-      last_appdefined_event_data = NO_APPDEFINED_DATA;
-
-      if (t == -2)
+      if ([ns_event type] == NSEventTypeApplicationDefined)
         {
-          /* The NX_APPDEFINED event we received was a timeout. */
-          result = 0;
+          if ([ns_event data1] < 0)
+            {
+              /* The NX_APPDEFINED event we received was a timeout. */
+              result = 0;
+            }
+          else
+            {
+              /* Received back from select () in fd_handler; copy the results */
+              pthread_mutex_lock (&select_mutex);
+              if (readfds) *readfds = select_readfds;
+              if (writefds) *writefds = select_writefds;
+              pthread_mutex_unlock (&select_mutex);
+              result = [ns_event data1];
+            }
+
+          /* Remove the NX_APPDEFINED event from the queue as it's no
+             longer needed. */
+          [NSApp nextEventMatchingMask: NSEventMaskAny
+                             untilDate: nil
+                                inMode: NSDefaultRunLoopMode
+                               dequeue: YES];
         }
-      else if (t == -1)
+      else
         {
-          /* The NX_APPDEFINED event we received was the result of
-             at least one real input event arriving.  */
+          /* A real NSEvent came in. */
+          handle_input_available_signal (0);
           errno = EINTR;
           result = -1;
         }
-      else
-        {
-          /* Received back from select () in fd_handler; copy the results */
-          pthread_mutex_lock (&select_mutex);
-          if (readfds) *readfds = select_readfds;
-          if (writefds) *writefds = select_writefds;
-          pthread_mutex_unlock (&select_mutex);
-          result = t;
-        }
     }
   else
     {
       errno = EINTR;
       result = -1;
+      /* Reading from the NSEvent queue timed out. */
+      result = 0;
     }
 
   return result;
-- 
2.12.0