[PULL,1/4] ui: Fix hanging up Cocoa display on macOS 10.15 (Catalina)
diff mbox series

Message ID 20191018101711.24105-2-kraxel@redhat.com
State New
Headers show
Series
  • [PULL,1/4] ui: Fix hanging up Cocoa display on macOS 10.15 (Catalina)
Related show

Commit Message

Gerd Hoffmann Oct. 18, 2019, 10:17 a.m. UTC
From: Hikaru Nishida <hikarupsp@gmail.com>

macOS API documentation says that before applicationDidFinishLaunching
is called, any events will not be processed. However, some events are
fired before it is called in macOS Catalina. This causes deadlock of
iothread_lock in handleEvent while it will be released after the
app_started_sem is posted.
This patch avoids processing events before the app_started_sem is
posted to prevent this deadlock.

Buglink: https://bugs.launchpad.net/qemu/+bug/1847906
Signed-off-by: Hikaru Nishida <hikarupsp@gmail.com>
Message-id: 20191015010734.85229-1-hikarupsp@gmail.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/cocoa.m | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Peter Maydell Oct. 18, 2019, 11:54 a.m. UTC | #1
Oops, just noticed that this patch should have been
Cc: qemu-stable@nongnu.org.

Hopefully the stable team can pick it up anyway.

thanks
-- PMM

On Fri, 18 Oct 2019 at 11:17, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> From: Hikaru Nishida <hikarupsp@gmail.com>
>
> macOS API documentation says that before applicationDidFinishLaunching
> is called, any events will not be processed. However, some events are
> fired before it is called in macOS Catalina. This causes deadlock of
> iothread_lock in handleEvent while it will be released after the
> app_started_sem is posted.
> This patch avoids processing events before the app_started_sem is
> posted to prevent this deadlock.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1847906
> Signed-off-by: Hikaru Nishida <hikarupsp@gmail.com>
> Message-id: 20191015010734.85229-1-hikarupsp@gmail.com
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/cocoa.m | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index f12e21df6e10..fbb5b1b45f81 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -134,6 +134,7 @@ NSArray * supportedImageFileTypes;
>
>  static QemuSemaphore display_init_sem;
>  static QemuSemaphore app_started_sem;
> +static bool allow_events;
>
>  // Utility functions to run specified code block with iothread lock held
>  typedef void (^CodeBlock)(void);
> @@ -729,6 +730,16 @@ QemuCocoaView *cocoaView;
>
>  - (bool) handleEvent:(NSEvent *)event
>  {
> +    if(!allow_events) {
> +        /*
> +         * Just let OSX have all events that arrive before
> +         * applicationDidFinishLaunching.
> +         * This avoids a deadlock on the iothread lock, which cocoa_display_init()
> +         * will not drop until after the app_started_sem is posted. (In theory
> +         * there should not be any such events, but OSX Catalina now emits some.)
> +         */
> +        return false;
> +    }
>      return bool_with_iothread_lock(^{
>          return [self handleEventLocked:event];
>      });
> @@ -1156,6 +1167,7 @@ QemuCocoaView *cocoaView;
>  - (void)applicationDidFinishLaunching: (NSNotification *) note
>  {
>      COCOA_DEBUG("QemuCocoaAppController: applicationDidFinishLaunching\n");
> +    allow_events = true;
>      /* Tell cocoa_display_init to proceed */
>      qemu_sem_post(&app_started_sem);
>  }
> --
> 2.18.1
Christian Schoenebeck Oct. 18, 2019, 1:01 p.m. UTC | #2
On Freitag, 18. Oktober 2019 12:17:08 CEST Gerd Hoffmann wrote:
> From: Hikaru Nishida <hikarupsp@gmail.com>
> 
> macOS API documentation says that before applicationDidFinishLaunching
> is called, any events will not be processed. However, some events are
> fired before it is called in macOS Catalina.

Even though fixed now on qemu side, I filed a bug report with Apple to let 
them know, since this indeed looks like unintended behaviour change a.k.a. bug 
in Catalina.

Best regards,
Christian Schoenebeck
Peter Maydell Oct. 18, 2019, 1:02 p.m. UTC | #3
On Fri, 18 Oct 2019 at 14:01, Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
>
> On Freitag, 18. Oktober 2019 12:17:08 CEST Gerd Hoffmann wrote:
> > From: Hikaru Nishida <hikarupsp@gmail.com>
> >
> > macOS API documentation says that before applicationDidFinishLaunching
> > is called, any events will not be processed. However, some events are
> > fired before it is called in macOS Catalina.
>
> Even though fixed now on qemu side, I filed a bug report with Apple to let
> them know, since this indeed looks like unintended behaviour change a.k.a. bug
> in Catalina.

Thanks -- do you have an apple bug number or something for that?

-- PMM
Christian Schoenebeck Oct. 18, 2019, 2:11 p.m. UTC | #4
On Freitag, 18. Oktober 2019 15:02:49 CEST Peter Maydell wrote:
> On Fri, 18 Oct 2019 at 14:01, Christian Schoenebeck
> 
> <qemu_oss@crudebyte.com> wrote:
> > On Freitag, 18. Oktober 2019 12:17:08 CEST Gerd Hoffmann wrote:
> > > From: Hikaru Nishida <hikarupsp@gmail.com>
> > > 
> > > macOS API documentation says that before applicationDidFinishLaunching
> > > is called, any events will not be processed. However, some events are
> > > fired before it is called in macOS Catalina.
> > 
> > Even though fixed now on qemu side, I filed a bug report with Apple to let
> > them know, since this indeed looks like unintended behaviour change a.k.a.
> > bug in Catalina.
> 
> Thanks -- do you have an apple bug number or something for that?

It's filed as FB7380815 (on October 15th).

As of May this year they changed the way how externals may report bugs. You 
now have to use "Feedback Assistant" (requires a developer account):

	https://developer.apple.com/bug-reporting/

Before that change, registered developers could use bugreport.apple.com for 
many years, which was more tightly coupled with their actual internal bug 
tracker called "Radar" (e.g. you saw their internal rdar number, internal bug 
status, duplicates) than this new Feedback Assistant.

Best regards,
Christian Schoenebeck

Patch
diff mbox series

diff --git a/ui/cocoa.m b/ui/cocoa.m
index f12e21df6e10..fbb5b1b45f81 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -134,6 +134,7 @@  NSArray * supportedImageFileTypes;
 
 static QemuSemaphore display_init_sem;
 static QemuSemaphore app_started_sem;
+static bool allow_events;
 
 // Utility functions to run specified code block with iothread lock held
 typedef void (^CodeBlock)(void);
@@ -729,6 +730,16 @@  QemuCocoaView *cocoaView;
 
 - (bool) handleEvent:(NSEvent *)event
 {
+    if(!allow_events) {
+        /*
+         * Just let OSX have all events that arrive before
+         * applicationDidFinishLaunching.
+         * This avoids a deadlock on the iothread lock, which cocoa_display_init()
+         * will not drop until after the app_started_sem is posted. (In theory
+         * there should not be any such events, but OSX Catalina now emits some.)
+         */
+        return false;
+    }
     return bool_with_iothread_lock(^{
         return [self handleEventLocked:event];
     });
@@ -1156,6 +1167,7 @@  QemuCocoaView *cocoaView;
 - (void)applicationDidFinishLaunching: (NSNotification *) note
 {
     COCOA_DEBUG("QemuCocoaAppController: applicationDidFinishLaunching\n");
+    allow_events = true;
     /* Tell cocoa_display_init to proceed */
     qemu_sem_post(&app_started_sem);
 }