Bug#23924 - crashes on next OS X release

Bug#23924 - crashes on next OS X release

[Alan Third]

Hi, we've had a couple of people report that Emacs crashes on the beta
release of OS X 10.12. The cause seems to be a fix put in place
exclusively for OS X 10.10. I've created a patch and am going to push
it to master, but I think it should be pushed to Emacs 25 as well. If
we don't then we run the risk, at the end of this year, of Emacs 25
not working on the current version of OS X.

Is there anything I need to do or provide?

Thanks!
-- 
Alan Third

Re: Bug#23924 - crashes on next OS X release

[Eli Zaretskii]

> From: Alan Third <alan@idiocy.org>
> Date: Mon, 18 Jul 2016 15:02:06 +0100
> 
> Hi, we've had a couple of people report that Emacs crashes on the beta
> release of OS X 10.12. The cause seems to be a fix put in place
> exclusively for OS X 10.10. I've created a patch and am going to push
> it to master, but I think it should be pushed to Emacs 25 as well. If
> we don't then we run the risk, at the end of this year, of Emacs 25
> not working on the current version of OS X.
> 
> Is there anything I need to do or provide?

If you are confident that it won't do any trouble, and that 10.10, and
only that version, needs this fix, feel free to push to emacs-25.

Thanks.

Re: Bug#23924 - crashes on next OS X release

[John Wiegley]

>>>>> "EZ" == Eli Zaretskii <eliz@gnu.org> writes:

EZ> If you are confident that it won't do any trouble, and that 10.10, and
EZ> only that version, needs this fix, feel free to push to emacs-25.

Before you do, can you show us the patch here so that I can have a sense of
what it changes?  I've been running emacs-25 crash-free on 10.11 for many
months now.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Bug#23924 - crashes on next OS X release

[Alan Third]

On Mon, Jul 18, 2016 at 09:16:09AM -0700, John Wiegley wrote:
> >>>>> "EZ" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> EZ> If you are confident that it won't do any trouble, and that 10.10, and
> EZ> only that version, needs this fix, feel free to push to emacs-25.
> 
> Before you do, can you show us the patch here so that I can have a sense of
> what it changes?  I've been running emacs-25 crash-free on 10.11 for many
> months now.

I’m not that confident as I couldn’t find anything about the bug in
question except what was in the bug report and comment in the code. I
tried removing the fix and can’t reproduce the original problem on
10.11. It’s possible it’s actually been fixed in later releases of
10.10 too.

We’ve had two reports of crashes on 10.12 which my patch resolves.

I haven’t yet found anyone running 10.10 who can test it.

The original bug is 18993.

* src/nsterm.m (ns_send_appdefined): Limit bugfix for bug#18993 to OS X
10.10.
---
 src/nsterm.m | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/nsterm.m b/src/nsterm.m
index a6160ed..a7e2649 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -3942,16 +3942,21 @@ overwriting cursor (usually when cursor on a tab) */
        this moment.  */
 
 #ifdef NS_IMPL_COCOA
-  if (! send_appdefined)
-    {
-      /* OSX 10.10.1 swallows the AppDefined event we are sending ourselves
-         in certain situations (rapid incoming events).
-         So check if we have one, if not add one.  */
-      NSEvent *appev = [NSApp nextEventMatchingMask:NSApplicationDefinedMask
-                                          untilDate:[NSDate distantPast]
-                                             inMode:NSDefaultRunLoopMode
-                                            dequeue:NO];
-      if (! appev) send_appdefined = YES;
+  if ([[NSProcessInfo processInfo] respondsToSelector:@selector(operatingSystemVersion)])
+    {
+      NSOperatingSystemVersion v = [[NSProcessInfo processInfo] operatingSystemVersion];
+
+      if (! send_appdefined && v.majorVersion == 10 && v.minorVersion == 10)
+        {
+          /* OSX 10.10.1 swallows the AppDefined event we are sending ourselves
+             in certain situations (rapid incoming events).
+             So check if we have one, if not add one.  */
+          NSEvent *appev = [NSApp nextEventMatchingMask:NSApplicationDefinedMask
+                                              untilDate:[NSDate distantPast]
+                                                 inMode:NSDefaultRunLoopMode
+                                                dequeue:NO];
+          if (! appev) send_appdefined = YES;
+        }
     }
 #endif
 
-- 
2.7.4


-- 
Alan Third

Re: Bug#23924 - crashes on next OS X release

[David Caldwell]

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

On 7/18/16 12:03 PM, Alan Third wrote:
> On Mon, Jul 18, 2016 at 09:16:09AM -0700, John Wiegley wrote:
>>>>>>> "EZ" == Eli Zaretskii <eliz@gnu.org> writes:
>>
>> EZ> If you are confident that it won't do any trouble, and that 10.10, and
>> EZ> only that version, needs this fix, feel free to push to emacs-25.
>>
>> Before you do, can you show us the patch here so that I can have a sense of
>> what it changes?  I've been running emacs-25 crash-free on 10.11 for many
>> months now.
> 
> I’m not that confident as I couldn’t find anything about the bug in
> question except what was in the bug report and comment in the code. I
> tried removing the fix and can’t reproduce the original problem on
> 10.11. It’s possible it’s actually been fixed in later releases of
> 10.10 too.
> 
> We’ve had two reports of crashes on 10.12 which my patch resolves.
> 
> I haven’t yet found anyone running 10.10 who can test it.

I have a 10.10 VM that I can test with. I built the latest pretest + the
patch you included here on 10.10 and ran it. There was no obvious
crashing. I tried double clicking some text, as the original bug report
(18993) mentioned that often caused the freeze, and I suffered no freezes.

If there's something specific you would like me to do, let me know.

-David



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3819 bytes --]

Re: Bug#23924 - crashes on next OS X release

[Alan Third]

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

On Mon, Jul 18, 2016 at 08:03:46PM +0100, Alan Third wrote:
> On Mon, Jul 18, 2016 at 09:16:09AM -0700, John Wiegley wrote:
> > >>>>> "EZ" == Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > EZ> If you are confident that it won't do any trouble, and that 10.10, and
> > EZ> only that version, needs this fix, feel free to push to emacs-25.
> > 
> > Before you do, can you show us the patch here so that I can have a sense of
> > what it changes?  I've been running emacs-25 crash-free on 10.11 for many
> > months now.
> 
> I’m not that confident as I couldn’t find anything about the bug in
> question except what was in the bug report and comment in the code. I
> tried removing the fix and can’t reproduce the original problem on
> 10.11. It’s possible it’s actually been fixed in later releases of
> 10.10 too.

I’ve got a new patch. One of the testers of the first patch ran into
bug#18993, so it’s clearly still an issue, and further investigation
leaves me suspecting that, contrary to the bug report thread, it’s not
an OS X bug, but something to do with how Emacs is handling events.

The new patch leaves the fix in place and uses an existing GNUStep fix
to ensure that postEvent always runs in the main thread. That fixes
bug#23924.

There’s probably an argument to be made that we should fix 18993
properly, but I don’t think we have anyone who knows the NS port well
enough to do it, as it would require a substantial rewrite of the
event loop.

If this is to be applied to Emacs 25, let me know if it’s to go to
master too. I’m unsure how it’s handled.
-- 
Alan Third

[-- Attachment #2: patch for bug#23924 --]
[-- Type: text/plain, Size: 2248 bytes --]

From 50d21628a5288420ead32ba97731a496101289b5 Mon Sep 17 00:00:00 2001
In-Reply-To: <6629E4B5-5D34-4840-B0A1-A62BA025C472@play-bow.org>
References: <6629E4B5-5D34-4840-B0A1-A62BA025C472@play-bow.org>
From: Alan Third <alan@idiocy.org>
Date: Wed, 20 Jul 2016 21:59:17 +0100
Subject: [PATCH] Post AppDefined events from the main thread ONLY (bug#23934)
To: Bob Halley <halley@play-bow.org>
Cc: 23924@debbugs.gnu.org

* src/nsterm.h: Make nextappdefined var not just GNUStep.
* src/nsterm.c (ns_send_appdefined, sendFromMainThread): Remove GNUStep
---
 src/nsterm.h | 2 +-
 src/nsterm.m | 5 +----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/nsterm.h b/src/nsterm.h
index 862ff2e..3d8b1a1 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -380,9 +380,9 @@ char const * nstrace_fullscreen_type_name (int);
 #endif
 #ifdef NS_IMPL_GNUSTEP
   BOOL applicationDidFinishLaunchingCalled;
+#endif
 @public
   int nextappdefined;
-#endif
 }
 - (void)logNotification: (NSNotification *)notification;
 - (void)antialiasThresholdDidChange:(NSNotification *)notification;
diff --git a/src/nsterm.m b/src/nsterm.m
index 8da2ffe..dcc1e87 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -3927,8 +3927,8 @@ overwriting cursor (usually when cursor on a tab) */
 {
   NSTRACE_WHEN (NSTRACE_GROUP_EVENTS, "ns_send_appdefined(%d)", value);
 
-#ifdef NS_IMPL_GNUSTEP
   // GNUstep needs postEvent to happen on the main thread.
+  // Cocoa needs nextEventMatchingMask to happen on the main thread too.
   if (! [[NSThread currentThread] isMainThread])
     {
       EmacsApp *app = (EmacsApp *)NSApp;
@@ -3938,7 +3938,6 @@ overwriting cursor (usually when cursor on a tab) */
                          waitUntilDone:YES];
       return;
     }
-#endif
 
   /* Only post this event if we haven't already posted one.  This will end
        the [NXApp run] main loop after having processed all events queued at
@@ -5551,12 +5550,10 @@ - (void)timeout_handler: (NSTimer *)timedEntry
   ns_send_appdefined (-2);
 }
 
-#ifdef NS_IMPL_GNUSTEP
 - (void)sendFromMainThread:(id)unused
 {
   ns_send_appdefined (nextappdefined);
 }
-#endif
 
 - (void)fd_handler:(id)unused
 /* --------------------------------------------------------------------------
-- 
2.7.4

Re: Bug#23924 - crashes on next OS X release

[Alan Third]

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

Hi John, are you OK with the patch?

On 22 July 2016 at 22:54, Alan Third <alan@idiocy.org> wrote:

> On Mon, Jul 18, 2016 at 08:03:46PM +0100, Alan Third wrote:
> > On Mon, Jul 18, 2016 at 09:16:09AM -0700, John Wiegley wrote:
> > > >>>>> "EZ" == Eli Zaretskii <eliz@gnu.org> writes:
> > >
> > > EZ> If you are confident that it won't do any trouble, and that 10.10,
> and
> > > EZ> only that version, needs this fix, feel free to push to emacs-25.
> > >
> > > Before you do, can you show us the patch here so that I can have a
> sense of
> > > what it changes?  I've been running emacs-25 crash-free on 10.11 for
> many
> > > months now.
> >
> > I’m not that confident as I couldn’t find anything about the bug in
> > question except what was in the bug report and comment in the code. I
> > tried removing the fix and can’t reproduce the original problem on
> > 10.11. It’s possible it’s actually been fixed in later releases of
> > 10.10 too.
>
> I’ve got a new patch. One of the testers of the first patch ran into
> bug#18993, so it’s clearly still an issue, and further investigation
> leaves me suspecting that, contrary to the bug report thread, it’s not
> an OS X bug, but something to do with how Emacs is handling events.
>
> The new patch leaves the fix in place and uses an existing GNUStep fix
> to ensure that postEvent always runs in the main thread. That fixes
> bug#23924.
>
> There’s probably an argument to be made that we should fix 18993
> properly, but I don’t think we have anyone who knows the NS port well
> enough to do it, as it would require a substantial rewrite of the
> event loop.
>
> If this is to be applied to Emacs 25, let me know if it’s to go to
> master too. I’m unsure how it’s handled.
> --
> Alan Third
>



-- 
Alan Third

[-- Attachment #2: Type: text/html, Size: 2512 bytes --]

Re: Bug#23924 - crashes on next OS X release

[John Wiegley]

>>>>> Alan Third <alan@idiocy.org> writes:

> Hi John, are you OK with the patch?

Yes, I am OK with the patch. Eli?

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Bug#23924 - crashes on next OS X release

[Eli Zaretskii]

> From: John Wiegley <jwiegley@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Emacs-Devel devel <emacs-devel@gnu.org>
> Date: Tue, 02 Aug 2016 13:25:10 -0700
> 
> >>>>> Alan Third <alan@idiocy.org> writes:
> 
> > Hi John, are you OK with the patch?
> 
> Yes, I am OK with the patch. Eli?

If all those affected by the problem agree it's TRT, I'm OK with it
going to emacs-25.

Re: Bug#23924 - crashes on next OS X release

[Marius Kjeldahl]

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

Well, I'm one of the testers I guess. I spend more or less all day with
Emacs, and before applying the patch I had several crashes a day, and after
zero. However, it's a hard bug to pin down (some kind of timing/racing
condition I suspect). Accordingly there is no simple test case that can be
applied that I am aware of. Based on the short time between the last two
macOS beta releases (3 and 4) and the fact that the last version barely had
any "change notes", I suspect macOS release is getting close. I would feel
a lot more comfortable if all modern distributions of Emacs on macOS had
this patch in rather than not. When/if we manage to pin it down with a
reliable test case, a real fix of this or the underlying issue should be
easier, but until then I vote to get the patch in ASAP. Additionally the
patch only affects macOS users so the breakage potential should be small
(at least compared to daily crashes which is the alternative).

On Wed, 3 Aug 2016 at 04:39 Eli Zaretskii <eliz@gnu.org> wrote:

> > From: John Wiegley <jwiegley@gmail.com>
> > Cc: Eli Zaretskii <eliz@gnu.org>,  Emacs-Devel devel <
> emacs-devel@gnu.org>
> > Date: Tue, 02 Aug 2016 13:25:10 -0700
> >
> > >>>>> Alan Third <alan@idiocy.org> writes:
> >
> > > Hi John, are you OK with the patch?
> >
> > Yes, I am OK with the patch. Eli?
>
> If all those affected by the problem agree it's TRT, I'm OK with it
> going to emacs-25.
>
>

[-- Attachment #2: Type: text/html, Size: 1996 bytes --]

Re: Bug#23924 - crashes on next OS X release

[Alan Third]

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

I'll push it to emacs-25 tonight.

Thanks.

On 3 August 2016 at 08:07, Marius Kjeldahl <marius.kjeldahl@gmail.com>
wrote:

> Well, I'm one of the testers I guess. I spend more or less all day with
> Emacs, and before applying the patch I had several crashes a day, and after
> zero. However, it's a hard bug to pin down (some kind of timing/racing
> condition I suspect). Accordingly there is no simple test case that can be
> applied that I am aware of. Based on the short time between the last two
> macOS beta releases (3 and 4) and the fact that the last version barely had
> any "change notes", I suspect macOS release is getting close. I would feel
> a lot more comfortable if all modern distributions of Emacs on macOS had
> this patch in rather than not. When/if we manage to pin it down with a
> reliable test case, a real fix of this or the underlying issue should be
> easier, but until then I vote to get the patch in ASAP. Additionally the
> patch only affects macOS users so the breakage potential should be small
> (at least compared to daily crashes which is the alternative).
>
> On Wed, 3 Aug 2016 at 04:39 Eli Zaretskii <eliz@gnu.org> wrote:
>
>> > From: John Wiegley <jwiegley@gmail.com>
>> > Cc: Eli Zaretskii <eliz@gnu.org>,  Emacs-Devel devel <
>> emacs-devel@gnu.org>
>> > Date: Tue, 02 Aug 2016 13:25:10 -0700
>> >
>> > >>>>> Alan Third <alan@idiocy.org> writes:
>> >
>> > > Hi John, are you OK with the patch?
>> >
>> > Yes, I am OK with the patch. Eli?
>>
>> If all those affected by the problem agree it's TRT, I'm OK with it
>> going to emacs-25.
>>
>>


-- 
Alan Third

[-- Attachment #2: Type: text/html, Size: 2629 bytes --]

Re: Bug#23924 - crashes on next OS X release

[Alan Third]

On Wed, Aug 03, 2016 at 09:39:00AM +0100, Alan Third wrote:
> I'll push it to emacs-25 tonight.

Committed to emacs‐25.

-- 
Alan Third

Re: Bug#23924 - crashes on next OS X release

[Nicolas Petton]

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

Alan Third <alan@idiocy.org> writes:

>> I'll push it to emacs-25 tonight.
>
> Committed to emacs‐25.

Thanks, I'll wait a few more days before the next RC.

Cheers,
Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]