Merge lp:~larsu/indicator-messages/fix-leak into lp:indicator-messages/14.04

[Lars Karlitski]

im-menu: fix leak

[Ted Gould (ted)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/indicator-messages-autolanding/44/
Executed test runs:
    FAILURE: http://s-jenkins:8080/job/generic-cleanup-mbs/3188/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-messages-trusty-amd64-autolanding/3
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-messages-trusty-armhf-autolanding/3/console
        deb: http://jenkins.qa.ubuntu.com/job/indicator-messages-trusty-armhf-autolanding/3/artifact/work/output/*zip*/output.zip

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:397
http://jenkins.qa.ubuntu.com/job/indicator-messages-ci/108/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-messages-trusty-amd64-ci/3
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-messages-trusty-armhf-ci/3
        deb: http://jenkins.qa.ubuntu.com/job/indicator-messages-trusty-armhf-ci/3/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/indicator-messages-ci/108/rebuild

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

This is a nice find, but the fix is slightly wrong:

if g_menu_model_get_item_attribute() returns true, we need to call "g_free (item_sort);" no matter what g_utf8_collate() returns.

Maybe something like

  if (g_menu_model_get_item_attribute (...)
    {
      gint collated = g_utf8_collate (sort_string, item_sort);
      g_free (item_sort);
      if (collated < 0)
        break;
    }

review: Needs Fixing

[Lars Karlitski (larsu)]

> This is a nice find, but the fix is slightly wrong:
>
> if g_menu_model_get_item_attribute() returns true, we need to call "g_free
> (item_sort);" no matter what g_utf8_collate() returns.

The fix is correct. g_free() is always called after the condition (item_sort is explicitly set to NULL). The problem this patch solves is that g_free() wasn't called when breaking out of the surrounding loop.

I'll change it anyway because your solution reads better. Thanks!

[lp:~larsu/indicator-messages/fix-leak]

398. By Lars Karlitski on 2013-11-19

im_menu_insert: make sorting logic more readable

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:398
http://jenkins.qa.ubuntu.com/job/indicator-messages-ci/109/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-messages-trusty-amd64-ci/4
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-messages-trusty-armhf-ci/4
        deb: http://jenkins.qa.ubuntu.com/job/indicator-messages-trusty-armhf-ci/4/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/indicator-messages-ci/109/rebuild

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

Ah, sorry about that. :)

review: Approve

=== modified file 'src/im-menu.c'
--- src/im-menu.c	2013-08-21 02:03:18 +0000
+++ src/im-menu.c	2013-11-19 10:37:47 +0000
@@ -179,11 +179,17 @@
 
   for (position = 1; position < g_menu_model_get_n_items(G_MENU_MODEL (priv->menu)) - 1; position++)
     {
-      gchar * item_sort = NULL;
+      gchar *item_sort;
+
       if (g_menu_model_get_item_attribute(G_MENU_MODEL(priv->menu), position, "x-messaging-menu-sort-string", "s", &item_sort))
-        if (g_utf8_collate(sort_string, item_sort) < 0)
-          break;
-      g_free(item_sort);
+        {
+          gint cmp;
+
+          cmp = g_utf8_collate(sort_string, item_sort);
+          g_free (item_sort);
+          if (cmp < 0)
+            break;
+        }
     }
 
   item = g_menu_item_new_section (NULL, section);