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

[Pavel Grunt]

On Fri, 2017-02-03 at 14:42 -0200, Eduardo Lima (Etrunko) wrote:
> On 03/02/17 14:34, Christophe Fergeau wrote:
> > Hey,
> > 
> > On Thu, Jan 26, 2017 at 06:01:23PM -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.
> > > 
> > 
> > This is going to depend on libgovirt bug
> > https://bugzilla.gnome.org/show_bug.cgi?id=777808 being fixed,
> > otherwise
> > we'd be getting a deadlock, is that correct?
> > 
> 
> Yes, this patch depends on the fix in libgovirt.

so there has to be a version check (or a bump of requirements).

Pavel

> 
> > > Signed-off-by: Eduardo Lima (Etrunko) <etrunko at redhat.com>
> > > ---
> > >  src/remote-viewer-iso-list-dialog.c | 30
> > > +++++++++++++++++++++++++++---
> > >  1 file changed, 27 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/src/remote-viewer-iso-list-dialog.c b/src/remote-
> > > viewer-iso-list-dialog.c
> > > index f23ddb2..a7ac98a 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,10 +67,16 @@
> > > remote_viewer_iso_list_dialog_dispose(GObject *object)
> > >      RemoteViewerISOListDialog *self =
> > > REMOTE_VIEWER_ISO_LIST_DIALOG(object);
> > >      RemoteViewerISOListDialogPrivate *priv = self->priv;
> > >  
> > > +    if (priv->cancellable) {
> > > +        g_cancellable_cancel(priv->cancellable);
> > > +        g_clear_object(&priv->cancellable);
> > > +    }
> > > +
> > 
> > g_cancellable_cancel() can be async, and at this point, the iso
> > dialog
> > is already dead, are you sure it's safe to cancel here?
> 
> 
> I am not sure, thinking better about it, it may be safer to cancel
> on
> the response signal, or maybe by the close signal.
> 
> > 
> > >      if (priv->foreign_menu) {
> > >          g_signal_handlers_disconnect_by_data(priv-
> > > >foreign_menu, object);
> > >          g_clear_object(&priv->foreign_menu);
> > >      }
> > > +
> > 
> > I'd drop this added blank line
> 
> Oops, slipped in.
> 
> > 
> > >      G_OBJECT_CLASS(remote_viewer_iso_list_dialog_parent_class)-
> > > >dispose(object);
> > >  }
> > >  
> > > @@ -157,6 +164,10 @@ 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)
> > > +            return;
> > > +
> > 
> > 'error' is leaked.
> 
> Fixed.
> 
> 
> > 
> > >          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);
> > > @@ -166,6 +177,7 @@ fetch_iso_names_cb(OvirtForeignMenu
> > > *foreign_menu,
> > >          return;
> > >      }
> > >  
> > > +    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);
> > >  }
> > > @@ -177,7 +189,10 @@
> > > remote_viewer_iso_list_dialog_refresh_iso_list(RemoteViewerISOLi
> > > stDialog *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,
> > >                                               (GAsyncReadyCallba
> > > ck) fetch_iso_names_cb,
> > >                                               self);
> > >  }
> > > @@ -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,
> > >                                                    (GAsyncReadyC
> > > allback)ovirt_foreign_menu_iso_name_changed,
> > >                                                    self);
> > >      gtk_tree_path_free(tree_path);
> > > @@ -308,10 +325,17 @@
> > > 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"));
> > > +        gchar *msg = error ? error->message : _("Failed to
> > > change CD");
> > > +        g_debug("Error changing ISO: %s", msg);
> > > +
> > > +        if (error && error->code == G_IO_ERROR_CANCELLED)
> > > +            return;
> > 
> > 'error' is leaked.
> 
> Also fixed, thanks for the review. V2 coming soon.