Merge branch 'cryptogopher/xfconf-issue16'

5 files changed, 84 insertions, 20 deletions

diff --git a/configure.ac.in b/configure.ac.in
index 4610dc6..4cbcd75 100644
--- a/configure.ac.in
+++ b/configure.ac.in
@@ -230,6 +230,7 @@ tests/get-properties/Makefile
 tests/reset-properties/Makefile
 tests/object-bindings/Makefile
 tests/property-changed-signal/Makefile
+tests/others/Makefile
 tests/tests-end/Makefile
 xfconf/Makefile
 xfconf/libxfconf-0.pc
diff --git a/tests/Makefile.am b/tests/Makefile.am
index f1b06c0..96ed2a8 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -6,6 +6,7 @@ SUBDIRS = \
 	reset-properties \
 	property-changed-signal \
 	object-bindings \
+	others \
 	tests-end
 #	list-channels
 
diff --git a/tests/others/Makefile.am b/tests/others/Makefile.am
new file mode 100644
index 0000000..2136eab
--- /dev/null
+++ b/tests/others/Makefile.am
@@ -0,0 +1,6 @@
+check_PROGRAMS = \
+	t-issue-16
+
+t_issue_16_SOURCES = t-issue-16.c
+
+include $(top_srcdir)/tests/Makefile.inc
diff --git a/tests/others/t-issue-16.c b/tests/others/t-issue-16.c
new file mode 100644
index 0000000..c7473f3
--- /dev/null
+++ b/tests/others/t-issue-16.c
@@ -0,0 +1,46 @@
+/*
+ *  xfconf
+ *
+ *  Copyright (c) 2020 cryptogopher
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License ONLY.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU Library General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "tests-common.h"
+
+int
+main(int argc, char **argv)
+{
+  XfconfChannel *channel;
+
+  if(!xfconf_tests_start())
+    return 1;
+
+  /* Generate at least one asynchronous set_property call to trigger
+   * asynchronous callback execution. Because removal of the channel directly
+   * follows and callback will only be invoked after control returns to main
+   * loop - callback will be executed with dangling pointer to
+   * XfconfCacheOldItem. */
+  channel = xfconf_channel_new(TEST_CHANNEL_NAME);
+  xfconf_channel_reset_property (channel, test_int_property, FALSE);
+  TEST_OPERATION(xfconf_channel_set_int(channel, test_int_property, test_int));
+  g_object_unref(G_OBJECT(channel));
+
+  while (g_main_context_pending(NULL))
+    g_main_context_iteration(NULL, FALSE);
+
+  xfconf_tests_end();
+
+  return 0;
+}
diff --git a/xfconf/xfconf-cache.c b/xfconf/xfconf-cache.c
index ca012f6..38d408e 100644
--- a/xfconf/xfconf-cache.c
+++ b/xfconf/xfconf-cache.c
@@ -398,8 +398,7 @@ xfconf_cache_init(XfconfCache *cache)
                                         (GDestroyNotify)xfconf_cache_item_free);
 
     cache->pending_calls = g_hash_table_new_full(g_direct_hash, g_direct_equal,
-                                                 NULL,
-                                                 (GDestroyNotify)xfconf_cache_old_item_free);
+                                                 NULL, NULL);
     cache->old_properties = g_hash_table_new_full(g_str_hash, g_str_equal,
                                                   NULL, NULL);
 
@@ -469,20 +468,25 @@ static void
 xfconf_cache_finalize(GObject *obj)
 {
     XfconfCache *cache = XFCONF_CACHE(obj);
-    GHashTable *pending_calls;
     GDBusProxy *proxy;
 
     proxy = _xfconf_get_gdbus_proxy();
 
     g_signal_handler_disconnect(proxy,cache->g_signal_id);
 
-    /* finish pending calls (without emitting signals, therefore we set
-     * the hash table in the cache to %NULL) */
-    pending_calls = cache->pending_calls;
-    cache->pending_calls = NULL;
-    g_hash_table_foreach_remove(pending_calls, xfconf_cache_old_item_end_call,
+    /* Finish pending calls with synchronous requests (without emitting
+     * signals, therefore we cancel the cancellable on old_item).
+     * Beware: even that we cancel cancellable objects for unfinished
+     * asynchronous calls, their handlers are guaranted to be run in the
+     * thread-default main context after we finish (i.e. after XfconfCache
+     * will be freed). Due to that, we must not free - outside of handler
+     * itself - the XfconfCacheOldItems provided as user_data to those
+     * handlers. Otherwise the handler will have no realiable way of
+     * knowing that call has been cancelled and will operate on freed data. */
+    g_hash_table_foreach_remove(cache->pending_calls,
+                                xfconf_cache_old_item_end_call,
                                 cache->channel_name);
-    g_hash_table_unref(pending_calls);
+    g_hash_table_unref(cache->pending_calls);
 
     g_free(cache->channel_name);
 
@@ -597,17 +601,26 @@ xfconf_cache_set_property_reply_handler(GDBusProxy *proxy,
                                         gpointer user_data)
 {
     XfconfCache *cache;
-    XfconfCacheOldItem *old_item = NULL;
+    XfconfCacheOldItem *old_item = (XfconfCacheOldItem*) user_data;
     XfconfCacheItem *item;
     GError *error = NULL;
     gboolean result;
-    old_item = (XfconfCacheOldItem *) user_data;
-    cache = old_item->cache;
 
     old_item->pending_calls_count--;
     if(old_item->pending_calls_count > 0)
         return;
 
+    /* cancellable is cancelled in xfconf_cache_old_item_end_call to inform that
+     * XconfCache finalization started. That means the last value of
+     * property has been set synchronously, invalidating the need to run this
+     * handler for any previously started, unfinished asynchronous calls. */
+    if (g_cancellable_is_cancelled(old_item->cancellable) == TRUE)
+    {
+        xfconf_cache_old_item_free(old_item);
+        return;
+    }
+
+    cache = old_item->cache;
     xfconf_cache_mutex_lock(cache);
 /*
     old_item = g_hash_table_lookup(cache->pending_calls, call);
@@ -619,8 +632,7 @@ xfconf_cache_set_property_reply_handler(GDBusProxy *proxy,
     }
 */
     g_hash_table_remove(cache->old_properties, old_item->property);
-    /* don't destroy old_item yet */
-    g_hash_table_steal(cache->pending_calls, old_item->cancellable);
+    g_hash_table_remove(cache->pending_calls, old_item->cancellable);
     item = g_tree_lookup(cache->properties, old_item->property);
     if(G_UNLIKELY(!item)) {
 #ifndef NDEBUG
@@ -653,9 +665,7 @@ xfconf_cache_set_property_reply_handler(GDBusProxy *proxy,
 
     /* we handled the call */
     g_cancellable_cancel(old_item->cancellable);
-
-    if(old_item)
-        xfconf_cache_old_item_free(old_item);
+    xfconf_cache_old_item_free(old_item);
 out:
     xfconf_cache_mutex_unlock(cache);
 }
@@ -880,11 +890,11 @@ xfconf_cache_set(XfconfCache *cache,
          * call hasn't returned yet.  let's cancel that call and
          * throw away the current not-yet-committed value of
          * the property.
-         * we also steal the old_item from the pending_calls table
-         * so there are no pending item left. */
+         * we also remove the old_item from the pending_calls table
+         * so there is no pending item left. */
         if(!g_cancellable_is_cancelled (old_item->cancellable)) {
             g_cancellable_cancel(old_item->cancellable);
-            g_hash_table_steal(cache->pending_calls, old_item->cancellable);
+            g_hash_table_remove(cache->pending_calls, old_item->cancellable);
             g_object_unref (old_item->cancellable);
             old_item->cancellable = g_cancellable_new();
         }