GNOME Bugzilla – Bug 763689
Fix cancelled async calls usage
Last modified: 2016-03-16 11:17:54 UTC
Mostly about printing warnings, but there are at least 2 real bugs fixed as well.
Created attachment 323987 [details] [review] housekeeping: Don't print warning on call cancellation
Created attachment 323988 [details] [review] housekeeping: Fix checking for cancellation We really don't need to call g_cancellable_is_cancelled() when we could just check the error code we get back.
Created attachment 323989 [details] [review] sharing: Don't print warning on call cancellation
Created attachment 323990 [details] [review] media-keys: Fix screensaver not locking in some cases If there was an XRandR call at the same time as we asked to lock the screen, it's possible that the screen lock would have got cancelled when it shouldn't.
Created attachment 323991 [details] [review] media-keys: Don't print warning on call cancellation
Created attachment 323992 [details] [review] media-keys: Fix checking for cancellation We really don't need to call g_cancellable_is_cancelled() when we could just check the error code we get back.
Created attachment 323993 [details] [review] media-keys: Fix possible crash in MPRIS support If the D-Bus proxy creation was cancelled, we might have a use-after-free problem here.
Created attachment 323994 [details] [review] datetime: Fix possible user-after-free when calls are cancelled 1) Don't use the possibly dangling user_data when the calls might have been cancelled 2) Don't print warnings when calls were cancelled 3) Don't print criticals for run-time warnings
Review of attachment 323987 [details] [review]: ++
Review of attachment 323988 [details] [review]: ++
Review of attachment 323989 [details] [review]: ++
Review of attachment 323991 [details] [review]: ++
Review of attachment 323992 [details] [review]: ++
Review of attachment 323993 [details] [review]: right
Review of attachment 323990 [details] [review]: I don't think what you describe is possible since ->cancellable is only cancelled on plugin stop and finalizing a cancellable as in on_xrandr_action_call_finished() doesn't cancel it. What could happen though is ->cancellable being NULL on do_lock_screen() and thus we'd never be able to cancel it. tl;dr the code is fine but the commit message needs to be changed :-)
Created attachment 324007 [details] [review] datetime: Fix possible use-after-free when calls are cancelled 1) Don't use the possibly dangling user_data when the calls might have been cancelled 2) Don't print warnings when calls were cancelled 3) Don't print criticals for run-time warnings
Created attachment 324008 [details] [review] media-keys: Fix screen lock call not being cancelled Because we used the wrong cancellable, it was possible for it not to exist and the call not being cancelled. As we only used the return value from the call, this shouldn't cause any crashes.
Review of attachment 323994 [details] [review]: right
Comment on attachment 324007 [details] [review] datetime: Fix possible use-after-free when calls are cancelled Moving the patch status from attachment 323994 [details] [review]
Comment on attachment 323994 [details] [review] datetime: Fix possible user-after-free when calls are cancelled Typo in the commit subject
Comment on attachment 324008 [details] [review] media-keys: Fix screen lock call not being cancelled Moving patch status from attachment 323990 [details] [review]
Comment on attachment 323990 [details] [review] media-keys: Fix screensaver not locking in some cases Newer version addresses comment 15
Attachment 323987 [details] pushed as fc0e8b6 - housekeeping: Don't print warning on call cancellation Attachment 323988 [details] pushed as 97a95f0 - housekeeping: Fix checking for cancellation Attachment 323989 [details] pushed as 86512f6 - sharing: Don't print warning on call cancellation Attachment 323991 [details] pushed as 86cbfda - media-keys: Don't print warning on call cancellation Attachment 323992 [details] pushed as 5147e6b - media-keys: Fix checking for cancellation Attachment 323993 [details] pushed as 645aa76 - media-keys: Fix possible crash in MPRIS support Attachment 324007 [details] pushed as 15e628d - datetime: Fix possible use-after-free when calls are cancelled Attachment 324008 [details] pushed as 214145c - media-keys: Fix screen lock call not being cancelled