After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 780299 - Do not crash while leaving the panel during actualize_printers_list ()
Do not crash while leaving the panel during actualize_printers_list ()
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: Printers
git master
Other Linux
: Normal critical
: ---
Assigned To: Marek Kašík
Control-Center Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-03-20 12:32 UTC by Felipe Borges
Modified: 2017-04-10 12:26 UTC
See Also:
GNOME target: 3.24
GNOME version: ---


Attachments
printers: Make actualize_printers_list cancellable (2.22 KB, patch)
2017-03-20 12:33 UTC, Felipe Borges
none Details | Review
printers: Make actualize_printers_list cancellable (3.91 KB, patch)
2017-03-21 10:58 UTC, Felipe Borges
none Details | Review
printers: Make actualize_printers_list cancellable (4.19 KB, patch)
2017-03-24 12:04 UTC, Felipe Borges
none Details | Review
printers: Make actualize_printers_list cancellable (4.19 KB, patch)
2017-03-27 14:05 UTC, Felipe Borges
none Details | Review
printers: Make actualize_printers_list cancellable (4.17 KB, patch)
2017-03-27 14:27 UTC, Felipe Borges
committed Details | Review
printers: Make actualize_printers_list* cancellable (4.49 KB, patch)
2017-04-06 11:14 UTC, Felipe Borges
committed Details | Review

Description Felipe Borges 2017-03-20 12:32:55 UTC
Filed downstream as https://bugzilla.redhat.com/show_bug.cgi?id=1432257
Comment 1 Felipe Borges 2017-03-20 12:33:29 UTC
Created attachment 348312 [details] [review]
printers: Make actualize_printers_list cancellable

This way we can safely interrupt an update without crashing
g-c-c.
Comment 2 Marek Kašík 2017-03-20 16:36:54 UTC
Review of attachment 348312 [details] [review]:

The printers panel still crashes with the patch. You can test it simply by inserting "g_usleep(3000000);" into "_pp_cups_get_dests_thread()". You should somehow check that the operation was cancelled. E.g. by "g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)" in the callback.
Comment 3 Felipe Borges 2017-03-21 10:58:13 UTC
Created attachment 348392 [details] [review]
printers: Make actualize_printers_list cancellable

This way we can safely interrupt an update without crashing
g-c-c.
Comment 4 Marek Kašík 2017-03-21 15:16:55 UTC
Review of attachment 348392 [details] [review]:

Printers panel is still crashing for me with the patch.

::: panels/printers/cc-printers-panel.c
@@ +91,3 @@
   GCancellable *get_all_ppds_cancellable;
   GCancellable *subscription_renew_cancellable;
+  GCancellable *atualize_printers_list_cancellable;

You have a typo here.

@@ +640,3 @@
   priv = PRINTERS_PANEL_PRIVATE (self);
 
   free_dests (self);

These usages of "self" should be after the check for an error. I'm surprised that it doesn't crash for you.

@@ +643,3 @@
+  cups_dests = pp_cups_get_dests_finish (cups, result, &error);
+
+  if (error != NULL)

You should check also "cups_dests == NULL" here since GTask will return NULL in case of an error.

@@ +1028,2 @@
   priv->subscription_renew_cancellable = g_cancellable_new ();
+  priv->atualize_printers_list_cancellable = g_cancellable_new ();

Initialize this sooner otherwise the handler of a permission change will use NULL cancellable which is another cause of crash which I can reproduce here.
Comment 5 Felipe Borges 2017-03-24 12:04:26 UTC
Created attachment 348644 [details] [review]
printers: Make actualize_printers_list cancellable

This way we can safely interrupt an update without crashing
g-c-c.
Comment 6 Marek Kašík 2017-03-27 13:56:12 UTC
Review of attachment 348644 [details] [review]:

::: panels/printers/cc-printers-panel.c
@@ +638,3 @@
   int                     i;
 
   priv = PRINTERS_PANEL_PRIVATE (self);

I meant also this usage of "self" to move below the check for error.

@@ +644,1 @@
+  if (cups_dests == NULL || error != NULL)

Change the "||" to "&&" please.
Comment 7 Felipe Borges 2017-03-27 14:05:30 UTC
Created attachment 348804 [details] [review]
printers: Make actualize_printers_list cancellable

This way we can safely interrupt an update without crashing
g-c-c.
Comment 8 Marek Kašík 2017-03-27 14:22:27 UTC
Review of attachment 348804 [details] [review]:

You've posted the same patch as before.
Comment 9 Felipe Borges 2017-03-27 14:27:54 UTC
Created attachment 348806 [details] [review]
printers: Make actualize_printers_list cancellable

This way we can safely interrupt an update without crashing
g-c-c.
Comment 10 Felipe Borges 2017-03-27 14:28:18 UTC
(In reply to Marek Kašík from comment #8)
> Review of attachment 348804 [details] [review] [review]:
> 
> You've posted the same patch as before.

heh, sorry. did not git add the changes.
Comment 11 Marek Kašík 2017-03-27 15:30:39 UTC
Review of attachment 348806 [details] [review]:

Thank you for the patch. Please push it to master and gnome-3-24 branches. Since the patch is not applicable to 3.22, prepare a version for gnome-3-22 branch and attach it here please. I'll review it.
Comment 12 Felipe Borges 2017-03-28 16:06:12 UTC
Comment on attachment 348806 [details] [review]
printers: Make actualize_printers_list cancellable

Thanks again Marek!

Attachment 348806 [details] pushed as 519e19f - printers: Make actualize_printers_list cancellable
Comment 13 Felipe Borges 2017-04-06 11:14:53 UTC
Created attachment 349353 [details] [review]
printers: Make actualize_printers_list* cancellable

This way we can safely interrupt an update without crashing
g-c-c.
Comment 14 Felipe Borges 2017-04-06 11:15:23 UTC
(In reply to Felipe Borges from comment #13)
> Created attachment 349353 [details] [review] [review]
> printers: Make actualize_printers_list* cancellable

this is the backported version of the patch for gnome-3-22.
Comment 15 Marek Kašík 2017-04-10 09:17:23 UTC
Review of attachment 349353 [details] [review]:

Thank you for the patch. Push it to gnome-3-22 branch please.
Comment 16 Felipe Borges 2017-04-10 12:26:20 UTC
Thanks!

Attachment 349353 [details] pushed as b8b0347 - printers: Make actualize_printers_list* cancellable