Merge lp:~3v1n0/bamf/factory-ref-rework into lp:bamf

[Marco Trevisan (Treviño)]

BamfFactory: fix references of allocated/closed views; improve re-matching of views

Now the ownership of the open views is in the hash-table, closed views must be owned
by clients (if they want to). The factory also tracks the allocated views until they
don't get destroyed to re-use them if possible.

The factory was owning the views it allocated in the bad way, and was re-opening closed views without taking the ownership of them.

Now the ownership of the open views is in the hash-table, closed views must be owned by clients (if they want to). The factory also tracks the allocated views until they don't get destroyed to re-use them if possible.

Added closed view re-matching by application name (only if the name is unique).

When the bamfdaemon is killed / stopped now all the views are set as closed, so that they can be correctly re-matched.

You can use this test-binary to check what happens in libbamf: http://pastebin.ubuntu.com/5761483/

Together with a script like
count=0; while [ $count -lt 20 ]; do xclock& sleep 0.3 && kill $(pidof xclock); count=$((count+1)); done

Can give some informations..

[lp:~3v1n0/bamf/factory-ref-rework]

569. By Marco Trevisan (Treviño) on 2013-06-13

BamfFactory: comment wording fix

[Ted Gould (ted)]

Really worried about reference leaks there, but I can't find any. You should probably consider in the future adding some tests that make sure you're not keeping any internal references of objects.

review: Approve

[Marco Trevisan (Treviño) (3v1n0)]

> Really worried about reference leaks there, but I can't find any. You should
> probably consider in the future adding some tests that make sure you're not
> keeping any internal references of objects.

Yes, true... Tests are something I must work on, but bamf was mostly untested since few months ago, so slowly adding them...

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:569
http://jenkins.qa.ubuntu.com/job/bamf-ci/48/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/bamf-saucy-amd64-ci/29
    SUCCESS: http://jenkins.qa.ubuntu.com/job/bamf-saucy-armhf-ci/29

Click here to trigger a rebuild:
http://s-jenkins:8080/job/bamf-ci/48/rebuild

review: Approve (continuous-integration)

[Andrea Azzarone (azzar1)]

LGTM too.

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2013-06-11 14:24:57 +0000
+++ debian/changelog	2013-06-13 13:28:30 +0000
@@ -1,5 +1,6 @@
 bamf (0.5.0) UNRELEASED; urgency=low
 
+  [ Marco Trevisan (Treviño) ]
   * New upstream release, bumping debian version to 0.5.0
   * LibBamfPrivate: add new private library to share code between client
     and daemon.
@@ -7,16 +8,14 @@
     allows to have correct values and signals when they get updated.
     Deprecating related dbus methods/signals.
   * debian/control:
-      - don't build-depend anymore on libdbus-glib-1-dev
-
- -- Marco Trevisan (Treviño) <[e-mail]>  Wed, 05 Jun 2013 14:00:27 +0200
-
-bamf (0.4.0daily13.06.07-0ubuntu2) UNRELEASED; urgency=low
-
-  * Have libbamf3-dev depend on gir1.2-bamf-3
-  * Drop explicit build-depends on gir1.2-glib-2.0 
-
- -- Jeremy Bicha <[e-mail]>  Mon, 10 Jun 2013 14:28:12 -0400
+    - don't build-depend anymore on libdbus-glib-1-dev
+
+  [ Jeremy Bicha ]
+  * debian/control:
+    - Have libbamf3-dev depend on gir1.2-bamf-3
+    - Drop explicit build-depends on gir1.2-glib-2.0 
+
+ -- Marco Trevisan (Treviño) <[e-mail]>  Wed, 12 Jun 2013 14:30:03 +0200
 
 bamf (0.4.0daily13.06.07-0ubuntu1) saucy; urgency=low
 

=== modified file 'lib/libbamf/bamf-factory.c'
--- lib/libbamf/bamf-factory.c	2013-06-10 16:58:39 +0000
+++ lib/libbamf/bamf-factory.c	2013-06-13 13:28:30 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright 2010-2012 Canonical Ltd.
+ * Copyright 2010-2013 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it
  * under the terms of either or both of the following licenses:
@@ -21,7 +21,7 @@
  *
  * Authored by: Jason Smith <[e-mail]>
  *              Neil Jagdish Patel <[e-mail]>
- *              Marco Trevisan (Treviño) <[e-mail]>
+ *              Marco Trevisan (Treviño) <[e-mail]>
  *
  */
 /**
@@ -49,40 +49,57 @@
 {
   GHashTable *open_views;
   GList *local_views;
-  GList *registered_views;
+  GList *allocated_views;
 };
 
 static BamfFactory *static_factory = NULL;
 
+static void on_view_weak_unref (BamfFactory *self, BamfView *view_was_here);
+
 static void
 bamf_factory_dispose (GObject *object)
 {
-  BamfFactory *self = (BamfFactory *) object;
-
-  if (self->priv->open_views)
-    {
-      g_hash_table_destroy (self->priv->open_views);
-      self->priv->open_views = NULL;
-    }
-
-  if (self->priv->registered_views)
-    {
-      g_list_free (self->priv->registered_views);
-      self->priv->registered_views = NULL;
+  GList *l, *next;
+  BamfFactory *self = BAMF_FACTORY (object);
+
+  if (self->priv->allocated_views)
+    {
+      for (l = self->priv->allocated_views, next = NULL; l; l = next)
+        {
+          g_object_weak_unref (G_OBJECT (l->data), (GWeakNotify) on_view_weak_unref, self);
+          next = l->next;
+          g_list_free1 (l);
+        }
+
+      self->priv->allocated_views = NULL;
     }
 
   if (self->priv->local_views)
     {
-      g_list_free_full (self->priv->local_views, g_object_unref);
+      g_list_free (self->priv->local_views);
       self->priv->local_views = NULL;
     }
 
+  if (self->priv->open_views)
+    {
+      g_hash_table_remove_all (self->priv->open_views);
+      self->priv->open_views = NULL;
+    }
+
   G_OBJECT_CLASS (bamf_factory_parent_class)->dispose (object);
 }
 
 static void
 bamf_factory_finalize (GObject *object)
 {
+  BamfFactory *self = BAMF_FACTORY (object);
+
+  if (self->priv->open_views)
+    {
+      g_hash_table_destroy (self->priv->open_views);
+      self->priv->open_views = NULL;
+    }
+
   static_factory = NULL;
 
   G_OBJECT_CLASS (bamf_factory_parent_class)->finalize (object);
@@ -103,62 +120,75 @@
 static void
 bamf_factory_init (BamfFactory *self)
 {
-  BamfFactoryPrivate *priv;
-
-  priv = self->priv = BAMF_FACTORY_GET_PRIVATE (self);
-
-  priv->open_views = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
+  self->priv = BAMF_FACTORY_GET_PRIVATE (self);
+  self->priv->open_views = g_hash_table_new_full (g_str_hash, g_str_equal,
+                                                  g_free, g_object_unref);
 }
 
 static void
 on_view_closed (BamfView *view, BamfFactory *self)
 {
   const char *path;
-
-  g_return_if_fail (BAMF_IS_VIEW (view));
-
+  gboolean removed;
+
+  removed = FALSE;
   path = _bamf_view_get_path (view);
 
+  g_signal_handlers_disconnect_by_func (view, on_view_closed, self);
+
   if (path)
-    g_hash_table_remove (self->priv->open_views, path);
-
-  g_object_unref (view);
-}
-
-static void
-on_view_weak_unref (BamfFactory *self, BamfView *view)
-{
-  GHashTableIter iter;
-  gpointer key, value;
-
-  g_return_if_fail (BAMF_IS_FACTORY (self));
-
-  self->priv->local_views = g_list_remove (self->priv->local_views, view);
-  self->priv->registered_views = g_list_remove (self->priv->registered_views, view);
-
-  g_hash_table_iter_init (&iter, self->priv->open_views);
-  while (g_hash_table_iter_next (&iter, &key, &value))
-    {
-      if (value == view)
+    {
+      removed = g_hash_table_remove (self->priv->open_views, path);
+    }
+
+  if (G_UNLIKELY (!removed))
+    {
+      /* Unlikely to happen, but who knows... */
+      GHashTableIter iter;
+      gpointer value;
+
+      g_hash_table_iter_init (&iter, self->priv->open_views);
+      while (g_hash_table_iter_next (&iter, NULL, &value))
         {
-          g_hash_table_iter_remove (&iter);
-          break;
+          if (value == view)
+            {
+              g_hash_table_iter_remove (&iter);
+              removed = TRUE;
+              break;
+            }
         }
     }
 }
 
 static void
+on_view_weak_unref (BamfFactory *self, BamfView *view_was_here)
+{
+  self->priv->local_views = g_list_remove (self->priv->local_views, view_was_here);
+  self->priv->allocated_views = g_list_remove (self->priv->allocated_views, view_was_here);
+}
+
+static void
+bamf_factory_track_view (BamfFactory *self, BamfView *view)
+{
+  g_return_if_fail (BAMF_IS_VIEW (view));
+
+  if (g_list_find (self->priv->allocated_views, view))
+    return;
+
+  g_object_weak_ref (G_OBJECT (view), (GWeakNotify) on_view_weak_unref, self);
+  self->priv->allocated_views = g_list_prepend (self->priv->allocated_views, view);
+}
+
+static void
 bamf_factory_register_view (BamfFactory *self, BamfView *view, const char *path)
 {
+  g_return_if_fail (BAMF_IS_VIEW (view));
+  g_return_if_fail (path != NULL);
+
+  g_object_ref_sink (view);
   g_hash_table_insert (self->priv->open_views, g_strdup (path), view);
-
-  if (g_list_find (self->priv->registered_views, view))
-    return;
-
-  g_signal_connect_after (G_OBJECT (view), "closed", (GCallback) on_view_closed, self);
-  g_object_weak_ref (G_OBJECT (view), (GWeakNotify) on_view_weak_unref, self);
-
-  self->priv->registered_views = g_list_prepend (self->priv->registered_views, view);
+  g_signal_connect_after (G_OBJECT (view), BAMF_VIEW_SIGNAL_CLOSED,
+                          G_CALLBACK (on_view_closed), self);
 }
 
 BamfApplication *
@@ -188,9 +218,11 @@
     {
       /* delay registration until match time */
       result = bamf_application_new_favorite (path);
-      if (result && !g_list_find (factory->priv->local_views, result))
+
+      if (BAMF_IS_APPLICATION (result))
         {
           factory->priv->local_views = g_list_prepend (factory->priv->local_views, result);
+          bamf_factory_track_view (factory, BAMF_VIEW (result));
         }
     }
 
@@ -249,7 +281,6 @@
   BamfView *view;
   BamfDBusItemView *vproxy;
   GList *l;
-  gboolean created = FALSE;
 
   g_return_val_if_fail (BAMF_IS_FACTORY (factory), NULL);
 
@@ -298,20 +329,25 @@
         break;
     }
 
-  created = TRUE;
   BamfView *matched_view = NULL;
 
+  /* handle case where another allocated (but closed) view exists and the new
+   * one matches it, so that we can reuse it. */
   if (BAMF_IS_APPLICATION (view))
     {
-      /* handle case where another living view exists and the new one matches it */
       const char *local_desktop_file = bamf_application_get_desktop_file (BAMF_APPLICATION (view));
       GList *local_children = _bamf_application_get_cached_xids (BAMF_APPLICATION (view));
+      char *local_name = bamf_view_get_name (view);
+      gboolean matched_by_name = FALSE;
 
-      for (l = factory->priv->local_views; l; l = l->next)
+      for (l = factory->priv->allocated_views; l; l = l->next)
         {
           if (!BAMF_IS_APPLICATION (l->data))
             continue;
 
+          if (!bamf_view_is_closed (l->data))
+            continue;
+
           BamfView *list_view = BAMF_VIEW (l->data);
           BamfApplication *list_app = BAMF_APPLICATION (l->data);
 
@@ -326,7 +362,7 @@
 
           /* If the primary search doesn't give out any result, we fallback
            * to children window comparison */
-          if (!list_desktop_file && !matched_view)
+          if (!list_desktop_file)
             {
               GList *list_children, *ll;
               list_children = _bamf_application_get_cached_xids (list_app);
@@ -341,18 +377,45 @@
                       break;
                     }
                 }
+
+              if ((!matched_view || matched_by_name) && local_name && local_name[0] != '\0')
+                {
+                  char *list_name = bamf_view_get_name (list_view);
+                  if (g_strcmp0 (local_name, list_name) == 0)
+                    {
+                      if (!matched_by_name)
+                        {
+                          matched_view = list_view;
+                          matched_by_name = TRUE;
+                        }
+                      else
+                        {
+                          /* We have already matched an app by its name, this
+                           * means that there are two apps with the same name.
+                           * It's safer to ignore both, then. */
+                          matched_view = NULL;
+                        }
+                    }
+
+                  g_free (list_name);
+                }
             }
         }
+
+      g_free (local_name);
     }
   else if (BAMF_IS_WINDOW (view))
     {
       guint32 local_xid = bamf_window_get_xid (BAMF_WINDOW (view));
 
-      for (l = factory->priv->local_views; l; l = l->next)
+      for (l = factory->priv->allocated_views; l; l = l->next)
         {
           if (!BAMF_IS_WINDOW (l->data))
             continue;
 
+          if (!bamf_view_is_closed (l->data))
+            continue;
+
           BamfView *list_view = BAMF_VIEW (l->data);
           BamfWindow *list_win = BAMF_WINDOW (l->data);
 
@@ -369,27 +432,24 @@
 
   if (matched_view)
     {
-      created = FALSE;
-
       if (view)
-        g_object_unref (view);
+        {
+          /* We don't need anymore the view we've just created, let's forget it */
+          g_object_unref (view);
+        }
 
+      /* If we are here, we're pretty sure that the view is not in the
+       * open_views, hash-table (since it has been closed) so we can safely
+       * re-register it here again. */
       view = matched_view;
       _bamf_view_set_path (view, path);
-
-      // FIXME: this is shouldn't be needed
-      g_object_ref_sink (view);
+      bamf_factory_register_view (factory, view, path);
     }
-
-  if (view)
+  else if (view)
     {
+      /* It's the first time we register this view, we also have to track it, then */
+      bamf_factory_track_view (factory, view);
       bamf_factory_register_view (factory, view, path);
-
-      if (created)
-        {
-          g_object_ref_sink (view);
-          factory->priv->local_views = g_list_prepend (factory->priv->local_views, view);
-        }
     }
 
   return view;

=== modified file 'lib/libbamf/bamf-view.c'
--- lib/libbamf/bamf-view.c	2013-06-11 13:00:51 +0000
+++ lib/libbamf/bamf-view.c	2013-06-13 13:28:30 +0000
@@ -506,6 +506,21 @@
 }
 
 static void
+bamf_view_on_name_owner_changed (BamfDBusItemView *proxy, GParamSpec *param, BamfView *self)
+{
+  /* This is called when the bamfdaemon is killed / started */
+  gchar *name_owner = g_dbus_proxy_get_name_owner (G_DBUS_PROXY (proxy));
+
+  if (!name_owner)
+    {
+      _bamf_view_set_closed (self, TRUE);
+      g_signal_emit (G_OBJECT (self), view_signals[CLOSED], 0);
+    }
+
+  g_free (name_owner);
+}
+
+static void
 bamf_view_on_active_changed (BamfDBusItemView *proxy, GParamSpec *param, BamfView *self)
 {
   gboolean active = _bamf_dbus_item_view_get_active (proxy);
@@ -768,6 +783,9 @@
   _bamf_view_reset_flags (view);
   g_object_notify_by_pspec (G_OBJECT (view), properties[PROP_PATH]);
 
+  g_signal_connect (priv->proxy, "notify::g-name-owner",
+                    G_CALLBACK (bamf_view_on_name_owner_changed), view);
+
   g_signal_connect (priv->proxy, "notify::active",
                     G_CALLBACK (bamf_view_on_active_changed), view);
 
@@ -835,7 +853,7 @@
   view_signals [CLOSED] =
     g_signal_new (BAMF_VIEW_SIGNAL_CLOSED,
                   G_OBJECT_CLASS_TYPE (klass),
-                  G_SIGNAL_RUN_FIRST,
+                  G_SIGNAL_RUN_LAST,
                   G_STRUCT_OFFSET (BamfViewClass, closed),
                   NULL, NULL,
                   g_cclosure_marshal_VOID__VOID,

=== modified file 'src/bamf-matcher.c'
--- src/bamf-matcher.c	2013-06-11 13:18:00 +0000
+++ src/bamf-matcher.c	2013-06-13 13:28:30 +0000
@@ -2821,6 +2821,7 @@
   return TRUE;
 }
 
+#ifdef HAVE_WEBAPPS
 static gboolean
 bamf_matcher_has_tab_with_parent_xid (BamfMatcher *matcher, guint64 xid)
 {
@@ -2839,7 +2840,6 @@
   return FALSE;
 }
 
-#ifdef HAVE_WEBAPPS
 static void
 on_webapp_child_added (BamfView *application,
                        BamfView *child,