[Evolution-hackers] Thread locking in gtkhtml!?

[Philip Van Hoof <spamfrommailing freax org>]

Hi there,

While trying to figure out why at some race condition the Evolution composer was crashing (well, actually it was hanging in a loop), I decided to go fishing in the code of gtkhtml.

I've found that there's a doubly-linked list being passed to some functions (but you can assume it's a global variable since it appears to be always the same pointer being passed): spell_errors. It contains SpellError objects.

The original author of remove_spell_errors used a clever hack for removing what I assume where "old" spelling errors. So spellings errors that now have been corrected (Am I right)? 

However. That clever hack might have worked if there wasn't any threading involved. The hack removed an element from the list while already keeping a pointer to the next item in the list. So it's removing the current list-element, and by prefetching keeping a useful pointer to the next element. The next loop would work on the "next" pointer. While it has removed the current item from the global spell_errors list.

Sounds like always correct. However. I've experienced moments when Evolution got into an endless loop at the remove_spell_errors routine: http://bugzilla.ximian.com/show_bug.cgi?id=72988

So I'm assuming that there's other functions who are manipulating the spell_errors list. And whats worse is that they are probably doing so in a different thread. Which in turn leads to race conditions in one of both parallel running processes. I don't know which thread or whatever. It's just a thought of mine.

So I decided to redo this remove_spell_errors function in a less clever but also less hackish way. Easily by keeping a new doubly-linked list which I called toremove and creating a copy of the global list spell_errors and utilising that copy during the first loop, rather than working on a pointer to the global list.

This E-mail and it's old spelling errors -- hehe -- are the first test of this function. So far it hasn't crashed on me once. Which is, for me at least, a whole new Evolution experience since a few weeks. 

Notice: If it's known which threads are working on this spell_errors list, some mutex-locking would be necessary here. You can't just remove an item from a list while another thread is continuing relying on it's contents. and GList hacks aren't to way to circumvent it. Why not just lock it and unlock when it's done? The way it's after this patch, shouldn't have any problems even without locking mechanisms. Since it's destroying the SpellError after removing it from that global spell_errors list.

A possible reason why the previous method had problems might be because of this:

static GList*
remove_one (GList *list, GList *link)
{
(a)	spell_error_destroy ((SpellError *) link->data);
        -- race conditions can happen here --
(b)	return g_list_remove_link (list, link);
}

As you can see it's first destroying and they removing the link from the GList. It's possible my kernel is going to interrupt my process between (a) and (b) and that the other process is going to try playing with the pointer link->data during the time it was given to play on my CPU.


This might also fix it. But then it's still using a hackish way for removing an item from the GList.

static GList*
remove_one (GList *list, GList *link)
{
    GList *retval = g_list_remove_link (list, link);
    spell_error_destroy ((SpellError *) link->data);
    return retval;
}

-- 
Philip Van Hoof, Software Developer @ Cronos
home: me at freax dot org
gnome: pvanhoof at gnome dot org
work: philip dot vanhoof at cronos dot be
junk: philip dot vanhoof at gmail dot com
http://www.freax.be, http://www.freax.eu.org

? test-suite
Index: htmltext.c
===================================================================
RCS file: /cvs/gnome/gtkhtml/src/htmltext.c,v
retrieving revision 1.274
diff -u -r1.274 htmltext.c
--- htmltext.c	3 Feb 2005 17:18:43 -0000	1.274
+++ htmltext.c	24 Feb 2005 20:49:24 -0000
@@ -2097,21 +2097,15 @@
 } 
 
 static GList *
-remove_one (GList *list, GList *link)
-{
-	spell_error_destroy ((SpellError *) link->data);
-	return g_list_remove_link (list, link);
-}
-
-static GList *
 remove_spell_errors (GList *spell_errors, guint offset, guint len)
 {
 	SpellError *se; 
-	GList *cur, *cnext;
+	GList *cur=NULL, *toremove=NULL;
+
+	cur = g_list_copy (spell_errors);
 
-	cur = spell_errors;
 	while (cur) { 
-		cnext = cur->next;
+
 		se = (SpellError *) cur->data;
 		if (se->off < offset) {
 			if (se->off + se->len > offset) {
@@ -2120,20 +2114,34 @@
 				else
 					se->len -= len;
 				if (se->len < 2)
-					spell_errors = remove_one (spell_errors, cur);
+					toremove = g_list_append (toremove, se);
 			}
 		} else if (se->off < offset + len) {
-			if (se->off + se->len <= offset + len)
-				spell_errors = remove_one (spell_errors, cur);
-			else {
+			if (se->off + se->len <= offset + len) {
+				toremove = g_list_append (toremove, se);
+			} else {
 				se->len -= offset + len - se->off;
 				se->off  = offset + len;
 				if (se->len < 2)
-					spell_errors = remove_one (spell_errors, cur);
+					toremove = g_list_append (toremove, se);
 			}
 		}
- 		cur = cnext;
+
+ 		cur = g_list_next (cur);
   	} 
+
+	g_list_free (cur);
+
+	while (toremove)
+	{
+		se = (SpellError *) toremove->data;
+		spell_errors = g_list_remove_all (spell_errors, se);
+		spell_error_destroy ((SpellError *) se);
+		toremove = g_list_next (toremove);
+	}
+
+	g_list_free (toremove);
+
 	return spell_errors;
 }