Re: Reparenting cleanup

[Timo Korvola <Timo Korvola iki fi>]

"Andrea Vettorello" <andrea vettorello gmail com> writes:
> I can fix GPP commenting out a line in gpm-prefs.c (it's the
> "gtk_widget_hide (main_window)" in hidegpm_prefs_create),

It should be fixed in GPP.  What is the point of such window flashing?

> but i would like to see this solved in Sawfish.

If my theory about the other race condition is correct (I haven't
actually managed to cause that situation), it should also be fixed in
Sawfish.

> On 3/1/07, Timo Korvola <Timo Korvola iki fi> wrote:
>> I can think of two possible wm-side solutions.  One would be that the
>> wm would watch out for synthetic UnmapNotify events about windows that
>> it thinks are mapped.  Such windows should then be unmapped by the wm.

Here is my current patch vs. svn trunk.  It includes both the
previous reparenting fix and the solution described above.

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

Index: src/windows.c
===================================================================
--- src/windows.c	(revision 4194)
+++ src/windows.c	(working copy)
@@ -514,8 +514,9 @@
     return w;
 }
 
-/* Remove W from the managed windows. If DESTROYED is nil, then the
-   window will be reparented back to the root window */
+/* Remove W from the managed windows. If DESTROYED is nil and
+   the window is currently reparented by us, it will be reparented back to
+   the root window */
 void
 remove_window (Lisp_Window *w, bool destroyed, bool from_error)
 {
Index: src/events.c
===================================================================
--- src/events.c	(revision 4194)
+++ src/events.c	(working copy)
@@ -689,10 +689,13 @@
     Lisp_Window *w = find_window_by_id (id);
     if (w == 0)
     {
+        /* Also adds the frame. */
 	w = add_window (id);
 	if (w == 0)
+	{
+	    fprintf (stderr, "warning: failed to allocate a window\n");
 	    return;
-
+	}
 	if (w->wmhints && w->wmhints->flags & StateHint
 	    && w->wmhints->initial_state == IconicState)
 	{
@@ -736,10 +739,13 @@
 	if (ev->xreparent.parent != root_window
 	    && ev->xreparent.parent != w->frame)
 	{
-	    /* Not us doing the reparenting.. */
+	    /* The window is no longer on our turf and we must not
+	       reparent it to the root. -- thk */
+	    w->reparented = FALSE;
+	    XRemoveFromSaveSet (dpy, w->id);
+
+	    /* Not us doing the reparenting. */
 	    remove_window (w, FALSE, FALSE);
-	    XReparentWindow (dpy, ev->xreparent.window, ev->xreparent.parent,
-			     ev->xreparent.x, ev->xreparent.y);
 	}
 	Fcall_window_hook (Qreparent_notify_hook, rep_VAL(w), Qnil, Qnil);
     }
@@ -757,6 +763,10 @@
 	{
 	    /* arrgh, the window changed its override redirect status.. */
 	    remove_window (w, FALSE, FALSE);
+#if 0
+	    fprintf(stderr, "warning: I've had it with window %#lx\n",
+		    (long)(w->id));
+#endif
 	}
 	else
 	{
@@ -765,11 +775,11 @@
 	    w->attr.height = wa.height;
 
 	    w->mapped = TRUE;
+	    /* This should not happen.  The window should have been
+	       framed at the map request. -- thk */
 	    if (w->frame == 0)
-		create_window_frame (w);
-	    install_window_frame (w);
-	    if (w->visible)
-		XMapWindow (dpy, w->frame);
+		fprintf (stderr, "warning: window %#1x has no frame\n",
+			 (long)(w->id));
 	    Fcall_window_hook (Qmap_notify_hook, rep_VAL(w), Qnil, Qnil);
 	}
     }
@@ -782,6 +792,9 @@
     if (w != 0 && ev->xunmap.window == w->id
 	&& (ev->xunmap.event == w->id || ev->xunmap.send_event))
     {
+	int being_reparented = FALSE;
+	XEvent reparent_ev;
+
 	w->mapped = FALSE;
 	if (w->reparented)
 	{
@@ -790,11 +803,32 @@
 		XUnmapWindow (dpy, w->frame);
 		reset_frame_parts (w);
 	    }
-	    /* Removing the frame reparents the client window back to
-	       the root. This means that we receive the next MapRequest
-	       for the window. */
-	    remove_window_frame (w);
-	    destroy_window_frame (w, FALSE);
+	    /* Careful now.  It is possible that the unmapping was
+	       caused by someone else reparenting the window.
+	       Removing the frame involves reparenting the window to
+	       the root.  Bad things may happen if we do that while
+	       a different reparenting is in progress. -- thk */
+	    being_reparented = XCheckTypedWindowEvent (dpy, w->id,
+						       ReparentNotify,
+						       &reparent_ev);
+	    if (!being_reparented)
+	    {
+	        /* Removing the frame reparents the client window back to
+		   the root. This means that we receive the next MapRequest
+		   for the window. */
+		remove_window_frame (w);
+		destroy_window_frame (w, FALSE);
+	    }
+
+	    /* Handle a possible race condition: if the client
+	       withdrew the window while we were in the process of
+	       mapping it, the window may be mapped now.  -- thk */
+	    if (ev->xunmap.send_event && !w->client_unmapped)
+	    {
+		before_local_map (w);
+		XUnmapWindow (dpy, w->id);
+		after_local_map (w);
+	    }
 	}
 	Fcall_window_hook (Qunmap_notify_hook, rep_VAL(w), Qnil, Qnil);
 
@@ -802,7 +836,10 @@
 
 	/* Changed the window-handling model, don't let windows exist
 	   while they're withdrawn */
-	remove_window (w, FALSE, FALSE);
+	if (being_reparented)
+	    reparent_notify(&reparent_ev);
+	else
+	    remove_window (w, FALSE, FALSE);
     }
 }