GNOME Bugzilla – Bug 763382
Improper async handling
Last modified: 2016-03-15 23:42:14 UTC
Reported as a crash here: https://bugzilla.redhat.com/show_bug.cgi?id=1288850 My take: Just briefly looking at the code, gsd_color_manager_start calls gsd_color_state_start, which starts all sort of async stuff, while operating on a borrowed reference to the state object that was passed in from the color manager. gsd_color_manager_stop does nothing to interrupt the async stuff that might be happening with the state object and finalize throws away the reference to the state object. That seems like an obvious opportunity for the async machinery to operate on a dead state object. Not saying this is what is happening here, but... A cancellable might be whats needed here.
Created attachment 323967 [details] [review] adds cancellables
Review of attachment 323967 [details] [review]: The code after this will still be crashy. If the objects you're manipulating are finalized when you're receiving the async result, you can't access that object as user_data. For example: Start the async: cd_client_find_profile_by_filename (profiles->priv->client, cd_icc_get_filename (icc), NULL, gcm_session_find_profile_by_filename_cb, profiles); ->finalize is called, the cancellable is cancelled, the object is freed. static void gcm_session_find_profile_by_filename_cb (GObject *object, GAsyncResult *res, gpointer user_data) { GError *error = NULL; CdProfile *profile; CdClient *client = CD_CLIENT (object); GsdColorProfiles *profiles = GSD_COLOR_PROFILES (user_data); Here the user_data might be a dangling pointer, you should only access it after checking that the async wasn't cancelled. profile = cd_client_find_profile_by_filename_finish (client, res, &error); if (profile == NULL) { g_warning ("%s", error->message); Don't print an error message if the call was cancelled either. There are a bunch of such errors in the color plugin. gcm_session_client_connect_cb() also pokes at memory that could be freed (and there's a few more elsewhere). ::: plugins/color/gsd-color-manager.c @@ +87,1 @@ gsd_color_manager_stop (GsdColorManager *manager) _stop() is supposed to undo everything that was done in _start(). So both your gsd_color_*_start() calls should be cancelled here (which you're doing) and the ->cancellable unref'ed. You'd create a new one in _start(). ::: plugins/color/gsd-color-profiles.c @@ +93,3 @@ GError **error) { + g_set_object (&profiles->priv->cancellable, cancellable); I wouldn't take a reference here, simply borrow the existing one and pass the pointer. @@ +255,3 @@ profiles = GSD_COLOR_PROFILES (object); + g_clear_object (&profiles->priv->cancellable); Which would remove the need to take care of the cancellable's lifecycle. ::: plugins/color/gsd-color-state.c @@ +1377,3 @@ { /* coldplug the list of screens */ + g_set_object (&state->priv->cancellable, cancellable); Ditto.
Created attachment 324010 [details] [review] adds cancellables
Created attachment 324011 [details] [review] no warning on cancel
Created attachment 324012 [details] [review] fixes crash
Created attachment 324014 [details] [review] fixes crash #2
Review of attachment 324010 [details] [review]: ::: plugins/color/gsd-color-profiles.c @@ +94,3 @@ + GsdColorProfilesPrivate *priv = profiles->priv; + + /* use a fresh cancellable for each start->stop operation */ Maybe you should cancel the outstanding operations as well? @@ +266,3 @@ profiles = GSD_COLOR_PROFILES (object); + g_clear_object (&profiles->priv->cancellable); You need to cancel the cancellable as well (so either call _stop() or add the one-liner here. ::: plugins/color/gsd-color-state.c @@ +1378,3 @@ + GsdColorStatePrivate *priv = state->priv; + + /* use a fresh cancellable for each start->stop operation */ Ditto about cancelling before starting anew. @@ +1445,3 @@ state = GSD_COLOR_STATE (object); + g_clear_object (&state->priv->cancellable); Same as for the other file, you're not cancelling on finalize().
Review of attachment 324011 [details] [review]: Rest looks good. ::: plugins/color/gsd-color-state.c @@ +1213,3 @@ for (i = 0; outputs[i] != NULL; i++) { /* get CdDevice for this output */ + cd_client_find_device_by_property (priv->client, Unrelated change here.
Review of attachment 324012 [details] [review]: Looks good.
Review of attachment 324014 [details] [review]: Yep. Might be worth filing a bug against gnome-rr to have the API changed though.
Created attachment 324028 [details] [review] adds cancellables
Created attachment 324029 [details] [review] no warning on cancel
And cherry-picked into gnome-3-18 as well.