[gnome-applets/wip-geiger-task-item: 1/2] window-picker: handle item lifecycle in TaskList

[Sebastian Geiger <segeiger src gnome org>]

commit e05d8d102fe1f8a6509147c2b64dabfed2df9949
Author: Sebastian Geiger <sbastig gmx net>
Date:   Wed Apr 1 00:34:41 2020 +0200

    window-picker: handle item lifecycle in TaskList
    
    When moving items between monitors we were seeing use-after-free errors
    because the TaskItem was accessing its WnckWindow after it was already
    destroyed. The actual cause however was, that TaskItems were not
    correctly removed from a TaskList.
    
    The actual cause for this problem was, that the user-data in the
    'task-item-closed' signal handler was not correctly updated when moving
    a TaskItem from one list to another. As a result we were removing
    TaskItems from the wrong list when the window was closed. This only
    happend when the Window was moved to another monitor before it was
    closed.
    
    To solve this issue we simplify the TaskItem lifecycle by moving it to
    the TaskList. We now use a GHashTable in each TaskList to map open
    windows to their respective TaskItems.

 gnome-applets/window-picker/task-item.c |  92 +++++++++++---------------
 gnome-applets/window-picker/task-item.h |  16 ++++-
 gnome-applets/window-picker/task-list.c | 113 ++++++++++++++++++--------------
 3 files changed, 114 insertions(+), 107 deletions(-)
---
diff --git a/gnome-applets/window-picker/task-item.c b/gnome-applets/window-picker/task-item.c
index aeacde419..9ed637a9a 100644
--- a/gnome-applets/window-picker/task-item.c
+++ b/gnome-applets/window-picker/task-item.c
@@ -37,6 +37,7 @@ struct _TaskItem {
   guint        blink_timer;
   gboolean     mouse_over;
   GdkMonitor  *monitor;
+  TaskList    *list;
   WpApplet    *windowPickerApplet;
 };
 
@@ -47,7 +48,6 @@ G_DEFINE_TYPE (TaskItem, task_item, GTK_TYPE_EVENT_BOX)
 #define DEFAULT_TASK_ITEM_WIDTH 28 + 2
 
 enum {
-    TASK_ITEM_CLOSED_SIGNAL,
     TASK_ITEM_MONITOR_CHANGED,
     LAST_SIGNAL
 };
@@ -77,8 +77,6 @@ static const GtkTargetEntry drag_types[] = {
 
 static const gint n_drag_types = G_N_ELEMENTS(drag_types);
 
-static void task_item_close (TaskItem *item);
-
 static void
 update_expand (TaskItem       *self,
                GtkOrientation  orientation)
@@ -542,26 +540,6 @@ static void on_window_icon_changed (WnckWindow *window, TaskItem *item) {
     gtk_widget_queue_draw (GTK_WIDGET (item));
 }
 
-static void
-on_window_type_changed (WnckWindow *window,
-                        TaskItem *item)
-{
-    WnckWindowType type;
-
-    if (item->window != window)
-        return;
-
-    type = wnck_window_get_window_type (window);
-
-    if (type == WNCK_WINDOW_DESKTOP ||
-        type == WNCK_WINDOW_DOCK ||
-        type == WNCK_WINDOW_SPLASHSCREEN ||
-        type == WNCK_WINDOW_MENU)
-      {
-          task_item_close (item);
-      }
-}
-
 static GdkMonitor *
 get_window_monitor (WnckWindow *window)
 {
@@ -650,25 +628,6 @@ static void on_screen_active_viewport_changed (
     task_item_set_visibility (item);
 }
 
-static void on_screen_window_closed (
-    WnckScreen   *screen,
-    WnckWindow   *window,
-    TaskItem     *item)
-{
-    g_return_if_fail (TASK_IS_ITEM(item));
-    g_return_if_fail (WNCK_IS_WINDOW (item->window));
-    if (item->window == window) {
-        task_item_close (item);
-    }
-}
-
-static void
-task_item_close (TaskItem   *item)
-{
-    g_signal_emit (G_OBJECT (item),
-                   task_item_signals[TASK_ITEM_CLOSED_SIGNAL], 0);
-}
-
 static gboolean activate_window (GtkWidget *widget) {
     TaskItem *item;
     GtkWidget *parent;
@@ -991,6 +950,8 @@ static void task_item_finalize (GObject *object) {
     }
 
     g_clear_object (&item->pixbuf);
+    g_clear_object (&item->window);
+    g_clear_object (&item->list);
 
     G_OBJECT_CLASS (task_item_parent_class)->finalize (object);
 }
@@ -1007,14 +968,6 @@ static void task_item_class_init (TaskItemClass *klass) {
     widget_class->get_preferred_height = task_item_get_preferred_height;
     widget_class->get_preferred_height_for_width = task_item_get_preferred_height_for_width;
 
-    task_item_signals [TASK_ITEM_CLOSED_SIGNAL] =
-    g_signal_new ("task-item-closed",
-        G_TYPE_FROM_CLASS (klass),
-        G_SIGNAL_RUN_FIRST | G_SIGNAL_ACTION,
-        0,
-        NULL, NULL,
-        g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0);
-
     task_item_signals [TASK_ITEM_MONITOR_CHANGED] =
         g_signal_new ("monitor-changed", TASK_TYPE_ITEM, G_SIGNAL_RUN_LAST,
                       0, NULL, NULL, NULL, G_TYPE_NONE, 0);
@@ -1024,7 +977,11 @@ static void task_item_init (TaskItem *item) {
     item->blink_timer = 0;
 }
 
-GtkWidget *task_item_new (WpApplet* windowPickerApplet, WnckWindow *window) {
+GtkWidget *
+task_item_new (WpApplet   *windowPickerApplet,
+               WnckWindow *window,
+               TaskList   *list)
+{
     TaskItem *taskItem;
     WnckScreen *screen;
     GtkWidget *item;
@@ -1041,13 +998,18 @@ GtkWidget *task_item_new (WpApplet* windowPickerApplet, WnckWindow *window) {
     gtk_widget_add_events (item, GDK_ALL_EVENTS_MASK);
     gtk_container_set_border_width (GTK_CONTAINER (item), 0);
     taskItem = TASK_ITEM (item);
+
     taskItem->window = window;
+    g_object_ref (taskItem->window);
+
     screen = wnck_window_get_screen (window);
     taskItem->screen = screen;
     taskItem->windowPickerApplet = windowPickerApplet;
 
     set_monitor (taskItem, get_window_monitor (window));
 
+    task_item_set_task_list (TASK_ITEM (item), list);
+
     g_signal_connect_object (windowPickerApplet,
                              "placement-changed",
                              G_CALLBACK (placement_changed_cb),
@@ -1109,16 +1071,12 @@ GtkWidget *task_item_new (WpApplet* windowPickerApplet, WnckWindow *window) {
         G_CALLBACK (on_screen_active_window_changed), item, 0);
     g_signal_connect_object (screen, "active-workspace-changed",
         G_CALLBACK (on_screen_active_workspace_changed), item, 0);
-    g_signal_connect_object (screen, "window-closed",
-        G_CALLBACK (on_screen_window_closed), item, 0);
     g_signal_connect_object (window, "workspace-changed",
         G_CALLBACK (on_window_workspace_changed), item, 0);
     g_signal_connect_object (window, "state-changed",
         G_CALLBACK (on_window_state_changed), item, 0);
     g_signal_connect_object (window, "icon-changed",
         G_CALLBACK (on_window_icon_changed), item, 0);
-    g_signal_connect_object (window, "type-changed",
-        G_CALLBACK (on_window_type_changed), item, 0);
     g_signal_connect_object (window, "geometry-changed",
         G_CALLBACK (on_window_geometry_changed), item, 0);
 
@@ -1146,3 +1104,27 @@ task_item_get_monitor (TaskItem *item)
 {
     return item->monitor;
 }
+
+TaskList *
+task_item_get_task_list (TaskItem *item)
+{
+  return item->list;
+}
+
+void
+task_item_set_task_list (TaskItem *item,
+                         TaskList *list)
+{
+  if (item->list)
+    g_object_unref (item->list);
+
+  item->list = list;
+
+  g_object_ref (item->list);
+}
+
+WnckWindow *
+task_item_get_window (TaskItem *item)
+{
+  return item->window;
+}
diff --git a/gnome-applets/window-picker/task-item.h b/gnome-applets/window-picker/task-item.h
index 2aaf5a956..ce1fb2d51 100644
--- a/gnome-applets/window-picker/task-item.h
+++ b/gnome-applets/window-picker/task-item.h
@@ -27,12 +27,22 @@
 #include <gtk/gtk.h>
 #include <libwnck/libwnck.h>
 
+#include "task-list.h"
+
 #define TASK_TYPE_ITEM (task_item_get_type ())
 G_DECLARE_FINAL_TYPE (TaskItem, task_item, TASK, ITEM, GtkEventBox)
 
-GtkWidget  *task_item_new         (WpApplet   *windowPickerApplet,
-                                   WnckWindow *window);
+GtkWidget  *task_item_new           (WpApplet   *windowPickerApplet,
+                                     WnckWindow *window,
+                                     TaskList   *list);
+
+GdkMonitor *task_item_get_monitor   (TaskItem *item);
+
+TaskList   *task_item_get_task_list (TaskItem *item);
+
+void        task_item_set_task_list (TaskItem *item,
+                                     TaskList *list);
 
-GdkMonitor *task_item_get_monitor (TaskItem *item);
+WnckWindow *task_item_get_window    (TaskItem *item);
 
 #endif /* _TASK_ITEM_H_ */
diff --git a/gnome-applets/window-picker/task-list.c b/gnome-applets/window-picker/task-list.c
index 126f73f34..86a6c3a68 100644
--- a/gnome-applets/window-picker/task-list.c
+++ b/gnome-applets/window-picker/task-list.c
@@ -28,6 +28,7 @@
 struct _TaskList {
     GtkBox parent;
     WnckScreen *screen;
+    GHashTable *items;
     WpApplet *windowPickerApplet;
     guint init_windows_event_source;
 };
@@ -70,15 +71,6 @@ get_task_list_for_monitor (GdkMonitor *monitor)
     return task_lists->data;
 }
 
-
-static void on_task_item_closed (
-    TaskItem *item,
-    TaskList *list)
-{
-    gtk_container_remove (GTK_CONTAINER (list),
-                          GTK_WIDGET (item));
-}
-
 static GdkMonitor *
 window_get_monitor (WnckWindow *window)
 {
@@ -94,15 +86,28 @@ window_get_monitor (WnckWindow *window)
                                              y + h / 2);
 }
 
+static void
+on_window_closed (WnckScreen *screen,
+                  WnckWindow *window,
+                  TaskList   *taskList)
+{
+  g_hash_table_remove (taskList->items, window);
+}
+
 static void
 on_task_item_monitor_changed_cb (TaskItem *item,
-                                 TaskList *current_list)
+                                 gpointer *user_data)
 {
     GdkMonitor *monitor;
     GdkMonitor *list_monitor;
-    TaskList *list;
+    TaskList *list, *current_list;
+    WnckWindow *window;
+
+    current_list = task_item_get_task_list (item);
+    window = task_item_get_window (item);
 
     monitor = task_item_get_monitor (item);
+
     list_monitor = task_list_get_monitor (current_list);
 
     if (monitor != list_monitor)
@@ -110,18 +115,16 @@ on_task_item_monitor_changed_cb (TaskItem *item,
         list = get_task_list_for_monitor (monitor);
 
         g_object_ref (item);
+
         gtk_container_remove (GTK_CONTAINER (current_list), GTK_WIDGET (item));
+        g_hash_table_steal (current_list->items, window);
 
         gtk_widget_queue_resize (GTK_WIDGET (current_list));
 
-        g_signal_handlers_disconnect_by_func (item,
-                                              on_task_item_monitor_changed_cb,
-                                              current_list);
-
         gtk_container_add (GTK_CONTAINER (list), GTK_WIDGET (item));
+        g_hash_table_insert (list->items, window, item);
 
-        g_signal_connect (TASK_ITEM (item), "monitor-changed",
-                          G_CALLBACK (on_task_item_monitor_changed_cb), list);
+        task_item_set_task_list (item, list);
 
         g_object_unref (item);
 
@@ -129,8 +132,9 @@ on_task_item_monitor_changed_cb (TaskItem *item,
       }
 }
 
-static void create_task_item (TaskList   *taskList,
-                              WnckWindow *window)
+static GtkWidget *
+create_task_item (TaskList   *taskList,
+                  WnckWindow *window)
 {
     GtkWidget *item;
     GdkMonitor *list_monitor;
@@ -145,34 +149,44 @@ static void create_task_item (TaskList   *taskList,
         window_monitor = window_get_monitor (window);
 
         if (list_monitor != window_monitor)
-            return;
+            return NULL;
       }
 
     item = task_item_new (taskList->windowPickerApplet,
-                          window);
+                          window, taskList);
 
     if (item)
       {
         gtk_container_add (GTK_CONTAINER (taskList), item);
 
-        g_signal_connect_object (TASK_ITEM (item), "task-item-closed",
-                                 G_CALLBACK (on_task_item_closed), taskList, 0);
-
-        g_signal_connect_object (TASK_ITEM (item), "monitor-changed",
-                                 G_CALLBACK (on_task_item_monitor_changed_cb),
-                                 taskList, 0);
+        g_signal_connect (TASK_ITEM (item), "monitor-changed",
+                          G_CALLBACK (on_task_item_monitor_changed_cb),
+                          NULL);
       }
+
+    return item;
 }
 
-static void type_changed (WnckWindow *window,
-                          gpointer user_data)
+static void
+on_window_type_changed (WnckWindow *window,
+                        TaskList   *list)
 {
-    TaskList *taskList = TASK_LIST (user_data);
+  GtkWidget *item;
 
-    if (!window_is_special (window))
-      {
-        create_task_item (taskList, window);
-      }
+  if (window_is_special (window))
+    {
+      g_hash_table_remove (list->items, window);
+    }
+  else
+    {
+      if (g_hash_table_lookup (list->items, window))
+        return;
+
+      item = create_task_item (list, window);
+
+      if (item)
+        g_hash_table_insert (list->items, window, item);
+    }
 }
 
 static void
@@ -188,15 +202,6 @@ on_task_list_placement_changed (GpApplet        *applet,
     gtk_widget_queue_resize(GTK_WIDGET(box));
 }
 
-static void
-remove_task_item (GtkWidget *item,
-                  gpointer   list)
-{
-  g_return_if_fail (TASK_IS_LIST (list));
-
-  gtk_container_remove (GTK_CONTAINER (list), item);
-}
-
 static void
 clear_windows (TaskList *list)
 {
@@ -209,21 +214,24 @@ clear_windows (TaskList *list)
                                                     window);
 
   if (task_list_get_monitor (list) == list_monitor)
-    gtk_container_foreach (GTK_CONTAINER (list), remove_task_item, list);
+    g_hash_table_remove_all (list->items);
 }
 
 static void
 add_window (TaskList *list, WnckWindow *window)
 {
-  g_signal_connect_object (window, "type-changed", G_CALLBACK (type_changed),
-                           list, 0);
+  GtkWidget *item;
+
+  g_signal_connect_object (window, "type-changed",
+                           G_CALLBACK (on_window_type_changed), list, 0);
 
   if (window_is_special (window))
-  {
     return;
-  }
 
-  create_task_item (list, window);
+  item = create_task_item (list, window);
+
+  if (item)
+    g_hash_table_insert (list->items, window, item);
 }
 
 static void
@@ -329,6 +337,10 @@ task_list_class_init(TaskListClass *class) {
 
 static void task_list_init (TaskList *list) {
     list->screen = wnck_screen_get_default ();
+
+    list->items = g_hash_table_new_full (NULL, NULL,
+                                         NULL, (GDestroyNotify) gtk_widget_destroy);
+
     gtk_container_set_border_width (GTK_CONTAINER (list), 0);
 }
 
@@ -353,6 +365,9 @@ GtkWidget *task_list_new (WpApplet *windowPickerApplet) {
     g_signal_connect_object (taskList->screen, "window-opened",
                              G_CALLBACK (on_window_opened), taskList, 0);
 
+    g_signal_connect_object (taskList->screen, "window-closed",
+                             G_CALLBACK (on_window_closed), taskList, 0);
+
     gdk_window_add_filter (gtk_widget_get_window (GTK_WIDGET (taskList)),
                            window_filter_function,
                            taskList);