Merge lp:~smspillaz/xig/xig.clean-up-after-clients into lp:xig

[Sam Spilsbury]

Remove window id's left behind by clients due to buggy X extension libraries or by SIGKILL.

Remove stale selections

[Robert Ancell (robert-ancell)]

This patch seems a little back to front... I'd expect the client to maintain the list of resources rather than have the server call back to it. I need to think this one over a bit more (though the concept is 100% correct of course).

[Sam Spilsbury (smspillaz)]

The server already maintains a list of resources, so when the client goes away we need to figure out which resources the client owns and get rid of them on the server side lists too.

[Robert Ancell (robert-ancell)]

From me via IRC:

"What you really should do is the server should connected to the destroyed signal for the resources and remove them that way
 so you're explicitly destroying the references from both sides, but the server should be implicitly destroying them
 (or the other way around). The patch has both the server and client responsible which is dangerous if one of them gets out of sync.

So there's two ways we can do this - the client destroys itself, and the server detects that and destroys all its resources, or the client destroys all its resources then itself, and the server doesn't care"

and Sam said "So I think the best way to handle this is to have a XigClientResource which XigSelection and XigDrawable inherit"

Which is correct.

review: Needs Fixing

Unmerged revisions

255. By Sam Spilsbury on 2012-01-21

Clean up after clients - remove stale resource id's and selections. Also
handle the edge case where clients die and leave stuff behind.

254. By Sam Spilsbury on 2012-01-21

Write message length according to format, not actual length as that's too much

=== modified file 'src/xig-remote-client-private.h'
--- src/xig-remote-client-private.h	2011-12-01 23:46:20 +0000
+++ src/xig-remote-client-private.h	2012-01-21 11:59:25 +0000
@@ -23,4 +23,9 @@
 
 void _xig_remote_client_circulate_request (XigRemoteClient *client, XigWindow *window, XigCirculatePlace place);
 
+GList * _xig_remote_client_get_owned_drawables (XigRemoteClient *client);
+
+void _xig_remote_client_add_drawable (XigRemoteClient *client, XigDrawable *drawable);
+void _xig_remote_client_remove_drawable (XigRemoteClient *client, XigDrawable *drawable);
+
 #endif /* _XIG_REMOTE_CLIENT_PRIVATE_H_ */

=== modified file 'src/xig-remote-client.c'
--- src/xig-remote-client.c	2012-01-10 16:45:57 +0000
+++ src/xig-remote-client.c	2012-01-21 11:59:25 +0000
@@ -47,6 +47,9 @@
 
     /* Event masks */
     GHashTable *window_events;
+
+    /* Owned windows */
+    GHashTable *owned_drawables;
 };
 
 enum
@@ -287,6 +290,24 @@
     xig_codec_feed_circulate_request (XIG_CODEC (client), &message);
 }
 
+GList *
+_xig_remote_client_get_owned_drawables (XigRemoteClient *client)
+{
+    return g_hash_table_get_keys (client->priv->owned_drawables);
+}
+
+void
+_xig_remote_client_add_drawable (XigRemoteClient *client, XigDrawable *drawable)
+{
+    g_hash_table_insert (client->priv->owned_drawables, GINT_TO_POINTER (xig_drawable_get_id (drawable)), g_object_ref (drawable));
+}
+
+void
+_xig_remote_client_remove_drawable (XigRemoteClient *client, XigDrawable *drawable)
+{
+    g_hash_table_remove (client->priv->owned_drawables, GINT_TO_POINTER (xig_drawable_get_id (drawable)));
+}
+
 static void
 circulate_notify_cb (XigWindow *window, XigRemoteClient *client)
 {
@@ -412,7 +433,7 @@
     if (n_read < 0)
         g_warning ("Error reading from socket: %s", strerror (errno));
     else if (n_read == 0)
-    {      
+    {
         g_signal_emit (client, signals[DISCONNECTED], 0);
         return FALSE;
     }
@@ -1574,6 +1595,7 @@
 
     reply.sequence_number = message->sequence_number;
     focus = xig_server_get_input_focus (client->priv->server, &reply.revert_to);
+
     reply.focus = focus ? xig_window_get_id (focus) : XIG_WINDOW_ID_NONE;
 
     xig_codec_feed_get_input_focus_reply (XIG_CODEC (codec), &reply);
@@ -2582,6 +2604,7 @@
     client->priv->server = server;
     client->priv->socket = g_object_ref (socket);
     client->priv->channel = g_io_channel_unix_new (g_socket_get_fd (socket));
+    client->priv->owned_drawables = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_object_unref);
     g_io_add_watch (client->priv->channel, G_IO_IN, socket_data_cb, client);
 
     return client;

=== modified file 'src/xig-server.c'
--- src/xig-server.c	2011-12-02 10:34:45 +0000
+++ src/xig-server.c	2012-01-21 11:59:25 +0000
@@ -150,12 +150,78 @@
     server->priv->listen_tcp = listen_tcp;
 }
 
+static gboolean
+_xig_server_hash_table_remove_selection_for_window (gpointer  key,
+                                                    gpointer  owner,
+                                                    gpointer  user_data)
+{
+    return owner == user_data ? TRUE : FALSE;
+}
+
+void
+_xig_server_remove_window (XigServer *server, XigWindow *window)
+{
+    g_hash_table_remove (server->priv->drawables, GINT_TO_POINTER (xig_drawable_get_id (XIG_DRAWABLE (window))));
+
+    /* Remove any selection owners too */
+    g_hash_table_foreach_remove (server->priv->selection_owners, _xig_server_hash_table_remove_selection_for_window, (gpointer) window);
+
+    /* Unset the focus window (revert to root window */
+    if (window == server->priv->focus_window)
+        server->priv->focus_window = NULL;
+}
+
+void
+_xig_server_add_window (XigServer *server, XigWindow *window)
+{
+    g_hash_table_insert (server->priv->drawables, GINT_TO_POINTER (xig_drawable_get_id (XIG_DRAWABLE (window))), g_object_ref (window));
+}
+
+void
+_xig_server_add_pixmap (XigServer *server, XigPixmap *pixmap)
+{
+    g_hash_table_insert (server->priv->drawables, GINT_TO_POINTER (xig_drawable_get_id (XIG_DRAWABLE (pixmap))), g_object_ref (pixmap));
+}
+
+void
+_xig_server_remove_pixmap (XigServer *server, XigPixmap *pixmap)
+{
+    g_hash_table_remove (server->priv->drawables, GINT_TO_POINTER (xig_drawable_get_id (XIG_DRAWABLE (pixmap))));
+}
+
+
 static void
 xig_remote_client_disconnected_cb (XigRemoteClient *client, XigServer *server)
 {
     g_signal_handlers_disconnect_matched (client, G_SIGNAL_MATCH_DATA, 0, 0, NULL, NULL, server);
     server->priv->clients = g_list_remove (server->priv->clients, client);
     g_signal_emit (server, signals[CLIENT_DISCONNECTED], 0, client);
+
+    GList *list = _xig_remote_client_get_owned_drawables (client);
+    GList *orig = list;
+
+    while (list)
+    {
+        gint id = GPOINTER_TO_INT (list->data);
+	XigDrawable *drawable = g_hash_table_lookup (server->priv->drawables, GINT_TO_POINTER (id));
+
+	if (XIG_IS_WINDOW (drawable))
+	{
+	    XigWindow *window = XIG_WINDOW (drawable);
+
+	    _xig_server_remove_window (server, window);
+	}
+	else if (XIG_IS_PIXMAP (drawable))
+	{
+	    XigPixmap *pixmap = XIG_PIXMAP (drawable);
+
+	    _xig_server_remove_pixmap (server, pixmap);
+	}
+        list = g_list_next (list);
+    }
+
+    g_list_free (orig);
+
     g_object_unref (client);
 }
 
@@ -257,12 +323,6 @@
 }
 
 void
-_xig_server_add_window (XigServer *server, XigWindow *window)
-{
-    g_hash_table_insert (server->priv->drawables, GINT_TO_POINTER (xig_drawable_get_id (XIG_DRAWABLE (window))), g_object_ref (window));
-}
-
-void
 _xig_server_set_selection_owner (XigServer *server, const gchar *name, XigWindow *window)
 {
     g_hash_table_insert (server->priv->selection_owners, g_strdup (name), g_object_ref (window));  
@@ -274,18 +334,6 @@
     return g_hash_table_lookup (server->priv->selection_owners, name);
 }
 
-void
-_xig_server_add_pixmap (XigServer *server, XigPixmap *pixmap)
-{
-    g_hash_table_insert (server->priv->drawables, GINT_TO_POINTER (xig_drawable_get_id (XIG_DRAWABLE (pixmap))), g_object_ref (pixmap));
-}
-
-void
-_xig_server_remove_pixmap (XigServer *server, XigPixmap *pixmap)
-{
-    g_hash_table_remove (server->priv->drawables, GINT_TO_POINTER (xig_drawable_get_id (XIG_DRAWABLE (pixmap))));
-}
-
 XigDrawable *
 xig_server_get_drawable (XigServer *server, guint32 drawable)
 {
@@ -346,7 +394,7 @@
 void
 xig_server_set_input_focus (XigServer *server, XigWindow *window, guint8 revert_to)
 {
-    server->priv->focus_window = g_object_ref (window);
+    server->priv->focus_window = window;
     // FIXME: Catch delete event
     server->priv->focus_revert_to = revert_to;
 }

=== modified file 'src/xig-window.c'
--- src/xig-window.c	2012-01-10 17:14:41 +0000
+++ src/xig-window.c	2012-01-21 11:59:25 +0000
@@ -76,6 +76,76 @@
 };
 static guint signals[LAST_SIGNAL] = { 0 };
 
+static void
+destroy_recursive (XigWindow *window)
+{
+    GList *link;
+
+    /* Destroy all children first */
+    for (link = window->priv->children; link; link = link->next)
+    {
+        XigWindow *w = link->data;
+        destroy_recursive (w);
+    }
+
+    g_signal_emit (window, signals[DESTROY_NOTIFY], 0);
+    g_signal_emit (window->priv->parent, signals[CHILD_DESTROY_NOTIFY], 0, window);
+
+    g_object_unref (window);
+}
+
+void
+xig_window_destroy (XigWindow *window)
+{
+    window->priv->parent->priv->children = g_list_remove (window->priv->parent->priv->children, window);
+    destroy_recursive (window);
+}
+
+void
+xig_window_destroy_subwindows (XigWindow *window)
+{
+    GList *link;
+    for (link = window->priv->children; link; link = link->next)
+    {
+        XigWindow *w = link->data;
+        destroy_recursive (w);
+    }
+    g_list_free (window->priv->children);
+    window->priv->children = NULL;
+}
+
+/* When clients disconnect, they are meant to (implicitly, through xcb)
+ * remove any resources that they held. When a client dies, it can't do that
+ * so clean up after it */
+static void
+xig_window_client_disconnected_cb (XigServer *server, XigRemoteClient *client, XigWindow *window)
+{
+    GList *list = _xig_remote_client_get_owned_drawables (client);
+    GList *orig = list;
+
+    while (list)
+    {
+        gint id = GPOINTER_TO_INT (list->data);
+
+        GList *child = window->priv->children;
+
+        while (child)
+        {
+            if (xig_window_get_id (XIG_WINDOW (child->data)) == id)
+	    {
+		xig_window_destroy (XIG_WINDOW (child->data));
+                break;
+            }
+
+            child = g_list_next (child);
+        }
+
+        list = g_list_next (list);
+    }
+
+    g_list_free (orig);
+}
+
 XigWindow *
 _xig_window_new (XigServer *server, XigRemoteClient *client, guint32 wid, XigWindow *parent, XigWindowClassType class, gint16 x, gint16 y, guint16 width, guint16 height, guint16 border_width, XigVisual *visual)
 {
@@ -97,6 +167,10 @@
     window->priv->border_width = border_width;
     window->priv->visual = g_object_ref (visual);
 
+    if (client)
+        _xig_remote_client_add_drawable (client, XIG_DRAWABLE (window));
+    g_signal_connect (server, "client-disconnected", G_CALLBACK (xig_window_client_disconnected_cb), window);
+
     if (parent)
         g_signal_emit (parent, signals[CREATE_NOTIFY], 0, window);
 
@@ -695,44 +769,6 @@
 }
 
 static void
-destroy_recursive (XigWindow *window)
-{
-    GList *link;
-
-    /* Destroy all children first */
-    for (link = window->priv->children; link; link = link->next)
-    {
-        XigWindow *w = link->data;
-        destroy_recursive (w);
-    }
-
-    g_signal_emit (window, signals[DESTROY_NOTIFY], 0);
-    g_signal_emit (window->priv->parent, signals[CHILD_DESTROY_NOTIFY], 0, window);
-
-    g_object_unref (window);
-}
-
-void
-xig_window_destroy (XigWindow *window)
-{
-    window->priv->parent->priv->children = g_list_remove (window->priv->parent->priv->children, window);
-    destroy_recursive (window);
-}
-
-void
-xig_window_destroy_subwindows (XigWindow *window)
-{
-    GList *link;
-    for (link = window->priv->children; link; link = link->next)
-    {
-        XigWindow *w = link->data;
-        destroy_recursive (w);
-    }
-    g_list_free (window->priv->children);
-    window->priv->children = NULL;
-}
-
-static void
 xig_window_init (XigWindow *window)
 {
     window->priv = G_TYPE_INSTANCE_GET_PRIVATE (window, xig_window_get_type (), XigWindowPrivate);
@@ -753,6 +789,10 @@
 xig_window_finalize (GObject *object)
 {
     XigWindow *window = (XigWindow *) object;
+
+    if (window->priv->client)
+        _xig_remote_client_remove_drawable (window->priv->client, XIG_DRAWABLE (window));
+
     if (window->priv->visual)
         g_object_unref (window->priv->visual);
     if (window->priv->background_pixmap)