Merge lp:~mardy/gnome-control-center-signon/lp1122520 into lp:gnome-control-center-signon

[Alberto Mardegan]

Don't crash when cancelling the authentication or disposing the plugin

This fixes a couple of crashes which would occur:

1) when cancelling an ongoing authentication by pressing the "Cancel" button: this is fixed by tracking the state of the asynchronous operations and delaying the emission of the "finished" signal until they have all completed.

2) when going back to the System Settings: this operation would destroy the plugin, but the asynchronous operations would still continue and the callbacks would be invoked later on an invalid pointer to "self" (see also https://bugzilla.gnome.org/show_bug.cgi?id=696369). This is fixed by guarding the "self" pointer with a weak GObject pointer.

Don't crash when cancelling the authentication or disposing the plugin

This fixes a couple of crashes which would occur:

1) when cancelling an ongoing authentication by pressing the "Cancel" button: this is fixed by tracking the state of the asynchronous operations and delaying the emission of the "finished" signal until they have all completed.

2) when going back to the System Settings: this operation would destroy the plugin, but the asynchronous operations would still continue and the callbacks would be invoked later on an invalid pointer to "self" (see also https://bugzilla.gnome.org/show_bug.cgi?id=696369). This is fixed by guarding the "self" pointer with a weak GObject pointer.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:143
http://s-jenkins:8080/job/gnome-control-center-signon-ci/40/
Executed test runs:
    SUCCESS: http://s-jenkins:8080/job/gnome-control-center-signon-ci/./build=pbuilder,distribution=raring,flavor=amd64/40
    SUCCESS: http://s-jenkins:8080/job/gnome-control-center-signon-ci/./build=pbuilder,distribution=raring,flavor=i386/40

Click here to trigger a rebuild:
http://s-jenkins:8080/job/gnome-control-center-signon-ci/40/rebuild

review: Approve (continuous-integration)

[David King (amigadave)]

I have a few reservations about this approach. This situation is broadly covered by an email that Alexander Larsson wrote about how to cancel asynchronous GIO operations:

https://mail.gnome.org/archives/gtk-devel-list/2010-April/msg00116.html

There is some good explanation there (and earlier in the thread) about this sort of occurrence which tends to support an approach of delaying finalization until all asynchronous operations owned by an object are finished.

Specifically, my concern with this approach is that the asynchronous completion callback will always be called, and therefore guarding access to the "self" pointer inside the callbacks is bogus. To put it another way, g_return_if_fail() is supposed to represent a programming error, and this change introduces an error in the case of going back to All Settings in System Settings. :-)

I think this is a neat approach for solving (1) in your description, but I do not think it is appropriate for solving (2). There, I think that my approach of delaying dispose/finalize is a better option.

[Alberto Mardegan (mardy)]

I'm afraid that this might turn into a philosophical discussion :-)

First, I think we need to recognize that this is a hack around what IMHO is a wrong practice yet relatively common in GLib: continuing keeping an object alive (and kicking at callbacks!) after all explicit references to it have been dropped.

I agree with most of what Alex was saying in that thread; I actually even considered the idea of running the main loop in the dispose method, but quickly discarded it. :-)

However, your criticism seems to be based on a misunderstanding: the g_return_if_fail() is used only as a non-essential extra check, just for safety. The line above it actually checks whether the pointer is NULL (and this is what happens if the object has been destroyed), so that g_return_if_fail() will never fail under normal usage (if you actually saw it emitting a critical message on the terminal, then it means that my solution is buggy).

So, what this fix does is to set that pointer to NULL if the object has been disposed, so that all callbacks happening after it don't perform any action.

However, I realized just now that this would introduce a memory leak. I'll get it fixed soon :-)

[lp:~mardy/gnome-control-center-signon/lp1122520]

144. By Alberto Mardegan on 2013-03-22

Don't leak errors and GSimpleAsync results

Remove the macros, to make the code more explicit.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:144
http://s-jenkins:8080/job/gnome-control-center-signon-ci/41/
Executed test runs:
    SUCCESS: http://s-jenkins:8080/job/gnome-control-center-signon-ci/./build=pbuilder,distribution=raring,flavor=amd64/41
    SUCCESS: http://s-jenkins:8080/job/gnome-control-center-signon-ci/./build=pbuilder,distribution=raring,flavor=i386/41

Click here to trigger a rebuild:
http://s-jenkins:8080/job/gnome-control-center-signon-ci/41/rebuild

review: Approve (continuous-integration)

[David King (amigadave)]

> I'm afraid that this might turn into a philosophical discussion :-)

Sounds interesting. :-)

> First, I think we need to recognize that this is a hack around what IMHO is a
> wrong practice yet relatively common in GLib: continuing keeping an object
> alive (and kicking at callbacks!) after all explicit references to it have
> been dropped.

This is true, but with one caveat: "Object methods should be able to run without program error in-between [dispose and finalize]." https://developer.gnome.org/gobject/stable/gobject-memory.html#gobject-memory-cycles

> I agree with most of what Alex was saying in that thread; I actually even
> considered the idea of running the main loop in the dispose method, but
> quickly discarded it. :-)
>
> However, your criticism seems to be based on a misunderstanding: the
> g_return_if_fail() is used only as a non-essential extra check, just for
> safety.

My mistake, thanks for pointing it out.

> So, what this fix does is to set that pointer to NULL if the object has been
> disposed, so that all callbacks happening after it don't perform any action.

Indeed, and this is the part of the approach that I disagree with. Admittedly, the object method will not access invalid memory, but checking whether the object is valid in a "private" method is a symptom to me that something is wrong. A neater solution might be for the asynchronous operations to own (or hold a reference to) everything required to complete the operation, or one of the other solutions mentioned in the mailing list thread.

[lp:~mardy/gnome-control-center-signon/lp1122520]

145. By Alberto Mardegan on 2013-03-22

Don't use weak pointers, but check on the error type

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:145
http://s-jenkins:8080/job/gnome-control-center-signon-ci/42/
Executed test runs:
    SUCCESS: http://s-jenkins:8080/job/gnome-control-center-signon-ci/./build=pbuilder,distribution=raring,flavor=amd64/42
    SUCCESS: http://s-jenkins:8080/job/gnome-control-center-signon-ci/./build=pbuilder,distribution=raring,flavor=i386/42

Click here to trigger a rebuild:
http://s-jenkins:8080/job/gnome-control-center-signon-ci/42/rebuild

review: Approve (continuous-integration)

[David King (amigadave)]

Much nicer!

review: Approve

=== modified file 'libaccount-plugin/oauth-plugin.c'
--- libaccount-plugin/oauth-plugin.c	2013-02-25 12:03:41 +0000
+++ libaccount-plugin/oauth-plugin.c	2013-03-22 13:53:19 +0000
@@ -62,6 +62,8 @@
     GCancellable *cancellable;
     gboolean identity_stored;
     gboolean deleting_identity;
+    gboolean authenticating;
+    gboolean storing_account;
 };
 
 static const gchar signon_id[] = AP_PLUGIN_CREDENTIALS_ID_FIELD;
@@ -136,25 +138,34 @@
 {
     ApOAuthPluginPrivate *priv = self->priv;
 
-    if (!priv->deleting_identity)
-    {
-        g_idle_add ((GSourceFunc)emit_finished, self);
-    }
+    /* Check if some asynchronous operations are still running */
+    if (priv->authenticating) return;
+    if (priv->storing_account) return;
+    if (priv->deleting_identity) return;
+
+    g_idle_add ((GSourceFunc)emit_finished, self);
 }
 
 static void
 identity_removed_cb (SignonIdentity *identity, const GError *error,
                      gpointer user_data)
 {
-    ApOAuthPlugin *self = AP_OAUTH_PLUGIN (user_data);
+    ApOAuthPlugin *self;
 
     if (G_UNLIKELY (error != NULL))
     {
+        if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+        {
+            return;
+        }
         g_critical ("Error removing identity: %s", error->message);
         /* No special handling, we are quitting anyways */
     }
 
+    self = AP_OAUTH_PLUGIN (user_data);
+
     self->priv->deleting_identity = FALSE;
+    self->priv->identity_stored = FALSE;
     finish_if_ready (self);
 }
 
@@ -169,7 +180,8 @@
     }
 
     if (priv->identity != NULL &&
-        priv->identity_stored)
+        priv->identity_stored &&
+        !priv->deleting_identity)
     {
         priv->deleting_identity = TRUE;
         signon_identity_remove (priv->identity, identity_removed_cb, self);
@@ -196,10 +208,19 @@
 account_store_cb (GObject *source_object, GAsyncResult *res,
                   gpointer user_data)
 {
-    ApOAuthPlugin *self = AP_OAUTH_PLUGIN (user_data);
+    ApOAuthPlugin *self;
     GError *error = NULL;
 
     ag_account_store_finish (AG_ACCOUNT (source_object), res, &error);
+    if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+    {
+        g_error_free (error);
+        return;
+    }
+
+    self = AP_OAUTH_PLUGIN (user_data);
+    self->priv->storing_account = FALSE;
+
     if (G_UNLIKELY (error != NULL))
     {
         g_critical ("Account write error: %s", error->message);
@@ -242,7 +263,6 @@
     ApOAuthPluginPrivate *priv = self->priv;
     GHashTableIter iter;
     gpointer key, value;
-    GHashTable *parameters;
     gchar *full_key;
     const gchar *mechanism;
     const gchar *username;
@@ -286,6 +306,7 @@
     account = ap_plugin_get_account ((ApPlugin *)self);
     store_authentication_parameters (self, account);
 
+    self->priv->storing_account = TRUE;
     ag_account_store_async (account, self->priv->cancellable,
                             account_store_cb, self);
 }
@@ -329,7 +350,7 @@
                          GAsyncResult *res,
                          gpointer user_data)
 {
-    ApOAuthPlugin *self = AP_OAUTH_PLUGIN (user_data);
+    ApOAuthPlugin *self;
     SignonAuthSession *auth_session = SIGNON_AUTH_SESSION (source_object);
     AgAccount *account;
     GVariant *session_data;
@@ -344,6 +365,15 @@
         g_variant_unref (session_data);
     }
 
+    if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+    {
+        g_error_free (error);
+        return;
+    }
+
+    self = AP_OAUTH_PLUGIN (user_data);
+    self->priv->authenticating = FALSE;
+
     if (G_UNLIKELY (error != NULL))
     {
         if (error->domain == SIGNON_ERROR &&
@@ -473,10 +503,12 @@
         g_clear_error (&error);
         return;
     }
+    priv->authenticating = TRUE;
     signon_auth_session_process_async (priv->auth_session, session_data,
                                        get_mechanism (priv),
                                        priv->cancellable,
-                                       auth_session_process_cb, self);
+                                       auth_session_process_cb,
+                                       self);
 }
 
 static void

=== modified file 'libaccount-plugin/plugin.c'
--- libaccount-plugin/plugin.c	2013-01-24 12:09:04 +0000
+++ libaccount-plugin/plugin.c	2013-03-22 13:53:19 +0000
@@ -267,7 +267,6 @@
 {
     ApPluginPrivate *priv = self->priv;
     GVariant *v_id;
-    AgSettingSource source;
     GSimpleAsyncResult *result;
     gboolean deleting_identity = FALSE;