Merge lp:~ted/unity/null-icon-info into lp:unity

Proposed by Ted Gould
Status: Rejected
Rejected by: Ted Gould
Proposed branch: lp:~ted/unity/null-icon-info
Merge into: lp:unity
Diff against target: 28 lines (+3/-3)
1 file modified
launcher/LauncherIcon.cpp (+3/-3)
To merge this branch: bzr merge lp:~ted/unity/null-icon-info
Reviewer Review Type Date Requested Status
Andrea Azzarone (community) Needs Fixing
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+164214@code.launchpad.net

Commit message

Handle the case where the info could be NULL

Description of the change

This branch sets the initial state (as there is some checking based on it) and uses g_clear_object to set the variable back to NULL after it is unset (and only unsets if it is non-NULL). This should fix the crash reported in bug 1180790.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

This still keeps a gtk_icon_info_free(info); around... I would instead use:

glib::Object<GtkIconInfo> info;

This would make all the unref's implicit on assignment, keeping everything safer.

Revision history for this message
Andrea Azzarone (azzar1) wrote :

And unity still fails to start.

review: Needs Fixing
Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

Ah, we also have a leak for g_icon_new_for_string, we should use glib::Object for that as well...

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

I suspect it could be one of those things where unref'ing the object is broken on older gnome versions (as they expect gtk_icon_info_free) - yet they've deprecated it in favor of generally just using unref in newer versions without fixing the behavior on older versions.

I'm just checking now with using glib::Object <> to wrap GtkIconInfo and using g_object_clear () inside of that instead of if (foo) g_object_unref () .

This branch could also be crashing on startup because the GtkIconInfo behavior was in LaucnherIcon, IconLoader and PanelIndicatorEntryView - but this branch only touches LauncherIcon.

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

g_object_clear / g_object_unref is broken on < 3.8 and gtk_icon_info_free is deprecated on >= 3.8. I like it when libraries do this. We'll have to ifdef it.

Revision history for this message
Ted Gould (ted) wrote :

Removing this item in favor of 3v1n0's

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'launcher/LauncherIcon.cpp'
2--- launcher/LauncherIcon.cpp 2013-05-14 15:54:28 +0000
3+++ launcher/LauncherIcon.cpp 2013-05-16 15:33:29 +0000
4@@ -396,7 +396,7 @@
5 bool update_glow_colors,
6 bool is_default_theme)
7 {
8- GtkIconInfo* info;
9+ GtkIconInfo* info = NULL;
10 nux::BaseTexture* result = NULL;
11 GIcon* icon;
12 GtkIconLookupFlags flags = (GtkIconLookupFlags) 0;
13@@ -423,13 +423,13 @@
14
15 if (gtk_icon_info_get_filename(info) == NULL)
16 {
17- g_object_unref(G_OBJECT(info));
18+ g_clear_object(&info);
19 info = gtk_icon_theme_lookup_icon(theme, DEFAULT_ICON.c_str(), size, flags);
20 }
21
22 glib::Error error;
23 glib::Object<GdkPixbuf> pbuf(gtk_icon_info_load_icon(info, &error));
24- g_object_unref(G_OBJECT(info));
25+ g_clear_object(&info);
26
27 if (GDK_IS_PIXBUF(pbuf.RawPtr()))
28 {