[Spice-devel] [spice-gtk 1/2] Revert "channel-usbredir: Fix crash due to a Task returning earlier than expected"

[Jonathon Jongsma]

On Wed, 2016-03-23 at 10:48 +0100, Fabiano Fidêncio wrote:
> This reverts commit 7774b8c0dd85ce2bb311d8bbe1c25deb73970b6e.
> 
> The crash was fixed, but not properly. GTask shouldn't return
> immediately in that scenario. Next patch brings a proper fix for the
> issue.
> 
> Signed-off-by: Fabiano Fidêncio <fidencio at redhat.com>
> ---
>  src/channel-usbredir.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index ab90800..0f76126 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -296,12 +296,12 @@ static void spice_usbredir_channel_open_acl_cb(
>          spice_usbredir_channel_open_device(channel, &err);
>      }
>      if (err) {
> +        g_task_return_error(priv->task, err);
>          libusb_unref_device(priv->device);
>          priv->device = NULL;
>          g_boxed_free(spice_usb_device_get_type(), priv->spice_device);
>          priv->spice_device = NULL;
>          priv->state  = STATE_DISCONNECTED;
> -        g_task_return_error(priv->task, err);
>      } else {
>          g_task_return_boolean(priv->task, TRUE);
>      }


I disagree with reverting this patch even if the task is returned in an idle
handler. The act of changing the state to STATE_DISCONNECTED is part of the
task. And we should not call g_task_return_* until the entire task is complete.
So regardless of whether any additional changes are made to this code or not, I
still think that this original patch is correct and should stay.