Re: [PATCH v2] ui: Fix hanging up Cocoa display on macOS 10.15 (Catalina)

[Hikaru Nishida]

> I think we don't need to mark this 'volatile sig_atomic_t',
> but could use a simple 'bool', because both applicationDidFinishLaunching()
> and handleEvent() are called from the same thread (the OSX run loop
> thread).

Oh, I didn't notice that.
I replaced 'volatile sig_atomic_t' with 'static bool' because it only
requires a file scope, and confirmed the allow_events is referenced by
the same thread.

* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x00000001004be802 qemu-system-x86_64`-[QemuCocoaView
handleEvent:](self=0x0000000111e46650, _cmd="handleEvent:",
event=0x0000000111ebdc30) at cocoa.m:733:8
   730
   731  - (bool) handleEvent:(NSEvent *)event
   732  {
-> 733      if(!allow_events) {
   734          /*
   735           * Just let OSX have all events that arrive before
   736           * applicationDidFinishLaunching.
(lldb) c
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 2.1
    frame #0: 0x00000001004c01e4
qemu-system-x86_64`-[QemuCocoaAppController
applicationDidFinishLaunching:](self=0x0000000111e46540,
_cmd="applicationDidFinishLaunching:",
note=@"NSApplicationDidFinishLaunchingNotification") at
cocoa.m:1170:18
   1167 - (void)applicationDidFinishLaunching: (NSNotification *) note
   1168 {
   1169     COCOA_DEBUG("QemuCocoaAppController:
applicationDidFinishLaunching\n");
-> 1170     allow_events = true;
   1171     /* Tell cocoa_display_init to proceed */
   1172     qemu_sem_post(&app_started_sem);
   1173 }

I resent the patch v3. Thanks!


Hikaru Nishida

2019年10月15日(火) 2:16 Peter Maydell <address@hidden>:
>
> On Mon, 14 Oct 2019 at 15:16, <address@hidden> wrote:
> >
> > From: Hikaru Nishida <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> >  ui/cocoa.m | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/ui/cocoa.m b/ui/cocoa.m
> > index f12e21df6e..bccd861d16 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;
>
> Sorry, I failed to spot this on version 1 of the patch...
> I think we don't need to mark this 'volatile sig_atomic_t',
> but could use a simple 'bool', because both applicationDidFinishLaunching()
> and handleEvent() are called from the same thread (the OSX run loop
> thread).  Could you test that it still works with plain 'bool',
> please?
>
> (If we did need to handle multiple thread accesses we should
> probably prefer one of the QEMU atomic primitives described
> in docs/devel/atomics.txt, since 'volatile' isn't really sufficient.)
>
> thanks
> -- PMM