| Message ID | 20191014141608.80923-1-hikarupsp@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ui: Fix hanging up Cocoa display on macOS 10.15 (Catalina) | expand |
On Mon, 14 Oct 2019 at 15:16, <hikarupsp@gmail.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> > --- > 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
> 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 <peter.maydell@linaro.org>: > > On Mon, 14 Oct 2019 at 15:16, <hikarupsp@gmail.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> > > --- > > 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
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; // Utility functions to run specified code block with iothread lock held typedef void (^CodeBlock)(void); @@ -729,6 +730,16 @@ - (void) handleMonitorInput:(NSEvent *)event - (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 @@ - (void) dealloc - (void)applicationDidFinishLaunching: (NSNotification *) note { COCOA_DEBUG("QemuCocoaAppController: applicationDidFinishLaunching\n"); + allow_events = true; /* Tell cocoa_display_init to proceed */ qemu_sem_post(&app_started_sem); }