| Browse all patches |
Author
Timo Korvola
Synopsis
This fixes reparenting fights that occur between Sawfish and the KDE system tray. Both try to reparent system tray icons as they are mapped, leading to a lot of flicker and an unpredictable end result. After the patch, Sawfish will reparent windows to their frames at MapRequest time, never at MapNotify. Also, windows that are unmapped by the client should normally be reparented to the root, but if the unmapping was caused by the window being reparented by some other client, problems ensue. So we check for that.
The patch also fixes an exotic race condition triggered at least with old versions of Monodevelop #308155 and Gnome Power Manager. Current versions of both programs don't expose the issue anymore. It was discussed on the mailing list on february 2007 and the follow up march 2007. There's a proof of concept available that should demonstrate the bug, a window isn't decorated if unmapped during the first reparenting.
Patch
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);
}
}
Community's reasons for inclusion or rejection
vote: yes. For a client to reparent a mapped top-level window is a violation of the ICCCM. Unfortunately KDE gets it wrong and maps system tray icons before reparenting them, thus briefly making them top-level windows. This reparenting then interferes with Sawfish trying to reparent the window out of its frame. The solution implemented in the patch is to peek for ReparentNotify events at unmap_notify. It has been copied from Fluxbox. I have also cleaned up reparenting done by Sawfish a bit. - Timo Korvola
- vote: yes. Seems to fix the race condition mentioned above, with apparently no regressions. - Aav 10:00, 16 January 2008 (UTC)