Re: [PATCH 1/2] ui/cocoa.m: Fix updateUIInfo threading issues

[Akihiko Odaki]

On Wed, Feb 23, 2022 at 2:06 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The updateUIInfo method makes Cocoa API calls.  It also calls back
> into QEMU functions like dpy_set_ui_info().  To do this safely, we
> need to follow two rules:
>  * Cocoa API calls are made on the Cocoa UI thread
>  * When calling back into QEMU we must hold the iothread lock
>
> Fix the places where we got this wrong, by taking the iothread lock
> while executing updateUIInfo, and moving the call in cocoa_switch()
> inside the dispatch_async block.
>
> Some of the Cocoa UI methods which call updateUIInfo are invoked as
> part of the initial application startup, while we're still doing the
> little cross-thread dance described in the comment just above
> call_qemu_main().  This meant they were calling back into the QEMU UI
> layer before we'd actually finished initializing our display and
> registered the DisplayChangeListener, which isn't really valid.  Once
> updateUIInfo takes the iothread lock, we no longer get away with
> this, because during this startup phase the iothread lock is held by
> the QEMU main-loop thread which is waiting for us to finish our
> display initialization.  So we must suppress updateUIInfo until
> applicationDidFinishLaunching allows the QEMU main-loop thread to
> continue, and instead defer telling the UI layer about our initial
> window size until later.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Thanks for fixing this bug.

> ---
>  ui/cocoa.m | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index a8f1cdaf926..f8d3d8ad7a6 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -522,8 +522,9 @@ QemuCocoaView *cocoaView;
>      }
>  }
>
> -- (void) updateUIInfo
> +- (void) doUpdateUIInfo

You may add the suffix "Locked" to align with handleEventLocked and
make the intention more explicit.

>  {
> +    /* Must be called with the iothread lock, i.e. via updateUIInfo */
>      NSSize frameSize;
>      QemuUIInfo info;
>
> @@ -554,6 +555,22 @@ QemuCocoaView *cocoaView;
>      dpy_set_ui_info(dcl.con, &info, TRUE);
>  }
>
> +- (void) updateUIInfo
> +{
> +    if (!allow_events) {
> +        /*
> +         * Don't try to tell QEMU about UI information in the application
> +         * startup phase -- we haven't yet registered dcl with the QEMU UI
> +         * layer, and also trying to take the iothread lock would deadlock.
> +         */
> +        return;
> +    }
> +
> +    with_iothread_lock(^{
> +        [self doUpdateUIInfo];
> +    });
> +}
> +
>  - (void)viewDidMoveToWindow
>  {
>      [self updateUIInfo];
> @@ -1985,8 +2002,6 @@ static void cocoa_switch(DisplayChangeListener *dcl,
>
>      COCOA_DEBUG("qemu_cocoa: cocoa_switch\n");
>
> -    [cocoaView updateUIInfo];
> -
>      // The DisplaySurface will be freed as soon as this callback returns.
>      // We take a reference to the underlying pixman image here so it does
>      // not disappear from under our feet; the switchSurface method will
> @@ -1994,6 +2009,7 @@ static void cocoa_switch(DisplayChangeListener *dcl,
>      pixman_image_ref(image);
>
>      dispatch_async(dispatch_get_main_queue(), ^{
> +        [cocoaView updateUIInfo];
>          [cocoaView switchSurface:image];
>      });
>      [pool release];
> @@ -2057,6 +2073,11 @@ static void cocoa_display_init(DisplayState *ds, 
> DisplayOptions *opts)
>      qemu_event_init(&cbevent, false);
>      cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init];
>      qemu_clipboard_peer_register(&cbpeer);
> +
> +    /* Now we're set up, tell the UI layer our initial window size */
> +    dispatch_async(dispatch_get_main_queue(), ^{
> +        [cocoaView updateUIInfo];
> +    });
>  }

This is not necessary since register_displaychangelistener calls dpy_gfx_switch.

>
>  static QemuDisplay qemu_display_cocoa = {
> --
> 2.25.1
>