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

[Eduardo Lima (Etrunko)]

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.

>> 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(RemoteViewerISOListDialog *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);
>>  }
>> @@ -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,
>>                                                    (GAsyncReadyCallback)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.
-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko at redhat.com