Re: NEWS on 1.6.0, crash fix... semi-fix (if you create and quickly delete a window sawfish will segfault)

[Timo Korvola <Timo Korvola iki fi>]

On Sunday 04 October 2009 19:21:04 Janek Kozicki wrote:
> I have found the bug. Exact place.

I hope it is the only place.  Wouldn't bet on it though.  There may be 
more instances, perhaps with so narrow time windows that they have never 
been seen.

> Problem is that in a very short piece of code sawfish does not expect
> a newly created window to disappear. If it does - then sawfish will
> crash.

But why, precisely?  One of the Xlib calls may have triggered 
error_handler, which sets w->id = 0, but is that enough to cause a SEGV?  
Is it possible that w gets garbage-collected before add_window is 
finished?  That would explain a lot.  It seems that the garbage 
collection would then have to occur after emit_pending_destroys.

I am not really familiar with the Librep C API and it is not too well 
documented, thus I am guessing here.  The enclosed patch tries to 
protect w from gc (you can also fetch branch "race" from my repository).  
People affected by the bug might want to test it.  Being unable to 
reproduce the bug myself, I can't say whether it helps.

-- 
	Timo Korvola		<URL:http://www.iki.fi/tkorvola>

From c3235edd1f570d543c1b60f2f70ce27cca08fd26 Mon Sep 17 00:00:00 2001
From: Timo Korvola <tkorvola iki fi>
Date: Tue, 6 Oct 2009 00:28:33 +0300
Subject: [PATCH] Wrap most of add_window in rep_PUSHGC/rep_POPGC.

There have been crashes that might be due to a window being
destroyed, garbage collected and then accessed, all during
add_window (client destroys window really quickly after mapping).
---
 src/windows.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/windows.c b/src/windows.c
index baf924a..ca98b13 100644
--- a/src/windows.c
+++ b/src/windows.c
@@ -459,6 +459,10 @@ add_window (Window id)
 	w->net_name = Qnil;
 	w->net_icon_name = Qnil;
 
+        /* Don't garbage collect the window before we are done. */
+        /* Note: must not return without rep_POPGC. */
+	rep_PUSHGC(gc_win, win);
+
 	/* have to put it somewhere until it finds the right place */
 	insert_in_stacking_list_above_all (w);
 	restack_window (w);
@@ -527,10 +531,8 @@ add_window (Window id)
 	  }
 	
 	/* ..then call the add-window-hook's.. */
-	rep_PUSHGC(gc_win, win);
 	Fcall_window_hook (Qbefore_add_window_hook, rep_VAL(w), Qnil, Qnil);
 	Fcall_window_hook (Qadd_window_hook, rep_VAL(w), Qnil, Qnil);
-	rep_POPGC;
 
 	/* In case the window disappeared during the hook call */
 	if (!WINDOW_IS_GONE_P (w))
@@ -551,13 +553,11 @@ add_window (Window id)
 
 	if (!WINDOW_IS_GONE_P (w))
 	{
-           repv tem = Fwindow_get (rep_VAL(w), Qplaced, Qnil);
+            repv tem = Fwindow_get (rep_VAL(w), Qplaced, Qnil);
 	    if (initialising || (tem && tem == Qnil))
 	    {
 		/* ..then the place-window-hook.. */
-		rep_PUSHGC(gc_win, win);
 		Fcall_window_hook (Qplace_window_hook, rep_VAL(w), Qnil, Qor);
-		rep_POPGC;
 	    }
 	}
 	Fwindow_put (rep_VAL(w), Qplaced, Qt);
@@ -570,6 +570,7 @@ add_window (Window id)
 	    /* Tell the window where it ended up.. */
 	    send_synthetic_configure (w);
 	}
+        rep_POPGC;
     }
     return w;
 }
-- 
1.6.3.3