[patch] awn_applet_simple_set_icon leaks memory and set_temp_icon could be more robust

Just an additional clarification:

This patch renders the priv->temp==FALSE code paths unused. If the patch proves to be effective then those code paths should then be removed.

attaching a proper patch

The patch looks cleaner, but after testing it a while the applets' icons got invalid (non-flipped reflection, then the icon was only white area - invalid memory?!). I did see this behaviour on non-patched AWN too (though very rarely), but this now seemed to happen sooner after start.

Can you check the patch again, please? Maybe writing the set_temp_icon function from scratch would be best.

I'll have another look at this one. My suspicion though is that the issue that you're experiencing may be related to a bug that may exist in awn-effects that I'm still trying to track down.

I'm fairly confident that, as written, this is extremely robust. But I have been known to be wrong in the past so I will have another look.

A few comments after having used this patch myself for several days. Along with several others.

1) The graphical corruption issues I saw with rendered icons under load have basically disappeared. As has console spam associated with changing icons while under load (less common than the simple icon graphical corruption). I have not been able to trigger this with stable testing even when I try recently while I more or less can do so with ease on trunk. A note: I suspect the higher spec a machine is the harder it is to trigger the condition.

2) From a subjective perspective I feel that applets that make use of icon change functions have greater stability. The tendency to just silently crash seems greatly reduced. I haven't experienced an overall decrease in awn crashing just because the trunk hasn't been prone to simple crashes for me for a long time. I attribute this to the fact that I do not use launchers - the code that deals with that seems to have been in need of some love.

I don't see any issue in the patched code yet. But I'm going to go ahead and clean out the priv->temp code today to make the source cleaner. Maybe an issue will pop out at me then.

I've seem to have tracked down the reflection bug. It was unrelated to this patch per se. Though it is in the same file.

I'm not exactly confident of the root cause but I actually suspect that it is a gdk bug. It seems that gdk_pixbuf_flip sometimes fails to generate a valid pixbuf even if it believes the pixbuf given as an argument is a valid pixbuf. Basically what I'm doing is checking if ->reflect is a valid pixbuf and if it isn't I'm using the orig_icon to create a reflection instead.

Not perfectly happy with this yet. But my current patch seems to be giving reflections without fail... no more disappearing reflection!

Patch to follow in a few minutes. It will contain, the previous fixes, the remove of the temp==false logic and the reflection fixes.

Here is the patch.

-fixes set_icon issues (fundamentally unchanged from earlier).
-removes temp == TRUE logic.
-fixed reflection

this patch is against
revno: 136
branch-nick: avant-window-navigator

I tested it against the stable-testing branch but I don't expect any issue against trunk.

On reflection I'm thinking I might have a slight memory leak where priv->reflect needs to be unref'd before trying to create it again. Going to check that.

some carefully placed asserts are telling me that there is nothing to unref... so we're good. I'm going to keep an eye on awn memory usage for a few hours and see if it climbs. If there is something leaking then it should so up as the reflection fix code path is hit quite often.

more problems found in awn_applet_simple.

To quote someone on #gtk regarding some of double unref code

"< tko> depends I suppose.. my first thought was that it's as valid as shooting yourself in the foot"

working on it.

It really seems the double unref code was dubious. I don't know if it is in reality causing issues... but it seems it is at least iffy.

This changes version of the patch changes thins around so G_IS_OBJECT is not use... #gtk comments seems to indicate this might not be valid if the refcount on the "object" is already 0...

it also makes sure that ->icon and org_icon are different pixbufs (they may be identical but they are in that case still copies). It just seems safer.

Now I'm inclined to think that a pixbuf copy or two can be optimized out of this... but if anyone tries it please, please make sure the reference count in such we're not working on a pointer that is pointing to memory location that's already been unreferenced to 0.

Patch to follow.

-fixes set_icon issues (fundamentally unchanged from earlier).
-removes temp == TRUE logic.
-fixed reflection
-changes double unref method.

this patch is against
revno: 136
branch-nick: avant-window-navigator

briefly tested against trunk. more extensively tested against stable-testing

addendum:

I'd like to give credit to Qball for whining about the unref code.

thanks.

TBH I also had suspicions about the unrefing and in fact I still don't like what's being done in the two functions (also the copying now). It's really dirty, but I guess we'll have to live with it in AWN 0.2

Thanks for all your effort, I'll commit this after testing it a while.

You're welcome,

Like you say it's not the cleanest and could be rewritten. At this point with 0.2 I think it's better to minimize the restructuring of logic if possible and just fix the problems. But I may rewrite it at some point, if someone else does not, just because I it is ugly. But I would suggest on any rewrite that the idea of a copy being made of the pixbuf will help keep errant applets from causing issues.

I don't know what you will experience in your case but this patch has seemingly eliminated the last of the corruption issues and more importantly the mysterious, silent, applet crashes (at least with the ones I use)

It's been pointed out that I messed up the whitespace with this patch... did not realize the convention is 8 characters.

That and I left the assert.h in there along with some asserts. if you like I can submit another patch to fix up these issues.

My apologies for overlooking those items... In retrospect I as a little bit tired at the time.

Fixes up whitespace issues (uses 8 char tab) and removes asserts. In my opinion there is no huge rush in terms of stability to apply it. But it will look a lot neater.

Hey, I'd like to revisit this issue, I really don't like that set_temp_icon touches and frees!!! the parameter which an applet passes in, in fact I think that this could in some cases crash an applet - because python will automatically unref the pixbuf if it can(it's no longer used), but if it was passed previously to this function it tries to unref and already freed object, and IMO this may lead to a crash.

I suggest marking the set_temp_icon as deprecated and making it do the same as set_icon does -> make a copy of the passed pixbuf and use it, but without the unrefing of the original pixbuf.

Yes, this might cause leaks for applets which already count on the fact that set_temp_icon frees the pixbuf, but that would only apply to applets written in C that use set_temp_icon. Are there such applets?

I think the current situation shows that there is no issue with python causing a crash. I suspect, based upon comments in the previous version of the code that python ref's the pixbuf therefore there is no issue. If I am wrong on the python behaviour please correct me :-)

I would be extremely opposed to changing the implementation before 0.2.1 and thus causing memory leaks to occur. This is stable as is. Though I think it's fine to mark the interface as deprecated. Though it seems to be Neil added this interface after the fact to help with applet devs and memory management issues. At this point I think it's not broken so don't fix it.

And yes there are C applets that us set_temp_icon.

>> I suspect, based upon comments in the previous version of the code that python ref's the pixbuf therefore there is no issue.

Looking at the older code, the function was shooting into it's own leg - first gdk_pixbuf_scale_simple or gdk_pixbuf_flip functions were called (both create new pixbuf and set it's ref count to 1), and then g_object_ref is called - so it alone refs the pixbuf twice, don't blame python.

But OK, maybe most of times applets will be lucky and OS will not SIGSEGV them...

:-) well the comment seemed to indicate such.

I'll go check out some python applets tonight that have a habit of changing the icon. And see if I can provoke them into an assertion since they should generate an assert if they try an unref a pixbuf that has already been destroyed. I may be wrong about the number of python applets that use it. If this does, in fact, turn out to be an issue my feeling is that it would just be best to remove the python bindings for the function... or if possible direct redirect the python binding to set_icon (don't know how easy that is )

as a side note. All of mosburger's python apps use set_temp_icon. They are rock solid and are not emitting any assertion as bout attempting to unref an pixbuf.

> If this does, in fact, turn out to be an issue my feeling is that it would just be best to remove the python bindings for the function...
> or if possible direct redirect the python binding to set_icon (don't know how easy that is )

FYI, it should be simply a matter of changing the C function alias that the set_temp_icon definition points to in pyawn/awn.defs.

Mark, what do you think? Can it theoretically cause a crash of python applet?

Well, if it can crash C applets, it can most definitely crash python applets.

0.2.1 released

Here's the one-liner patch that does what I talk about in comment 23.

...apparently that doesn't compile (because pygtk-codegen isn't smart enough to handle that), so here's a version that does compile.

Was this patch tested if the applets using set_temp_icon are not leaking memory?

As far as I know it has not been tested by anyone with a testing applet . I'd write an applet to test it but I don't do python...

That being said I have been running the patch for a few days now and I haven't seen anything that would indicate a memory leak and I do use both the weather applet and clock/cal which should be prime candidates. But that is just anecdotal of course.

My strong suspicion is that python is fine with this. But it would be nice if someone would write a an python applet that repeatedly, and at a high freq, uses set_temp_icon. If there are any leaks it should show up within a few minutes.