brasero r1219 - in trunk: . src

[philippr svn gnome org]

Author: philippr
Date: Sun Aug 31 11:42:10 2008
New Revision: 1219
URL: http://svn.gnome.org/viewvc/brasero?rev=1219&view=rev

Log:
	Fix race condition in brasero-io

	* src/brasero-async-task-manager.c
	(brasero_async_task_manager_thread),
	(brasero_async_task_manager_foreach_active_remove),
	(brasero_async_task_manager_foreach_unprocessed_remove):
	* src/brasero-async-task-manager.h:
	* src/brasero-io.c (brasero_io_unref_result_callback_data),
	(brasero_io_return_result), (brasero_io_set_job),
	(brasero_io_job_free), (brasero_io_job_destroy),
	(brasero_io_get_file_count_destroy),
	(brasero_io_load_directory_destroy),
	(brasero_io_load_directory_thread), (brasero_io_xfer_destroy),
	(brasero_io_free_async_queue):


Modified:
   trunk/ChangeLog
   trunk/src/brasero-async-task-manager.c
   trunk/src/brasero-async-task-manager.h
   trunk/src/brasero-io.c

Modified: trunk/src/brasero-async-task-manager.c
==============================================================================
--- trunk/src/brasero-async-task-manager.c	(original)
+++ trunk/src/brasero-async-task-manager.c	Sun Aug 31 11:42:10 2008
@@ -265,32 +265,31 @@
 		self->priv->active_tasks = g_slist_remove (self->priv->active_tasks, ctx);
 		g_cond_signal (self->priv->task_finished);
 
-		if (g_cancellable_is_cancelled (cancel)) {
-			if (ctx->type->destroy)
-				ctx->type->destroy (self, ctx->data);
-
-			g_free (ctx);
-		}
-		else if (res == BRASERO_ASYNC_TASK_RESCHEDULE) {
-			if (self->priv->waiting_tasks) {
-				BraseroAsyncTaskCtx *next;
-
-				next = self->priv->waiting_tasks->data;
-				if (next->priority > ctx->priority)
-					brasero_async_task_manager_insert_task (self, ctx);
+		/* NOTE: when threads are cancelled then they are destroyed in
+		 * the function that cancelled them to destroy callback_data in
+		 * the active main loop */
+		if (!g_cancellable_is_cancelled (cancel)) {
+			if (res == BRASERO_ASYNC_TASK_RESCHEDULE) {
+				if (self->priv->waiting_tasks) {
+					BraseroAsyncTaskCtx *next;
+
+					next = self->priv->waiting_tasks->data;
+					if (next->priority > ctx->priority)
+						brasero_async_task_manager_insert_task (self, ctx);
+					else
+						self->priv->waiting_tasks = g_slist_prepend (self->priv->waiting_tasks, ctx);
+				}
 				else
 					self->priv->waiting_tasks = g_slist_prepend (self->priv->waiting_tasks, ctx);
 			}
-			else
-				self->priv->waiting_tasks = g_slist_prepend (self->priv->waiting_tasks, ctx);
-		}
-		else {
-			if (ctx->type->destroy)
-				ctx->type->destroy (self, ctx->data);
-			g_free (ctx);
+			else {
+				if (ctx->type->destroy)
+					ctx->type->destroy (self, FALSE, ctx->data);
+				g_free (ctx);
+			}
 		}
-
-		g_cancellable_reset (cancel);
+		else
+			g_cancellable_reset (cancel);
 	}
 
 end:
@@ -423,8 +422,11 @@
 
 			tasks = g_slist_remove (tasks, ctx);
 
-			/* NOTE: no need to call destroy callback here 
-			 * since it was done in the thread loop. */
+			/* destroy it */
+			if (ctx->type->destroy)
+				ctx->type->destroy (self, TRUE, ctx->data);
+
+			g_free (ctx);
 		}
 	}
 
@@ -455,7 +457,7 @@
 
 			/* call the destroy callback */
 			if (ctx->type->destroy)
-				ctx->type->destroy (self, ctx->data);
+				ctx->type->destroy (self, TRUE, ctx->data);
 
 			g_free (ctx);
 		}

Modified: trunk/src/brasero-async-task-manager.h
==============================================================================
--- trunk/src/brasero-async-task-manager.h	(original)
+++ trunk/src/brasero-async-task-manager.h	Sun Aug 31 11:42:10 2008
@@ -63,6 +63,7 @@
 								 GCancellable *cancel,
 								 gpointer user_data);
 typedef void			(*BraseroAsyncDestroy)		(BraseroAsyncTaskManager *manager,
+								 gboolean cancelled,
 								 gpointer user_data);
 typedef gboolean		(*BraseroAsyncFindTask)		(BraseroAsyncTaskManager *manager,
 								 gpointer task,

Modified: trunk/src/brasero-io.c
==============================================================================
--- trunk/src/brasero-io.c	(original)
+++ trunk/src/brasero-io.c	Sun Aug 31 11:42:10 2008
@@ -77,7 +77,7 @@
 
 struct _BraseroIOResultCallbackData {
 	gpointer callback_data;
-	guint ref;
+	gint ref;
 };
 typedef struct _BraseroIOResultCallbackData BraseroIOResultCallbackData;
 
@@ -251,8 +251,8 @@
 	if (!data)
 		return;
 
-	data->ref --;
-	if (data->ref > 0)
+	/* see atomically if we are the last to hold a lock */
+	if (!g_atomic_int_dec_and_test (&data->ref))
 		return;
 
 	if (destroy)
@@ -277,41 +277,6 @@
 	g_free (result);
 }
 
-static void
-brasero_io_job_free (BraseroIOJob *job)
-{
-	/* NOTE: the callback_data member is never destroyed here since it would
-	 * be destroyed in a thread different from the main loop.
-	 * Either it's destroyed in the thread that called brasero_io_cancel ()
-	 * or after all results are returned (and therefore in main loop).
-	 * As a special case, some jobs like read directory contents have to
-	 * return a dummy result to destroy the callback_data if the directory
-	 * is empty. */
-
-	if (job->callback_data)
-		job->callback_data->ref --;
-
-	g_free (job->uri);
-	g_free (job);
-}
-
-static void
-brasero_io_job_destroy (BraseroAsyncTaskManager *self,
-			gpointer callback_data)
-{
-	BraseroIOJob *job = callback_data;
-
-	/* If a job is destroyed we don't call the destroy callback since it
-	 * otherwise it would be called in a different thread. All object that
-	 * cancel io ops are doing it either in destroy () and therefore handle
-	 * all destruction for callback_data or if they don't they usually don't
-	 * pass any callback data anyway. */
-	/* NOTE: usually threads are cancelled from the main thread/loop and
-	 * block until the active task is removed which means that if we called
-	 * the destroy () then the destruction would be done in the main loop */
-	brasero_io_job_free (job);
-}
-
 /**
  * Used to return the results
  */
@@ -392,8 +357,8 @@
 	result->uri = g_strdup (uri);
 
 	if (callback_data) {
+		g_atomic_int_inc (&callback_data->ref);
 		result->callback_data = callback_data;
-		callback_data->ref ++;
 	}
 
 	brasero_io_queue_result (self, result);
@@ -416,7 +381,7 @@
 	job->callback_data = callback_data;
 
 	if (callback_data)
-		job->callback_data->ref ++;
+		g_atomic_int_inc (&job->callback_data->ref);
 }
 
 static void
@@ -442,6 +407,72 @@
 }
 
 /**
+ * Job destruction
+ */
+
+static void
+brasero_io_job_free (BraseroIO *self,
+		     gboolean cancelled,
+		     BraseroIOJob *job)
+{
+	/* NOTE: the callback_data member is never destroyed here since it would
+	 * be destroyed in a thread different from the main loop.
+	 * Either it's destroyed in the thread that called brasero_io_cancel ()
+	 * or after all results are returned (and therefore in main loop).
+	 * As a special case, some jobs like read directory contents have to
+	 * return a dummy result to destroy the callback_data if the directory
+	 * is empty.
+	 * If the job happens to be the last to carry a reference then destroy
+	 * it but do it in the main loop by sending a dummy result. */
+
+	/* NOTE2: that's also used for directory loading when there aren't any
+	 * result to notify the caller that the operation has finished but that
+	 * there weren't any result to report. */
+	if (job->callback_data) {
+		/* see atomically if we are the last to hold a lock:
+		 * If so, and if cancelled is TRUE destroy it now since we are
+		 * always cancelled (and destroyed in the main loop). Otherwise
+		 * add a dummy result to destroy callback_data. */
+		if (g_atomic_int_dec_and_test (&job->callback_data->ref)) {
+			if (cancelled) {
+				job->base->destroy (job->base->object,
+						    TRUE,
+						    job->callback_data);
+				g_free (job->callback_data);
+			}
+			else
+				brasero_io_return_result (self,
+							  job->base,
+							  NULL,
+							  NULL,
+							  NULL,
+							  job->callback_data);
+		}
+	}
+
+	g_free (job->uri);
+	g_free (job);
+}
+
+static void
+brasero_io_job_destroy (BraseroAsyncTaskManager *manager,
+			gboolean cancelled,
+			gpointer callback_data)
+{
+	BraseroIOJob *job = callback_data;
+
+	/* If a job is destroyed we don't call the destroy callback since it
+	 * otherwise it would be called in a different thread. All object that
+	 * cancel io ops are doing it either in destroy () and therefore handle
+	 * all destruction for callback_data or if they don't they usually don't
+	 * pass any callback data anyway. */
+	/* NOTE: usually threads are cancelled from the main thread/loop and
+	 * block until the active task is removed which means that if we called
+	 * the destroy () then the destruction would be done in the main loop */
+	brasero_io_job_free (BRASERO_IO (manager), cancelled, job);
+}
+
+/**
  * This part deals with symlinks, that allows to get unique filenames by
  * replacing any parent symlink by its target and check for recursive
  * symlinks
@@ -1197,6 +1228,7 @@
 
 static void
 brasero_io_get_file_count_destroy (BraseroAsyncTaskManager *manager,
+				   gboolean cancelled,
 				   gpointer callback_data)
 {
 	BraseroIOCountData *data = callback_data;
@@ -1209,7 +1241,7 @@
 
 	brasero_io_job_progress_report_stop (BRASERO_IO (manager), callback_data);
 
-	brasero_io_job_free (callback_data);
+	brasero_io_job_free (BRASERO_IO (manager), cancelled, callback_data);
 }
 
 #ifdef BUILD_PLAYLIST
@@ -1528,6 +1560,7 @@
 
 static void
 brasero_io_load_directory_destroy (BraseroAsyncTaskManager *manager,
+				   gboolean cancelled,
 				   gpointer callback_data)
 {
 	BraseroIOContentsData *data = callback_data;
@@ -1535,7 +1568,7 @@
 	g_slist_foreach (data->children, (GFunc) g_object_unref, NULL);
 	g_slist_free (data->children);
 
-	brasero_io_job_free (BRASERO_IO_JOB (data));
+	brasero_io_job_free (BRASERO_IO (manager), cancelled, BRASERO_IO_JOB (data));
 }
 
 #ifdef BUILD_PLAYLIST
@@ -1778,17 +1811,6 @@
 		g_object_unref (child);
 	}
 
-	if (data->job.callback_data && data->job.callback_data->ref < 2) {
-		/* No result was returned so we need to return a dummy one to 
-		 * clean the callback_data in the main loop. */
-		brasero_io_return_result (BRASERO_IO (manager),
-					  data->job.base,
-					  NULL,
-					  NULL,
-					  NULL,
-					  data->job.callback_data);
-	}
-
 	g_file_enumerator_close (enumerator, NULL, NULL);
 	g_object_unref (enumerator);
 	g_object_unref (file);
@@ -1868,6 +1890,7 @@
 
 static void
 brasero_io_xfer_destroy (BraseroAsyncTaskManager *manager,
+			 gboolean cancelled,
 			 gpointer callback_data)
 {
 	BraseroIOXferData *data = callback_data;
@@ -1879,7 +1902,7 @@
 	g_mutex_free (data->current_lock);
 
 	/* no need to stop progress report as the following function will do it */
-	brasero_io_get_file_count_destroy (manager, callback_data);
+	brasero_io_get_file_count_destroy (manager, cancelled, callback_data);
 }
 
 static void
@@ -2384,8 +2407,7 @@
 				    TRUE,
 				    job->callback_data);
 
-	brasero_io_job_free (job);
-
+	brasero_io_job_free (BRASERO_IO (manager), TRUE, job);
 	return TRUE;
 }