TnyIdleStopper

[Murray Cumming <murrayc murrayc com>]

As discussed on IRC, tinymail has a problem with the 2 refresh callbacks
given to tny_folder_refresh_async(). The status callback is sometimes
called after the main callback, because tinymail uses different
g_idle_add_full() priorities.

This causes modest to crash when quickly clicking on different folders. 

This patch adds a generic TnyIdleStopper API that allows the main
callback to tell the status callback to stop. It uses a refcount to
decide when to release the shared gboolean (and the shared refcount). I
suspect that we should use it for other callbacks too, so I'd like to
put it in separeate .h/.c files.

I worry a bit that I am missing some simpler implementation but this API
at least looks quite straightforward to use. It seems to fix the crash
and the valgrind error.

-- 
Murray Cumming
murrayc murrayc com
www.murrayc.com
www.openismus.com

Index: libtinymail-camel/tny-camel-folder.c
===================================================================
--- libtinymail-camel/tny-camel-folder.c	(revision 1884)
+++ libtinymail-camel/tny-camel-folder.c	(working copy)
@@ -697,7 +697,143 @@
 	return;
 }
 
+typedef struct _TnyIdleStopper TnyIdleStopper;
 
+/** TnyIdleStopper:
+ * 
+ * This API can be used to allow 2 idle callbacks to cooperate, 
+ * so that one idle callback (for instance the main callback)
+ * can tell the other one (for instance a repeated status/progress callback)
+ * to stop.
+ * 
+ * This is necessary when the two idle callbacks have different priorities, 
+ * meaning that the status/progress callback might be called after the main 
+ * callback. The main callback would normally be expected to called last, and 
+ * may therefore be the point at which other shared objects are finally 
+ * released, meaning that their use in a further status/referesh callback might 
+ * access invalid or released memory.
+ * 
+ * Instantiate an TnyIdleStopper with tny_idle_stopper_new() and create a second shared 
+ * instance with tny_idle_stopper_copy(). Calling tny_idle_stopper_stopped() on one 
+ * instance will then cause tny_idle_stopper_is_stopped() to return TRUE for either 
+ * instance.
+ * Call tny_idle_stopper_destroy() on each instance when you are finished with it.
+ */
+struct _TnyIdleStopper
+{
+	/* This is a pointer to a gboolean so we can share the value with both 
+	 * idle callbacks, so that each callback can stop the the other from calling 
+	 * the callbacks. */
+	gboolean* stopped;
+	
+	/* This is a pointer an int so we can share a refcount,
+	 * so we can release the shared stopped and ref_count only 
+	 * when nobody else needs to check stopped. */
+	gint* refcount;
+};
+
+/** tny_idle_stopper_new:
+ * @returns: A new TnyIdleStopper instance. Release this with tny_idle_stopper_destroy().
+ * 
+ * Creates a new ref count object, with an initial ref count.
+ * Use tny_idle_stopper_copy() to create a second instance sharing 
+ * the same underlying stopped status.
+ * You must call tny_idle_stopper_stop() on one of these instances,
+ * and call tny_idle_stopper_destroy() on all instances.
+ */
+TnyIdleStopper* tny_idle_stopper_new()
+{
+	TnyIdleStopper *result = g_new0(TnyIdleStopper, 1);
+	
+	result->stopped = g_malloc0(sizeof(gboolean));
+	*(result->stopped) = FALSE;
+	
+	result->refcount = g_malloc0(sizeof(gint));
+	*(result->refcount) = 1;
+	
+	return result;
+}
+
+/** tny_idle_stopper_copy
+ * @stopper: The TnyIdleStopper instance to be shared.
+ * 
+ * Create a shared copy of the stopper, 
+ * so that you can call tny_idle_stopper_stop() on one instance, 
+ * so that tny_idle_stopper_is_stopped() returns TRUE for ther other instance.
+ */
+TnyIdleStopper* tny_idle_stopper_copy (TnyIdleStopper *stopper)
+{
+	g_return_val_if_fail (stopper, NULL);
+	g_return_val_if_fail (stopper->refcount, NULL);
+	
+	TnyIdleStopper *result = g_new0(TnyIdleStopper, 1);
+	
+	/* Share the gboolean: */
+	result->stopped = stopper->stopped;
+	
+	result->refcount = stopper->refcount;
+	++(*(result->refcount));
+	
+	return result;
+}
+
+/** tny_idle_stopper_stop:
+ * @stopper: The TnyIdleStopper instance.
+ * 
+ * Call this to make tny_idle_stopper_is_stopped() return TRUE for all 
+ * instances of the stopper.
+ */
+void tny_idle_stopper_stop (TnyIdleStopper *stopper)
+{
+	g_return_if_fail (stopper);
+	g_return_if_fail(stopper->stopped);
+	g_return_if_fail (stopper->refcount);
+	
+	*(stopper->stopped) = TRUE;
+}
+
+/** tny_idle_stopper_destroy:
+ * @stopper: The TnyIdleStopper instance.
+ * 
+ * Call this when you are sure that the callback will never be called again.
+ * For instance, in your GDestroyNotify callback provided to g_idle_add_full().
+ * Do not attempt to use @stopper after calling this.
+ */
+void tny_idle_stopper_destroy(TnyIdleStopper *stopper)
+{
+	g_return_if_fail (stopper);
+		
+	--(*(stopper->refcount));
+	
+	/* Free the shared variables if this is the last destroy: */
+	if(*(stopper->refcount) == 0) {
+		g_free (stopper->refcount);
+		stopper->refcount = 0;
+		
+		g_free (stopper->stopped);
+		stopper->stopped = 0;
+	}
+	
+	g_free (stopper);
+}
+
+/* tny_idle_stopper_is_stopped:
+ * @stopper: The TnyIdleStopper instance.
+ * @returns: Whether tny_idle_stopper_stop() has been called on one of the shared instances.
+ * 
+ * This returns TRUE is tny_idle_stopper_stop() was called 
+ * one one of the shared instances.
+ */
+gboolean tny_idle_stopper_is_stopped(TnyIdleStopper* stopper) 
+{
+	g_return_val_if_fail(stopper, FALSE);
+	g_return_val_if_fail(stopper->stopped, FALSE);
+	
+	/* If tny_idle_stopper_stop() was called, 
+	 * then pointer was freed and set to NULL: */
+	return *(stopper->stopped);
+}
+
 typedef struct 
 {
 	TnyFolder *self;
@@ -705,13 +841,20 @@
 	TnyStatusCallback status_callback;
 	gpointer user_data;
 	gboolean cancelled;
+	
+	/* This stops us from calling a status callback after the operation has 
+	 * finished. */
+	TnyIdleStopper* stopper;
+	
 	guint depth;
 	GError *err;
 	TnySessionCamel *session;
 } RefreshFolderInfo;
 
 
-
+/** This is the GDestroyNotify callback provided to g_idle_add_full()
+ * for tny_camel_folder_refresh_async_callback().
+ */
 static void
 tny_camel_folder_refresh_async_destroyer (gpointer thr_user_data)
 {
@@ -728,6 +871,9 @@
 
 	_tny_session_stop_operation (info->session);
 
+	tny_idle_stopper_destroy (info->stopper);
+	info->stopper = NULL;
+	
 	g_slice_free (RefreshFolderInfo, thr_user_data);
 
 	return;
@@ -744,6 +890,11 @@
 	if (info->callback)
 		info->callback (info->self, info->cancelled, &info->err, info->user_data);
 
+	/* Prevent status callbacks from being called after this
+	 * (can happen because the 2 idle callbacks have different priorities)
+	 * by causing tny_idle_stopper_is_stopped() to return TRUE. */
+	tny_idle_stopper_stop (info->stopper);
+	
 	tny_folder_change_set_new_all_count (change, priv->cached_length);
 	tny_folder_change_set_new_unread_count (change, priv->unread_length);
 	notify_folder_observers_about (self, change);
@@ -771,6 +922,9 @@
 	g_object_unref (G_OBJECT (info->minfo->self));
 	g_free (info->what);
 
+	tny_idle_stopper_destroy (info->minfo->stopper);
+	info->minfo->stopper = NULL;
+    
 	g_slice_free (RefreshFolderInfo, info->minfo);
 	g_slice_free (ProgressInfo, data);
 
@@ -782,7 +936,13 @@
 {
 	ProgressInfo *info = data;
 	RefreshFolderInfo *minfo = info->minfo;
-
+	
+	/* Do not call the status callback after the main callback 
+	 * has already been called, because we should assume that 
+	 * the user_data is invalid after that time: */
+	if (tny_idle_stopper_is_stopped (minfo->stopper))
+		return FALSE;
+	
 	if (minfo && minfo->status_callback)
 	{
 		TnyStatus *status = tny_status_new (TNY_FOLDER_STATUS, 
@@ -804,7 +964,7 @@
  *
  * This is non-public API documentation
  *
- * The reason why we need to copy thist stuff is because it seems Camel has some
+ * The reason why we need to copy this stuff is because it seems Camel has some
  * of its stuff allocated on the stack. When you do g_idle tricks, it's possible
  * that by the time the g_idle happens, the stack allocation is already killed.
  *
@@ -829,6 +989,11 @@
 	info->minfo->status_callback = oinfo->status_callback;
 	info->minfo->user_data = oinfo->user_data;
 	info->oftotal = oftotal;
+	
+	/* Share the TnyIdleStopper, so one callback can tell the other to stop,
+	 * because they may be called in an unexpected sequence: */
+	/* This is destroyed in the idle GDestroyNotify callback. */
+	info->minfo->stopper = tny_idle_stopper_copy(oinfo->stopper);
 
 	if (info->oftotal < 1)
 		info->oftotal = 1;
@@ -969,6 +1134,7 @@
 		return;
 	}
 
+	/* Idle info for the status callback: */
 	info = g_slice_new (RefreshFolderInfo);
 	info->session = TNY_FOLDER_PRIV_GET_SESSION (priv);
 	info->err = NULL;
@@ -977,14 +1143,22 @@
 	info->status_callback = status_callback;
 	info->user_data = user_data;
 	info->depth = g_main_depth ();
+	
+	/* Use a ref count because we do not know which of the 2 idle callbacks 
+	 * will be the last, and we can only unref self in the last callback:
+	 * This is destroyed in the idle GDestroyNotify callback.
+	 */
+	info->stopper = tny_idle_stopper_new();
 
 	/* thread reference */
 	g_object_ref (G_OBJECT (self));
 	_tny_camel_folder_reason (priv);
 
+	/* This will cause the idle status callback to be called,
+	 * via _tny_camel_account_start_camel_operation,
+	 * and also calls the idle main callback: */
 	thread = g_thread_create (tny_camel_folder_refresh_async_thread,
 			info, FALSE, NULL);
-
 	return;
 }
 
Index: libtinymail/tny-folder.c
===================================================================
--- libtinymail/tny-folder.c	(revision 1884)
+++ libtinymail/tny-folder.c	(working copy)
@@ -529,8 +529,8 @@
 /**
  * tny_folder_refresh_async:
  * @self: a TnyFolder object
- * @callback: The callback handler
- * @status_callback: A callback for status notifications
+ * @callback: The callback handler. This is called last.
+ * @status_callback: A callback for status notifications. This is never called after @callback.
  * @user_data: user data for the callback
  *
  * Refresh @self and call back when finished. This gets the summary information