[evolution-patches] fix for file backend race

[Chris Toshok <toshok ximian com>]

So, all the addressbook backends suffer from this problem potentionally.
I know the ldap one does, in fact that's the only backend I've seen
crash because of it.  I'll have a patch for that soon.  The file backend
is a little simpler, so I did it first.

The problem is that since start() and stop() are oneway methods, and
handled on their own threads, it's entirely possible if a client is
doing many queries very fast, stopping the old one and starting the new
one, that the stop() code might actually be executed before the start()
code on a given view.

This leads to a query running on a book view that (shortly after stop()
is called, at least by evolution) gets destroyed out from under it,
causing the crash.

This patch adds a mutex to EDataBookView that backends can use to
synchronize their start/stop methods, and adds some code to both the
start and stop methods of the file backend to guard against the
possibility of the other being called first.

fun fun fun

Chris

Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/evolution-data-server/addressbook/ChangeLog,v
retrieving revision 1.99
diff -u -r1.99 ChangeLog
--- ChangeLog	11 Mar 2004 10:23:24 -0000	1.99
+++ ChangeLog	12 Mar 2004 20:16:59 -0000
@@ -1,3 +1,27 @@
+2004-03-12  Chris Toshok  <toshok ximian com>
+
+	* libedata-book/e-data-book-view.h: add prototype for
+	e_data_book_view_get_mutex.
+
+	* libedata-book/e-data-book-view.c (e_data_book_view_get_mutex):
+	new function, return the book view's mutex.
+	(e_data_book_view_dispose): free priv->mutex.
+	(e_data_book_view_init): init priv->mutex.
+
+	* backends/file/e-book-backend-file.c (closure_destroy):
+	GDestroyNotify to clear up the search closure.
+	(init_closure): initialize the object data we use to store search
+	information (just a "stopped" flag at the moment)
+	(get_closure): return the object data.
+	(e_book_backend_file_start_book_view): add code to deal with the
+	race between start and stop, locking the book_view's mutex around
+	the initial get/set of the closure.  also, switch from using
+	g_object_ref to bonobo_object_ref on the book_view, and don't do
+	any freeing of the search closure here.  it's handled
+	automatically for us by virtue of the GDestroyNotify.
+	(e_book_backend_file_stop_book_view): flesh this out to fix the
+	race between start/stop.
+
 2004-03-09 Sivaioah Nallagatla <snallagatla novell com>
 
 	* backends/groupwise/e-book-backend-groupwise.[ch] : added 
Index: backends/file/e-book-backend-file.c
===================================================================
RCS file: /cvs/gnome/evolution-data-server/addressbook/backends/file/e-book-backend-file.c,v
retrieving revision 1.12
diff -u -r1.12 e-book-backend-file.c
--- backends/file/e-book-backend-file.c	8 Feb 2004 20:44:53 -0000	1.12
+++ backends/file/e-book-backend-file.c	12 Mar 2004 20:16:59 -0000
@@ -32,6 +32,8 @@
 #include <libedata-book/e-data-book-view.h>
 #include "e-book-backend-file.h"
 
+#define d(x) x
+
 #define CHANGES_DB_SUFFIX ".changes.db"
 
 #define E_BOOK_BACKEND_FILE_VERSION_NAME "PAS-DB-VERSION"
@@ -306,7 +308,7 @@
 	const char *search = query;
 	GList *contact_list = NULL;
 
-	printf ("e_book_backend_file_get_contact_list (%s)\n", search);
+	d(printf ("e_book_backend_file_get_contact_list (%s)\n", search));
 
 	search_needed = TRUE;
 
@@ -357,10 +359,38 @@
 } FileBackendSearchClosure;
 
 static void
+closure_destroy (FileBackendSearchClosure *closure)
+{
+	d(printf ("destroying search closure\n"));
+	g_mutex_free (closure->mutex);
+	g_free (closure);
+}
+
+static FileBackendSearchClosure*
+init_closure (EDataBookView *book_view)
+{
+	FileBackendSearchClosure *closure = g_new (FileBackendSearchClosure, 1);
+
+	closure->mutex = g_mutex_new();
+	closure->stopped = FALSE;
+
+	g_object_set_data_full (G_OBJECT (book_view), "EBookBackendFile.BookView::closure",
+				closure, (GDestroyNotify)closure_destroy);
+
+	return closure;
+}
+
+static FileBackendSearchClosure*
+get_closure (EDataBookView *book_view)
+{
+	return g_object_get_data (G_OBJECT (book_view), "EBookBackendFile.BookView::closure");
+}
+
+static void
 e_book_backend_file_start_book_view (EBookBackend  *backend,
 				     EDataBookView *book_view)
 {
-	FileBackendSearchClosure *closure = g_new0 (FileBackendSearchClosure, 1);
+	FileBackendSearchClosure *closure;
 	EBookBackendFile *bf = E_BOOK_BACKEND_FILE (backend);
 	const char *query;
 	DB  *db;
@@ -368,19 +398,36 @@
 	int db_error;
 	gboolean stopped = FALSE;
 
-	printf ("starting initial population of book view\n");
+	d(printf ("starting initial population of book view\n"));
+
+	g_mutex_lock (e_data_book_view_get_mutex (book_view));
+
+	closure = get_closure (book_view);
+	if (closure) {
+		/* the only way op can be set here is if we raced with
+		   stop_book_view and lost.  make sure by checking
+		   op->stopped. */
+		if (!closure->stopped) {
+			g_warning ("lost race with stop_book_view, but op->stopped != TRUE");
+		}
+		g_object_set_data (G_OBJECT (book_view), "EBookBackendFile.BookView::closure", NULL);
+		g_mutex_unlock (e_data_book_view_get_mutex (book_view));
+		return;
+	}
+	else {
+		closure = init_closure (book_view);
+		closure->stopped = FALSE;
+	}
 
 	/* ref the book view because it'll be removed and unrefed
 	   when/if it's stopped */
-	g_object_ref (book_view);
+	bonobo_object_ref (book_view);
 
+	g_mutex_unlock (e_data_book_view_get_mutex (book_view));
+	
 	db = bf->priv->file_db;
 	query = e_data_book_view_get_card_query (book_view);
 
-	closure->mutex = g_mutex_new();
-	closure->stopped = FALSE;
-	g_object_set_data (G_OBJECT (book_view), "EBookBackendFile.BookView::closure", closure);
-
 	if ( ! strcmp (query, "(contains \"x-evolution-any-field\" \"\")"))
 		e_data_book_view_notify_status_message (book_view, _("Loading..."));
 	else
@@ -398,11 +445,8 @@
 			stopped = closure->stopped;
 			g_mutex_unlock (closure->mutex);
 
-			if (stopped) {
-				g_mutex_free (closure->mutex);
-				g_free (closure);
+			if (stopped)
 				break;
-			}
 
 			string_to_dbt (id, &id_dbt);
 			memset (&vcard_dbt, 0, sizeof (vcard_dbt));
@@ -460,31 +504,35 @@
 	if (!stopped)
 		e_data_book_view_notify_complete (book_view, GNOME_Evolution_Addressbook_Success);
 
-	g_mutex_free (closure->mutex);
-	g_free (closure);
-
-	g_object_set_data (G_OBJECT (book_view), "EBookBackendFile.BookView::closure", NULL);
-
 	/* unref the */
-	g_object_unref (book_view);
+	bonobo_object_unref (book_view);
 
-	printf ("finished initial population of book view\n");
+	d(printf ("finished initial population of book view\n"));
 }
 
 static void
 e_book_backend_file_stop_book_view (EBookBackend  *backend,
 				    EDataBookView *book_view)
 {
-	FileBackendSearchClosure *closure = g_object_get_data (G_OBJECT (book_view), "EBookBackendFile.BookView::closure");
-	if (!closure) {
-		printf ("book view is already done populating\n");
-		return;
+	FileBackendSearchClosure *closure;
+
+	g_mutex_lock (e_data_book_view_get_mutex (book_view));
+
+	closure = get_closure (book_view);
+
+	if (closure) {
+		d(printf ("stopping running query!\n"));
+		g_mutex_lock (closure->mutex);
+		closure->stopped = TRUE;
+		g_mutex_unlock (closure->mutex);
+	}
+	else {
+		d(printf ("either the query is already finished or hasn't started yet (we won the race)\n"));
+		closure = init_closure (book_view);
+		closure->stopped = TRUE;
 	}
 
-	printf ("stopping book view!\n");
-	g_mutex_lock (closure->mutex);
-	closure->stopped = TRUE;
-	g_mutex_unlock (closure->mutex);
+	g_mutex_unlock (e_data_book_view_get_mutex (book_view));
 }
 
 typedef struct {
Index: libedata-book/e-data-book-view.c
===================================================================
RCS file: /cvs/gnome/evolution-data-server/addressbook/libedata-book/e-data-book-view.c,v
retrieving revision 1.4
diff -u -r1.4 e-data-book-view.c
--- libedata-book/e-data-book-view.c	11 Feb 2004 17:34:24 -0000	1.4
+++ libedata-book/e-data-book-view.c	12 Mar 2004 20:16:59 -0000
@@ -16,6 +16,8 @@
 #include "e-book-backend-sexp.h"
 #include "e-data-book-view.h"
 
+#define d(x) x
+
 static BonoboObjectClass *e_data_book_view_parent_class;
 
 struct _EDataBookViewPrivate {
@@ -24,6 +26,8 @@
 #define INITIAL_THRESHOLD 20
 #define THRESHOLD_MAX 3000
 
+	GMutex *mutex;
+
 	GMutex *pending_mutex;
 
 	CORBA_sequence_GNOME_Evolution_Addressbook_VCard adds;
@@ -373,6 +377,7 @@
 {
 	EDataBookView *view = E_DATA_BOOK_VIEW (bonobo_object (servant));
 
+	d(printf("in impl_GNOME_Evolution_Addressbook_BookView_dispose\n"));
 	ORBit_small_unlisten_for_broken (e_data_book_view_get_listener (view), G_CALLBACK (view_listener_died_cb));
 
 	bonobo_object_unref (view);
@@ -408,6 +413,14 @@
 	return book_view->priv->backend;
 }
 
+GMutex*
+e_data_book_view_get_mutex (EDataBookView *book_view)
+{
+	g_return_val_if_fail (E_IS_DATA_BOOK_VIEW (book_view), NULL);
+
+	return book_view->priv->mutex;
+}
+
 GNOME_Evolution_Addressbook_BookViewListener
 e_data_book_view_get_listener (EDataBookView  *book_view)
 {
@@ -441,6 +454,7 @@
 {
 	EDataBookView *book_view = E_DATA_BOOK_VIEW (object);
 
+	d(printf ("disposing of EDataBookView\n"));
 	if (book_view->priv) {
 		bonobo_object_release_unref (book_view->priv->listener, NULL);
 
@@ -457,6 +471,9 @@
 		g_mutex_free (book_view->priv->pending_mutex);
 		book_view->priv->pending_mutex = NULL;
 
+		g_mutex_free (book_view->priv->mutex);
+		book_view->priv->mutex = NULL;
+
 		g_free (book_view->priv);
 		book_view->priv = NULL;
 	}
@@ -489,6 +506,7 @@
 	book_view->priv = g_new0 (EDataBookViewPrivate, 1);
 
 	book_view->priv->pending_mutex = g_mutex_new();
+	book_view->priv->mutex = g_mutex_new();
 
 	book_view->priv->next_threshold = INITIAL_THRESHOLD;
 	book_view->priv->threshold_max = THRESHOLD_MAX;
Index: libedata-book/e-data-book-view.h
===================================================================
RCS file: /cvs/gnome/evolution-data-server/addressbook/libedata-book/e-data-book-view.h,v
retrieving revision 1.3
diff -u -r1.3 e-data-book-view.h
--- libedata-book/e-data-book-view.h	6 Nov 2003 01:37:37 -0000	1.3
+++ libedata-book/e-data-book-view.h	12 Mar 2004 20:16:59 -0000
@@ -40,24 +40,25 @@
 };
 
 
-EDataBookView *e_data_book_view_new                    (EBookBackend                 *backend,
-						   GNOME_Evolution_Addressbook_BookViewListener  listener,
-						   const char                 *card_query,
-						   EBookBackendSExp         *card_sexp);
+EDataBookView *e_data_book_view_new                  (EBookBackend                 *backend,
+						      GNOME_Evolution_Addressbook_BookViewListener  listener,
+						      const char                   *card_query,
+						      EBookBackendSExp             *card_sexp);
 
-const char*  e_data_book_view_get_card_query         (EDataBookView                *book_view);
-EBookBackendSExp* e_data_book_view_get_card_sexp   (EDataBookView                *book_view);
-EBookBackend*  e_data_book_view_get_backend            (EDataBookView                *book_view);
+const char*       e_data_book_view_get_card_query    (EDataBookView                *book_view);
+EBookBackendSExp* e_data_book_view_get_card_sexp     (EDataBookView                *book_view);
+EBookBackend*     e_data_book_view_get_backend       (EDataBookView                *book_view);
 GNOME_Evolution_Addressbook_BookViewListener e_data_book_view_get_listener (EDataBookView  *book_view);
+GMutex*           e_data_book_view_get_mutex         (EDataBookView                *book_view);
 
 void         e_data_book_view_notify_update          (EDataBookView                *book_view,
-						   EContact                   *contact);
+						      EContact                     *contact);
 void         e_data_book_view_notify_remove          (EDataBookView                *book_view,
-						   const char                 *id);
+						      const char                   *id);
 void         e_data_book_view_notify_complete        (EDataBookView                *book_view,
-						   GNOME_Evolution_Addressbook_CallStatus);
+						      GNOME_Evolution_Addressbook_CallStatus);
 void         e_data_book_view_notify_status_message  (EDataBookView                *book_view,
-						   const char                 *message);
+						      const char                   *message);
 
 GType        e_data_book_view_get_type               (void);