Message ID | 20191014125254.74913-1-hikarupsp@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | This patch fixes hanging up Cocoa display on macOS 10.15 (Catalina) | expand |
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
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
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 >
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
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
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
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 */