[virt-tools-list] [PATCH virt-viewer v2] iso-dialog: Avoid crash when closing dialog early

[Pavel Grunt]

Hi Eduardo,

On Thu, 2017-02-09 at 15:13 -0200, Eduardo Lima (Etrunko) wrote:
> We must take into account that users can close the dialog at
> anytime,
> even during an operation of fetch or set ISO has not been finished.
> This
> will cause the callbacks for those operations to be invoked with an
> invalid object, crashing the application.
> 
> To fix this issue we need to pass a GCancellable to the asynchronous
> operations, so they can be cancelled if the dialog happens to get
> closed
> before they complete.
> 
> Signed-off-by: Eduardo Lima (Etrunko) <etrunko at redhat.com>
> ---
> v2:
>  * Move call to g_cancellable_cancel() to response handler.
>  * Don't leak error objects if operation is cancelled.
> ---
>  src/remote-viewer-iso-list-dialog.c | 42
> ++++++++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/src/remote-viewer-iso-list-dialog.c b/src/remote-
> viewer-iso-list-dialog.c
> index 2ab5435..ed51ffa 100644
> --- a/src/remote-viewer-iso-list-dialog.c
> +++ b/src/remote-viewer-iso-list-dialog.c
> @@ -42,6 +42,7 @@ struct _RemoteViewerISOListDialogPrivate
>      GtkWidget *stack;
>      GtkWidget *tree_view;
>      OvirtForeignMenu *foreign_menu;
> +    GCancellable *cancellable;
>  };
>  
>  enum RemoteViewerISOListDialogModel
> @@ -66,6 +67,8 @@ remote_viewer_iso_list_dialog_dispose(GObject
> *object)
>      RemoteViewerISOListDialog *self =
> REMOTE_VIEWER_ISO_LIST_DIALOG(object);
>      RemoteViewerISOListDialogPrivate *priv = self->priv;
>  
> +    g_clear_object(&priv->cancellable);
> +
>      if (priv->foreign_menu) {
>          g_signal_handlers_disconnect_by_data(priv->foreign_menu,
> object);
>          g_clear_object(&priv->foreign_menu);
> @@ -157,17 +160,24 @@ fetch_iso_names_cb(OvirtForeignMenu
> *foreign_menu,
>          const gchar *msg = error ? error->message : _("Failed to
> fetch CD names");
>          gchar *markup = g_strdup_printf("<b>%s</b>", msg);
>  
> +        g_debug("Error fetching ISO names: %s", msg);
> +        if (error && error->code == G_IO_ERROR_CANCELLED)
use g_error_matches if possible

> +            goto end;
> +
>          gtk_label_set_markup(GTK_LABEL(priv->status), markup);
>          gtk_spinner_stop(GTK_SPINNER(priv->spinner));
>          remote_viewer_iso_list_dialog_show_error(self, msg);
>          gtk_dialog_set_response_sensitive(GTK_DIALOG(self),
> GTK_RESPONSE_NONE, TRUE);
>          g_free(markup);
> -        g_clear_error(&error);
> -        return;
> +        goto end;
>      }
>  
> +    g_clear_object(&priv->cancellable);
>      g_list_foreach(iso_list, (GFunc)
> remote_viewer_iso_list_dialog_foreach, self);
>      remote_viewer_iso_list_dialog_show_files(self);
> +
> +end:
> +    g_clear_error(&error);
>  }
>  
>  
> @@ -177,7 +187,10 @@
> remote_viewer_iso_list_dialog_refresh_iso_list(RemoteViewerISOListDi
> alog *self)
>      RemoteViewerISOListDialogPrivate *priv = self->priv;
>  
>      gtk_list_store_clear(priv->list_store);
> -    ovirt_foreign_menu_fetch_iso_names_async(priv->foreign_menu,
> NULL,
> +
> +    priv->cancellable = g_cancellable_new();
> +    ovirt_foreign_menu_fetch_iso_names_async(priv->foreign_menu,
> +                                             priv->cancellable,
>                                               (GAsyncReadyCallback)
> fetch_iso_names_cb,
>                                               self);
>  }
> @@ -190,8 +203,10 @@
> remote_viewer_iso_list_dialog_response(GtkDialog *dialog,
>      RemoteViewerISOListDialog *self =
> REMOTE_VIEWER_ISO_LIST_DIALOG(dialog);
>      RemoteViewerISOListDialogPrivate *priv = self->priv;
>  
> -    if (response_id != GTK_RESPONSE_NONE)
> +    if (response_id != GTK_RESPONSE_NONE) {
> +        g_cancellable_cancel(priv->cancellable);
>          return;
> +    }
>  
>      gtk_spinner_start(GTK_SPINNER(priv->spinner));
>      gtk_label_set_markup(GTK_LABEL(priv->status),
> _("<b>Loading...</b>"));
> @@ -223,7 +238,9 @@
> remote_viewer_iso_list_dialog_toggled(GtkCellRendererToggle
> *cell_renderer G_GNU
>      gtk_dialog_set_response_sensitive(GTK_DIALOG(self),
> GTK_RESPONSE_NONE, FALSE);
>      gtk_widget_set_sensitive(priv->tree_view, FALSE);
>  
> -    ovirt_foreign_menu_set_current_iso_name_async(priv-
> >foreign_menu, active ? NULL : name, NULL,
> +    priv->cancellable = g_cancellable_new();
> +    ovirt_foreign_menu_set_current_iso_name_async(priv-
> >foreign_menu, active ? NULL : name,
> +                                                  priv-
> >cancellable,
>                                                    (GAsyncReadyCallb
> ack)ovirt_foreign_menu_iso_name_changed,
>                                                    self);
>      gtk_tree_path_free(tree_path);
> @@ -308,12 +325,18 @@
> ovirt_foreign_menu_iso_name_changed(OvirtForeignMenu *foreign_menu,
>       * change the ISO back to the original, avoiding a possible
> inconsistency.
>       */
>      if
> (!ovirt_foreign_menu_set_current_iso_name_finish(foreign_menu,
> result, &error)) {
> -        remote_viewer_iso_list_dialog_show_error(self, error ?
> error->message : _("Failed to change CD"));
> -        g_clear_error(&error);
> +        const gchar *msg = error ? error->message : _("Failed to
> change CD");
> +        g_debug("Error changing ISO: %s", msg);
> +
> +        if (error && error->code == G_IO_ERROR_CANCELLED)

g_error_matches

> +            goto end;
> +
> +        remote_viewer_iso_list_dialog_show_error(self, msg);
>      }
>  
> +    g_clear_object(&priv->cancellable);
>      if (!gtk_tree_model_get_iter_first(model, &iter))
> -        return;
> +        goto end;
>  
>      current_iso =
> ovirt_foreign_menu_get_current_iso_name(foreign_menu);
>  
> @@ -340,6 +363,9 @@
> ovirt_foreign_menu_iso_name_changed(OvirtForeignMenu *foreign_menu,
>      gtk_dialog_set_response_sensitive(GTK_DIALOG(self),
> GTK_RESPONSE_NONE, TRUE);
>      gtk_widget_set_sensitive(priv->tree_view, TRUE);
>      g_free(current_iso);
> +
> +end:
> +    g_clear_error(&error);
>  }
>  
 GtkWidget *

I haven't test it, but it looks good, ack with the g_error_matches
change

Pavel