Merge lp:~cimi/overlay-scrollbar/various-fixes-to-mem-leaks-and-dispose into lp:overlay-scrollbar

[Andrea Cimitan]

A lot of object were not freed/disposed/unref

[Ted Gould (ted)]

  review needsfixing

On Thu, 2011-04-07 at 16:18 +0000, Andrea Cimitan wrote:
> @@ -265,6 +267,20 @@
> priv->animation = NULL;
> }
>
> + if (priv->pager_window != NULL)
> + {
> + /* FIXME(Cimi) g_object_destroy doesn't seem
> + * to clear the background.
> + * Maybe I need to clear it using cairo,
> + * but gdk_window_destroy() does this for me. */
> + gdk_window_destroy (priv->pager_window);
> +
> + g_object_unref (priv->pager_window);
> + priv->pager_window = NULL;

You shouldn't need to destroy and unref. That could cause problems.

> + if (priv->grabbed_widget != NULL)
> + g_object_unref (priv->grabbed_widget);
> +
> priv->grabbed_widget = gtk_grab_get_current ();
>
> if (priv->grabbed_widget != NULL)
> - gtk_grab_remove (priv->grabbed_widget);
> + {
> + g_object_ref_sink (priv->grabbed_widget);
> +
> + gtk_grab_remove (priv->grabbed_widget);
> + }
>
> GTK_WIDGET_CLASS (os_thumb_parent_class)->map (widget);
> }

After unref'ing the grabbed_widget you need to set it to NULL otherwise
the if won't work correctly.

review: Needs Fixing

[lp:~cimi/overlay-scrollbar/various-fixes-to-mem-leaks-and-dispose]

202. By Andrea Cimitan on 2011-04-07

updated comment

[Ted Gould (ted)]

review: Approve

=== modified file 'os/os-pager.c'
--- os/os-pager.c	2011-04-06 16:18:11 +0000
+++ os/os-pager.c	2011-04-07 17:16:07 +0000
@@ -104,6 +104,8 @@
                                            &attributes,
                                            GDK_WA_VISUAL | GDK_WA_COLORMAP);
 
+      g_object_ref_sink (priv->pager_window);
+
       gdk_window_set_transient_for (priv->pager_window,
                                     gtk_widget_get_window (priv->parent));
 
@@ -265,6 +267,22 @@
       priv->animation = NULL;
     }
 
+  if (priv->pager_window != NULL)
+    {
+      /* From the Gdk documentation:
+       * "Note that a window will not be destroyed
+       *  automatically when its reference count
+       *  reaches zero. You must call
+       *  gdk_window_destroy ()
+       *  yourself before that happens". */
+      gdk_window_destroy (priv->pager_window);
+
+      g_object_unref (priv->pager_window);
+      priv->pager_window = NULL;
+    }
+
+  os_pager_set_parent (pager, NULL);
+
   g_signal_handler_disconnect (gtk_settings_get_default (),
                                priv->handler_id);
 

=== modified file 'os/os-scrollbar.c'
--- os/os-scrollbar.c	2011-04-07 13:36:15 +0000
+++ os/os-scrollbar.c	2011-04-07 17:16:07 +0000
@@ -1438,21 +1438,15 @@
       priv->pager = NULL;
     }
 
+  os_scrollbar_swap_adjustment (scrollbar, NULL);
+  os_scrollbar_swap_thumb (scrollbar, NULL);
+
   G_OBJECT_CLASS (os_scrollbar_parent_class)->dispose (object);
 }
 
 static void
 os_scrollbar_finalize (GObject *object)
 {
-  OsScrollbar *scrollbar;
-  OsScrollbarPrivate *priv;
-
-  scrollbar = OS_SCROLLBAR (object);
-  priv = scrollbar->priv;
-
-  os_scrollbar_swap_adjustment (scrollbar, NULL);
-  os_scrollbar_swap_thumb (scrollbar, NULL);
-
   G_OBJECT_CLASS (os_scrollbar_parent_class)->finalize (object);
 }
 

=== modified file 'os/os-thumb.c'
--- os/os-thumb.c	2011-04-06 10:54:14 +0000
+++ os/os-thumb.c	2011-04-07 17:16:07 +0000
@@ -161,10 +161,10 @@
   widget_class->unmap                = os_thumb_unmap;
 
   gobject_class->constructor  = os_thumb_constructor;
+  gobject_class->dispose      = os_thumb_dispose;
+  gobject_class->finalize     = os_thumb_finalize;
   gobject_class->get_property = os_thumb_get_property;
   gobject_class->set_property = os_thumb_set_property;
-  gobject_class->dispose = os_thumb_dispose;
-  gobject_class->finalize = os_thumb_finalize;
 
   g_object_class_install_property
       (gobject_class, PROP_ORIENTATION,
@@ -230,6 +230,12 @@
       priv->animation = NULL;
     }
 
+  if (priv->grabbed_widget != NULL)
+    {
+      g_object_unref (priv->grabbed_widget);
+      priv->grabbed_widget = NULL;
+    }
+
   G_OBJECT_CLASS (os_thumb_parent_class)->dispose (object);
 }
 
@@ -291,14 +297,15 @@
   thumb = OS_THUMB (widget);
   priv = thumb->priv;
 
-  OS_DCHECK (priv->source_id == 0);
-
   gtk_widget_get_allocation (widget, &allocation);
 
-  /* priv->source_id should be always 0 here,
-   * because it's set to 0 in both motion_notify_event
-   * and button_press_event.
-   * Add the fade-out timeout only
+  if (priv->source_id != 0)
+    {
+      g_source_remove (priv->source_id);
+      priv->source_id = 0;
+    }
+
+  /* Add the fade-out timeout only
    * if the pointer is inside the thumb.
    * allocation.x and allocation.y are always 0. */
   if ((event->x >= 0 && event->x <= allocation.width) &&
@@ -595,10 +602,17 @@
 
   gtk_window_set_opacity (GTK_WINDOW (widget), 1.0f);
 
+  if (priv->grabbed_widget != NULL)
+    g_object_unref (priv->grabbed_widget);
+
   priv->grabbed_widget = gtk_grab_get_current ();
 
   if (priv->grabbed_widget != NULL)
-    gtk_grab_remove (priv->grabbed_widget);
+    {
+      g_object_ref_sink (priv->grabbed_widget);
+
+      gtk_grab_remove (priv->grabbed_widget);
+    }
 
   GTK_WIDGET_CLASS (os_thumb_parent_class)->map (widget);
 }