[PATCH] This patch fixes hanging up Cocoa display on macOS 10.15 (Catalina)

hikarupsp@gmail.com posted 1 patch 4 years, 5 months ago
Test asan passed
Test FreeBSD passed
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test docker-quick@centos7 passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191014125254.74913-1-hikarupsp@gmail.com
Maintainers: Gerd Hoffmann <kraxel@redhat.com>, Peter Maydell <peter.maydell@linaro.org>
ui/cocoa.m | 5 +++++
1 file changed, 5 insertions(+)
[PATCH] This patch fixes hanging up Cocoa display on macOS 10.15 (Catalina)
Posted by hikarupsp@gmail.com 4 years, 5 months ago
From: Hikaru Nishida <hikarupsp@gmail.com>

An NSEvent is fired before applicationDidFinishLaunching on macOS
Catalina. This causes deadlock of iothread_lock by calling
bool_with_iothread_lock in handleEvent while its already locked.
This patch prevents to call bool_with_iothread_lock until the
app_started_sem is released to prevent this deadlock.

Buglink: https://bugs.launchpad.net/qemu/+bug/1847906
Signed-off-by: Hikaru Nishida <hikarupsp@gmail.com>
---
 ui/cocoa.m | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index f12e21df6e..f16d341a0a 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -134,6 +134,7 @@
 
 static QemuSemaphore display_init_sem;
 static QemuSemaphore app_started_sem;
+volatile sig_atomic_t allow_events;
 
 // Utility functions to run specified code block with iothread lock held
 typedef void (^CodeBlock)(void);
@@ -729,6 +730,9 @@ - (void) handleMonitorInput:(NSEvent *)event
 
 - (bool) handleEvent:(NSEvent *)event
 {
+    if(!allow_events) {
+        return false;
+    }
     return bool_with_iothread_lock(^{
         return [self handleEventLocked:event];
     });
@@ -1897,6 +1901,7 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
     /* Tell main thread to go ahead and create the app and enter the run loop */
     qemu_sem_post(&display_init_sem);
     qemu_sem_wait(&app_started_sem);
+    allow_events = true;
     COCOA_DEBUG("cocoa_display_init: app start completed\n");
 
     /* if fullscreen mode is to be used */
-- 
2.21.0 (Apple Git-122)


Re: [PATCH] This patch fixes hanging up Cocoa display on macOS 10.15 (Catalina)
Posted by Peter Maydell 4 years, 5 months ago
On Mon, 14 Oct 2019 at 13:53, <hikarupsp@gmail.com> wrote:
>
> From: Hikaru Nishida <hikarupsp@gmail.com>
>
> An NSEvent is fired before applicationDidFinishLaunching on macOS
> Catalina. This causes deadlock of iothread_lock by calling
> bool_with_iothread_lock in handleEvent while its already locked.
> This patch prevents to call bool_with_iothread_lock until the
> app_started_sem is released to prevent this deadlock.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1847906
> Signed-off-by: Hikaru Nishida <hikarupsp@gmail.com>
> ---
>  ui/cocoa.m | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index f12e21df6e..f16d341a0a 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -134,6 +134,7 @@
>
>  static QemuSemaphore display_init_sem;
>  static QemuSemaphore app_started_sem;
> +volatile sig_atomic_t allow_events;
>
>  // Utility functions to run specified code block with iothread lock held
>  typedef void (^CodeBlock)(void);
> @@ -729,6 +730,9 @@ - (void) handleMonitorInput:(NSEvent *)event
>
>  - (bool) handleEvent:(NSEvent *)event
>  {
> +    if(!allow_events) {
> +        return false;
> +    }
>      return bool_with_iothread_lock(^{
>          return [self handleEventLocked:event];
>      });
> @@ -1897,6 +1901,7 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
>      /* Tell main thread to go ahead and create the app and enter the run loop */
>      qemu_sem_post(&display_init_sem);
>      qemu_sem_wait(&app_started_sem);
> +    allow_events = true;
>      COCOA_DEBUG("cocoa_display_init: app start completed\n");

If we do use a flag to fix this race, I think it would be
better to set allow_events = true in the
applicationDidFinishLaunching() function before posting
the app_started_sem. Otherwise there's a window after
applicationDidFinishLaunching() returns but before
cocoa_display_init() resumes and sets the flag where we
will unnecessarily drop events.

Could you try that and check that it still fixes the hang?

thanks
-- PMM

Re: [PATCH] This patch fixes hanging up Cocoa display on macOS 10.15 (Catalina)
Posted by Hikaru Nishida 4 years, 5 months ago
Thank you for your reply.

The event gets fired before applicationDidFinishLaunching is: (output of
 NSLog(@"event: %@", event);)
event: NSEvent: type=Kitdefined loc=(0,1440) time=164310.0 flags=0x40
win=0x0 winNum=0 ctxt=0x0 subtype=1 data1=834 data2=0

I moved allow_events = true just before qemu_sem_post(&app_started_sem) and
it also works.


Hikaru Nishida

2019年10月14日(月) 22:19 Peter Maydell <peter.maydell@linaro.org>:

> On Mon, 14 Oct 2019 at 13:53, <hikarupsp@gmail.com> wrote:
> >
> > From: Hikaru Nishida <hikarupsp@gmail.com>
> >
> > An NSEvent is fired before applicationDidFinishLaunching on macOS
> > Catalina. This causes deadlock of iothread_lock by calling
> > bool_with_iothread_lock in handleEvent while its already locked.
> > This patch prevents to call bool_with_iothread_lock until the
> > app_started_sem is released to prevent this deadlock.
> >
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1847906
> > Signed-off-by: Hikaru Nishida <hikarupsp@gmail.com>
> > ---
> >  ui/cocoa.m | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/ui/cocoa.m b/ui/cocoa.m
> > index f12e21df6e..f16d341a0a 100644
> > --- a/ui/cocoa.m
> > +++ b/ui/cocoa.m
> > @@ -134,6 +134,7 @@
> >
> >  static QemuSemaphore display_init_sem;
> >  static QemuSemaphore app_started_sem;
> > +volatile sig_atomic_t allow_events;
> >
> >  // Utility functions to run specified code block with iothread lock held
> >  typedef void (^CodeBlock)(void);
> > @@ -729,6 +730,9 @@ - (void) handleMonitorInput:(NSEvent *)event
> >
> >  - (bool) handleEvent:(NSEvent *)event
> >  {
> > +    if(!allow_events) {
> > +        return false;
> > +    }
> >      return bool_with_iothread_lock(^{
> >          return [self handleEventLocked:event];
> >      });
> > @@ -1897,6 +1901,7 @@ static void cocoa_display_init(DisplayState *ds,
> DisplayOptions *opts)
> >      /* Tell main thread to go ahead and create the app and enter the
> run loop */
> >      qemu_sem_post(&display_init_sem);
> >      qemu_sem_wait(&app_started_sem);
> > +    allow_events = true;
> >      COCOA_DEBUG("cocoa_display_init: app start completed\n");
>
> If we do use a flag to fix this race, I think it would be
> better to set allow_events = true in the
> applicationDidFinishLaunching() function before posting
> the app_started_sem. Otherwise there's a window after
> applicationDidFinishLaunching() returns but before
> cocoa_display_init() resumes and sets the flag where we
> will unnecessarily drop events.
>
> Could you try that and check that it still fixes the hang?
>
> thanks
> -- PMM
>
Re: [PATCH] This patch fixes hanging up Cocoa display on macOS 10.15 (Catalina)
Posted by Hikaru Nishida 4 years, 5 months ago
Thank you for your reply.

The event gets fired before applicationDidFinishLaunching is: (output
of  NSLog(@"event: %@", event);)
event: NSEvent: type=Kitdefined loc=(0,1440) time=164310.0 flags=0x40
win=0x0 winNum=0 ctxt=0x0 subtype=1 data1=834 data2=0

I moved allow_events = true just before
qemu_sem_post(&app_started_sem) and it also works.


Hikaru Nishida
(resending in plaintext)


2019年10月14日(月) 22:19 Peter Maydell <peter.maydell@linaro.org>:
>
> On Mon, 14 Oct 2019 at 13:53, <hikarupsp@gmail.com> wrote:
> >
> > From: Hikaru Nishida <hikarupsp@gmail.com>
> >
> > An NSEvent is fired before applicationDidFinishLaunching on macOS
> > Catalina. This causes deadlock of iothread_lock by calling
> > bool_with_iothread_lock in handleEvent while its already locked.
> > This patch prevents to call bool_with_iothread_lock until the
> > app_started_sem is released to prevent this deadlock.
> >
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1847906
> > Signed-off-by: Hikaru Nishida <hikarupsp@gmail.com>
> > ---
> >  ui/cocoa.m | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/ui/cocoa.m b/ui/cocoa.m
> > index f12e21df6e..f16d341a0a 100644
> > --- a/ui/cocoa.m
> > +++ b/ui/cocoa.m
> > @@ -134,6 +134,7 @@
> >
> >  static QemuSemaphore display_init_sem;
> >  static QemuSemaphore app_started_sem;
> > +volatile sig_atomic_t allow_events;
> >
> >  // Utility functions to run specified code block with iothread lock held
> >  typedef void (^CodeBlock)(void);
> > @@ -729,6 +730,9 @@ - (void) handleMonitorInput:(NSEvent *)event
> >
> >  - (bool) handleEvent:(NSEvent *)event
> >  {
> > +    if(!allow_events) {
> > +        return false;
> > +    }
> >      return bool_with_iothread_lock(^{
> >          return [self handleEventLocked:event];
> >      });
> > @@ -1897,6 +1901,7 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
> >      /* Tell main thread to go ahead and create the app and enter the run loop */
> >      qemu_sem_post(&display_init_sem);
> >      qemu_sem_wait(&app_started_sem);
> > +    allow_events = true;
> >      COCOA_DEBUG("cocoa_display_init: app start completed\n");
>
> If we do use a flag to fix this race, I think it would be
> better to set allow_events = true in the
> applicationDidFinishLaunching() function before posting
> the app_started_sem. Otherwise there's a window after
> applicationDidFinishLaunching() returns but before
> cocoa_display_init() resumes and sets the flag where we
> will unnecessarily drop events.
>
> Could you try that and check that it still fixes the hang?
>
> thanks
> -- PMM

Re: [PATCH] This patch fixes hanging up Cocoa display on macOS 10.15 (Catalina)
Posted by Peter Maydell 4 years, 5 months ago
On Mon, 14 Oct 2019 at 14:41, Hikaru Nishida <hikarupsp@gmail.com> wrote:
>
> Thank you for your reply.
>
> The event gets fired before applicationDidFinishLaunching is: (output
> of  NSLog(@"event: %@", event);)
> event: NSEvent: type=Kitdefined loc=(0,1440) time=164310.0 flags=0x40
> win=0x0 winNum=0 ctxt=0x0 subtype=1 data1=834 data2=0
>
> I moved allow_events = true just before
> qemu_sem_post(&app_started_sem) and it also works.

OK, great. I think we should go with that, then.
I think a brief comment explaining the purpose of the flag
would also be useful in the handleEvent function; something
like this:

/*
 * 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.)
 */

Could you send a v2 of the patch with 'allow_events = true' in the
applicationDidFinishLaunching function, and that comment
in handleEvent please, and we'll get that into the tree?

thanks
-- PMM

Re: [PATCH] This patch fixes hanging up Cocoa display on macOS 10.15 (Catalina)
Posted by Hikaru Nishida 4 years, 5 months ago
I resent the patch. Thank you for your assistance!

Hikaru Nishida

2019年10月14日(月) 22:50 Peter Maydell <peter.maydell@linaro.org>:
>
> On Mon, 14 Oct 2019 at 14:41, Hikaru Nishida <hikarupsp@gmail.com> wrote:
> >
> > Thank you for your reply.
> >
> > The event gets fired before applicationDidFinishLaunching is: (output
> > of  NSLog(@"event: %@", event);)
> > event: NSEvent: type=Kitdefined loc=(0,1440) time=164310.0 flags=0x40
> > win=0x0 winNum=0 ctxt=0x0 subtype=1 data1=834 data2=0
> >
> > I moved allow_events = true just before
> > qemu_sem_post(&app_started_sem) and it also works.
>
> OK, great. I think we should go with that, then.
> I think a brief comment explaining the purpose of the flag
> would also be useful in the handleEvent function; something
> like this:
>
> /*
>  * 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.)
>  */
>
> Could you send a v2 of the patch with 'allow_events = true' in the
> applicationDidFinishLaunching function, and that comment
> in handleEvent please, and we'll get that into the tree?
>
> thanks
> -- PMM

Re: [PATCH] This patch fixes hanging up Cocoa display on macOS 10.15 (Catalina)
Posted by Peter Maydell 4 years, 5 months ago
On Mon, 14 Oct 2019 at 13:53, <hikarupsp@gmail.com> wrote:
>
> From: Hikaru Nishida <hikarupsp@gmail.com>
>
> An NSEvent is fired before applicationDidFinishLaunching on macOS
> Catalina. This causes deadlock of iothread_lock by calling
> bool_with_iothread_lock in handleEvent while its already locked.
> This patch prevents to call bool_with_iothread_lock until the
> app_started_sem is released to prevent this deadlock.

Thanks for this analysis and patch.

Do you know what the event that gets fired is?
I'm wondering if it's something we need to handle
(or if other events might in future fire in this window
that matter to us).

Incidentally, getting an NSEvent before applicationDidFinishLaunching
seems to contradict the OSX API docs:

https://developer.apple.com/documentation/appkit/nsapplicationdelegate/1428385-applicationdidfinishlaunching

which say "This method is called after the application's main
run loop has been started but before it has processed any events."

thanks
-- PMM